OCPBUGS-100366: Re-queue ContainerRuntimeConfig on status update failure

[nispriha]

Fixes: OCPBUGS-100366

- What I did

During SNO bootstrap, the ContainerRuntimeConfig controller's syncStatusOnly function silently swallows status write failures. When the status write fails with a 409 Conflict (caused by a concurrent finalizer patch bumping the resourceVersion), the function logs a warning but returns nil. The workqueue considers the item done and never re-queues it. The CR is left without .status (observedGeneration=0 while generation=1), causing the render controller to loop forever on: "status for ContainerRuntimeConfig enable-crun-master is being reported for 0, expecting it for 1".

Fix: return statusUpdateErr when the original sync error is nil, so the item is re-queued and the status write succeeds on the next sync (by which time the informer cache has caught up).

- How to verify it

• TestStatusUpdateConflictRequeues — injects a 409 Conflict on UpdateStatus and verifies syncHandler returns an error (fails without the fix, passes with it)
• All existing container-runtime-config tests pass (no regressions)
• Deploy on a 5.0 cluster, create a ContainerRuntimeConfig, simulate stuck status with oc patch ctrcfg <name> --type=merge --subresource=status -p '{"status":{"observedGeneration":0,"conditions":[]}}', verify the controller recovers

- Description for the changelog

Fix ContainerRuntimeConfig controller silently swallowing status update failures, which could permanently block the render controller during SNO bootstrap.

Summary by CodeRabbit

• Bug Fixes

  • Fixed an issue where status update failures could be silently ignored.
  • Conflicts during status updates now return an error so the operation can be retried.
  • Preserved the original synchronization error when one occurs.

• Tests

  • Added regression coverage for status update conflicts across supported platforms.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@nispriha: This pull request references Jira Issue OCPBUGS-100366, which is invalid:

• expected the bug to target the "5.1.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Fixes: OCPBUGS-100366

- What I did

During SNO bootstrap, the ContainerRuntimeConfig controller's syncStatusOnly function silently swallows status write failures. When the status write fails with a 409 Conflict (caused by a concurrent finalizer patch bumping the resourceVersion), the function logs a warning but returns nil. The workqueue considers the item done and never re-queues it. The CR is left without .status (observedGeneration=0 while generation=1), causing the render controller to loop forever on: "status for ContainerRuntimeConfig enable-crun-master is being reported for 0, expecting it for 1".

Fix: return statusUpdateErr when the original sync error is nil, so the item is re-queued and the status write succeeds on the next sync (by which time the informer cache has caught up).

- How to verify it

• TestStatusUpdateConflictRequeues — injects a 409 Conflict on UpdateStatus and verifies syncHandler returns an error (fails without the fix, passes with it)
• All existing container-runtime-config tests pass (no regressions)
• Deploy on a 5.0 cluster, create a ContainerRuntimeConfig, simulate stuck status with oc patch ctrcfg <name> --type=merge --subresource=status -p '{"status":{"observedGeneration":0,"conditions":[]}}', verify the controller recovers

- Description for the changelog

Fix ContainerRuntimeConfig controller silently swallowing status update failures, which could permanently block the render controller during SNO bootstrap.

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

• pkg/controller/container-runtime-config/OWNERS

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 63bcc236-c687-4349-9a7d-688346cc4de2

📥 Commits

Reviewing files that changed from the base of the PR and between 6f70415 and fb50adc.

📒 Files selected for processing (2)

• pkg/controller/container-runtime-config/container_runtime_config_controller.go
• pkg/controller/container-runtime-config/container_runtime_config_controller_test.go

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

---

Walkthrough

The controller now propagates status update errors when the original sync succeeds. A regression test verifies that a 409 Conflict returned during status update causes syncHandler to return an error.

Changes

Status Error Propagation

Layer / File(s)	Summary
Status update error handling pkg/controller/container-runtime-config/container_runtime_config_controller.go, pkg/controller/container-runtime-config/container_runtime_config_controller_test.go	syncStatusOnly returns the status update error when no prior sync error exists. The regression test injects a 409 Conflict and verifies that syncHandler returns an error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to fb50a

The change re-queues failed ContainerRuntimeConfig status updates, with no actionable merge-blocking risk remaining after normal checks and review.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The PR adds a static Go test name and stable platform subtest names. No Ginkgo tests or dynamic pod, namespace, node, timestamp, UUID, IP, or generated identifier appears in a title.
Test Structure And Quality	✅ Passed	The added test checks one behavior, uses the existing fake-client fixture, creates no cluster resources, has a diagnostic failure message, and adds no indefinite Eventually/Consistently wait.
Microshift Test Compatibility	✅ Passed	The added regression test is a standard Go func Test...(*testing.T) unit test in pkg/controller/..., not a new Ginkgo e2e test; the MicroShift compatibility check does not apply.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The added regression test is a standard Go testing test, not a new Ginkgo e2e test; it uses fake clients and MCP objects without multi-node or HA assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The parent diff only changes status-error propagation and adds a conflict test; it introduces no affinity, topology spread, replica, node, taint, or PDB scheduling constraints.
Ote Binary Stdout Contract	✅ Passed	The diff adds no stdout writes or process-level setup; it changes controller error handling and adds a unit test. The existing klog warning line is unchanged and is not entrypoint code.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The added regression test is a standard Go func Test using fake clients, not a Ginkgo e2e test; it adds no IPv4 literals or external network calls.
No-Weak-Crypto	✅ Passed	The PR changes status error propagation and adds a 409 test; the added code contains no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom crypto, or secret comparisons.
Container-Privileges	✅ Passed	The PR changes only Go controller logic and a regression test; the exact diff adds no privileged, host namespace, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs	✅ Passed	The diff adds no logging. It only returns statusUpdateErr; the existing warning logs a generic API error and exposes no password, token, key, PII, hostname, or customer data.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: re-queuing ContainerRuntimeConfig items when status updates fail.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[openshift-ci]

@nispriha: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/perfscale-control-plane-6nodes	fb50adc	link	false	/test perfscale-control-plane-6nodes

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[nispriha]

/jira refresh

[openshift-ci-robot]

@nispriha: This pull request references Jira Issue OCPBUGS-100366, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.1.0) matches configured target version for branch (5.1.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

/jira refresh

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.

[ngopalak-redhat]

There are 2 other places in the same file where I see if statusUpdateErr != nil . Can you check if the fix is applicable to those code paths also?

[ngopalak-redhat]

Can you try what happens if we do:

newcfg, getErr := ctrl.client.MachineconfigurationV1().ContainerRuntimeConfigs().Get(
      context.TODO(), cfg.Name, metav1.GetOptions{})

That is get the data from API server rather than the internal cache. I may be wrong, but I'm thinking we might not get 409 error as its a new object from API server.

[QiWang19]

The fix makes sense to me. And I agree we can try to retrieve the objects from the API server instead of the mccrLister cache.