diff --git a/pkg/controller/statusmanager/pod_status.go b/pkg/controller/statusmanager/pod_status.go index 6e83c910f9..28febb38e8 100644 --- a/pkg/controller/statusmanager/pod_status.go +++ b/pkg/controller/statusmanager/pod_status.go @@ -7,6 +7,7 @@ import ( "log" "os" "reflect" + "strconv" "strings" "time" @@ -94,25 +95,25 @@ func (status *StatusManager) SetFromPods() { for _, ds := range daemonSets { dsName := NewClusteredName(ds) dsState, hadState := daemonsetStates[dsName] - dsRolloutActive := !status.installComplete || ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled + dsNeverObservedStable := !isObservedStable(ds) dsProgressing := false if isNonCritical(ds) && ds.Status.NumberReady == 0 && !status.installComplete { progressing = append(progressing, fmt.Sprintf("DaemonSet %q is waiting for other operators to become ready", dsName.String())) dsProgressing = true - } else if ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled { + } else if ds.Status.ObservedGeneration < ds.Generation || ds.Status.UpdatedNumberScheduled < ds.Status.CurrentNumberScheduled { progressing = append(progressing, fmt.Sprintf("DaemonSet %q update is rolling out (%d out of %d updated)", dsName.String(), ds.Status.UpdatedNumberScheduled, ds.Status.CurrentNumberScheduled)) dsProgressing = true } else if ds.Status.NumberUnavailable > 0 { - if dsRolloutActive { + if dsNeverObservedStable { progressing = append(progressing, fmt.Sprintf("DaemonSet %q is not available (awaiting %d nodes)", dsName.String(), ds.Status.NumberUnavailable)) dsProgressing = true } if !isNonCritical(ds) { clbo = append(clbo, status.CheckCrashLoopBackOffPods(dsName, ds.Spec.Selector.MatchLabels, "DaemonSet")...) } - } else if ds.Status.NumberAvailable == 0 && dsRolloutActive { + } else if ds.Status.NumberAvailable == 0 && dsNeverObservedStable { progressing = append(progressing, fmt.Sprintf("DaemonSet %q is not yet scheduled on any nodes", dsName.String())) dsProgressing = true } @@ -127,7 +128,7 @@ func (status *StatusManager) SetFromPods() { reachedAvailableLevel = false if !hadState || !reflect.DeepEqual(dsState.LastSeenStatus, ds.Status) { - dsState.LastChangeTime = time.Now() + dsState.LastChangeTime = status.clock.Now() ds.Status.DeepCopyInto(&dsState.LastSeenStatus) } @@ -146,23 +147,29 @@ func (status *StatusManager) SetFromPods() { if err := status.setAnnotation(context.TODO(), ds, names.RolloutHungAnnotation, dsHung); err != nil { log.Printf("Error setting DaemonSet %q annotation: %v", dsName, err) } + if !dsProgressing && ds.Status.NumberUnavailable == 0 { + gen := strconv.FormatInt(ds.Status.ObservedGeneration, 10) + if err := status.setAnnotation(context.TODO(), ds, names.ObservedStableGenerationAnnotation, &gen); err != nil { + log.Printf("Error setting DaemonSet %q annotation: %v", dsName, err) + } + } } for _, ss := range statefulSets { ssName := NewClusteredName(ss) ssState, hadState := statefulsetStates[ssName] - ssRolloutActive := !status.installComplete || ss.Status.UpdatedReplicas < ss.Status.Replicas + ssNeverObservedStable := !isObservedStable(ss) ssProgressing := false if isNonCritical(ss) && ss.Status.ReadyReplicas == 0 && !status.installComplete { progressing = append(progressing, fmt.Sprintf("StatefulSet %q is waiting for other operators to become ready", ssName.String())) ssProgressing = true - } else if ss.Status.UpdatedReplicas < ss.Status.Replicas { + } else if ss.Status.ObservedGeneration < ss.Generation || ss.Status.UpdatedReplicas < ss.Status.Replicas { progressing = append(progressing, fmt.Sprintf("StatefulSet %q update is rolling out (%d out of %d updated)", ssName.String(), ss.Status.UpdatedReplicas, ss.Status.Replicas)) ssProgressing = true } else if ss.Status.ReadyReplicas > 0 && ss.Status.ReadyReplicas < ss.Status.Replicas { - if ssRolloutActive { + if ssNeverObservedStable { progressing = append(progressing, fmt.Sprintf("StatefulSet %q is not available (awaiting %d nodes)", ssName.String(), (ss.Status.Replicas-ss.Status.ReadyReplicas))) ssProgressing = true } @@ -170,7 +177,7 @@ func (status *StatusManager) SetFromPods() { if !isNonCritical(ss) { clbo = append(clbo, status.CheckCrashLoopBackOffPods(ssName, ss.Spec.Selector.MatchLabels, "StatefulSet")...) } - } else if ss.Status.AvailableReplicas == 0 && ssRolloutActive { + } else if ss.Status.AvailableReplicas == 0 && ssNeverObservedStable { progressing = append(progressing, fmt.Sprintf("StatefulSet %q is not yet scheduled on any nodes", ssName.String())) ssProgressing = true } @@ -185,7 +192,7 @@ func (status *StatusManager) SetFromPods() { reachedAvailableLevel = false if !hadState || !reflect.DeepEqual(ssState.LastSeenStatus, ss.Status) { - ssState.LastChangeTime = time.Now() + ssState.LastChangeTime = status.clock.Now() ss.Status.DeepCopyInto(&ssState.LastSeenStatus) } @@ -204,22 +211,28 @@ func (status *StatusManager) SetFromPods() { if err := status.setAnnotation(context.TODO(), ss, names.RolloutHungAnnotation, ssHung); err != nil { log.Printf("Error setting StatefulSet %q annotation: %v", ssName, err) } + if !ssProgressing && ss.Status.ReadyReplicas == ss.Status.Replicas { + gen := strconv.FormatInt(ss.Status.ObservedGeneration, 10) + if err := status.setAnnotation(context.TODO(), ss, names.ObservedStableGenerationAnnotation, &gen); err != nil { + log.Printf("Error setting StatefulSet %q annotation: %v", ssName, err) + } + } } for _, dep := range deployments { depName := NewClusteredName(dep) depState, hadState := deploymentStates[depName] - depRolloutActive := !status.installComplete || dep.Status.UpdatedReplicas < dep.Status.Replicas + depNeverObservedStable := !isObservedStable(dep) depProgressing := false if isNonCritical(dep) && dep.Status.UnavailableReplicas > 0 && !status.installComplete { progressing = append(progressing, fmt.Sprintf("Deployment %q is waiting for other operators to become ready", depName.String())) depProgressing = true - } else if dep.Status.UpdatedReplicas < dep.Status.Replicas { + } else if dep.Status.ObservedGeneration < dep.Generation || dep.Status.UpdatedReplicas < dep.Status.Replicas { progressing = append(progressing, fmt.Sprintf("Deployment %q update is rolling out (%d out of %d updated)", depName.String(), dep.Status.UpdatedReplicas, dep.Status.Replicas)) depProgressing = true } else if dep.Status.UnavailableReplicas > 0 { - if depRolloutActive { + if depNeverObservedStable { progressing = append(progressing, fmt.Sprintf("Deployment %q is not available (awaiting %d nodes)", depName.String(), dep.Status.UnavailableReplicas)) depProgressing = true } @@ -227,7 +240,7 @@ func (status *StatusManager) SetFromPods() { if !isNonCritical(dep) { clbo = append(clbo, status.CheckCrashLoopBackOffPods(depName, dep.Spec.Selector.MatchLabels, "Deployment")...) } - } else if dep.Status.AvailableReplicas == 0 && depRolloutActive { + } else if dep.Status.AvailableReplicas == 0 && depNeverObservedStable { progressing = append(progressing, fmt.Sprintf("Deployment %q is not yet scheduled on any nodes", depName.String())) depProgressing = true } @@ -242,7 +255,7 @@ func (status *StatusManager) SetFromPods() { reachedAvailableLevel = false if !hadState || !reflect.DeepEqual(depState.LastSeenStatus, dep.Status) { - depState.LastChangeTime = time.Now() + depState.LastChangeTime = status.clock.Now() dep.Status.DeepCopyInto(&depState.LastSeenStatus) } @@ -261,6 +274,12 @@ func (status *StatusManager) SetFromPods() { if err := status.setAnnotation(context.TODO(), dep, names.RolloutHungAnnotation, depHung); err != nil { log.Printf("Error setting Deployment %q annotation: %v", depName, err) } + if !depProgressing && dep.Status.UnavailableReplicas == 0 { + gen := strconv.FormatInt(dep.Status.ObservedGeneration, 10) + if err := status.setAnnotation(context.TODO(), dep, names.ObservedStableGenerationAnnotation, &gen); err != nil { + log.Printf("Error setting Deployment %q annotation: %v", depName, err) + } + } } status.setNotDegraded(PodDeployment) @@ -430,6 +449,13 @@ func isNonCritical(obj metav1.Object) bool { return exists } +// isObservedStable returns true if the object was last observed fully available +// at its current metadata.generation. This is tracked via an annotation on the +// object itself, so it survives controller restarts. +func isObservedStable(obj metav1.Object) bool { + return obj.GetAnnotations()[names.ObservedStableGenerationAnnotation] == strconv.FormatInt(obj.GetGeneration(), 10) +} + func (status *StatusManager) listAllStatusObjects() (dss []*appsv1.DaemonSet, deps []*appsv1.Deployment, sss []*appsv1.StatefulSet) { // these lists can't fail, they're backed by informers for _, lister := range status.dsListers { diff --git a/pkg/controller/statusmanager/status_manager_test.go b/pkg/controller/statusmanager/status_manager_test.go index 6911e0c0e5..5567c93ee7 100644 --- a/pkg/controller/statusmanager/status_manager_test.go +++ b/pkg/controller/statusmanager/status_manager_test.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "log" + "maps" "reflect" + "strconv" "testing" "time" @@ -86,11 +88,32 @@ func setOC(t *testing.T, client cnoclient.Client, oc *operv1.Network) { func set(t *testing.T, client cnoclient.Client, obj crclient.Object) { t.Helper() - err := client.ClientFor("").CRClient().Update(t.Context(), obj) - if apierrors.IsNotFound(err) { - err = client.ClientFor("").CRClient().Create(t.Context(), obj) + // Re-read the object to pick up any annotation writes (e.g. + // observed-stable-generation) that SetFromPods may have applied. + current := obj.DeepCopyObject().(crclient.Object) + err := client.ClientFor("").CRClient().Get(t.Context(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, current) + if err != nil { + if apierrors.IsNotFound(err) { + if err := client.ClientFor("").CRClient().Create(t.Context(), obj); err != nil { + t.Fatalf("Failed to create: %v", err) + } + return + } else { + t.Fatalf("Failed to get current object: %v", err) + } + } + // Avoid wiping annotations added by SetFromPods. + merged := current.GetAnnotations() + if merged == nil { + merged = map[string]string{} } + maps.Copy(merged, current.GetAnnotations()) + obj.SetAnnotations(merged) + + obj.SetResourceVersion(current.GetResourceVersion()) + + err = client.ClientFor("").CRClient().Update(t.Context(), obj) if err != nil { t.Fatalf("Failed to set: %v", err) } @@ -98,8 +121,16 @@ func set(t *testing.T, client cnoclient.Client, obj crclient.Object) { func setStatus(t *testing.T, client cnoclient.Client, obj crclient.Object) { t.Helper() - err := client.ClientFor("").CRClient().Status().Update(t.Context(), obj) - if err != nil { + // Re-read the object to pick up any annotation writes (e.g. + // observed-stable-generation) that SetFromPods may have applied. + current := obj.DeepCopyObject().(crclient.Object) + if err := client.ClientFor("").CRClient().Get(t.Context(), + types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, current); err != nil { + t.Fatalf("Failed to get current object: %v", err) + } + obj.SetResourceVersion(current.GetResourceVersion()) + obj.SetAnnotations(current.GetAnnotations()) + if err := client.ClientFor("").CRClient().Status().Update(t.Context(), obj); err != nil { t.Fatalf("Failed to set status: %v", err) } } @@ -1180,9 +1211,8 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { t.Fatalf("Progressing condition unexpectedly missing") } - // Now, bump the generation of one of the daemonsets. Without the - // generation-based rollout signal, this alone should not enter - // Progressing until the daemonset status starts to reflect rollout work. + // Now, bump the generation of one of the daemonsets. We should observe the + // stale status and enter Progressing. dsA.Generation = 2 set(t, client, dsA) status.SetFromPods() @@ -1198,7 +1228,7 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { }, { Type: operv1.OperatorStatusTypeProgressing, - Status: operv1.ConditionFalse, + Status: operv1.ConditionTrue, }, { Type: operv1.OperatorStatusTypeUpgradeable, @@ -1314,9 +1344,8 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { if err != nil { t.Fatalf("error getting ClusterOperator: %v", err) } - // With the simplified rollout detection logic, once UpdatedNumberScheduled >= CurrentNumberScheduled, - // the rollout is complete. Unavailability after rollout completion is treated as - // reboot churn, not a network rollout, so Progressing should be False. + // We have a new rollout because observedGeneration was bumped. We are + // progressing=True until the rollout is complete. if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{ { Type: operv1.OperatorStatusTypeDegraded, @@ -1324,7 +1353,7 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { }, { Type: operv1.OperatorStatusTypeProgressing, - Status: operv1.ConditionFalse, + Status: operv1.ConditionTrue, }, { Type: operv1.OperatorStatusTypeUpgradeable, @@ -1341,16 +1370,6 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { t.Fatalf("unexpected Status.Versions: %#v", co.Status.Versions) } - // With the new simplified logic, since the rollout is complete (UpdatedNumberScheduled >= CurrentNumberScheduled), - // the DaemonSet state is not tracked. Verify the state is empty. - ps := getLastPodState(t, client, "testing") - nsn := ClusteredName{Namespace: "one", Name: "alpha"} - for _, ds := range ps.DaemonsetStates { - if ds.ClusteredName == nsn { - t.Fatalf("DaemonSet state should not be tracked when rollout is complete, but found: %#v", ds) - } - } - // done: numberReady -> 1, numberUnavailable -> 0 dsA.Status = appsv1.DaemonSetStatus{ CurrentNumberScheduled: 1, @@ -1364,6 +1383,15 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { setStatus(t, client, dsA) status.SetFromPods() + // Rollout is complete. Verify the pod state is empty. + ps := getLastPodState(t, client, "testing") + nsn := ClusteredName{Namespace: "one", Name: "alpha"} + for _, ds := range ps.DaemonsetStates { + if ds.ClusteredName == nsn { + t.Fatalf("DaemonSet state should not be tracked when rollout is complete, but found: %#v", ds) + } + } + // see that the pod state is sensible co, oc, err = getStatuses(client, "testing") if err != nil { @@ -1409,7 +1437,8 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { Name: "non-critical", Generation: 1, Annotations: map[string]string{ - names.NonCriticalAnnotation: "", + names.NonCriticalAnnotation: "", + names.ObservedStableGenerationAnnotation: "1", }, Labels: sl, }, @@ -1528,6 +1557,351 @@ func TestStatusManagerSetFromDaemonSets(t *testing.T) { } } +func TestObjectSetsProgressing(t *testing.T) { + selectorSpec := &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + } + dsSpec := appsv1.DaemonSetSpec{Selector: selectorSpec} + depSpec := appsv1.DeploymentSpec{Selector: selectorSpec} + ssSpec := appsv1.StatefulSetSpec{Selector: selectorSpec} + + dsAvailableStatus := func(observedGeneration int64) appsv1.DaemonSetStatus { + return appsv1.DaemonSetStatus{ + CurrentNumberScheduled: 1, + DesiredNumberScheduled: 1, + UpdatedNumberScheduled: 1, + NumberAvailable: 1, + NumberReady: 1, + NumberUnavailable: 0, + ObservedGeneration: observedGeneration, + } + } + dsUnavailableStatus := func(observedGeneration int64) appsv1.DaemonSetStatus { + return appsv1.DaemonSetStatus{ + CurrentNumberScheduled: 1, + DesiredNumberScheduled: 1, + UpdatedNumberScheduled: 1, + NumberUnavailable: 1, + ObservedGeneration: observedGeneration, + } + } + depAvailableStatus := func(observedGeneration int64) appsv1.DeploymentStatus { + return appsv1.DeploymentStatus{ + Replicas: 1, + UpdatedReplicas: 1, + AvailableReplicas: 1, + UnavailableReplicas: 0, + ObservedGeneration: observedGeneration, + } + } + depUnavailableStatus := func(observedGeneration int64) appsv1.DeploymentStatus { + return appsv1.DeploymentStatus{ + Replicas: 1, + UpdatedReplicas: 1, + AvailableReplicas: 0, + UnavailableReplicas: 1, + ObservedGeneration: observedGeneration, + } + } + ssAvailableStatus := func(observedGeneration int64) appsv1.StatefulSetStatus { + return appsv1.StatefulSetStatus{ + Replicas: 1, + UpdatedReplicas: 1, + ReadyReplicas: 1, + AvailableReplicas: 1, + ObservedGeneration: observedGeneration, + } + } + ssUnavailableStatus := func(observedGeneration int64) appsv1.StatefulSetStatus { + return appsv1.StatefulSetStatus{ + Replicas: 1, + UpdatedReplicas: 1, + ReadyReplicas: 0, + AvailableReplicas: 0, + ObservedGeneration: observedGeneration, + } + } + + for _, tt := range []struct { + name string + object crclient.Object + observedStableGeneration int64 + expectProgressing bool + }{ + // DaemonSet cases + { + name: "New DaemonSet observed stable", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: dsSpec, + Status: dsAvailableStatus(1), + }, + expectProgressing: false, + }, + { + name: "New DaemonSet with unavailable pods", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: dsSpec, + Status: dsUnavailableStatus(1), + }, + expectProgressing: true, + }, + { + name: "New DaemonSet with no status", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: dsSpec, + }, + expectProgressing: true, + }, + { + name: "Updated DaemonSet observed stable", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: dsSpec, + Status: dsAvailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + { + name: "Updated DaemonSet with unavailable pods", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: dsSpec, + Status: dsUnavailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Updated DaemonSet with out of date stable status", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "alpha"}, + }, + }, + Status: dsAvailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Previously observed stable DaemonSet with unavailable pods (presumed reboot in progress)", + object: &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: dsSpec, + Status: dsUnavailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + + // Deployment cases + { + name: "New Deployment observed stable", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: depSpec, + Status: depAvailableStatus(1), + }, + expectProgressing: false, + }, + { + name: "New Deployment with unavailable pods", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: depSpec, + Status: depUnavailableStatus(1), + }, + expectProgressing: true, + }, + { + name: "New Deployment with no status", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: depSpec, + }, + expectProgressing: true, + }, + { + name: "Updated Deployment observed stable", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: depSpec, + Status: depAvailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + { + name: "Updated Deployment with unavailable pods", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: depSpec, + Status: depUnavailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Updated Deployment with out of date stable status", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: depSpec, + Status: depAvailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Previously observed stable Deployment with unavailable pods (presumed reboot in progress)", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: depSpec, + Status: depUnavailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + + // StatefulSet cases + { + name: "New StatefulSet observed stable", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: ssSpec, + Status: ssAvailableStatus(1), + }, + expectProgressing: false, + }, + { + name: "New StatefulSet with unavailable pods", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: ssSpec, + Status: ssUnavailableStatus(1), + }, + expectProgressing: true, + }, + { + name: "New StatefulSet with no status", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: ssSpec, + }, + expectProgressing: true, + }, + { + name: "Updated StatefulSet observed stable", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: ssSpec, + Status: ssAvailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + { + name: "Updated StatefulSet with unavailable pods", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: ssSpec, + Status: ssUnavailableStatus(2), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Updated StatefulSet with out of date stable status", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: ssSpec, + Status: ssAvailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: true, + }, + { + name: "Previously observed stable StatefulSet with unavailable pods (presumed reboot in progress)", + object: &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + Spec: ssSpec, + Status: ssUnavailableStatus(1), + }, + observedStableGeneration: 1, + expectProgressing: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + client := fake.NewFakeClient() + status := New(client, "testing", names.StandAloneClusterName) + status.clock = testingclock.NewFakeClock(time.Now()) + setFakeListers(status) + + no := &operv1.Network{ObjectMeta: metav1.ObjectMeta{Name: names.OPERATOR_CONFIG}} + setOC(t, client, no) + + tt.object.SetName("test-object") + tt.object.SetNamespace("test-namespace") + tt.object.SetLabels(sl) + annotations := tt.object.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string, 1) + } + annotations[names.ObservedStableGenerationAnnotation] = strconv.FormatInt(tt.observedStableGeneration, 10) + tt.object.SetAnnotations(annotations) + + set(t, client, tt.object) + setStatus(t, client, tt.object) + status.SetFromPods() + + _, oc, err := getStatuses(client, "testing") + if err != nil { + t.Fatalf("error getting ClusterOperator: %v", err) + } + + var expectedProgressing operv1.ConditionStatus + if tt.expectProgressing { + expectedProgressing = operv1.ConditionTrue + } else { + expectedProgressing = operv1.ConditionFalse + } + + // Both DSes have unavailable pods and neither has been observed stable + // at its current generation, so Progressing must be True. + if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{ + { + Type: operv1.OperatorStatusTypeProgressing, + Status: expectedProgressing, + }, + }) { + t.Fatalf("expected Progressing=%v for %q, got: %#v", expectedProgressing, tt.name, oc.Status.Conditions) + } + + // The observed stable generation should be set if we reported a stable state. + if !tt.expectProgressing { + current := tt.object.DeepCopyObject().(crclient.Object) + err := client.ClientFor("").CRClient().Get(t.Context(), types.NamespacedName{Namespace: tt.object.GetNamespace(), Name: tt.object.GetName()}, current) + if err != nil { + t.Fatalf("error getting current object: %v", err) + } + + observedStableGeneration, err := strconv.ParseInt(current.GetAnnotations()[names.ObservedStableGenerationAnnotation], 10, 64) + if err != nil { + t.Fatalf("error parsing observed stable generation: %v", err) + } + if observedStableGeneration != tt.object.GetGeneration() { + t.Fatalf("expected observed stable generation %d, got %d", tt.object.GetGeneration(), observedStableGeneration) + } + } + }) + } +} + func TestStatusManagerIgnoresDaemonSetNodeScaleChurnAfterInstall(t *testing.T) { client := fake.NewFakeClient() status := New(client, "testing", names.StandAloneClusterName) @@ -1747,8 +2121,8 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { if err != nil { t.Fatalf("error getting ClusterOperator: %v", err) } - // Generation skew by itself no longer starts rollout tracking. Until the - // deployment counters show an update in progress, we stay non-Progressing. + // We know depB is newly created because it does not have an observed-stable-generation annotation. + // Therefore, we expect Progressing to be True until the deployment counters show rollout is complete. if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{ { Type: operv1.OperatorStatusTypeDegraded, @@ -1756,7 +2130,7 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { }, { Type: operv1.OperatorStatusTypeProgressing, - Status: operv1.ConditionFalse, + Status: operv1.ConditionTrue, }, { Type: operv1.OperatorStatusTypeUpgradeable, @@ -1773,14 +2147,6 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { t.Fatalf("unexpected Status.Versions: %#v", co.Status.Versions) } - ps := getLastPodState(t, client, "testing") - nsn := ClusteredName{Namespace: "one", Name: "beta"} - for _, ds := range ps.DeploymentStates { - if ds.ClusteredName == nsn { - t.Fatalf("expected deployment rollout state for %s to stay empty before rollout counters change: %#v", nsn, ps.DeploymentStates) - } - } - err = client.ClientFor("").CRClient().Get(t.Context(), types.NamespacedName{Namespace: depB.Namespace, Name: depB.Name}, depB) if err != nil { t.Fatalf("error getting Deployment: %v", err) @@ -1792,8 +2158,8 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { depB.Status.UnavailableReplicas = 0 depB.Status.AvailableReplicas = depB.Status.Replicas - t0 := time.Now() - time.Sleep(time.Second / 10) + t0 := status.clock.Now() + status.clock.(*testingclock.FakeClock).Step(time.Second / 10) setStatus(t, client, depB) status.SetFromPods() @@ -1826,7 +2192,8 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { t.Fatalf("unexpected Status.Versions: %#v", co.Status.Versions) } - ps = getLastPodState(t, client, "testing") + ps := getLastPodState(t, client, "testing") + nsn := ClusteredName{Namespace: depB.Namespace, Name: depB.Name} found := false for _, ds := range ps.DeploymentStates { if ds.ClusteredName == nsn { @@ -1861,9 +2228,7 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { if err != nil { t.Fatalf("error getting ClusterOperator: %v", err) } - // With the simplified rollout detection logic, once UpdatedReplicas >= Replicas, - // the rollout is complete. Unavailability after rollout completion is treated as - // reboot churn, not a network rollout, so Progressing should be False. + // Pods are unavailable, so Progressing should still be True. if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{ { Type: operv1.OperatorStatusTypeDegraded, @@ -1871,7 +2236,7 @@ func TestStatusManagerSetFromDeployments(t *testing.T) { }, { Type: operv1.OperatorStatusTypeProgressing, - Status: operv1.ConditionFalse, + Status: operv1.ConditionTrue, }, { Type: operv1.OperatorStatusTypeUpgradeable, @@ -2000,7 +2365,8 @@ func TestStatusManagerRestoresInstallCompleteAfterRestart(t *testing.T) { Name: "non-critical", Generation: 1, Annotations: map[string]string{ - names.NonCriticalAnnotation: "", + names.NonCriticalAnnotation: "", + names.ObservedStableGenerationAnnotation: "1", }, Labels: sl, }, @@ -2096,7 +2462,8 @@ func TestStatusManagerRestoresInstallCompleteFromLegacyAnnotation(t *testing.T) Name: "non-critical", Generation: 1, Annotations: map[string]string{ - names.NonCriticalAnnotation: "", + names.NonCriticalAnnotation: "", + names.ObservedStableGenerationAnnotation: "1", }, Labels: sl, }, @@ -2167,13 +2534,15 @@ func TestStatusManagerRestoresActiveRolloutAfterRestart(t *testing.T) { status.SetFromPods() depB := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{Namespace: "one", Name: "beta", Generation: 1, Labels: sl}, + ObjectMeta: metav1.ObjectMeta{Namespace: "one", Name: "beta", Generation: 1, Labels: sl, + Annotations: map[string]string{names.ObservedStableGenerationAnnotation: "1"}, + }, Status: appsv1.DeploymentStatus{ Replicas: 1, - UpdatedReplicas: 0, - AvailableReplicas: 1, - UnavailableReplicas: 0, - ObservedGeneration: 0, + UpdatedReplicas: 1, + AvailableReplicas: 0, + UnavailableReplicas: 1, + ObservedGeneration: 1, }, Spec: appsv1.DeploymentSpec{ Selector: &metav1.LabelSelector{ @@ -2182,13 +2551,6 @@ func TestStatusManagerRestoresActiveRolloutAfterRestart(t *testing.T) { }, } set(t, client, depB) - status.SetFromPods() - - depB.Status.UpdatedReplicas = depB.Status.Replicas - depB.Status.AvailableReplicas = 0 - depB.Status.UnavailableReplicas = 1 - depB.Status.ObservedGeneration = depB.Generation - setStatus(t, client, depB) restarted := New(client, "testing", names.StandAloneClusterName) restarted.clock = testingclock.NewFakeClock(time.Now()) @@ -2246,13 +2608,15 @@ func TestStatusManagerRestoresStatefulSetActiveRolloutAfterRestart(t *testing.T) status.SetFromPods() ssB := &appsv1.StatefulSet{ - ObjectMeta: metav1.ObjectMeta{Namespace: "one", Name: "beta", Generation: 1, Labels: sl}, + ObjectMeta: metav1.ObjectMeta{Namespace: "one", Name: "beta", Generation: 1, Labels: sl, + Annotations: map[string]string{names.ObservedStableGenerationAnnotation: "1"}, + }, Status: appsv1.StatefulSetStatus{ Replicas: 2, - UpdatedReplicas: 0, - ReadyReplicas: 2, - AvailableReplicas: 2, - ObservedGeneration: 0, + UpdatedReplicas: 2, + ReadyReplicas: 1, + AvailableReplicas: 1, + ObservedGeneration: 1, }, Spec: appsv1.StatefulSetSpec{ Selector: &metav1.LabelSelector{ @@ -2261,13 +2625,6 @@ func TestStatusManagerRestoresStatefulSetActiveRolloutAfterRestart(t *testing.T) }, } set(t, client, ssB) - status.SetFromPods() - - ssB.Status.UpdatedReplicas = ssB.Status.Replicas - ssB.Status.ReadyReplicas = ssB.Status.Replicas - 1 - ssB.Status.AvailableReplicas = ssB.Status.Replicas - 1 - ssB.Status.ObservedGeneration = ssB.Generation - setStatus(t, client, ssB) restarted := New(client, "testing", names.StandAloneClusterName) restarted.clock = testingclock.NewFakeClock(time.Now()) @@ -2512,6 +2869,7 @@ func TestStatusManagerCheckCrashLoopBackOffPods(t *testing.T) { }, Status: appsv1.DeploymentStatus{ UnavailableReplicas: 1, + ObservedGeneration: 1, }, } set(t, client, dep) diff --git a/pkg/names/names.go b/pkg/names/names.go index d409253adc..4f2916a16e 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -90,6 +90,13 @@ const IPsecEnableAnnotation = "networkoperator.openshift.io/ipsec-enabled" // (i.e. DaemonSet or Deployment) is not making progress, unset otherwise. const RolloutHungAnnotation = "networkoperator.openshift.io/rollout-hung" +// ObservedStableGenerationAnnotation records the metadata.generation at which a +// DaemonSet, Deployment, or StatefulSet was last observed fully available by +// SetFromPods. A resource whose current generation does not match this value +// has never completed a successful rollout at its current spec and must be +// tracked as Progressing until all pods are available. +const ObservedStableGenerationAnnotation = "networkoperator.openshift.io/observed-stable-generation" + // CopyFromAnnotation is an annotation that allows copying resources from specified clusters // value format: cluster/namespace/name const CopyFromAnnotation = "network.operator.openshift.io/copy-from"