Skip to content

Commit

Permalink
Merge pull request #646 from fluxcd/positive-conditions
Browse files Browse the repository at this point in the history
  • Loading branch information
@hiddeco
hiddeco authored Mar 30, 2022
2 parents de8f32b + b869716 commit ae0b38c
Show file tree
Hide file tree
Showing 15 changed files with 554 additions and 68 deletions.
20 changes: 16 additions & 4 deletions api/v1beta2/condition_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -85,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"
)
17 changes: 11 additions & 6 deletions controllers/bucket_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -510,18 +515,18 @@ 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)
}
}()

// 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
}

Expand Down
103 changes: 98 additions & 5 deletions controllers/bucket_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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'"),
},
},
{
Expand All @@ -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'"),
},
},
{
Expand All @@ -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'"),
},
},
{
Expand All @@ -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'"),
},
},
{
Expand Down Expand Up @@ -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))
})
}
}
23 changes: 14 additions & 9 deletions controllers/gitrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -446,18 +451,18 @@ 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)
}
}()

// 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
}

Expand Down
Loading

0 comments on commit ae0b38c

Please sign in to comment.