From ed4cfb2d391ca9b41ffa604c1a7f754fab0ad505 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Thu, 23 Nov 2023 12:19:06 +0000 Subject: [PATCH] Requeue when the pipeline isn't ready If a cache has not yet synced, or a target is not to be found, and the controller has incomplete information, then the safest thing to do is not make any decisions on promotions, and instead requeue the pipeline for another try later. This fixes a problem that was exposed by adding a predicate to the watch on Pipeline: if a cluster cache isn't ready when the controller looks, and the target doesn't exist, there's nothing to trigger another go-round. This presented itself as a problem with the remote and Kustomization tests, which explicitly check for a "not found" target status. (I fixed another problem with those tests, in passing -- if they are going to check for an updated target status, they had better fetch the object again.) Signed-off-by: Michael Bridgen --- controllers/leveltriggered/controller.go | 9 ++++++++- controllers/leveltriggered/controller_remote_test.go | 1 + controllers/leveltriggered/controller_test.go | 5 ++++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/controllers/leveltriggered/controller.go b/controllers/leveltriggered/controller.go index 3a59521..9c1b7cc 100644 --- a/controllers/leveltriggered/controller.go +++ b/controllers/leveltriggered/controller.go @@ -215,6 +215,11 @@ func (r *PipelineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c pipeline.GetNamespace(), pipeline.GetName(), ) + // If it's not ready, we can't make any promotion decisions. Requeue, presuming backoff. + if unready { + return ctrl.Result{Requeue: true}, nil + } + firstEnv := pipeline.Spec.Environments[0] firstEnvStatus, ok := pipeline.Status.Environments[firstEnv.Name] @@ -470,7 +475,9 @@ func (r *PipelineReconciler) SetupWithManager(mgr ctrl.Manager) error { } return ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.Pipeline{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + For(&v1alpha1.Pipeline{}, + builder.WithPredicates(predicate.GenerationChangedPredicate{}), + ). Watches( &clusterctrlv1alpha1.GitopsCluster{}, handler.EnqueueRequestsFromMapFunc(r.requestsForCluster(gitopsClusterIndexKey)), diff --git a/controllers/leveltriggered/controller_remote_test.go b/controllers/leveltriggered/controller_remote_test.go index 26b939f..e7cf368 100644 --- a/controllers/leveltriggered/controller_remote_test.go +++ b/controllers/leveltriggered/controller_remote_test.go @@ -99,6 +99,7 @@ func TestRemoteTargets(t *testing.T) { g.Expect(getTargetStatus(g, p, "test", 0).Ready).NotTo(BeTrue()) // we can see "target cluster client not synced" before "not found" g.Eventually(func() string { + p = getPipeline(ctx, g, client.ObjectKeyFromObject(pipeline)) return getTargetStatus(g, p, "test", 0).Error }).Should(ContainSubstring("not found")) diff --git a/controllers/leveltriggered/controller_test.go b/controllers/leveltriggered/controller_test.go index 71bf55c..c529172 100644 --- a/controllers/leveltriggered/controller_test.go +++ b/controllers/leveltriggered/controller_test.go @@ -139,7 +139,10 @@ func TestReconcile(t *testing.T) { // the application hasn't been created, so we expect "not found" p := getPipeline(ctx, g, client.ObjectKeyFromObject(pipeline)) g.Expect(getTargetStatus(g, p, "test", 0).Ready).NotTo(BeTrue()) - g.Expect(getTargetStatus(g, p, "test", 0).Error).To(ContainSubstring("not found")) + g.Eventually(func() string { + p = getPipeline(ctx, g, client.ObjectKeyFromObject(pipeline)) + return getTargetStatus(g, p, "test", 0).Error + }, "2s").Should(ContainSubstring("not found")) // FIXME create the app app := kustomv1.Kustomization{