Skip to content

Commit c146fb8

Browse files
haoqing0110claude
haoqing0110
and
claude
committed
fix: Check Applied condition before evaluating rollout status
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Qing Hao <qhao@redhat.com>
1 parent 8f8cd01 commit c146fb8

3 files changed

Lines changed: 102 additions & 1 deletion

File tree

‎pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,13 +218,23 @@ func (d *deployReconciler) clusterRolloutStatusFunc(clusterName string, manifest
218218
// Get all relevant conditions
219219
progressingCond := apimeta.FindStatusCondition(manifestWork.Status.Conditions, workv1.WorkProgressing)
220220
degradedCond := apimeta.FindStatusCondition(manifestWork.Status.Conditions, workv1.WorkDegraded)
221+
appliedCond := apimeta.FindStatusCondition(manifestWork.Status.Conditions, workv1.WorkApplied)
221222

222223
// Return ToApply if:
224+
// - No Applied condition exists yet (work hasn't been applied by hub controller)
225+
// - Applied condition hasn't observed the latest spec
223226
// - No Progressing condition exists yet (work hasn't been reconciled by agent)
224227
// - Progressing condition hasn't observed the latest spec
225228
// - Degraded condition exists but hasn't observed the latest spec
226229
// (Degraded is optional, but if it exists, we wait for it to catch up)
227-
if progressingCond == nil ||
230+
//
231+
// IMPORTANT: Check Applied condition FIRST to ensure the work has been properly applied
232+
// before checking agent-side conditions. This prevents using stale timestamps from
233+
// previous generations when conditions update their ObservedGeneration without changing Status.
234+
if appliedCond == nil ||
235+
appliedCond.ObservedGeneration != manifestWork.Generation ||
236+
appliedCond.Status != metav1.ConditionTrue ||
237+
progressingCond == nil ||
228238
progressingCond.ObservedGeneration != manifestWork.Generation ||
229239
(degradedCond != nil && degradedCond.ObservedGeneration != manifestWork.Generation) {
230240
return clsRolloutStatus, nil

‎pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,14 @@ func TestRequeueWithProgressDeadline(t *testing.T) {
591591
},
592592
}
593593
mw, _ := CreateManifestWork(mwrSet, "cls1", "place-test")
594+
// Set Applied=True first to ensure work has been applied by hub controller
595+
apimeta.SetStatusCondition(&mw.Status.Conditions, metav1.Condition{
596+
Type: workapiv1.WorkApplied,
597+
Status: metav1.ConditionTrue,
598+
Reason: "Applied",
599+
ObservedGeneration: mw.Generation,
600+
LastTransitionTime: metav1.NewTime(time.Now()),
601+
})
594602
// Set Progressing=True AND Degraded=True to simulate a failed work (matching new logic)
595603
apimeta.SetStatusCondition(&mw.Status.Conditions, metav1.Condition{
596604
Type: workapiv1.WorkProgressing,
@@ -891,6 +899,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
891899
},
892900
Status: workapiv1.ManifestWorkStatus{
893901
Conditions: []metav1.Condition{
902+
{
903+
Type: workapiv1.WorkApplied,
904+
Status: metav1.ConditionTrue,
905+
ObservedGeneration: 2,
906+
LastTransitionTime: now,
907+
Reason: "Applied",
908+
},
894909
{
895910
Type: workapiv1.WorkProgressing,
896911
Status: metav1.ConditionTrue,
@@ -915,6 +930,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
915930
},
916931
Status: workapiv1.ManifestWorkStatus{
917932
Conditions: []metav1.Condition{
933+
{
934+
Type: workapiv1.WorkApplied,
935+
Status: metav1.ConditionTrue,
936+
ObservedGeneration: 2,
937+
LastTransitionTime: now,
938+
Reason: "Applied",
939+
},
918940
{
919941
Type: workapiv1.WorkProgressing,
920942
Status: metav1.ConditionTrue,
@@ -946,6 +968,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
946968
},
947969
Status: workapiv1.ManifestWorkStatus{
948970
Conditions: []metav1.Condition{
971+
{
972+
Type: workapiv1.WorkApplied,
973+
Status: metav1.ConditionTrue,
974+
ObservedGeneration: 2,
975+
LastTransitionTime: now,
976+
Reason: "Applied",
977+
},
949978
{
950979
Type: workapiv1.WorkProgressing,
951980
Status: metav1.ConditionTrue,
@@ -977,6 +1006,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
9771006
},
9781007
Status: workapiv1.ManifestWorkStatus{
9791008
Conditions: []metav1.Condition{
1009+
{
1010+
Type: workapiv1.WorkApplied,
1011+
Status: metav1.ConditionTrue,
1012+
ObservedGeneration: 2,
1013+
LastTransitionTime: now,
1014+
Reason: "Applied",
1015+
},
9801016
{
9811017
Type: workapiv1.WorkProgressing,
9821018
Status: metav1.ConditionTrue,
@@ -1008,6 +1044,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10081044
},
10091045
Status: workapiv1.ManifestWorkStatus{
10101046
Conditions: []metav1.Condition{
1047+
{
1048+
Type: workapiv1.WorkApplied,
1049+
Status: metav1.ConditionTrue,
1050+
ObservedGeneration: 2,
1051+
LastTransitionTime: now,
1052+
Reason: "Applied",
1053+
},
10111054
{
10121055
Type: workapiv1.WorkProgressing,
10131056
Status: metav1.ConditionFalse,
@@ -1032,6 +1075,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10321075
},
10331076
Status: workapiv1.ManifestWorkStatus{
10341077
Conditions: []metav1.Condition{
1078+
{
1079+
Type: workapiv1.WorkApplied,
1080+
Status: metav1.ConditionTrue,
1081+
ObservedGeneration: 2,
1082+
LastTransitionTime: now,
1083+
Reason: "Applied",
1084+
},
10351085
{
10361086
Type: workapiv1.WorkProgressing,
10371087
Status: metav1.ConditionFalse,
@@ -1063,6 +1113,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10631113
},
10641114
Status: workapiv1.ManifestWorkStatus{
10651115
Conditions: []metav1.Condition{
1116+
{
1117+
Type: workapiv1.WorkApplied,
1118+
Status: metav1.ConditionTrue,
1119+
ObservedGeneration: 2,
1120+
LastTransitionTime: now,
1121+
Reason: "Applied",
1122+
},
10661123
{
10671124
Type: workapiv1.WorkProgressing,
10681125
Status: metav1.ConditionUnknown,

‎test/integration/work/manifestworkreplicaset_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,12 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
386386
gomega.Expect(err).ToNot(gomega.HaveOccurred())
387387
for _, work := range works.Items {
388388
workCopy := work.DeepCopy()
389+
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
390+
Type: workapiv1.WorkApplied,
391+
Status: metav1.ConditionTrue,
392+
Reason: "AppliedManifestWorkComplete",
393+
ObservedGeneration: workCopy.Generation,
394+
})
389395
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
390396
Type: workapiv1.WorkProgressing,
391397
Status: metav1.ConditionFalse,
@@ -404,6 +410,12 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
404410
gomega.Expect(err).ToNot(gomega.HaveOccurred())
405411
for _, work := range works.Items {
406412
workCopy := work.DeepCopy()
413+
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
414+
Type: workapiv1.WorkApplied,
415+
Status: metav1.ConditionTrue,
416+
Reason: "AppliedManifestWorkComplete",
417+
ObservedGeneration: workCopy.Generation,
418+
})
407419
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
408420
Type: workapiv1.WorkProgressing,
409421
Status: metav1.ConditionFalse,
@@ -441,6 +453,12 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
441453
gomega.Expect(err).ToNot(gomega.HaveOccurred())
442454
for _, work := range works.Items {
443455
workCopy := work.DeepCopy()
456+
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
457+
Type: workapiv1.WorkApplied,
458+
Status: metav1.ConditionTrue,
459+
Reason: "Applied",
460+
ObservedGeneration: workCopy.Generation,
461+
})
444462
meta.SetStatusCondition(&workCopy.Status.Conditions, metav1.Condition{
445463
Type: workapiv1.WorkProgressing,
446464
Status: metav1.ConditionTrue,
@@ -497,6 +515,14 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
497515
gomega.Expect(err).ToNot(gomega.HaveOccurred())
498516
for _, work := range works.Items {
499517
workCopy := work.DeepCopy()
518+
meta.SetStatusCondition(
519+
&workCopy.Status.Conditions,
520+
metav1.Condition{
521+
Type: workapiv1.WorkApplied,
522+
Status: metav1.ConditionTrue,
523+
Reason: "Applied",
524+
ObservedGeneration: workCopy.Generation,
525+
})
500526
meta.SetStatusCondition(
501527
&workCopy.Status.Conditions,
502528
metav1.Condition{
@@ -564,6 +590,14 @@ var _ = ginkgo.Describe("ManifestWorkReplicaSet", func() {
564590
gomega.Expect(err).ToNot(gomega.HaveOccurred())
565591
for _, work := range works.Items {
566592
workCopy := work.DeepCopy()
593+
meta.SetStatusCondition(
594+
&workCopy.Status.Conditions,
595+
metav1.Condition{
596+
Type: workapiv1.WorkApplied,
597+
Status: metav1.ConditionTrue,
598+
Reason: "Applied",
599+
ObservedGeneration: workCopy.Generation,
600+
})
567601
meta.SetStatusCondition(
568602
&workCopy.Status.Conditions,
569603
metav1.Condition{

0 commit comments

Comments
 (0)