OCPBUGS-100292: Fix MCO degradation issue when Cluster API Machine resource is missing - #6368
OCPBUGS-100292: Fix MCO degradation issue when Cluster API Machine resource is missing#6368proietfb wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@proietfb: This pull request references Jira Issue OCPBUGS-100292, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesCloud configuration synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: proietfb 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
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@proietfb: This pull request references Jira Issue OCPBUGS-100292, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3b2a0df0-9019-11f1-8661-04e67b8a1eb8-0 |
|
/pipeline required |
|
Scheduling tests matching the |
|
/payload 5.0 nightly blocking |
|
@proietfb: trigger 14 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/93fb5420-9019-11f1-9ea5-297d9dbedc4d-0 |
|
@proietfb: This pull request references Jira Issue OCPBUGS-100292, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/sync.go (2)
1988-2011: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new retry and timeout paths.
This change introduces three new observable behaviors: retry on ConfigMap-get failure, retry on version mismatch, and immediate termination on parse error, with an aggregated error on timeout. None of these paths are visible in the provided test file.
Add unit tests in
pkg/operator/sync_test.gocovering:
- ConfigMap-get failure that resolves before the 3-second timeout.
- Version mismatch that never resolves, expecting an aggregated timeout error containing the last version-mismatch message.
- Immediate parse-error termination without waiting for the full timeout.
Do you want me to draft these test cases?
🤖 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 `@pkg/operator/sync.go` around lines 1988 - 2011, Add unit tests in sync_test.go for the osImageURL ConfigMap polling flow around the function containing PollUntilContextTimeout: verify ConfigMap-get failures retry and succeed before timeout, persistent release-version mismatches time out with an aggregated error containing the latest mismatch message, and ParseOSImageURLConfigMap errors terminate immediately without waiting for the full timeout.
1988-2011: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate a cancellable context into the ConfigMap polling.
getOSImageURLsFromConfigMapis called during operator sync, but it usescontext.TODO()for a new 3-second retry path. Accept a caller context and pass it towait.PollUntilContextTimeoutso the wait can stop on shutdown or caller cancellation before returningcould not get OS images.🤖 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 `@pkg/operator/sync.go` around lines 1988 - 2011, Update getOSImageURLsFromConfigMap to accept a caller-provided context and pass it to wait.PollUntilContextTimeout instead of context.TODO(). Update its callers to supply the operator sync context, preserving the existing polling and error aggregation behavior while allowing shutdown or cancellation to interrupt the wait.Source: Path instructions
🤖 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 `@pkg/operator/sync.go`:
- Around line 1988-2011: Add unit tests in sync_test.go for the osImageURL
ConfigMap polling flow around the function containing PollUntilContextTimeout:
verify ConfigMap-get failures retry and succeed before timeout, persistent
release-version mismatches time out with an aggregated error containing the
latest mismatch message, and ParseOSImageURLConfigMap errors terminate
immediately without waiting for the full timeout.
- Around line 1988-2011: Update getOSImageURLsFromConfigMap to accept a
caller-provided context and pass it to wait.PollUntilContextTimeout instead of
context.TODO(). Update its callers to supply the operator sync context,
preserving the existing polling and error aggregation behavior while allowing
shutdown or cancellation to interrupt the wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fabb757d-972a-47eb-b617-37e8245ef73b
📒 Files selected for processing (1)
pkg/operator/sync.go
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ac3f98e0-90b7-11f1-9b62-a4f63012d7d1-0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/sync.go (1)
347-347: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePropagate the reconciliation context into the cloud config poll.
syncCloudConfigis called frompkg/operator/sync.goandsyncCloudConfigonly has a 3-second deadline viacontext.TODO(). Make it accept acontext.Context, pass it intowait.PollUntilContextTimeout, and propagate the context through any callers that need cancellation.🤖 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 `@pkg/operator/sync.go` at line 347, Update syncCloudConfig to accept a context.Context and pass that context to wait.PollUntilContextTimeout instead of context.TODO(). Update every syncCloudConfig caller to provide and propagate the reconciliation context so cancellation and deadlines reach the cloud config poll.Source: Path instructions
🤖 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.
Inline comments:
In `@pkg/operator/sync.go`:
- Around line 353-354: Prevent the nil dereference in the error construction
around isKubeCloudConfigCMRequired by deriving a safe platform label when
infra.Status.PlatformStatus is nil, then use that label in the fmt.Errorf call.
Preserve the existing platform type when PlatformStatus is available and keep
the retry error behavior unchanged.
---
Nitpick comments:
In `@pkg/operator/sync.go`:
- Line 347: Update syncCloudConfig to accept a context.Context and pass that
context to wait.PollUntilContextTimeout instead of context.TODO(). Update every
syncCloudConfig caller to provide and propagate the reconciliation context so
cancellation and deadlines reach the cloud config poll.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39d47c0b-19a0-4beb-a6f9-704f7438cf1b
📒 Files selected for processing (1)
pkg/operator/sync.go
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/8114c330-90e8-11f1-927e-f9475bf39886-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ffecdcb0-916a-11f1-8d4f-aa6785e535ca-0 |
|
/test perfscale-control-plane-6nodes |
|
/payload 5.0 nightly blocking |
|
@proietfb: trigger 14 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/34181e00-916b-11f1-940c-b8ea4e1b0d41-0 |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@proietfb: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/41fec5a0-916b-11f1-9c7e-f0c4648aa487-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/432e26a0-91a7-11f1-90f6-7733e196236e-0 |
57270d5 to
2b0d138
Compare
2b0d138 to
8a25c51
Compare
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 |
|
@proietfb: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/27eb7b00-94a3-11f1-89a1-adfe69a6db9a-0 |
openshift-ci-robot
added
the
jira/severity-moderate
|
@proietfb: This pull request references Jira Issue OCPBUGS-100292, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-gcp-op-part2 e2e-gcp-op-part1 e2e-gcp-op-ocl-part1 e2e-gcp-op-ocl-part2 e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages e2e-vsphere-zones-mco-bootimages perfscale-control-plane-6nodes |
|
/test tls-pqc-readiness |
|
@proietfb: The following tests failed, say
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. |
3 participants
MCO goes Degraded when Cluster API Machine resource is not found during node mapping
- What I did
Implemented a Poll with 3s timeout inside getOSImageURLsFromConfigMap() to tolerate transient not found errors and version mismatches, trying to prevent MCO from going Degraded on a recoverable race condition.
- How to verify it
run
periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3job. Errors on[Monitor:legacy-cvo-invariants][bz-Machine Config Operator] clusteroperator/machine-config should not change condition/Degradedwith reasonopenshift-config-managed/kube-cloud-config configmap is required on platform AWS but not found: configmap "kube-cloud-config" not foundshould not appear.Summary by CodeRabbit