Distinguish transient from terminal condition failures - #83
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcpowermac The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe migration controller now classifies transient failures separately from permanent errors. It validates storage management state, performs idempotent destination setup, tracks rollout from cluster state, requires sustained readiness, bounds diagnostics, and merges status updates against the latest resource. ChangesMigration reliability and lifecycle handling
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The change distinguishes transient condition failures from terminal failures so retryable situations can report an appropriate status; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant MigrationController
participant ClusterState
participant vSphere
MigrationController->>ClusterState: read worker and rollout state
ClusterState-->>MigrationController: return readiness and generation data
MigrationController->>vSphere: create missing folders and ownership tags
vSphere-->>MigrationController: return destination objects
MigrationController->>ClusterState: remove source MachineSets after worker removal
MigrationController->>ClusterState: verify stable operators and MachineConfigPools
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Stable And Deterministic Test NamesExplanation No changed Ginkgo test title exists. The pull request adds standard Go tests with static Full details: Test Structure And QualityExplanation PASS. The pull request adds standard Full details: Microshift Test CompatibilityExplanation No new Ginkgo e2e tests were added. The pull request adds Full details: Single Node Openshift (Sno) Test CompatibilityExplanation No new Ginkgo e2e tests were added. The only changed test files use standard Go Full details: Topology-Aware Scheduling CompatibilityExplanation PASS: The pull request introduces no topology-related scheduling constraint. The exact diff changes a condition-reason constant, transient-error handling, two preflight error classifications, tests, and the controller's condition-reason selection. It does not add or modify pod affinity, topology spread, replica scheduling, node selectors or affinity, tolerations, taints, or PDBs. Existing MachineSet replica logic is unchanged and therefore is not pull-request-caused. Full details: Ote Binary Stdout ContractExplanation No OTE stdout contract violation was introduced. The diff changes API constants, controller error handling, preflight logic, and unit tests only. It adds no stdout writer, logging configuration, or suite setup. Existing suite messages use GinkgoWriter, which the check allows, and the repository contains no OTE binary entry point. Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation PASS: The pull request adds no new Ginkgo e2e tests. The changed tests are Go Full details: No-Weak-CryptoExplanation PASS: The pull request adds only condition-reason and transient-error handling. The added lines contain no MD5, SHA1, DES, RC4, Blowfish, ECB, custom cryptography, or secret/token comparison. A non-vendor scan also found no weak-crypto or constant-time comparison patterns. Existing vendor references are not introduced by this pull request. Full details: Container-PrivilegesExplanation PASS. The pull request changes only Go source and test files. It does not change any container or Kubernetes manifest. The repository's existing manager manifest sets Full details: No-Sensitive-Data-In-LogsExplanation No sensitive-data logging was introduced. The patch adds a
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/vmwarecloudfoundationmigration_controller.go (1)
163-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a controller-level test for the new reason mapping.
Please ensure a handler returning a transient error persists
Status=False,ReasonRetrying, and the original message, while a plain error still persistsReasonFailed. This guards the end-to-end wiring beyond the existing classifier unit tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/vmwarecloudfoundationmigration_controller.go` at line 163, Add a controller-level test covering the status update path around setCondition: verify a handler returning a transient error persists Status=False, ReasonRetrying, and the original error message, and verify a plain error persists ReasonFailed. Exercise the controller wiring end to end rather than only testing the error classifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/vmwarecloudfoundationmigration_controller.go`:
- Line 163: Add a controller-level test covering the status update path around
setCondition: verify a handler returning a transient error persists
Status=False, ReasonRetrying, and the original error message, and verify a plain
error persists ReasonFailed. Exercise the controller wiring end to end rather
than only testing the error classifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bfe3b7b3-11d4-4398-88be-941d2656146e
📒 Files selected for processing (6)
api/v1alpha1/vmwarecloudfoundationmigration_types.gointernal/controller/errors.gointernal/controller/errors_test.gointernal/controller/preflight.gointernal/controller/preflight_test.gointernal/controller/vmwarecloudfoundationmigration_controller.go
ca9317c to
519081d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Reconcile previously set every handler error to reason=Failed, even when the underlying cause was transient (e.g. cluster operators still stabilizing or an upgrade in progress) and the operator was about to retry and succeed on its own. Consumers watching for a terminal Failed reason had no way to tell "needs intervention" apart from "will resolve itself." Add a transientError wrapper and a Retrying reason so preflight can mark known-transient conditions and have Reconcile record the right reason automatically.
519081d to
bcd714e
Compare
|
@jcpowermac: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Reconcile previously set every handler error to reason=Failed, even when the underlying cause was transient (e.g. cluster operators still stabilizing or an upgrade in progress) and the operator was about to retry and succeed on its own. Consumers watching for a terminal Failed reason had no way to tell "needs intervention" apart from "will resolve itself."
Add a transientError wrapper and a Retrying reason so preflight can mark known-transient conditions and have Reconcile record the right reason automatically.
Summary by CodeRabbit
New Features
Bug Fixes
Tests