From d939e98ec2892263201b19cc6d03abb0f5942caa Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 24 Mar 2022 02:49:56 +0530 Subject: [PATCH 1/4] Introduce separate positive polarity conditions Introduce separate positive polarity conditions which are used to set Ready condition. Move the "artifact stored" ready condition into ArtifactInStorage positive polarity condition. If ArtifactInStorage is True and there's no negative polarity condition present, the Ready condition is summarized with ArtifactInStorage condition value. Also, update the priorities of the conditions. ArtifactInStorage has higher priority than SourceVerfied condition. If both are present, the Ready condition will have ArtifactInStorage. The negative polarity conditions are reordered to have the most likely actual cause of failure condition the highest priority, for example StorageOperationFailed, followed by the conditions that are reconciled first in the whole reconciliation so as to prioritize the first failure which may be the cause of subsequent failures. Signed-off-by: Sunny --- api/v1beta2/condition_types.go | 16 ++- controllers/gitrepository_controller.go | 21 +-- controllers/gitrepository_controller_test.go | 127 +++++++++++++++++-- go.mod | 2 +- go.sum | 5 +- 5 files changed, 143 insertions(+), 28 deletions(-) diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 647b8aa7f..4425cddf7 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -19,16 +19,24 @@ package v1beta2 const SourceFinalizer = "finalizers.fluxcd.io" const ( + // ArtifactInStorageCondition indicates the availability of the Artifact in + // the storage. + // If True, the Artifact is stored successfully. + // This Condition is only present on the resource if the Artifact is + // successfully stored. + ArtifactInStorageCondition string = "ArtifactInStorage" + // ArtifactOutdatedCondition indicates the current Artifact of the Source // is outdated. // This is a "negative polarity" or "abnormal-true" type, and is only // present on the resource if it is True. ArtifactOutdatedCondition string = "ArtifactOutdated" - // SourceVerifiedCondition indicates the integrity of the Source has been - // verified. If True, the integrity check succeeded. If False, it failed. - // The Condition is only present on the resource if the integrity has been - // verified. + // SourceVerifiedCondition indicates the integrity verification of the + // Source. + // If True, the integrity check succeeded. If False, it failed. + // This Condition is only present on the resource if the integrity check + // is enabled. SourceVerifiedCondition string = "SourceVerified" // FetchFailedCondition indicates a transient or persistent fetch failure diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 9c9189ca8..2460df32f 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -61,28 +61,30 @@ import ( var gitRepositoryReadyCondition = summarize.Conditions{ Target: meta.ReadyCondition, Owned: []string{ - sourcev1.SourceVerifiedCondition, - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, + sourcev1.SourceVerifiedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, }, Summarize: []string{ - sourcev1.IncludeUnavailableCondition, - sourcev1.SourceVerifiedCondition, - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.IncludeUnavailableCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, + sourcev1.SourceVerifiedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ + sourcev1.StorageOperationFailedCondition, sourcev1.FetchFailedCondition, sourcev1.IncludeUnavailableCondition, - sourcev1.StorageOperationFailedCondition, sourcev1.ArtifactOutdatedCondition, meta.StalledCondition, meta.ReconcilingCondition, @@ -279,11 +281,14 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + // Remove the condition as the artifact doesn't exist. + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) return sreconcile.ResultSuccess, nil } @@ -446,11 +451,11 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Create potential new artifact with current available metadata artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String())) - // Always restore the Ready condition in case it got removed due to a transient error + // Set the ArtifactInStorageCondition if there's no drift. defer func() { if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision '%s'", artifact.Revision) } }() diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 7b6aeba35..88fceb7e7 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -51,11 +51,13 @@ import ( kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" + "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/pkg/git" ) @@ -706,7 +708,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "Archiving artifact to storage makes Ready=True", + name: "Archiving artifact to storage makes ArtifactInStorage=True", dir: "testdata/git/repository", beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} @@ -717,11 +719,11 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { - name: "Archiving artifact to storage with includes makes Ready=True", + name: "Archiving artifact to storage with includes makes ArtifactInStorage=True", dir: "testdata/git/repository", includes: artifactSet{&sourcev1.Artifact{Revision: "main/revision"}}, beforeFunc: func(obj *sourcev1.GitRepository) { @@ -735,7 +737,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -752,7 +754,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -768,7 +770,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -783,7 +785,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -800,7 +802,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -820,7 +822,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"), }, }, { @@ -1171,7 +1173,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { }, }, { - name: "Invalid commit makes SourceVerifiedCondition=False and returns error", + name: "Invalid commit sets no SourceVerifiedCondition and returns error", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "existing", @@ -1197,7 +1199,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { }, }, { - name: "Secret get failure makes SourceVerified=False and returns error", + name: "Secret get failure sets no SourceVerifiedCondition and returns error", beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} obj.Spec.Verification = &sourcev1.GitRepositoryVerification{ @@ -1289,10 +1291,11 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "no condition", + name: "no failure condition", want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, { @@ -1303,6 +1306,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, { @@ -1313,6 +1317,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, { @@ -1326,6 +1331,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, { @@ -1337,6 +1343,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, { @@ -1348,6 +1355,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { want: ctrl.Result{RequeueAfter: interval}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, "Succeeded", "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, "Succeeded", "stored artifact for revision"), }, }, } @@ -1531,3 +1539,98 @@ func remoteTagForHead(repo *gogit.Repository, head *plumbing.Reference, tag stri RefSpecs: []config.RefSpec{config.RefSpec(refSpec)}, }) } + +func TestGitRepositoryReconciler_statusConditions(t *testing.T) { + tests := []struct { + name string + beforeFunc func(obj *sourcev1.GitRepository) + assertConditions []metav1.Condition + }{ + { + name: "multiple positive conditions", + beforeFunc: func(obj *sourcev1.GitRepository) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of commit") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of commit"), + }, + }, + { + name: "multiple failures", + beforeFunc: func(obj *sourcev1.GitRepository) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "IllegalPath", "some error") + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "IllegalPath", "some error"), + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"), + }, + }, + { + name: "mixed positive and negative conditions", + beforeFunc: func(obj *sourcev1.GitRepository) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.GitRepository{ + TypeMeta: metav1.TypeMeta{ + Kind: sourcev1.GitRepositoryKind, + APIVersion: "source.toolkit.fluxcd.io/v1beta2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "gitrepo", + Namespace: "foo", + }, + } + clientBuilder := fake.NewClientBuilder() + clientBuilder.WithObjects(obj) + c := clientBuilder.Build() + + patchHelper, err := patch.NewHelper(obj, c) + g.Expect(err).ToNot(HaveOccurred()) + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + ctx := context.TODO() + recResult := sreconcile.ResultSuccess + var retErr error + + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeOpts := []summarize.Option{ + summarize.WithConditions(gitRepositoryReadyCondition), + summarize.WithReconcileResult(recResult), + summarize.WithReconcileError(retErr), + summarize.WithIgnoreNotFound(), + summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetRequeueAfter()}), + summarize.WithPatchFieldOwner("source-controller"), + } + _, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) + + key := client.ObjectKeyFromObject(obj) + g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +} diff --git a/go.mod b/go.mod index d49ff728b..0ee8fafc4 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,7 @@ require ( github.com/fluxcd/pkg/gitutil v0.1.0 github.com/fluxcd/pkg/helmtestserver v0.7.1 github.com/fluxcd/pkg/lockedfile v0.1.0 - github.com/fluxcd/pkg/runtime v0.13.2 + github.com/fluxcd/pkg/runtime v0.13.4 github.com/fluxcd/pkg/ssh v0.3.2 github.com/fluxcd/pkg/testserver v0.2.0 github.com/fluxcd/pkg/untar v0.1.0 diff --git a/go.sum b/go.sum index b08cc4fd0..4343a6581 100644 --- a/go.sum +++ b/go.sum @@ -365,8 +365,8 @@ github.com/fluxcd/pkg/helmtestserver v0.7.1/go.mod h1:ULIZt2ozO36FLfvjABUwHJn5Ex github.com/fluxcd/pkg/lockedfile v0.1.0 h1:YsYFAkd6wawMCcD74ikadAKXA4s2sukdxrn7w8RB5eo= github.com/fluxcd/pkg/lockedfile v0.1.0/go.mod h1:EJLan8t9MiOcgTs8+puDjbE6I/KAfHbdvIy9VUgIjm8= github.com/fluxcd/pkg/runtime v0.13.0-rc.6/go.mod h1:4oKUO19TeudXrnCRnxCfMSS7EQTYpYlgfXwlQuDJ/Eg= -github.com/fluxcd/pkg/runtime v0.13.2 h1:6jkQQUbp17WxHsbozlJFCvHmOS4JIB+yB20CdCd8duE= -github.com/fluxcd/pkg/runtime v0.13.2/go.mod h1:dzWNKqFzFXeittbpFcJzR3cdC9CWlbzw+pNOgaVvF/0= +github.com/fluxcd/pkg/runtime v0.13.4 h1:RJSO+jmAlr6aF5Mia7zZTUrysoRjFSjjuuSTbFURbxg= +github.com/fluxcd/pkg/runtime v0.13.4/go.mod h1:dzWNKqFzFXeittbpFcJzR3cdC9CWlbzw+pNOgaVvF/0= github.com/fluxcd/pkg/ssh v0.3.2 h1:HZlDF6Qu4yplsU4Tisv6hxsRIbIOwwr7rKus8/Q/Dn0= github.com/fluxcd/pkg/ssh v0.3.2/go.mod h1:OVnuv9y2WCx7AoOIid0sxqe9lLKKfDS4PMl+4ta5DIo= github.com/fluxcd/pkg/testserver v0.2.0 h1:Mj0TapmKaywI6Fi5wvt1LAZpakUHmtzWQpJNKQ0Krt4= @@ -1151,7 +1151,6 @@ golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.0.0-20210817164053-32db794688a5/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.0.0-20211117183948-ae814b36b871/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.0.0-20220214200702-86341886e292/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= -golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd h1:XcWmESyNjXJMLahc3mqVQJcgSTDxFxhETVlfk9uGc38= golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.0.0-20220321153916-2c7772ba3064 h1:S25/rfnfsMVgORT4/J61MJ7rdyseOZOyvLIrZEZ7s6s= golang.org/x/crypto v0.0.0-20220321153916-2c7772ba3064/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= From 86860ec913a786e5662651b16e3786825d3649b4 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 29 Mar 2022 22:33:05 +0530 Subject: [PATCH 2/4] Update all reconcilers with ArtifactInStorage cond Update alll the other reconcilers similar to the GitRepository reconcilers to introduce positive condition ArtifactInStorage and reorder the status conditions. Signed-off-by: Sunny --- controllers/bucket_controller.go | 15 ++- controllers/bucket_controller_test.go | 103 ++++++++++++++++- controllers/helmchart_controller.go | 21 ++-- controllers/helmchart_controller_test.go | 104 +++++++++++++++++- controllers/helmrepository_controller.go | 15 ++- controllers/helmrepository_controller_test.go | 103 ++++++++++++++++- 6 files changed, 328 insertions(+), 33 deletions(-) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 84a4e38c5..3df1bcc82 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -74,23 +74,25 @@ const maxConcurrentBucketFetches = 100 var bucketReadyCondition = summarize.Conditions{ Target: meta.ReadyCondition, Owned: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, }, Summarize: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, meta.StalledCondition, meta.ReconcilingCondition, @@ -375,11 +377,14 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.B if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + // Remove the condition as the artifact doesn't exist. + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) return sreconcile.ResultSuccess, nil } @@ -510,11 +515,11 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. // Create artifact artifact := r.Storage.NewArtifactFor(obj.Kind, obj, revision, fmt.Sprintf("%s.tar.gz", revision)) - // Always restore the Ready condition in case it got removed due to a transient error + // Set the ArtifactInStorageCondition if there's no drift. defer func() { if obj.GetArtifact().HasRevision(artifact.Revision) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision '%s'", artifact.Revision) } }() diff --git a/controllers/bucket_controller_test.go b/controllers/bucket_controller_test.go index 0732f1f2b..2f432a4bb 100644 --- a/controllers/bucket_controller_test.go +++ b/controllers/bucket_controller_test.go @@ -38,12 +38,14 @@ import ( "k8s.io/client-go/tools/record" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" gcsmock "github.com/fluxcd/source-controller/internal/mock/gcs" s3mock "github.com/fluxcd/source-controller/internal/mock/s3" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" + "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) // Environment variable to set the GCP Storage host for the GCP client. @@ -886,13 +888,13 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "Archiving artifact to storage makes Ready=True", + name: "Archiving artifact to storage makes ArtifactInStorage=True", beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), }, }, { @@ -909,7 +911,7 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), }, }, { @@ -920,7 +922,7 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), }, }, { @@ -937,7 +939,7 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"), }, }, { @@ -1070,3 +1072,94 @@ func Test_etagIndex_Revision(t *testing.T) { }) } } + +func TestBucketReconciler_statusConditions(t *testing.T) { + tests := []struct { + name string + beforeFunc func(obj *sourcev1.Bucket) + assertConditions []metav1.Condition + }{ + { + name: "positive conditions only", + beforeFunc: func(obj *sourcev1.Bucket) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + { + name: "multiple failures", + beforeFunc: func(obj *sourcev1.Bucket) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"), + }, + }, + { + name: "mixed positive and negative conditions", + beforeFunc: func(obj *sourcev1.Bucket) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.Bucket{ + TypeMeta: metav1.TypeMeta{ + Kind: sourcev1.BucketKind, + APIVersion: "source.toolkit.fluxcd.io/v1beta2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "bucket", + Namespace: "foo", + }, + } + clientBuilder := fake.NewClientBuilder() + clientBuilder.WithObjects(obj) + c := clientBuilder.Build() + + patchHelper, err := patch.NewHelper(obj, c) + g.Expect(err).ToNot(HaveOccurred()) + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + ctx := context.TODO() + recResult := sreconcile.ResultSuccess + var retErr error + + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeOpts := []summarize.Option{ + summarize.WithConditions(bucketReadyCondition), + summarize.WithReconcileResult(recResult), + summarize.WithReconcileError(retErr), + summarize.WithIgnoreNotFound(), + summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetRequeueAfter()}), + summarize.WithPatchFieldOwner("source-controller"), + } + _, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) + + key := client.ObjectKeyFromObject(obj) + g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +} diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 3fa0c0271..951e37c3c 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -69,26 +69,28 @@ import ( var helmChartReadyCondition = summarize.Conditions{ Target: meta.ReadyCondition, Owned: []string{ - sourcev1.BuildFailedCondition, - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.BuildFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, }, Summarize: []string{ - sourcev1.BuildFailedCondition, - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.BuildFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ - sourcev1.BuildFailedCondition, - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, + sourcev1.BuildFailedCondition, sourcev1.ArtifactOutdatedCondition, meta.StalledCondition, meta.ReconcilingCondition, @@ -284,11 +286,14 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + // Remove the condition as the artifact doesn't exist. + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) return sreconcile.ResultSuccess, nil } @@ -620,11 +625,11 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source return sreconcile.ResultRequeue, nil } - // Always restore the conditions in case they got overwritten by transient errors + // Set the ArtifactInStorageCondition if there's no drift. defer func() { if obj.Status.ObservedChartName == b.Name && obj.GetArtifact().HasRevision(b.Version) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) - conditions.MarkTrue(obj, meta.ReadyCondition, reasonForBuild(b), b.Summary()) + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, reasonForBuild(b), b.Summary()) } }() diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 43d568b85..522908c32 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -50,6 +50,7 @@ import ( serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" + "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) func TestHelmChartReconciler_Reconcile(t *testing.T) { @@ -981,7 +982,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, }, { - name: "Copying artifact to storage from build makes Ready=True", + name: "Copying artifact to storage from build makes ArtifactInStorage=True", build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), beforeFunc: func(obj *sourcev1.HelmChart) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") @@ -995,7 +996,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -1038,7 +1039,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { t.Expect(obj.Status.URL).To(BeEmpty()) }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPackageSucceededReason, "packaged 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, sourcev1.ChartPackageSucceededReason, "packaged 'helmchart' chart with version '0.1.0'"), }, }, { @@ -1056,7 +1057,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), }, }, { @@ -1073,7 +1074,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), }, }, } @@ -1483,3 +1484,96 @@ func mockChartBuild(name, version, path string) *chart.Build { Path: copyP, } } + +func TestHelmChartReconciler_statusConditions(t *testing.T) { + tests := []struct { + name string + beforeFunc func(obj *sourcev1.HelmChart) + assertConditions []metav1.Condition + }{ + { + name: "positive conditions only", + beforeFunc: func(obj *sourcev1.HelmChart) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + { + name: "multiple failures", + beforeFunc: func(obj *sourcev1.HelmChart) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory") + conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, "ChartPackageError", "some error") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartPackageError", "some error"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"), + }, + }, + { + name: "mixed positive and negative conditions", + beforeFunc: func(obj *sourcev1.HelmChart) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.HelmChart{ + TypeMeta: metav1.TypeMeta{ + Kind: sourcev1.HelmChartKind, + APIVersion: "source.toolkit.fluxcd.io/v1beta2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "helmchart", + Namespace: "foo", + }, + } + clientBuilder := fake.NewClientBuilder() + clientBuilder.WithObjects(obj) + c := clientBuilder.Build() + + patchHelper, err := patch.NewHelper(obj, c) + g.Expect(err).ToNot(HaveOccurred()) + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + ctx := context.TODO() + recResult := sreconcile.ResultSuccess + var retErr error + + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeOpts := []summarize.Option{ + summarize.WithConditions(helmChartReadyCondition), + summarize.WithReconcileResult(recResult), + summarize.WithReconcileError(retErr), + summarize.WithIgnoreNotFound(), + summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetRequeueAfter()}), + summarize.WithPatchFieldOwner("source-controller"), + } + _, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) + + key := client.ObjectKeyFromObject(obj) + g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +} diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 618cd35a6..4d291d273 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -57,23 +57,25 @@ import ( var helmRepositoryReadyCondition = summarize.Conditions{ Target: meta.ReadyCondition, Owned: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, }, Summarize: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, + sourcev1.ArtifactInStorageCondition, meta.StalledCondition, meta.ReconcilingCondition, }, NegativePolarity: []string{ - sourcev1.FetchFailedCondition, sourcev1.StorageOperationFailedCondition, + sourcev1.FetchFailedCondition, sourcev1.ArtifactOutdatedCondition, meta.StalledCondition, meta.ReconcilingCondition, @@ -245,11 +247,14 @@ func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *so if artifact := obj.GetArtifact(); artifact != nil && !r.Storage.ArtifactExist(*artifact) { obj.Status.Artifact = nil obj.Status.URL = "" + // Remove the condition as the artifact doesn't exist. + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) } // Record that we do not have an artifact if obj.GetArtifact() == nil { conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage") + conditions.Delete(obj, sourcev1.ArtifactInStorageCondition) return sreconcile.ResultSuccess, nil } @@ -392,11 +397,11 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, obj *sou // On a successful archive, the Artifact in the Status of the object is set, // and the symlink in the Storage is updated to its path. func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) { - // Always restore the Ready condition in case it got removed due to a transient error. + // Set the ArtifactInStorageCondition if there's no drift. defer func() { if obj.GetArtifact().HasRevision(artifact.Revision) { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision '%s'", artifact.Revision) } diff --git a/controllers/helmrepository_controller_test.go b/controllers/helmrepository_controller_test.go index 83cd57bb2..95b770915 100644 --- a/controllers/helmrepository_controller_test.go +++ b/controllers/helmrepository_controller_test.go @@ -37,11 +37,13 @@ import ( "k8s.io/client-go/tools/record" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/helm/repository" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" + "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) func TestHelmRepositoryReconciler_Reconcile(t *testing.T) { @@ -515,13 +517,13 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { assertConditions []metav1.Condition }{ { - name: "Archiving artifact to storage makes Ready=True", + name: "Archiving artifact to storage makes ArtifactInStorage=True", beforeFunc: func(t *WithT, obj *sourcev1.HelmRepository, artifact sourcev1.Artifact, index *repository.ChartRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), }, }, { @@ -535,7 +537,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), }, }, { @@ -546,7 +548,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), }, }, { @@ -563,7 +565,7 @@ func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { }, want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'existing'"), }, }, } @@ -744,3 +746,94 @@ func TestHelmRepositoryReconciler_reconcileSubRecs(t *testing.T) { }) } } + +func TestHelmRepositoryReconciler_statusConditions(t *testing.T) { + tests := []struct { + name string + beforeFunc func(obj *sourcev1.HelmRepository) + assertConditions []metav1.Condition + }{ + { + name: "positive conditions only", + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "stored artifact for revision"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + { + name: "multiple failures", + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"), + }, + }, + { + name: "mixed positive and negative conditions", + beforeFunc: func(obj *sourcev1.HelmRepository) { + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision") + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret") + }, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"), + *conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.HelmRepository{ + TypeMeta: metav1.TypeMeta{ + Kind: sourcev1.HelmRepositoryKind, + APIVersion: "source.toolkit.fluxcd.io/v1beta2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "helmrepo", + Namespace: "foo", + }, + } + clientBuilder := fake.NewClientBuilder() + clientBuilder.WithObjects(obj) + c := clientBuilder.Build() + + patchHelper, err := patch.NewHelper(obj, c) + g.Expect(err).ToNot(HaveOccurred()) + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + ctx := context.TODO() + recResult := sreconcile.ResultSuccess + var retErr error + + summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), patchHelper) + summarizeOpts := []summarize.Option{ + summarize.WithConditions(helmRepositoryReadyCondition), + summarize.WithReconcileResult(recResult), + summarize.WithReconcileError(retErr), + summarize.WithIgnoreNotFound(), + summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetRequeueAfter()}), + summarize.WithPatchFieldOwner("source-controller"), + } + _, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) + + key := client.ObjectKeyFromObject(obj) + g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +} From b41c717e167a69037f8b6bd7cc690d66e4cbfb25 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 30 Mar 2022 01:39:51 +0530 Subject: [PATCH 3/4] controllers: emit event and log source up-to-date Signed-off-by: Sunny --- api/v1beta2/condition_types.go | 4 ++++ controllers/bucket_controller.go | 2 +- controllers/gitrepository_controller.go | 2 +- controllers/helmchart_controller.go | 2 +- controllers/helmrepository_controller.go | 2 +- 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 4425cddf7..c35c2c528 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -93,4 +93,8 @@ const ( // SymlinkUpdateFailedReason signals a failure in updating a symlink. SymlinkUpdateFailedReason string = "SymlinkUpdateFailed" + + // ArtifactUpToDateReason signals that an existing Artifact is up-to-date + // with the Source. + ArtifactUpToDateReason string = "ArtifactUpToDate" ) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 3df1bcc82..b01236828 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -526,7 +526,7 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1. // The artifact is up-to-date if obj.GetArtifact().HasRevision(artifact.Revision) { - ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision) + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 2460df32f..2aa0f8589 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -462,7 +462,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // The artifact is up-to-date if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) { - ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision) + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 951e37c3c..332e0ca4a 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -638,7 +638,7 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source // Return early if the build path equals the current artifact path if curArtifact := obj.GetArtifact(); curArtifact != nil && r.Storage.LocalPath(*curArtifact) == b.Path { - ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision) + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 4d291d273..cbad94102 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -411,7 +411,7 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s }() if obj.GetArtifact().HasRevision(artifact.Revision) { - ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision) + r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } From b869716ddf032d15e552ed18b35e19f4c68f1c04 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 30 Mar 2022 02:35:39 +0530 Subject: [PATCH 4/4] Update docs with new conditions and events Signed-off-by: Sunny --- docs/spec/v1beta2/buckets.md | 18 ++++++++++++++++++ docs/spec/v1beta2/gitrepositories.md | 18 ++++++++++++++++++ docs/spec/v1beta2/helmcharts.md | 18 ++++++++++++++++++ docs/spec/v1beta2/helmrepositories.md | 24 +++++++++++++++++++++--- 4 files changed, 75 insertions(+), 3 deletions(-) diff --git a/docs/spec/v1beta2/buckets.md b/docs/spec/v1beta2/buckets.md index 7fc630989..196c9d617 100644 --- a/docs/spec/v1beta2/buckets.md +++ b/docs/spec/v1beta2/buckets.md @@ -93,6 +93,12 @@ control over. Reason: Succeeded Status: True Type: Ready + Last Transition Time: 2022-02-01T23:43:38Z + Message: stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' + Observed Generation: 1 + Reason: Succeeded + Status: True + Type: ArtifactInStorage Observed Generation: 1 URL: http://source-controller.source-system.svc.cluster.local./bucket/default/minio-bucket/latest.tar.gz Events: @@ -780,6 +786,7 @@ lists ```console LAST SEEN TYPE REASON OBJECT MESSAGE 2m30s Normal NewArtifact bucket/ fetched 16 files with revision from 'my-new-bucket' +36s Normal ArtifactUpToDate bucket/ artifact up-to-date with remote revision: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' 18s Warning BucketOperationFailed bucket/ bucket 'my-new-bucket' does not exist ``` @@ -892,6 +899,17 @@ This `Ready` Condition will retain a status value of `"True"` until the Bucket is marked as [reconciling](#reconciling-bucket), or e.g. a [transient error](#failed-bucket) occurs due to a temporary network issue. +When the Bucket Artifact is archived in the controller's Artifact +storage, the controller sets a Condition with the following attributes in the +Bucket's `.status.conditions`: + +- `type: ArtifactInStorage` +- `status: "True"` +- `reason: Succeeded` + +This `ArtifactInStorage` Condition will retain a status value of `"True"` until +the Artifact in the storage no longer exists. + #### Failed Bucket The source-controller may get stuck trying to produce an Artifact for a Bucket diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 720be7fe0..e922eb131 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -71,6 +71,12 @@ You can run this example by saving the manifest into `gitrepository.yaml`. Reason: Succeeded Status: True Type: Ready + Last Transition Time: 2022-02-14T11:23:36Z + Message: stored artifact for revision 'master/132f4e719209eb10b9485302f8593fc0e680f4fc' + Observed Generation: 1 + Reason: Succeeded + Status: True + Type: ArtifactInStorage Observed Generation: 1 URL: http://source-controller.source-system.svc.cluster.local./gitrepository/default/podinfo/latest.tar.gz Events: @@ -647,6 +653,7 @@ lists ```console LAST SEEN TYPE REASON OBJECT MESSAGE 2m14s Normal NewArtifact gitrepository/ stored artifact for commit 'Merge pull request #160 from stefanprodan/release-6.0.3' +36s Normal ArtifactUpToDate gitrepository/ artifact up-to-date with remote revision: 'master/132f4e719209eb10b9485302f8593fc0e680f4fc' 94s Warning GitOperationFailed gitrepository/ failed to checkout and determine revision: unable to clone 'https://github.com/stefanprodan/podinfo': couldn't find remote ref "refs/heads/invalid" ``` @@ -760,6 +767,17 @@ This `Ready` Condition will retain a status value of `"True"` until the GitRepository is marked as [reconciling](#reconciling-gitrepository), or e.g. a [transient error](#failed-gitrepository) occurs due to a temporary network issue. +When the GitRepository Artifact is archived in the controller's Artifact +storage, the controller sets a Condition with the following attributes in the +GitRepository's `.status.conditions`: + +- `type: ArtifactInStorage` +- `status: "True"` +- `reason: Succeeded` + +This `ArtifactInStorage` Condition will retain a status value of `"True"` until +the Artifact in the storage no longer exists. + #### Failed GitRepository The source-controller may get stuck trying to produce an Artifact for a diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index b3f118ab6..8f8f9800a 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -79,6 +79,12 @@ helm-controller. Reason: ChartPullSucceeded Status: True Type: Ready + Last Transition Time: 2022-02-13T11:24:10Z + Message: pulled 'podinfo' chart with version '5.2.1' + Observed Generation: 1 + Reason: ChartPullSucceeded + Status: True + Type: ArtifactInStorage Observed Chart Name: podinfo Observed Generation: 1 URL: http://source-controller.flux-system.svc.cluster.local./helmchart/default/podinfo/latest.tar.gz @@ -377,6 +383,7 @@ lists LAST SEEN TYPE REASON OBJECT MESSAGE 22s Warning InvalidChartReference helmchart/ invalid chart reference: failed to get chart version for remote reference: no 'podinfo' chart with version matching '9.*' found 2s Normal ChartPullSucceeded helmchart/ pulled 'podinfo' chart with version '6.0.3' +2s Normal ArtifactUpToDate helmchart/ artifact up-to-date with remote revision: '6.0.3' ``` Besides being reported in Events, the reconciliation errors are also logged by @@ -522,6 +529,17 @@ This `Ready` Condition will retain a status value of `"True"` until the HelmChart is marked as [reconciling](#reconciling-helmchart), or e.g. a [transient error](#failed-helmchart) occurs due to a temporary network issue. +When the HelmChart Artifact is archived in the controller's Artifact +storage, the controller sets a Condition with the following attributes in the +HelmChart's `.status.conditions`: + +- `type: ArtifactInStorage` +- `status: "True"` +- `reason: Succeeded` + +This `ArtifactInStorage` Condition will retain a status value of `"True"` until +the Artifact in the storage no longer exists. + #### Failed HelmChart The source-controller may get stuck trying to produce an Artifact for a diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index b3ef08f66..f4dd41dfd 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -69,6 +69,12 @@ You can run this example by saving the manifest into `helmrepository.yaml`. Reason: Succeeded Status: True Type: Ready + Last Transition Time: 2022-02-04T09:55:58Z + Message: stored artifact for revision '83a3c595163a6ff0333e0154c790383b5be441b9db632cb36da11db1c4ece111' + Observed Generation: 1 + Reason: Succeeded + Status: True + Type: ArtifactInStorage Observed Generation: 1 URL: http://source-controller.flux-system.svc.cluster.local./helmrepository/default/podinfo/index.yaml Events: @@ -359,9 +365,10 @@ kubectl get events --field-selector involvedObject.kind=HelmRepository,involvedO lists ```console -LAST SEEN TYPE REASON OBJECT MESSAGE -107s Warning Failed helmrepository/ failed to construct Helm client: scheme "invalid" not supported -7s Normal NewArtifact helmrepository/ fetched index of size 30.88kB from 'https://stefanprodan.github.io/podinfo' +LAST SEEN TYPE REASON OBJECT MESSAGE +107s Warning Failed helmrepository/ failed to construct Helm client: scheme "invalid" not supported +7s Normal NewArtifact helmrepository/ fetched index of size 30.88kB from 'https://stefanprodan.github.io/podinfo' +3s Normal ArtifactUpToDate helmrepository/ artifact up-to-date with remote revision: '83a3c595163a6ff0333e0154c790383b5be441b9db632cb36da11db1c4ece111' ``` Besides being reported in Events, the reconciliation errors are also logged by @@ -464,6 +471,17 @@ HelmRepository is marked as [reconciling](#reconciling-helmrepository), or e.g. a [transient error](#failed-helmrepository) occurs due to a temporary network issue. +When the HelmRepository Artifact is archived in the controller's Artifact +storage, the controller sets a Condition with the following attributes in the +HelmRepository's `.status.conditions`: + +- `type: ArtifactInStorage` +- `status: "True"` +- `reason: Succeeded` + +This `ArtifactInStorage` Condition will retain a status value of `"True"` until +the Artifact in the storage no longer exists. + #### Failed HelmRepository The source-controller may get stuck trying to produce an Artifact for a