🐛 set RecheckTime when minSuccessTime is defined

[haoqing0110]

Summary

Related issue(s)

open-cluster-management-io/ocm#1237

Fixes #

Summary by CodeRabbit

• Refactor

  • Unified and simplified rollout recheck timing and semantics for clearer timeout/soak handling and more accurate next-check scheduling.

• Tests

  • Updated and added tests to validate the revised recheck timing behavior across multiple rollout strategies and edge cases.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoqing0110
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Walkthrough

Replaced TimeOutTime with RecheckTime in ClusterRolloutStatus, added calculateRecheckTime to compute next check times, and refactored recheck calculation so minRecheckAfter derives its result from per-cluster RecheckTime values. Tests updated accordingly.

Changes

Cohort / File(s)	Summary
Core Rollout Logic pkg/apis/cluster/v1alpha1/rollout.go	Replaced public field TimeOutTime *metav1.Time with RecheckTime *metav1.Time in ClusterRolloutStatus. Added calculateRecheckTime(startTime *metav1.Time, duration time.Duration) *metav1.Time. Refactored determineRolloutStatus() and related flows to set and use RecheckTime (soak end or timeout) and replaced uses of getTimeOutTime. Updated minRecheckAfter(rolloutClusters []ClusterRolloutStatus) *time.Duration to compute minimum from RecheckTime fields. Fixed comment typo.
Tests pkg/apis/cluster/v1alpha1/rollout_test.go	Updated expected structs and test logic to use RecheckTime instead of TimeOutTime across rollout scenarios. Added TestMinRecheckAfter() to validate selection of the minimum RecheckTime (nil/empty/mixed cases). Adjusted time-based expectations to align with new recheck semantics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Inspect determineRolloutStatus() and callers to ensure RecheckTime is set for the correct statuses (succeeded soak end vs. timeout/soak for others).
• Verify calculateRecheckTime nil-handling matches previous semantics where no timeout/soak is intended.
• Confirm minRecheckAfter correctly computes the minimum positive duration from RecheckTime and that all callers were updated appropriately.
• Review updated tests, especially TestMinRecheckAfter(), for comprehensive coverage of edge cases (nil, empty, mixed).

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete; the Summary section is empty, and the Fixes field lacks a specific issue number despite referencing an issue URL.	Complete the Summary section with a clear description of the changes and populate the 'Fixes #' field with the specific issue number (1237).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: setting RecheckTime when minSuccessTime is defined, which aligns with the refactoring to replace TimeOutTime with RecheckTime.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 81959d2 and aab7c47.

📒 Files selected for processing (2)

• pkg/apis/cluster/v1alpha1/rollout.go (8 hunks)
• pkg/apis/cluster/v1alpha1/rollout_test.go (26 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/apis/cluster/v1alpha1/rollout_test.go (1)

pkg/apis/cluster/v1alpha1/rollout.go (8)

• Failed (33-33)
• Progressing (29-29)
• ToApply (27-27)
• ClusterRolloutStatus (42-57)
• TimeOut (36-36)
• Succeeded (31-31)
• RolloutResult (62-73)
• RolloutClock (19-19)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: unit
• GitHub Check: integration
• GitHub Check: verify

🔇 Additional comments (8)

pkg/apis/cluster/v1alpha1/rollout_test.go (4)

145-151: LGTM: Test expectations correctly updated to use RecheckTime.

The systematic replacement of TimeOutTime with RecheckTime in test expectations is correct. The calculated values are accurate:

• cluster4: fakeTime_60s + 90s = fakeTime30s ✓
• cluster2 (timeout): fakeTime_120s + 90s = fakeTime_30s ✓

---

564-630: Excellent test coverage for mixed soak and timeout scenarios.

This new test case comprehensively validates the RecheckTime calculation when clusters have different states:

• cluster1: In 60s soak period, 30s remaining → RecheckTime = fakeTime30s
• cluster2: Progressing with 90s timeout, 30s remaining → RecheckTime = fakeTime30s
• cluster3: Progressing with 90s timeout, 60s remaining → RecheckTime = fakeTime60s
• cluster4: Outside soak period, correctly excluded

The expected RecheckAfter of 30s correctly reflects the minimum across clusters 1, 2, and 3.

---

1639-1697: Good addition: ProgressivePerGroup test with minSuccessTime.

This test case validates the interaction between soak periods and group-based rollout:

• cluster1: In soak period with RecheckTime set
• cluster2: Outside soak period, correctly excluded from rollout
• cluster3: Progressing with timeout tracking

The test confirms that RecheckAfter correctly derives from the minimum RecheckTime (30s).

---

2474-2548: Comprehensive test coverage for minRecheckAfter function.

The TestMinRecheckAfter test thoroughly validates all scenarios:

• Single cluster with RecheckTime
• Multiple clusters with different RecheckTimes (correctly selects minimum)
• Mixed soak and timeout periods
• Nil/empty inputs (correctly returns nil)
• Clusters without RecheckTime (correctly ignored)

This provides strong confidence in the refactored minRecheckAfter implementation.

pkg/apis/cluster/v1alpha1/rollout.go (4)

53-56: Improved field naming and documentation.

Renaming TimeOutTime to RecheckTime better reflects the dual purpose:

• For succeeded status: tracks when the minSuccessTime (soak time) period ends
• For progressing/failed status: tracks when the timeout occurs

The updated documentation clearly explains both semantics.

---

475-479: Correct soak period handling for succeeded clusters.

The logic properly calculates and sets RecheckTime for succeeded clusters still within their soak period, ensuring they remain in rolloutClusters until the minSuccessTime elapses.

---

500-515: Well-designed calculateRecheckTime helper function.

This consolidates the recheck time calculation logic with good semantics:

• Handles nil startTime by using current time (reasonable default for new clusters)
• Returns nil for maxTimeDuration (indicating no timeout/soak period)
• Simple, focused responsibility

---

606-623: Simplified and more intuitive minRecheckAfter implementation.

The refactored function is cleaner and more consistent:

• Derives minimum from per-cluster RecheckTime values (single source of truth)
• Correctly filters out past recheck times with recheckDuration > 0 check
• Returns nil when no future rechecks are needed

This eliminates the need to pass timeout and minSuccessTime parameters, as those are already encoded in each cluster's RecheckTime.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[haoqing0110]

/assign @qiujian16
cc @youngbupark