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 <darkowlzz@protonmail.com>
This commit is contained in:
Sunny 2022-03-24 02:49:56 +05:30
parent de8f32bc90
commit d939e98ec2
No known key found for this signature in database
GPG Key ID: 9F3D25DDFF7FA3CF
5 changed files with 143 additions and 28 deletions

View File

@ -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

View File

@ -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)
}
}()

View File

@ -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))
})
}
}

2
go.mod
View File

@ -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

5
go.sum
View File

@ -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=