From 652476de170d9e286a75594be57b5b68efb56ec6 Mon Sep 17 00:00:00 2001 From: "frank.spano" Date: Thu, 23 Jul 2026 16:33:01 -0400 Subject: [PATCH 1/4] Fix long durations being emitted because of state ltt --- .../ksm/customresources/rollout_tracker.go | 28 ++++-- .../customresources/rollout_tracker_test.go | 86 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go index 8e783ccf9bed..ec05a80609f3 100644 --- a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go +++ b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go @@ -24,6 +24,16 @@ 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 bounds how far in the past a Progressing condition's LastTransitionTime +// may be before we treat it as a stale anchor rather than the current rollout's start time. +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,12 +203,18 @@ 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. +// +// The LastTransitionTime is only trusted when it is recent enough to plausibly belong to the current +// rollout (see maxPlausibleRolloutDuration): the Deployment controller preserves it across successive +// rollouts while Status stays True, so a stale value can otherwise be pinned at the deployment's +// creation and yield absurd durations. When it is missing or stale, we fall back instead. func getProgressingConditionTime(dep *appsv1.Deployment, 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.Status == corev1.ConditionTrue && isInProgressRolloutReason(cond.Reason) { + lastTransition := cond.LastTransitionTime.Time + if !lastTransition.IsZero() && time.Since(lastTransition) <= maxPlausibleRolloutDuration { + return lastTransition } } } @@ -290,12 +306,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 diff --git a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go index efc863cb08b3..bce8fe7481a9 100644 --- a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go +++ b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go @@ -2382,3 +2382,89 @@ 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()") +} From f69d1602be0851c2879c66bbbd079ef86f8656b0 Mon Sep 17 00:00:00 2001 From: "frank.spano" Date: Thu, 23 Jul 2026 16:36:29 -0400 Subject: [PATCH 2/4] reno --- ...x-ongoing-rollout-transition-9ac44f31a3ce2528.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml diff --git a/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml b/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml new file mode 100644 index 000000000000..4620fc50e5d9 --- /dev/null +++ b/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 emtric occasionally reporting erroneous large values after a cluster agent restart From e0bf5e138b893bc110db1711b074776e0c2150bc Mon Sep 17 00:00:00 2001 From: "frank.spano" Date: Thu, 23 Jul 2026 16:56:22 -0400 Subject: [PATCH 3/4] Fix codex found bug for legit long rollouts --- .../ksm/customresources/rollout_tracker.go | 59 +++++++++---- .../customresources/rollout_tracker_test.go | 82 +++++++++++++++++++ 2 files changed, 126 insertions(+), 15 deletions(-) diff --git a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go index ec05a80609f3..0f3e6b417eb1 100644 --- a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go +++ b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker.go @@ -24,8 +24,11 @@ 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 bounds how far in the past a Progressing condition's LastTransitionTime -// may be before we treat it as a stale anchor rather than the current rollout's start time. +// 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 @@ -204,20 +207,40 @@ 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. // -// The LastTransitionTime is only trusted when it is recent enough to plausibly belong to the current -// rollout (see maxPlausibleRolloutDuration): the Deployment controller preserves it across successive -// rollouts while Status stays True, so a stale value can otherwise be pinned at the deployment's -// creation and yield absurd durations. When it is missing or stale, we fall back instead. -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 && isInProgressRolloutReason(cond.Reason) { - lastTransition := cond.LastTransitionTime.Time - if !lastTransition.IsZero() && time.Since(lastTransition) <= maxPlausibleRolloutDuration { - return lastTransition - } + 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 } @@ -259,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. diff --git a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go index bce8fe7481a9..8919fa02698f 100644 --- a/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go +++ b/pkg/collector/corechecks/cluster/ksm/customresources/rollout_tracker_test.go @@ -2468,3 +2468,85 @@ func TestDetermineDeploymentStartTime_StaleProgressingCondition(t *testing.T) { 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()") +} From 67c1648bd8b20e02a22866288affe9e59780af22 Mon Sep 17 00:00:00 2001 From: frank-spano Date: Mon, 27 Jul 2026 08:51:28 -0400 Subject: [PATCH 4/4] Update releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml Co-authored-by: Alexandre Lavigne --- .../notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml b/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml index 4620fc50e5d9..92ccf7430a58 100644 --- a/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml +++ b/releasenotes/notes/fix-ongoing-rollout-transition-9ac44f31a3ce2528.yaml @@ -8,4 +8,4 @@ --- fixes: - | - Fixed the kubernetes_state.deployment.rollout_duration emtric occasionally reporting erroneous large values after a cluster agent restart + Fixed the kubernetes_state.deployment.rollout_duration metric occasionally reporting erroneous large values after a cluster agent restart