From 56478cf1c8e0ac0783fb1d618e8eae22adf87ee9 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 17 Apr 2024 19:59:54 +0000 Subject: [PATCH] Update Ready condition during drift correction Update the Ready condition during drift correction to reflect the current state of reconciliation. Without this, any previous Ready condition value continues to persist on the object. If there was a previous failure due to which Ready=False condition is present on the object, the same value continues to persist if the atomic release reconciliation enters a drift detection and correction loop. Resulting in the status to show inaccurate state of the reconciliation. Examples of two different scenarios that arise due to this issue: - If a release without any dependency is installed, the status shows Ready=True for InstallSucceeded reason. But right after the installation, if a drift is detected the status continues to show the same Ready=True value. There's no indication that a drift correction is going on in the status. The events and logs do show that drift correction is taking place. But it can be confusing to see positive Ready value. Also, since the Ready condition message is copied for Reconciling condition, Reconciling=True with a "Helm install succeeded..." is seen. - If a release depends on another release, and reconciliation results in dependency not ready error at first, Ready=False condition is added on the object. On subsequent runs, even when the dependencies are ready, the Ready=False condition isn't updated, resulting in stale Ready value until atomic release reconciliation completes. But if the atomic reconciliation enters a drift detection and correction loop, the Ready=False with dependency error persists in the status. This gives the impression that something is wrong with dependency check but based on the logs and events, the controller could be stuck in drift detection and correction loop. Updating the Ready condition during drift detection shows the current state of reconciliation, avoiding the confusing scenarios described above. Signed-off-by: Sunny --- internal/reconcile/correct_cluster_drift.go | 5 +++++ internal/reconcile/correct_cluster_drift_test.go | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/internal/reconcile/correct_cluster_drift.go b/internal/reconcile/correct_cluster_drift.go index a6b4c889..320b4fee 100644 --- a/internal/reconcile/correct_cluster_drift.go +++ b/internal/reconcile/correct_cluster_drift.go @@ -24,6 +24,8 @@ import ( apierrutil "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/ssa" "github.com/fluxcd/pkg/ssa/jsondiff" @@ -64,6 +66,9 @@ func (r *CorrectClusterDrift) Reconcile(ctx context.Context, req *Request) error ctx, cancel := context.WithTimeout(ctx, req.Object.GetTimeout().Duration) defer cancel() + // Update condition to reflect the current status. + conditions.MarkUnknown(req.Object, meta.ReadyCondition, meta.ProgressingReason, "correcting cluster drift") + changeSet, err := action.ApplyDiff(ctx, r.configFactory.Build(nil), r.diff, r.fieldManager) r.report(req.Object, changeSet, err) return nil diff --git a/internal/reconcile/correct_cluster_drift_test.go b/internal/reconcile/correct_cluster_drift_test.go index 83c14a07..3efde795 100644 --- a/internal/reconcile/correct_cluster_drift_test.go +++ b/internal/reconcile/correct_cluster_drift_test.go @@ -24,10 +24,13 @@ import ( . "github.com/onsi/gomega" extjsondiff "github.com/wI2L/jsondiff" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apierrutil "k8s.io/apimachinery/pkg/util/errors" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/ssa" "github.com/fluxcd/pkg/ssa/jsondiff" @@ -154,6 +157,10 @@ func TestCorrectClusterDrift_Reconcile(t *testing.T) { } else { g.Expect(recorder.GetEvents()).To(BeEmpty()) } + + g.Expect(tt.obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "correcting cluster drift"), + })) }) } }