From 85c2ac3f2b88097f53b7d16df6d68fa825614f4c Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Thu, 30 Jan 2025 15:02:55 +0100 Subject: [PATCH 1/4] Don't force requeuing after adding finalizer in GitRepo controller This change is meant to avoid an extra `Reconcile` call when adding the finalizer to `GitRepo`. The reconciler will get called anyway because generation changes and, also because generation changes, we'll avoid an extra call to the polling function. See below the difference of with and without the change. I've added a new extra log lines to show the difference (that are not part of this pull request) **without change** ```bash 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER Reconciling GitRepo {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "5517b2f5-2d4e-415b-b0e5-859cc070d9af"} 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER -- finalizer added {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "5517b2f5-2d4e-415b-b0e5-859cc070d9af"} 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER Reconciling GitRepo {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "94d228b5-c018-46af-9b69-b04b17ed976a"} 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER --- GitRepo polled {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "94d228b5-c018-46af-9b69-b04b17ed976a", "generation": 2, "commit": "", "conditions": null} 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER Reconciling GitRepo {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "1c71ae46-ea6b-452c-8896-704293f24584"} 2025-01-30T13:45:49Z INFO gitjob TESTFINALIZER --- GitRepo polled {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "1c71ae46-ea6b-452c-8896-704293f24584", "generation": 2, "commit": "", "conditions": [{"type":"Ready","status":"True","lastUpdateTime":"2025-01-30T13:45:49Z"}]} ``` `GitRepo` is polled twice. 1 time with generation=1 and the second with generation=2 **with change** ```bash 2025-01-30T13:54:19Z INFO gitjob TESTFINALIZER Reconciling GitRepo {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "4cb52a70-d552-4cda-85f7-058a9d129e1f"} 2025-01-30T13:54:19Z INFO gitjob TESTFINALIZER -- finalizer added {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "4cb52a70-d552-4cda-85f7-058a9d129e1f"} 2025-01-30T13:54:19Z INFO gitjob TESTFINALIZER Reconciling GitRepo {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "52cce150-c17c-4242-acb8-6b05ba3ecd58"} 2025-01-30T13:54:19Z INFO gitjob TESTFINALIZER --- GitRepo polled {"controller": "gitrepo", "controllerGroup": "fleet.cattle.io", "controllerKind": "GitRepo", "GitRepo": {"name":"simple","namespace":"fleet-local"}, "namespace": "fleet-local", "name": "simple", "reconcileID": "52cce150-c17c-4242-acb8-6b05ba3ecd58", "generation": 2, "commit": "", "conditions": null} ``` `GitRepo` is only polled 1 time when generation=2 Signed-off-by: Xavi Garcia --- internal/cmd/controller/gitops/reconciler/gitjob_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 41d2457e84..f1cc7aa895 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -158,7 +158,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } // requeue as adding the finalizer changes the spec - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{}, nil } logger = logger.WithValues("generation", gitrepo.Generation, "commit", gitrepo.Status.Commit).WithValues("conditions", gitrepo.Status.Conditions) From fc28643b02f0157befd97ee1e544ad150590c7cf Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Thu, 30 Jan 2025 15:31:19 +0100 Subject: [PATCH 2/4] Fix unit test Signed-off-by: Xavi Garcia --- internal/cmd/controller/gitops/reconciler/gitjob_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index d4dcf720ea..f24901d9ae 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -89,7 +89,7 @@ func getGitPollingCondition(gitrepo *fleetv1.GitRepo) (genericcondition.GenericC return genericcondition.GenericCondition{}, false } -func TestReconcile_ReturnsAndRequeuesAfterAddingFinalizer(t *testing.T) { +func TestReconcile_ReturnsAndDontRequeuesAfterAddingFinalizer(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scheme := runtime.NewScheme() @@ -137,8 +137,8 @@ func TestReconcile_ReturnsAndRequeuesAfterAddingFinalizer(t *testing.T) { if err != nil { t.Errorf("unexpected error %v", err) } - if !res.Requeue { - t.Errorf("expecting Requeue set to true, it was false") + if res.Requeue { + t.Errorf("expecting Requeue set to false, it was true") } } From 8e84d5447799f48d4e3b93d1196ec71ac2aa9e61 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Fri, 31 Jan 2025 13:51:01 +0100 Subject: [PATCH 3/4] Fixes integration tests Signed-off-by: Xavi Garcia --- .../gitjob/controller/controller_test.go | 31 +++++++++++++------ .../gitjob/controller/gitrepo_test.go | 3 ++ .../gitjob/controller/suite_test.go | 12 +++++++ 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/integrationtests/gitjob/controller/controller_test.go b/integrationtests/gitjob/controller/controller_test.go index d484820f17..2a039ef2d3 100644 --- a/integrationtests/gitjob/controller/controller_test.go +++ b/integrationtests/gitjob/controller/controller_test.go @@ -64,15 +64,9 @@ var _ = Describe("GitJob controller", func() { gitRepo.Spec.CABundle = caBundle Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) - Eventually(func() string { - var gitRepoFromCluster v1alpha1.GitRepo - err := k8sClient.Get(ctx, types.NamespacedName{Name: gitRepo.Name, Namespace: gitRepo.Namespace}, &gitRepoFromCluster) - if err != nil { - // maybe the resource gitrepo is not created yet - return "" - } - return gitRepoFromCluster.Status.Display.ReadyBundleDeployments - }).Should(ContainSubstring("0/0")) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("Creating a job") Eventually(func() error { @@ -367,6 +361,9 @@ var _ = Describe("GitJob controller", func() { expectedCommit = commit gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("creating a Job") Eventually(func() error { @@ -402,6 +399,10 @@ var _ = Describe("GitJob controller", func() { JustBeforeEach(func() { gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) + Eventually(func() error { jobName = names.SafeConcatName(gitRepoName, names.Hex(repo+commit, 5)) return k8sClient.Get(ctx, types.NamespacedName{Name: jobName, Namespace: gitRepoNamespace}, &job) @@ -517,6 +518,10 @@ var _ = Describe("GitJob controller", func() { JustBeforeEach(func() { gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) + Eventually(func() error { jobName = names.SafeConcatName(gitRepoName, names.Hex(repo+commit, 5)) return k8sClient.Get(ctx, types.NamespacedName{Name: jobName, Namespace: gitRepoNamespace}, &job) @@ -582,6 +587,9 @@ var _ = Describe("GitJob controller", func() { JustBeforeEach(func() { gitRepo = createGitRepoWithDisablePolling(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).To(Succeed()) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("Creating a job") Eventually(func() error { @@ -697,9 +705,12 @@ var _ = Describe("GitJob controller", func() { gitRepo = createGitRepo(gitRepoName) gitRepo.Spec.HelmSecretNameForPaths = helmSecretNameForPaths gitRepo.Spec.HelmSecretName = helmSecretName - // Create should return an error + // Create should not return an error err := k8sClient.Create(ctx, &gitRepo) Expect(err).ToNot(HaveOccurred()) + waitGitrepoCreated(gitRepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) }) AfterEach(func() { diff --git a/integrationtests/gitjob/controller/gitrepo_test.go b/integrationtests/gitjob/controller/gitrepo_test.go index 1da2afe99b..fc8a5266bd 100644 --- a/integrationtests/gitjob/controller/gitrepo_test.go +++ b/integrationtests/gitjob/controller/gitrepo_test.go @@ -56,6 +56,9 @@ var _ = Describe("GitRepo", func() { expectedCommit = commit err := k8sClient.Create(ctx, gitrepo) Expect(err).NotTo(HaveOccurred()) + waitGitrepoCreated(*gitrepo) + // simulate update of generation after adding finalizer (the test environment does not do it) + Expect(simulateIncreaseForceSyncGeneration(*gitrepo)).ToNot(HaveOccurred()) }) It("updates the gitrepo status", func() { diff --git a/integrationtests/gitjob/controller/suite_test.go b/integrationtests/gitjob/controller/suite_test.go index 1935e5d0b7..1a188b688c 100644 --- a/integrationtests/gitjob/controller/suite_test.go +++ b/integrationtests/gitjob/controller/suite_test.go @@ -205,6 +205,18 @@ func waitDeleteGitrepo(gitRepo v1alpha1.GitRepo) { }).Should(BeTrue()) } +func waitGitrepoCreated(gitRepo v1alpha1.GitRepo) { + Eventually(func() string { + var gitRepoFromCluster v1alpha1.GitRepo + err := k8sClient.Get(ctx, types.NamespacedName{Name: gitRepo.Name, Namespace: gitRepo.Namespace}, &gitRepoFromCluster) + if err != nil { + // maybe the resource gitrepo is not created yet + return "" + } + return gitRepoFromCluster.Status.Display.ReadyBundleDeployments + }).Should(ContainSubstring("0/0")) +} + func beOwnedBy(expected interface{}) gomegatypes.GomegaMatcher { return gcustom.MakeMatcher(func(meta metav1.ObjectMeta) (bool, error) { ref, ok := expected.(metav1.OwnerReference) From 46777d6af443d0c916e77a67e71cebccc0ff34dd Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Wed, 5 Feb 2025 17:10:03 +0100 Subject: [PATCH 4/4] Change ImageScanCommit to a pointer in the GitRepo Spec Changes ImageScanCommit to a pointer because otherwise default values are stored on the first time a `GitRepo` is updated and generation is changed, which means another call to the reconciler. This creates a double call to fetch for new commits (poll the `GitRepo`) when a `GitRepo` is added. On the first time it will poll because `LastPollingTime` in Status is not set yet. And the second time it will fetch for new commits is when generations changes from 1 to 2 (which is almost immediate after the `GitRepo` is updated to add the finalizer. Signed-off-by: Xavi Garcia --- .../gitjob/controller/controller_test.go | 12 ------------ .../gitops/reconciler/gitjob_controller.go | 3 +-- .../cmd/controller/gitops/reconciler/gitjob_test.go | 6 +++--- internal/cmd/controller/imagescan/gitcommit_job.go | 5 ++++- pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go | 2 +- .../v1alpha1/zz_generated.deepcopy.go | 6 +++++- 6 files changed, 14 insertions(+), 20 deletions(-) diff --git a/integrationtests/gitjob/controller/controller_test.go b/integrationtests/gitjob/controller/controller_test.go index 2a039ef2d3..5440096df0 100644 --- a/integrationtests/gitjob/controller/controller_test.go +++ b/integrationtests/gitjob/controller/controller_test.go @@ -65,8 +65,6 @@ var _ = Describe("GitJob controller", func() { Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("Creating a job") Eventually(func() error { @@ -362,8 +360,6 @@ var _ = Describe("GitJob controller", func() { gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("creating a Job") Eventually(func() error { @@ -400,8 +396,6 @@ var _ = Describe("GitJob controller", func() { gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) Eventually(func() error { jobName = names.SafeConcatName(gitRepoName, names.Hex(repo+commit, 5)) @@ -519,8 +513,6 @@ var _ = Describe("GitJob controller", func() { gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) Eventually(func() error { jobName = names.SafeConcatName(gitRepoName, names.Hex(repo+commit, 5)) @@ -588,8 +580,6 @@ var _ = Describe("GitJob controller", func() { gitRepo = createGitRepoWithDisablePolling(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).To(Succeed()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) By("Creating a job") Eventually(func() error { @@ -709,8 +699,6 @@ var _ = Describe("GitJob controller", func() { err := k8sClient.Create(ctx, &gitRepo) Expect(err).ToNot(HaveOccurred()) waitGitrepoCreated(gitRepo) - // simulate update of generation after adding finalizer (the test environment does not do it) - Expect(simulateIncreaseForceSyncGeneration(gitRepo)).ToNot(HaveOccurred()) }) AfterEach(func() { diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index f1cc7aa895..6113735b67 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -157,8 +157,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, err } - // requeue as adding the finalizer changes the spec - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, nil } logger = logger.WithValues("generation", gitrepo.Generation, "commit", gitrepo.Status.Commit).WithValues("conditions", gitrepo.Status.Conditions) diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index f24901d9ae..d4dcf720ea 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -89,7 +89,7 @@ func getGitPollingCondition(gitrepo *fleetv1.GitRepo) (genericcondition.GenericC return genericcondition.GenericCondition{}, false } -func TestReconcile_ReturnsAndDontRequeuesAfterAddingFinalizer(t *testing.T) { +func TestReconcile_ReturnsAndRequeuesAfterAddingFinalizer(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scheme := runtime.NewScheme() @@ -137,8 +137,8 @@ func TestReconcile_ReturnsAndDontRequeuesAfterAddingFinalizer(t *testing.T) { if err != nil { t.Errorf("unexpected error %v", err) } - if res.Requeue { - t.Errorf("expecting Requeue set to false, it was true") + if !res.Requeue { + t.Errorf("expecting Requeue set to true, it was false") } } diff --git a/internal/cmd/controller/imagescan/gitcommit_job.go b/internal/cmd/controller/imagescan/gitcommit_job.go index 6b1d07cd81..577ccc3d27 100644 --- a/internal/cmd/controller/imagescan/gitcommit_job.go +++ b/internal/cmd/controller/imagescan/gitcommit_job.go @@ -333,7 +333,10 @@ func setupKnownHosts(gitrepo *fleet.GitRepo, data []byte) error { return nil } -func commitAllAndPush(ctx context.Context, repo *gogit.Repository, auth transport.AuthMethod, commit fleet.CommitSpec) (string, error) { +func commitAllAndPush(ctx context.Context, repo *gogit.Repository, auth transport.AuthMethod, commit *fleet.CommitSpec) (string, error) { + if commit == nil { + return "", nil + } working, err := repo.Worktree() if err != nil { return "", err diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go index 76d00319c2..bc9f3c0136 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/gitrepo_types.go @@ -121,7 +121,7 @@ type GitRepoSpec struct { ImageSyncInterval *metav1.Duration `json:"imageScanInterval,omitempty"` // Commit specifies how to commit to the git repo when a new image is scanned and written back to git repo. - ImageScanCommit CommitSpec `json:"imageScanCommit,omitempty"` + ImageScanCommit *CommitSpec `json:"imageScanCommit,omitempty"` // KeepResources specifies if the resources created must be kept after deleting the GitRepo. KeepResources bool `json:"keepResources,omitempty"` diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go index 78dd5f628a..8067b8b22c 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go @@ -1510,7 +1510,11 @@ func (in *GitRepoSpec) DeepCopyInto(out *GitRepoSpec) { *out = new(v1.Duration) **out = **in } - out.ImageScanCommit = in.ImageScanCommit + if in.ImageScanCommit != nil { + in, out := &in.ImageScanCommit, &out.ImageScanCommit + *out = new(CommitSpec) + **out = **in + } if in.CorrectDrift != nil { in, out := &in.CorrectDrift, &out.CorrectDrift *out = new(CorrectDrift)