Fix long durations being emitted because of state ltt

pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go

@@ -24,6 +24,19 @@ const RevisionAnnotationKey = "deployment.kubernetes.io/revision"
 // creation time. If older, we assume it's a rollback (reusing existing RS/CR) and use time.Now().
 const RecentCreationThreshold = 5 * time.Minute
 
+// maxPlausibleRolloutDuration is the backstop bound on how far in the past a Progressing condition's
+// LastTransitionTime may be, used only when no ReplicaSet is known to validate staleness against (see
+// getProgressingConditionTime). The Deployment controller preserves LastTransitionTime across
+// successive rollouts while Status stays True, so an unbounded value can be pinned at the deployment's
+// creation and yield absurd (multi-hundred-day) durations after an Agent restart.
+const maxPlausibleRolloutDuration = 24 * time.Hour
+
+// isInProgressRolloutReason reports whether a Deployment Progressing condition Reason (observed with
+// Status=True) indicates an actively in-progress rollout for duration-tracking purposes.
+func isInProgressRolloutReason(reason string) bool {
+	return reason == "ReplicaSetUpdated" || reason == "NewReplicaSetCreated"
+}
+
 // ReplicaSetInfo holds information about a ReplicaSet for Deployment rollout tracking
 type ReplicaSetInfo struct {
 	Name         string
@@ -193,15 +206,41 @@ func (rt *RolloutTracker) StoreDeployment(dep *appsv1.Deployment) {
 
 // getProgressingConditionTime extracts the LastTransitionTime from the Progressing condition
 // when it indicates an active rollout. This provides restart resilience.
-func getProgressingConditionTime(dep *appsv1.Deployment, fallback time.Time) time.Time {
+//
+// The Deployment controller preserves LastTransitionTime across successive rollouts while the
+// Progressing Status stays True, so a stale value can be pinned at the deployment's creation and yield
+// absurd durations. We therefore only trust it when it is not stale:
+//   - When notBefore is set (the newest ReplicaSet's creation time is known), the current rollout
+//     cannot have started before its ReplicaSet existed, so a LastTransitionTime earlier than
+//     notBefore is treated as stale. This still surfaces genuinely long-running rollouts whose
+//     condition time is consistent with their ReplicaSet.
+//   - When notBefore is zero (no ReplicaSet is known yet, e.g. the informer has not synced right after
+//     a restart), fall back to the coarse maxPlausibleRolloutDuration age backstop.
+//
+// When the Progressing condition is missing, not in-progress, zero, or stale, we return fallback.
+func getProgressingConditionTime(dep *appsv1.Deployment, notBefore, fallback time.Time) time.Time {
 	for _, cond := range dep.Status.Conditions {
-		if cond.Type == appsv1.DeploymentProgressing {
-			if cond.Status == corev1.ConditionTrue && cond.Reason == "ReplicaSetUpdated" {
-				if !cond.LastTransitionTime.IsZero() {
-					return cond.LastTransitionTime.Time
-				}
+		if cond.Type != appsv1.DeploymentProgressing {
+			continue
+		}
+		// There is at most one Progressing condition; once found, evaluate it and stop.
+		if cond.Status != corev1.ConditionTrue || !isInProgressRolloutReason(cond.Reason) {
+			break
+		}
+		lastTransition := cond.LastTransitionTime.Time
+		if lastTransition.IsZero() {
+			break
+		}
+		if !notBefore.IsZero() {
+			// The rollout cannot have started before its newest ReplicaSet was created.
+			if lastTransition.Before(notBefore) {
+				break
 			}
+		} else if time.Since(lastTransition) > maxPlausibleRolloutDuration {
+			// No ReplicaSet to bound against - reject an implausibly old anchor.
+			break
 		}
+		return lastTransition
 	}
 	return fallback
 }
@@ -243,8 +282,14 @@ func (rt *RolloutTracker) determineDeploymentStartTime(dep *appsv1.Deployment) t
 		}
 	}
 
-	// RS is old or not found - try Progressing condition, fall back to now
-	return getProgressingConditionTime(dep, now)
+	// RS is old or not found - try the Progressing condition, rejecting a stale anchor, then fall
+	// back to now. When the newest ReplicaSet's creation time is known, use it as the staleness
+	// bound: the current rollout cannot have started before its ReplicaSet existed.
+	var notBefore time.Time
+	if hasRS {
+		notBefore = rsCreationTime
+	}
+	return getProgressingConditionTime(dep, notBefore, now)
 }
 
 // CleanupDeployment removes a deployment from active rollout tracking.
@@ -290,12 +335,14 @@ func (rt *RolloutTracker) HasActiveRollout(d *appsv1.Deployment) bool {
 	return exists
 }
 
-// HasRolloutCondition checks if Kubernetes reports the deployment as progressing
+// HasRolloutCondition checks if Kubernetes reports the deployment as progressing.
+// See isInProgressRolloutReason for which Progressing reasons count as an active rollout (and why
+// "FoundNewReplicaSet" and "NewReplicaSetAvailable" are deliberately excluded).
 func (rt *RolloutTracker) HasRolloutCondition(d *appsv1.Deployment) bool {
 	for _, condition := range d.Status.Conditions {
 		if condition.Type == appsv1.DeploymentProgressing {
 			return condition.Status == corev1.ConditionTrue &&
-				condition.Reason == "ReplicaSetUpdated"
+				isInProgressRolloutReason(condition.Reason)
 		}
 	}
 	return false

pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go

@@ -2382,3 +2382,171 @@ func TestRecentCreationThreshold(t *testing.T) {
 	assert.Equal(t, 5*time.Minute, RecentCreationThreshold,
 		"RecentCreationThreshold should be 5 minutes")
 }
+
+// TestIsInProgressRolloutReason pins which Progressing reasons count as an active rollout.
+// FoundNewReplicaSet (rollback/RS-reuse) and NewReplicaSetAvailable (completion) must NOT count.
+func TestIsInProgressRolloutReason(t *testing.T) {
+	assert.True(t, isInProgressRolloutReason("ReplicaSetUpdated"), "steady-state rolling is in progress")
+	assert.True(t, isInProgressRolloutReason("NewReplicaSetCreated"), "new RS created is in progress")
+	assert.False(t, isInProgressRolloutReason("FoundNewReplicaSet"), "reused RS (rollback) must not count")
+	assert.False(t, isInProgressRolloutReason("NewReplicaSetAvailable"), "completion must not count")
+	assert.False(t, isInProgressRolloutReason("ProgressDeadlineExceeded"), "failure must not count")
+	assert.False(t, isInProgressRolloutReason(""), "empty reason must not count")
+}
+
+// TestHasRolloutCondition_Reasons verifies HasRolloutCondition honors isInProgressRolloutReason,
+// including the newly-tracked NewReplicaSetCreated and the still-excluded FoundNewReplicaSet.
+func TestHasRolloutCondition_Reasons(t *testing.T) {
+	tracker := NewRolloutTracker()
+
+	deploymentWith := func(status corev1.ConditionStatus, reason string) *appsv1.Deployment {
+		return &appsv1.Deployment{
+			Status: appsv1.DeploymentStatus{
+				Conditions: []appsv1.DeploymentCondition{
+					{Type: appsv1.DeploymentProgressing, Status: status, Reason: reason},
+				},
+			},
+		}
+	}
+
+	assert.True(t, tracker.HasRolloutCondition(deploymentWith(corev1.ConditionTrue, "ReplicaSetUpdated")))
+	assert.True(t, tracker.HasRolloutCondition(deploymentWith(corev1.ConditionTrue, "NewReplicaSetCreated")),
+		"NewReplicaSetCreated should now be treated as an ongoing rollout")
+	assert.False(t, tracker.HasRolloutCondition(deploymentWith(corev1.ConditionTrue, "FoundNewReplicaSet")),
+		"FoundNewReplicaSet (rollback/RS-reuse) must remain excluded to avoid false positives")
+	assert.False(t, tracker.HasRolloutCondition(deploymentWith(corev1.ConditionTrue, "NewReplicaSetAvailable")),
+		"NewReplicaSetAvailable is completion, not ongoing")
+	assert.False(t, tracker.HasRolloutCondition(deploymentWith(corev1.ConditionFalse, "ReplicaSetUpdated")),
+		"Status=False is not ongoing regardless of reason")
+}
+
+// TestDetermineDeploymentStartTime_StaleProgressingCondition guards against the ~300-day-duration bug:
+// a fresh tracker (agent restart) observing a deployment whose newest RS is old and whose Progressing
+// condition carries an ancient LastTransitionTime must NOT anchor the rollout start to that stale time.
+func TestDetermineDeploymentStartTime_StaleProgressingCondition(t *testing.T) {
+	tracker := NewRolloutTracker()
+
+	namespace := "default"
+	deploymentName := "stale-progressing-deploy"
+
+	// Newest tracked RS is old (outside RecentCreationThreshold), forcing the condition-time fallback.
+	tracker.deploymentMutex.Lock()
+	tracker.replicaSetMap[namespace+"/old-rs"] = &ReplicaSetInfo{
+		Name:         "old-rs",
+		Namespace:    namespace,
+		OwnerName:    deploymentName,
+		OwnerUID:     "dep-stale",
+		CreationTime: time.Now().Add(-10 * time.Minute),
+	}
+	tracker.deploymentMutex.Unlock()
+
+	// Progressing=True/ReplicaSetUpdated, but LastTransitionTime is pinned ~300 days in the past
+	// (as happens when the condition has stayed True across many successful rollouts).
+	staleTime := time.Now().Add(-300 * 24 * time.Hour)
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: namespace},
+		Status: appsv1.DeploymentStatus{
+			Conditions: []appsv1.DeploymentCondition{
+				{
+					Type:               appsv1.DeploymentProgressing,
+					Status:             corev1.ConditionTrue,
+					Reason:             "ReplicaSetUpdated",
+					LastTransitionTime: metav1.Time{Time: staleTime},
+				},
+			},
+		},
+	}
+
+	before := time.Now()
+	tracker.deploymentMutex.Lock()
+	startTime := tracker.determineDeploymentStartTime(deployment)
+	tracker.deploymentMutex.Unlock()
+	after := time.Now()
+
+	assert.False(t, startTime.Equal(staleTime),
+		"Must not anchor to a stale LastTransitionTime (would yield a ~300-day duration)")
+	assert.True(t, !startTime.Before(before) && !startTime.After(after),
+		"Stale condition time should fall back to now()")
+}
+
+// TestDetermineDeploymentStartTime_LongRunningRolloutPreserved verifies that a genuinely long-running
+// rollout (progressing for well over maxPlausibleRolloutDuration) is NOT truncated when its Progressing
+// LastTransitionTime is consistent with its ReplicaSet's creation time. The RS-based staleness bound
+// must accept it rather than capping to now.
+func TestDetermineDeploymentStartTime_LongRunningRolloutPreserved(t *testing.T) {
+	tracker := NewRolloutTracker()
+
+	namespace := "default"
+	deploymentName := "long-running-deploy"
+
+	// A rollout that has genuinely been progressing for 30h: its ReplicaSet is 30h old (outside
+	// RecentCreationThreshold) and the Progressing condition transitioned at (approximately) the same
+	// time - i.e. the condition time is NOT older than the ReplicaSet, so it is not stale.
+	rsCreation := time.Now().Add(-30 * time.Hour)
+	progressingTime := rsCreation.Add(time.Second)
+
+	tracker.deploymentMutex.Lock()
+	tracker.replicaSetMap[namespace+"/long-rs"] = &ReplicaSetInfo{
+		Name:         "long-rs",
+		Namespace:    namespace,
+		OwnerName:    deploymentName,
+		OwnerUID:     "dep-long",
+		CreationTime: rsCreation,
+	}
+	tracker.deploymentMutex.Unlock()
+
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: namespace},
+		Status: appsv1.DeploymentStatus{
+			Conditions: []appsv1.DeploymentCondition{
+				{
+					Type:               appsv1.DeploymentProgressing,
+					Status:             corev1.ConditionTrue,
+					Reason:             "ReplicaSetUpdated",
+					LastTransitionTime: metav1.Time{Time: progressingTime},
+				},
+			},
+		},
+	}
+
+	tracker.deploymentMutex.Lock()
+	startTime := tracker.determineDeploymentStartTime(deployment)
+	tracker.deploymentMutex.Unlock()
+
+	assert.Equal(t, progressingTime, startTime,
+		"A long-running rollout whose condition time is consistent with its ReplicaSet must be preserved, not capped to now")
+	assert.Greater(t, time.Since(startTime), maxPlausibleRolloutDuration,
+		"Duration should reflect the real >24h rollout age, not a truncated value")
+}
+
+// TestDetermineDeploymentStartTime_NoReplicaSetFallsBackToBackstop verifies that when no ReplicaSet is
+// known to validate staleness against (e.g. the informer has not synced right after a restart), an
+// implausibly old Progressing LastTransitionTime is rejected by the coarse age backstop.
+func TestDetermineDeploymentStartTime_NoReplicaSetFallsBackToBackstop(t *testing.T) {
+	tracker := NewRolloutTracker()
+
+	// No ReplicaSet stored for this deployment - forces the notBefore==zero backstop path.
+	staleTime := time.Now().Add(-300 * 24 * time.Hour)
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{Name: "no-rs-deploy", Namespace: "default"},
+		Status: appsv1.DeploymentStatus{
+			Conditions: []appsv1.DeploymentCondition{
+				{
+					Type:               appsv1.DeploymentProgressing,
+					Status:             corev1.ConditionTrue,
+					Reason:             "ReplicaSetUpdated",
+					LastTransitionTime: metav1.Time{Time: staleTime},
+				},
+			},
+		},
+	}
+
+	before := time.Now()
+	tracker.deploymentMutex.Lock()
+	startTime := tracker.determineDeploymentStartTime(deployment)
+	tracker.deploymentMutex.Unlock()
+	after := time.Now()
+
+	assert.True(t, !startTime.Before(before) && !startTime.After(after),
+		"With no ReplicaSet to bound against, an implausibly old condition time should fall back to now()")
+}

releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml

@@ -0,0 +1,11 @@
+# Each section from every release note are combined when the
+# CHANGELOG.rst is rendered. So the text needs to be worded so that
+# it does not depend on any information only available in another
+# section. This may mean repeating some details, but each section
+# must be readable independently of the other.
+#
+# Each section note must be formatted as reStructuredText.
+---
+fixes:
+  - |
+    Fixed the kubernetes_state.deployment.rollout_duration metric occasionally reporting erroneous large values after a cluster agent restart