Skip to content

Commit

Permalink
summarize: Consider obj status condition in result
Browse files Browse the repository at this point in the history 
SummarizeAndPatch() should also consider the object's status conditions
when computing and returning the runtime results to avoid any
inconsistency in the runtime result and status condition of the object.
When an object's Ready condition is False, the reconciler should retry
unless it's in stalled condition.

Signed-off-by: Sunny <[email protected]>
  • Loading branch information
@darkowlzz
darkowlzz committed Apr 30, 2022
1 parent 89a4e52 commit d64def3
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 31 deletions.
11 changes: 11 additions & 0 deletions internal/reconcile/reconcile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@ const (
// can be implemented to build custom results based on the context of the
// reconciler.
type RuntimeResultBuilder interface {
// BuildRuntimeResult analyzes the result and error to return a runtime
// result.
BuildRuntimeResult(rr Result, err error) ctrl.Result
// IsSuccess returns if a given runtime result is success for a
// RuntimeResultBuilder.
IsSuccess(ctrl.Result) bool
}

// AlwaysRequeueResultBuilder implements a RuntimeResultBuilder for always
Expand Down Expand Up @@ -82,6 +87,12 @@ func (r AlwaysRequeueResultBuilder) BuildRuntimeResult(rr Result, err error) ctr
}
}

// IsSuccess returns true if the given Result has the same RequeueAfter value
// as of the AlwaysRequeueResultBuilder.
func (r AlwaysRequeueResultBuilder) IsSuccess(result ctrl.Result) bool {
return result.RequeueAfter == r.RequeueAfter
}

// ComputeReconcileResult analyzes the reconcile results (result + error),
// updates the status conditions of the object with any corrections and returns
// object patch configuration, runtime result and runtime error. The caller is
Expand Down
53 changes: 43 additions & 10 deletions internal/reconcile/reconcile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,6 @@ func TestComputeReconcileResult(t *testing.T) {
t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeFalse())
},
},
{
name: "requeue result",
result: ResultRequeue,
recErr: nil,
wantResult: ctrl.Result{Requeue: true},
wantErr: false,
afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) {
t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeFalse())
},
},
{
name: "stalling error",
result: ResultEmpty,
Expand Down Expand Up @@ -203,6 +193,49 @@ func TestComputeReconcileResult(t *testing.T) {
}
}

func TestAlwaysRequeueResultBuilder_IsSuccess(t *testing.T) {
interval := 5 * time.Second

tests := []struct {
name string
resultBuilder AlwaysRequeueResultBuilder
runtimeResult ctrl.Result
result bool
}{
{
name: "success result",
resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval},
runtimeResult: ctrl.Result{RequeueAfter: interval},
result: true,
},
{
name: "requeue result",
resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval},
runtimeResult: ctrl.Result{Requeue: true},
result: false,
},
{
name: "zero result",
resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval},
runtimeResult: ctrl.Result{},
result: false,
},
{
name: "different requeue after",
resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval},
runtimeResult: ctrl.Result{RequeueAfter: time.Second},
result: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
g.Expect(tt.resultBuilder.IsSuccess(tt.runtimeResult)).To(Equal(tt.result))
})
}
}

