Skip to content

Commit 4cb6735

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 4cb6735

3 files changed

Lines changed: 174 additions & 25 deletions

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: 129 additions & 24 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,
@@ -850,7 +858,7 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
850858
expectedLastTransition: nil,
851859
},
852860
{
853-
name: "degraded condition with unobserved generation - should return ToApply",
861+
name: "no Applied condition with stale Degraded - should return ToApply",
854862
manifestWork: &workapiv1.ManifestWork{
855863
ObjectMeta: metav1.ObjectMeta{
856864
Name: "test-mw",
@@ -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,
@@ -936,7 +958,7 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
936958
expectedLastTransition: &now,
937959
},
938960
{
939-
name: "progressing true but degraded false - should return Progressing",
961+
name: "progressing false - should return Succeeded",
940962
manifestWork: &workapiv1.ManifestWork{
941963
ObjectMeta: metav1.ObjectMeta{
942964
Name: "test-mw",
@@ -947,27 +969,27 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
947969
Status: workapiv1.ManifestWorkStatus{
948970
Conditions: []metav1.Condition{
949971
{
950-
Type: workapiv1.WorkProgressing,
972+
Type: workapiv1.WorkApplied,
951973
Status: metav1.ConditionTrue,
952974
ObservedGeneration: 2,
953975
LastTransitionTime: now,
954-
Reason: "Applying",
976+
Reason: "Applied",
955977
},
956978
{
957-
Type: workapiv1.WorkDegraded,
979+
Type: workapiv1.WorkProgressing,
958980
Status: metav1.ConditionFalse,
959981
ObservedGeneration: 2,
960982
LastTransitionTime: now,
961-
Reason: "Healthy",
983+
Reason: "Completed",
962984
},
963985
},
964986
},
965987
},
966-
expectedStatus: clustersdkv1alpha1.Progressing,
988+
expectedStatus: clustersdkv1alpha1.Succeeded,
967989
expectedLastTransition: &now,
968990
},
969991
{
970-
name: "progressing true with degraded unknown - should return Progressing",
992+
name: "progressing false with degraded true - should return Succeeded",
971993
manifestWork: &workapiv1.ManifestWork{
972994
ObjectMeta: metav1.ObjectMeta{
973995
Name: "test-mw",
@@ -978,14 +1000,52 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
9781000
Status: workapiv1.ManifestWorkStatus{
9791001
Conditions: []metav1.Condition{
9801002
{
981-
Type: workapiv1.WorkProgressing,
1003+
Type: workapiv1.WorkApplied,
9821004
Status: metav1.ConditionTrue,
9831005
ObservedGeneration: 2,
9841006
LastTransitionTime: now,
985-
Reason: "Applying",
1007+
Reason: "Applied",
1008+
},
1009+
{
1010+
Type: workapiv1.WorkProgressing,
1011+
Status: metav1.ConditionFalse,
1012+
ObservedGeneration: 2,
1013+
LastTransitionTime: now,
1014+
Reason: "Completed",
9861015
},
9871016
{
9881017
Type: workapiv1.WorkDegraded,
1018+
Status: metav1.ConditionTrue,
1019+
ObservedGeneration: 2,
1020+
LastTransitionTime: now,
1021+
Reason: "Failed",
1022+
},
1023+
},
1024+
},
1025+
},
1026+
expectedStatus: clustersdkv1alpha1.Succeeded,
1027+
expectedLastTransition: &now,
1028+
},
1029+
{
1030+
name: "progressing unknown status - should return Progressing",
1031+
manifestWork: &workapiv1.ManifestWork{
1032+
ObjectMeta: metav1.ObjectMeta{
1033+
Name: "test-mw",
1034+
Namespace: "cls1",
1035+
Generation: 2,
1036+
CreationTimestamp: creationTime,
1037+
},
1038+
Status: workapiv1.ManifestWorkStatus{
1039+
Conditions: []metav1.Condition{
1040+
{
1041+
Type: workapiv1.WorkApplied,
1042+
Status: metav1.ConditionTrue,
1043+
ObservedGeneration: 2,
1044+
LastTransitionTime: now,
1045+
Reason: "Applied",
1046+
},
1047+
{
1048+
Type: workapiv1.WorkProgressing,
9891049
Status: metav1.ConditionUnknown,
9901050
ObservedGeneration: 2,
9911051
LastTransitionTime: now,
@@ -998,7 +1058,7 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
9981058
expectedLastTransition: &now,
9991059
},
10001060
{
1001-
name: "progressing false - should return Succeeded",
1061+
name: "WorkApplied Status=False with current generation - should return ToApply",
10021062
manifestWork: &workapiv1.ManifestWork{
10031063
ObjectMeta: metav1.ObjectMeta{
10041064
Name: "test-mw",
@@ -1008,6 +1068,13 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10081068
},
10091069
Status: workapiv1.ManifestWorkStatus{
10101070
Conditions: []metav1.Condition{
1071+
{
1072+
Type: workapiv1.WorkApplied,
1073+
Status: metav1.ConditionFalse,
1074+
ObservedGeneration: 2,
1075+
LastTransitionTime: now,
1076+
Reason: "ApplyFailed",
1077+
},
10111078
{
10121079
Type: workapiv1.WorkProgressing,
10131080
Status: metav1.ConditionFalse,
@@ -1018,11 +1085,11 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10181085
},
10191086
},
10201087
},
1021-
expectedStatus: clustersdkv1alpha1.Succeeded,
1022-
expectedLastTransition: &now,
1088+
expectedStatus: clustersdkv1alpha1.ToApply,
1089+
expectedLastTransition: nil,
10231090
},
10241091
{
1025-
name: "progressing false with degraded true - should return Succeeded",
1092+
name: "WorkApplied Status=Unknown with current generation - should return ToApply",
10261093
manifestWork: &workapiv1.ManifestWork{
10271094
ObjectMeta: metav1.ObjectMeta{
10281095
Name: "test-mw",
@@ -1032,49 +1099,87 @@ func TestClusterRolloutStatusFunc(t *testing.T) {
10321099
},
10331100
Status: workapiv1.ManifestWorkStatus{
10341101
Conditions: []metav1.Condition{
1102+
{
1103+
Type: workapiv1.WorkApplied,
1104+
Status: metav1.ConditionUnknown,
1105+
ObservedGeneration: 2,
1106+
LastTransitionTime: now,
1107+
Reason: "ApplyStatusUnknown",
1108+
},
10351109
{
10361110
Type: workapiv1.WorkProgressing,
10371111
Status: metav1.ConditionFalse,
10381112
ObservedGeneration: 2,
10391113
LastTransitionTime: now,
10401114
Reason: "Completed",
10411115
},
1116+
},
1117+
},
1118+
},
1119+
expectedStatus: clustersdkv1alpha1.ToApply,
1120+
expectedLastTransition: nil,
1121+
},
1122+
{
1123+
name: "WorkApplied with stale ObservedGeneration (old gen 1, current gen 2) - should return ToApply",
1124+
manifestWork: &workapiv1.ManifestWork{
1125+
ObjectMeta: metav1.ObjectMeta{
1126+
Name: "test-mw",
1127+
Namespace: "cls1",
1128+
Generation: 2,
1129+
CreationTimestamp: creationTime,
1130+
},
1131+
Status: workapiv1.ManifestWorkStatus{
1132+
Conditions: []metav1.Condition{
10421133
{
1043-
Type: workapiv1.WorkDegraded,
1134+
Type: workapiv1.WorkApplied,
10441135
Status: metav1.ConditionTrue,
1136+
ObservedGeneration: 1,
1137+
LastTransitionTime: now,
1138+
Reason: "Applied",
1139+
},
1140+
{
1141+
Type: workapiv1.WorkProgressing,
1142+
Status: metav1.ConditionFalse,
10451143
ObservedGeneration: 2,
10461144
LastTransitionTime: now,
1047-
Reason: "Failed",
1145+
Reason: "Completed",
10481146
},
10491147
},
10501148
},
10511149
},
1052-
expectedStatus: clustersdkv1alpha1.Succeeded,
1053-
expectedLastTransition: &now,
1150+
expectedStatus: clustersdkv1alpha1.ToApply,
1151+
expectedLastTransition: nil,
10541152
},
10551153
{
1056-
name: "progressing unknown status - should return Progressing",
1154+
name: "Progressing with old ObservedGeneration but newer WorkApplied - should return ToApply",
10571155
manifestWork: &workapiv1.ManifestWork{
10581156
ObjectMeta: metav1.ObjectMeta{
10591157
Name: "test-mw",
10601158
Namespace: "cls1",
1061-
Generation: 2,
1159+
Generation: 3,
10621160
CreationTimestamp: creationTime,
10631161
},
10641162
Status: workapiv1.ManifestWorkStatus{
10651163
Conditions: []metav1.Condition{
1164+
{
1165+
Type: workapiv1.WorkApplied,
1166+
Status: metav1.ConditionTrue,
1167+
ObservedGeneration: 3,
1168+
LastTransitionTime: now,
1169+
Reason: "Applied",
1170+
},
10661171
{
10671172
Type: workapiv1.WorkProgressing,
1068-
Status: metav1.ConditionUnknown,
1173+
Status: metav1.ConditionFalse,
10691174
ObservedGeneration: 2,
10701175
LastTransitionTime: now,
1071-
Reason: "Unknown",
1176+
Reason: "Completed",
10721177
},
10731178
},
10741179
},
10751180
},
1076-
expectedStatus: clustersdkv1alpha1.Progressing,
1077-
expectedLastTransition: &now,
1181+
expectedStatus: clustersdkv1alpha1.ToApply,
1182+
expectedLastTransition: nil,
10781183
},
10791184
}
10801185

‎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)