From 752b6b6bf196626d5bea3aac10d9cb33c8f66d2a Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 28 Oct 2020 20:53:02 +0100 Subject: [PATCH 1/2] Bundle revision change predicates into one Signed-off-by: Hidde Beydals --- controllers/gitrepository_predicate.go | 63 ------------------- controllers/kustomization_controller.go | 4 +- ...ucket_predicate.go => source_predicate.go} | 18 +++--- 3 files changed, 11 insertions(+), 74 deletions(-) delete mode 100644 controllers/gitrepository_predicate.go rename controllers/{bucket_predicate.go => source_predicate.go} (64%) diff --git a/controllers/gitrepository_predicate.go b/controllers/gitrepository_predicate.go deleted file mode 100644 index a8ff0a91..00000000 --- a/controllers/gitrepository_predicate.go +++ /dev/null @@ -1,63 +0,0 @@ -/* -Copyright 2020 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controllers - -import ( - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" - - sourcev1 "github.com/fluxcd/source-controller/api/v1beta1" -) - -type GitRepositoryRevisionChangePredicate struct { - predicate.Funcs -} - -func (GitRepositoryRevisionChangePredicate) Update(e event.UpdateEvent) bool { - if e.MetaOld == nil || e.MetaNew == nil { - return false - } - - oldRepo, ok := e.ObjectOld.(*sourcev1.GitRepository) - if !ok { - return false - } - - newRepo, ok := e.ObjectNew.(*sourcev1.GitRepository) - if !ok { - return false - } - - if oldRepo.GetArtifact() == nil && newRepo.GetArtifact() != nil { - return true - } - - if oldRepo.GetArtifact() != nil && newRepo.GetArtifact() != nil && - oldRepo.GetArtifact().Revision != newRepo.GetArtifact().Revision { - return true - } - - return false -} - -func (GitRepositoryRevisionChangePredicate) Create(e event.CreateEvent) bool { - return false -} - -func (GitRepositoryRevisionChangePredicate) Delete(e event.DeleteEvent) bool { - return false -} diff --git a/controllers/kustomization_controller.go b/controllers/kustomization_controller.go index 3999758c..6a624031 100644 --- a/controllers/kustomization_controller.go +++ b/controllers/kustomization_controller.go @@ -126,12 +126,12 @@ func (r *KustomizationReconciler) SetupWithManager(mgr ctrl.Manager, opts Kustom Watches( &source.Kind{Type: &sourcev1.GitRepository{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.kustomizationsForGitRepository)}, - builder.WithPredicates(GitRepositoryRevisionChangePredicate{}), + builder.WithPredicates(SourceRevisionChangePredicate{}), ). Watches( &source.Kind{Type: &sourcev1.Bucket{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.kustomizationsForBucket)}, - builder.WithPredicates(BucketRevisionChangePredicate{}), + builder.WithPredicates(SourceRevisionChangePredicate{}), ). WithOptions(controller.Options{MaxConcurrentReconciles: opts.MaxConcurrentReconciles}). Complete(r) diff --git a/controllers/bucket_predicate.go b/controllers/source_predicate.go similarity index 64% rename from controllers/bucket_predicate.go rename to controllers/source_predicate.go index a1d922be..67dda841 100644 --- a/controllers/bucket_predicate.go +++ b/controllers/source_predicate.go @@ -23,41 +23,41 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta1" ) -type BucketRevisionChangePredicate struct { +type SourceRevisionChangePredicate struct { predicate.Funcs } -func (BucketRevisionChangePredicate) Update(e event.UpdateEvent) bool { +func (SourceRevisionChangePredicate) Update(e event.UpdateEvent) bool { if e.MetaOld == nil || e.MetaNew == nil { return false } - oldRepo, ok := e.ObjectOld.(*sourcev1.Bucket) + oldSource, ok := e.ObjectOld.(sourcev1.Source) if !ok { return false } - newRepo, ok := e.ObjectNew.(*sourcev1.Bucket) + newSource, ok := e.ObjectNew.(sourcev1.Source) if !ok { return false } - if oldRepo.GetArtifact() == nil && newRepo.GetArtifact() != nil { + if oldSource.GetArtifact() == nil && newSource.GetArtifact() != nil { return true } - if oldRepo.GetArtifact() != nil && newRepo.GetArtifact() != nil && - oldRepo.GetArtifact().Checksum != newRepo.GetArtifact().Checksum { + if oldSource.GetArtifact() != nil && newSource.GetArtifact() != nil && + oldSource.GetArtifact().Revision != newSource.GetArtifact().Revision { return true } return false } -func (BucketRevisionChangePredicate) Create(e event.CreateEvent) bool { +func (SourceRevisionChangePredicate) Create(e event.CreateEvent) bool { return false } -func (BucketRevisionChangePredicate) Delete(e event.DeleteEvent) bool { +func (SourceRevisionChangePredicate) Delete(e event.DeleteEvent) bool { return false } From 26db48b9f019239b9dea63c60fbd2372e45695e3 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 28 Oct 2020 21:45:54 +0100 Subject: [PATCH 2/2] Compare artifact <> Kustomizations in enqueuers The reason for this is the `EnqueueRequestsFromMapFunc` calling the enqueuer for _both_ the old and the new object, and we only want to act on the ones that contain a revision different from the one that we have recorded in the status object of the `Kustomization`. Signed-off-by: Hidde Beydals --- controllers/kustomization_controller.go | 46 +++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/controllers/kustomization_controller.go b/controllers/kustomization_controller.go index 6a624031..13685e10 100644 --- a/controllers/kustomization_controller.go +++ b/controllers/kustomization_controller.go @@ -125,12 +125,12 @@ func (r *KustomizationReconciler) SetupWithManager(mgr ctrl.Manager, opts Kustom For(&kustomizev1.Kustomization{}, builder.WithPredicates(predicates.ChangePredicate{})). Watches( &source.Kind{Type: &sourcev1.GitRepository{}}, - &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.kustomizationsForGitRepository)}, + &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.requestsForGitRepositoryRevisionChange)}, builder.WithPredicates(SourceRevisionChangePredicate{}), ). Watches( &source.Kind{Type: &sourcev1.Bucket{}}, - &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.kustomizationsForBucket)}, + &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.requestsForBucketRevisionChange)}, builder.WithPredicates(SourceRevisionChangePredicate{}), ). WithOptions(controller.Options{MaxConcurrentReconciles: opts.MaxConcurrentReconciles}). @@ -855,7 +855,16 @@ func (r *KustomizationReconciler) checkDependencies(kustomization kustomizev1.Ku return nil } -func (r *KustomizationReconciler) kustomizationsForGitRepository(obj handler.MapObject) []reconcile.Request { +func (r *KustomizationReconciler) requestsForGitRepositoryRevisionChange(obj handler.MapObject) []reconcile.Request { + repo, ok := obj.Object.(*sourcev1.GitRepository) + if !ok { + panic(fmt.Sprintf("Expected a GitRepository but got a %T", obj)) + } + // If we do not have an artifact, we have no requests to make + if repo.GetArtifact() == nil { + return nil + } + ctx := context.Background() var list kustomizev1.KustomizationList if err := r.List(ctx, &list, client.MatchingFields{ @@ -866,6 +875,11 @@ func (r *KustomizationReconciler) kustomizationsForGitRepository(obj handler.Map } var dd []dependency.Dependent for _, d := range list.Items { + // If the revision of the artifact equals to the last attempted revision, + // we should not make a request for this Kustomization + if repo.GetArtifact().Revision == d.Status.LastAttemptedRevision { + continue + } dd = append(dd, d) } sorted, err := dependency.Sort(dd) @@ -878,22 +892,38 @@ func (r *KustomizationReconciler) kustomizationsForGitRepository(obj handler.Map reqs[i].NamespacedName.Name = sorted[i].Name reqs[i].NamespacedName.Namespace = sorted[i].Namespace - r.Log.Info("requesting reconciliation", kustomizev1.KustomizationKind, reqs[i].NamespacedName) + r.Log.Info("requesting reconciliation due to GitRepository revision change", + strings.ToLower(kustomizev1.KustomizationKind), &reqs[i].NamespacedName, + "revision", repo.GetArtifact().Revision) } return reqs } -func (r *KustomizationReconciler) kustomizationsForBucket(obj handler.MapObject) []reconcile.Request { +func (r *KustomizationReconciler) requestsForBucketRevisionChange(obj handler.MapObject) []reconcile.Request { + bucket, ok := obj.Object.(*sourcev1.Bucket) + if !ok { + panic(fmt.Sprintf("Expected a Bucket but got a %T", obj)) + } + // If we do not have an artifact, we have no requests to make + if bucket.GetArtifact() == nil { + return nil + } + ctx := context.Background() var list kustomizev1.KustomizationList if err := r.List(ctx, &list, client.MatchingFields{ kustomizev1.BucketIndexKey: fmt.Sprintf("%s/%s", obj.Meta.GetNamespace(), obj.Meta.GetName()), }); err != nil { - r.Log.Error(err, "failed to list Kustomizations for GitRepository") + r.Log.Error(err, "failed to list Kustomizations for Bucket") return nil } var dd []dependency.Dependent for _, d := range list.Items { + // If the revision of the artifact equals to the last attempted revision, + // we should not make a request for this Kustomization + if bucket.GetArtifact().Revision == d.Status.LastAttemptedRevision { + continue + } dd = append(dd, d) } sorted, err := dependency.Sort(dd) @@ -906,7 +936,9 @@ func (r *KustomizationReconciler) kustomizationsForBucket(obj handler.MapObject) reqs[i].NamespacedName.Name = sorted[i].Name reqs[i].NamespacedName.Namespace = sorted[i].Namespace - r.Log.Info("requesting reconciliation", kustomizev1.KustomizationKind, reqs[i].NamespacedName) + r.Log.Info("requesting reconciliation due to Bucket revision change", + strings.ToLower(kustomizev1.KustomizationKind), &reqs[i].NamespacedName, + "revision", bucket.GetArtifact().Revision) } return reqs }