func TestFailureRecovery(t *testing.T) {
failCondns := []string{
"FooFailed",
Expand Down
27 changes: 27 additions & 0 deletions internal/reconcile/summarize/summary.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ package summarize

import (
"context"
"errors"

apierrors "k8s.io/apimachinery/pkg/api/errors"
kerrors "k8s.io/apimachinery/pkg/util/errors"
kuberecorder "k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"

"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/runtime/conditions"
"github.com/fluxcd/pkg/runtime/patch"

Expand Down Expand Up @@ -204,6 +206,18 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o
)
}

// If object is not stalled, result is success and runtime error is nil,
// ensure that Ready=True. Else, use the Ready failure message as the
// runtime error message. This ensures that the reconciliation would be
// retried as the object isn't ready.
// NOTE: This is applicable to Ready condition only because it is a special
// condition in kstatus that reflects the overall state of an object.
if isNonStalledSuccess(obj, opts.ResultBuilder, result, recErr) {
if !conditions.IsReady(obj) {
recErr = errors.New(conditions.GetMessage(obj, meta.ReadyCondition))
}
}

// Finally, patch the resource.
if err := h.patchHelper.Patch(ctx, obj, patchOpts...); err != nil {
// Ignore patch error "not found" when the object is being deleted.
Expand All @@ -215,3 +229,16 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o

return result, recErr
}

// isNonStalledSuccess checks if the reconciliation was successful and has not
// resulted in stalled situation.
func isNonStalledSuccess(obj conditions.Setter, rb reconcile.RuntimeResultBuilder, result ctrl.Result, recErr error) bool {
if !conditions.IsStalled(obj) && recErr == nil {
// Without result builder, it can't be determined if the result is
// success.
if rb != nil {
return rb.IsSuccess(result)
}
}
return false
}
122 changes: 101 additions & 21 deletions internal/reconcile/summarize/summary_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package summarize

import (
"context"
"errors"
"fmt"
"testing"
"time"
Expand All @@ -27,6 +28,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"

Expand Down Expand Up @@ -91,18 +93,19 @@ func TestSummarizeAndPatch(t *testing.T) {
afterFunc func(t *WithT, obj client.Object)
assertConditions []metav1.Condition
}{
// Success/Fail indicates if a reconciliation succeeded or failed. On
// a successful reconciliation, the object generation is expected to
// match the observed generation in the object status.
// Success/Fail indicates if a reconciliation succeeded or failed.
// The object generation is expected to match the observed generation in
// the object status if Ready=True or Stalled=True at the end.
// All the cases have some Ready condition set, even if a test case is
// unrelated to the conditions, because it's neseccary for a valid
// status.
{
name: "Success, no extra conditions",
name: "Success, Ready=True",
generation: 4,
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
},
result: reconcile.ResultSuccess,
conditions: []Conditions{testReadyConditions},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"),
Expand All @@ -111,20 +114,6 @@ func TestSummarizeAndPatch(t *testing.T) {
t.Expect(obj).To(HaveStatusObservedGeneration(4))
},
},
{
name: "Success, Ready=True",
generation: 5,
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "created")
},
conditions: []Conditions{testReadyConditions},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "created"),
},
afterFunc: func(t *WithT, obj client.Object) {
t.Expect(obj).To(HaveStatusObservedGeneration(5))
},
},
{
name: "Success, removes reconciling for successful result",
generation: 2,
Expand Down Expand Up @@ -216,7 +205,22 @@ func TestSummarizeAndPatch(t *testing.T) {
},
},
{
name: "Success, multiple conditions summary",
name: "Success, multiple target conditions summary",
generation: 3,
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
conditions.MarkTrue(obj, "AAA", "ZZZ", "zzz") // Positive polarity True.
},
conditions: []Conditions{testReadyConditions, testFooConditions},
result: reconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"),
*conditions.TrueCondition("Foo", "ZZZ", "zzz"), // True summary.
*conditions.TrueCondition("AAA", "ZZZ", "zzz"),
},
},
{
name: "Success, multiple target conditions, False non-Ready summary don't affect result",
generation: 3,
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
Expand All @@ -232,6 +236,20 @@ func TestSummarizeAndPatch(t *testing.T) {
*conditions.TrueCondition("AAA", "ZZZ", "zzz"),
},
},
{
name: "Fail, success result but Ready=False",
generation: 3,
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision")
},
conditions: []Conditions{testReadyConditions},
result: reconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"),
*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"),
},
wantErr: true,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -291,6 +309,8 @@ func TestSummarizeAndPatch(t *testing.T) {
// This tests the scenario where SummarizeAndPatch is used in the middle of
// reconciliation.
func TestSummarizeAndPatch_Intermediate(t *testing.T) {
interval := 5 * time.Second

var testStageAConditions = Conditions{
Target: "StageA",
Owned: []string{"StageA", "A1", "A2", "A3"},
Expand Down Expand Up @@ -335,7 +355,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) {
},
},
{
name: "multiple Conditions",
name: "multiple Conditions, mixed results",
conditions: []Conditions{testStageAConditions, testStageBConditions},
beforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, "A3", "ZZZ", "zzz") // Negative polarity True.
Expand Down Expand Up @@ -365,7 +385,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) {
GenerateName: "test-",
},
Spec: sourcev1.GitRepositorySpec{
Interval: metav1.Duration{Duration: 5 * time.Second},
Interval: metav1.Duration{Duration: interval},
},
Status: sourcev1.GitRepositoryStatus{
Conditions: []metav1.Condition{
Expand All @@ -386,6 +406,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) {
summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper)
summaryOpts := []Option{
WithConditions(tt.conditions...),
WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}),
}
_, err = summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...)
g.Expect(err).ToNot(HaveOccurred())
Expand All @@ -394,3 +415,62 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) {
})
}
}

func TestIsNonStalledSuccess(t *testing.T) {
interval := 5 * time.Second

tests := []struct {
name string
beforeFunc func(obj conditions.Setter)
rb reconcile.RuntimeResultBuilder
recResult ctrl.Result
recErr error
wantResult bool
}{
{
name: "non stalled success",
rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval},
recResult: ctrl.Result{RequeueAfter: interval},
wantResult: true,
},
{
name: "stalled success",
beforeFunc: func(obj conditions.Setter) {
conditions.MarkStalled(obj, "FooReason", "test-msg")
},
rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval},
recResult: ctrl.Result{RequeueAfter: interval},
wantResult: false,
},
{
name: "error result",
rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval},
recResult: ctrl.Result{RequeueAfter: interval},
recErr: errors.New("some-error"),
wantResult: false,
},
{
name: "non success result",
rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval},
recResult: ctrl.Result{RequeueAfter: 2 * time.Second},
wantResult: false,
},
{
name: "no result builder",
recResult: ctrl.Result{RequeueAfter: interval},
wantResult: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

obj := &sourcev1.GitRepository{}
if tt.beforeFunc != nil {
tt.beforeFunc(obj)
}
g.Expect(isNonStalledSuccess(obj, tt.rb, tt.recResult, tt.recErr)).To(Equal(tt.wantResult))
})
}
}

0 comments on commit d64def3

Please sign in to comment.