OCPBUGS-99751: Verify MOSC existence via API before serving layered image by umohnani8 · Pull Request #6352 · openshift/machine-config-operator

umohnani8 · 2026-07-30T13:58:05Z

Closes https://redhat.atlassian.net/browse/OCPBUGS-99751

- What I did

The MCS runs as a DaemonSet with per-pod informer caches. When a MOSC is deleted (Image Mode disabled), different pods receive the watch event at different times. A new node hitting a stale pod gets the wrong OS image, leading to a double pivot that can leave the node NotReady.

Add a direct API verification in resolveDesiredImageForPool: after finding a MOSC in the informer cache, do a live GET to confirm it still exists before serving the layered image. This closes the race window with negligible latency cost (one small-object GET, only when a MOSC is cached for the pool).

- How to verify it

Opt a pool into layering then opt it out. Scale up a new node and it to that pool, the node should not be using the layered image and should stay Ready after being added to the pool.

- Description for the changelog
Verify MOSC existence via API before serving layered image

Summary by CodeRabbit

Bug Fixes
- Improved layered-image handling when configuration data is removed but stale cached information remains.
- Prevented layered images from being served when their associated configuration no longer exists.
- Added safer fallback behavior when current configuration data cannot be retrieved.
- Prevented configuration updates from unintentionally altering cached data or retaining outdated layered-image information.

openshift-merge-bot · 2026-07-30T13:58:07Z

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-robot · 2026-07-30T13:58:12Z

@umohnani8: This pull request references Jira Issue OCPBUGS-99751, 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.0.0) matches configured target version for branch (5.0.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

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

Details

In response to this:

Closes https://redhat.atlassian.net/browse/OCPBUGS-99751

- What I did

The MCS runs as a DaemonSet with per-pod informer caches. When a MOSC is deleted (Image Mode disabled), different pods receive the watch event at different times. A new node hitting a stale pod gets the wrong OS image, leading to a double pivot that can leave the node NotReady.

Add a direct API verification in resolveDesiredImageForPool: after finding a MOSC in the informer cache, do a live GET to confirm it still exists before serving the layered image. This closes the race window with negligible latency cost (one small-object GET, only when a MOSC is cached for the pool).

- How to verify it

Opt a pool into layering then opt it out. Scale up a new node and it to that pool, the node should not be using the layered image and should stay Ready after being added to the pool.

- Description for the changelog
Verify MOSC existence via API before serving layered image

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.

umohnani8 · 2026-07-30T13:58:18Z

/pipeline required

openshift-merge-bot · 2026-07-30T13:58:21Z

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-upgrade
/test e2e-gcp-op-ocl-part1
/test e2e-gcp-op-ocl-part2
/test e2e-gcp-op-part1
/test e2e-gcp-op-part2
/test e2e-gcp-op-single-node
/test e2e-hypershift
/test tls-pqc-readiness

coderabbitai · 2026-07-30T13:59:40Z

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: c81a48a5-128b-40f3-9c72-f0c5cf5a84d2

📥 Commits

Reviewing files that changed from the base of the PR and between 28918b0 and 70e9f45.

📒 Files selected for processing (2)

pkg/server/cluster_server.go
pkg/server/server_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

pkg/server/cluster_server.go

Walkthrough

The cluster server now verifies cached MachineOSConfig objects through the Machine Configuration API before resolving layered images. It deep-copies cached MachineConfig objects before mutation. Tests cover deleted API objects and informer-cache immutability.

Changes

Layered-image consistency

Layer / File(s)	Summary
Server verification and cache-safe configuration copying `pkg/server/cluster_server.go`	Initializes a Machine Configuration client, verifies `MachineOSConfig` existence, suppresses layered images for deleted objects, logs other verification errors, and deep-copies cached `MachineConfig` objects before mutation.
Stale-cache regression coverage `pkg/server/server_test.go`	Injects fake Machine Configuration clients and verifies API deletion handling, base-image fallback, and informer-cache immutability.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: proietfb, isabella-janssen

Sequence Diagram(s)

sequenceDiagram
  participant ConfigRequest
  participant clusterServer
  participant MOSCLister
  participant MachineConfigurationAPI
  ConfigRequest->>clusterServer: GetConfig
  clusterServer->>MOSCLister: Read cached MachineOSConfig
  clusterServer->>MachineConfigurationAPI: Verify MachineOSConfig
  MachineConfigurationAPI-->>clusterServer: Object or NotFound
  clusterServer-->>ConfigRequest: Layered image or base image

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	The new Warningf logs raw MachineOSConfig GET errors; client-go transport errors include the request URL, which can expose an internal API hostname.	Log only a sanitized error category or status. Do not include the raw client-go error or request URL.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: verifying MOSC existence through the API before serving a layered image.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 added test title and function name are static descriptive strings; no generated names, timestamps, node/namespace values, or runtime-composed titles were introduced.
Test Structure And Quality	✅ Passed	The changed tests are standard Go/testify unit tests, not Ginkgo tests. They use synchronous fake clients, create no cluster resources, and contain no Eventually or Consistently waits.
Microshift Test Compatibility	✅ Passed	The added coverage uses standard Go testing.T functions and fake clients, not Ginkgo e2e tests; the diff adds no unavailable MicroShift APIs, namespaces, or unsupported assumptions.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The diff adds only Go testing tests in pkg/server/server_test.go; it adds no Ginkgo e2e It, Describe, Context, or When tests, so SNO compatibility criteria do not apply.
Topology-Aware Scheduling Compatibility	✅ Passed	The diff changes only MCS API/cache logic and tests. It adds no replicas, affinity, topology spread, node selectors, tolerations, PDBs, or deployment manifests.
Ote Binary Stdout Contract	✅ Passed	The diff adds no OTE entry-point or stdout writes; new klog calls are in clusterServer, and machine-config-server sets logtostderr=true before creating it.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds only standard Go Test functions, not Ginkgo e2e tests. Tests use fake clients and image strings; they make no IPv4 assumptions or external network connections.
No-Weak-Crypto	✅ Passed	The PR adds API verification, deep-copy logic, and tests. The diff introduces no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom crypto, or secret/token comparisons.
Container-Privileges	✅ Passed	The PR changes only Go source and tests; the diff adds no Kubernetes/container manifests or privilege settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalat...

✨ 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

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

openshift-ci-robot · 2026-07-30T14:00:37Z

@umohnani8: This pull request references Jira Issue OCPBUGS-99751, which is valid.

3 validation(s) were run on this bug

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

Details

In response to this:

Closes https://redhat.atlassian.net/browse/OCPBUGS-99751

- What I did

The MCS runs as a DaemonSet with per-pod informer caches. When a MOSC is deleted (Image Mode disabled), different pods receive the watch event at different times. A new node hitting a stale pod gets the wrong OS image, leading to a double pivot that can leave the node NotReady.

Add a direct API verification in resolveDesiredImageForPool: after finding a MOSC in the informer cache, do a live GET to confirm it still exists before serving the layered image. This closes the race window with negligible latency cost (one small-object GET, only when a MOSC is cached for the pool).

- How to verify it

Opt a pool into layering then opt it out. Scale up a new node and it to that pool, the node should not be using the layered image and should stay Ready after being added to the pool.

- Description for the changelog
Verify MOSC existence via API before serving layered image

Summary by CodeRabbit

Bug Fixes

Improved layered-image handling when configuration data is removed but stale cached information remains.

Prevented serving layered images when the associated configuration no longer exists in the API.

Added safer fallback behavior when current configuration data cannot be retrieved.

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 · 2026-07-30T14:01:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [umohnani8]

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

coderabbitai

Actionable comments posted: 1

🤖 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/server/cluster_server.go`:
- Around line 300-307: Update the MachineOSConfig lookup in the layered-image
path of pkg/server/cluster_server.go: use a bounded context with a deadline
instead of context.TODO(), and return an empty image for every Get error,
including non-NotFound failures. Add a reactor in pkg/server/server_test.go
covering a non-NotFound MachineOSConfig Get error and assert that no layered
image is returned.

🪄 Autofix (Beta)

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: 6a8e2c2a-a78e-49fb-9c46-0055645c6fc7

📥 Commits

Reviewing files that changed from the base of the PR and between d6688c6 and 28918b0.

📒 Files selected for processing (2)

pkg/server/cluster_server.go
pkg/server/server_test.go

umohnani8 · 2026-07-30T19:01:52Z

/retest

sergiordlr · 2026-07-31T14:20:26Z

The fix is not working. This is the AI analysis of the failure

Root Cause: Why PR #6352 doesn't fix the problem                                                                                                                                                                 
                                                                                                                                                                                                                   
  The PR correctly prevents resolveDesiredImageForPool from returning the layered image after the MOSC is deleted. But the bug is actually a shared informer cache mutation.                                       
                                                                                                                                                                                                                   
  Here's what happens:                                                                                                                                                                                             
                                                                                                                                                                                                                   
  1. 12:52 — MOSC exists. resolveDesiredImageForPool returns quay.io/mcoqe/layering@sha256:19c2873a.... The appendDesiredOSImage function runs and mutates the in-memory machineconfig object:                     
  mc.Spec.OSImageURL = desired  // mutates the shared cached object!                                                                                                                                               
  1. The mc object was retrieved via cs.machineConfigLister.Get(currConf), which returns a shared pointer from the informer cache. This mutation permanently changes the cached MC's osImageURL to the layered     
  image.                                                                                                                                                                                                           
  2. 13:15+ (and still now) — MOSC is deleted. The PR fix correctly returns desiredImage="". appendDesiredOSImage is a no-op. But the mc object from the lister already has Spec.OSImageURL =                      
  "quay.io/mcoqe/layering@..." because it was mutated at 12:52. appendEncapsulated serializes this mutated MC into the encapsulated file.                                                                          
                                                                                                                                                                                                                   
  The fix needs to either:                                                                                                                                                                                         
  - DeepCopy the MC before mutating it: mc = mc.DeepCopy() before the appenders run                                                                                                                                
  - Or explicitly reset mc.Spec.OSImageURL back to the original when desiredImage="" in appendDesiredOSImage                                                                                                       
  - Or never mutate the cached object — write the override only into the encapsulated MC copy

We can find a must gather file with the journal logs for the failed nodes and the served ignition config for the worker pool in this jira comment https://redhat.atlassian.net/browse/OCPBUGS-99751?focusedCommentId=17811358

…mage The MCS runs as a DaemonSet with per-pod informer caches. When a MOSC is deleted (Image Mode disabled), different pods receive the watch event at different times. A new node hitting a stale pod gets the wrong OS image, leading to a double pivot that can leave the node NotReady. Add a direct API verification in resolveDesiredImageForPool: after finding a MOSC in the informer cache, do a live GET to confirm it still exists before serving the layered image. This closes the race window with negligible latency cost (one small-object GET, only when a MOSC is cached for the pool). Signed-off-by: Urvashi <umohnani@redhat.com>

umohnani8 · 2026-08-12T18:01:05Z

/pipeline required

openshift-merge-bot · 2026-08-12T18:01:08Z

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-upgrade
/test e2e-gcp-op-ocl-part1
/test e2e-gcp-op-ocl-part2
/test e2e-gcp-op-part1
/test e2e-gcp-op-part2
/test e2e-gcp-op-single-node
/test e2e-hypershift
/test tls-pqc-readiness

openshift-ci-robot · 2026-08-12T18:13:09Z

@umohnani8: This pull request references Jira Issue OCPBUGS-99751, 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.0.0) matches configured target version for branch (5.0.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

Closes https://redhat.atlassian.net/browse/OCPBUGS-99751

- What I did

The MCS runs as a DaemonSet with per-pod informer caches. When a MOSC is deleted (Image Mode disabled), different pods receive the watch event at different times. A new node hitting a stale pod gets the wrong OS image, leading to a double pivot that can leave the node NotReady.

Add a direct API verification in resolveDesiredImageForPool: after finding a MOSC in the informer cache, do a live GET to confirm it still exists before serving the layered image. This closes the race window with negligible latency cost (one small-object GET, only when a MOSC is cached for the pool).

- How to verify it

Opt a pool into layering then opt it out. Scale up a new node and it to that pool, the node should not be using the layered image and should stay Ready after being added to the pool.

- Description for the changelog
Verify MOSC existence via API before serving layered image

Summary by CodeRabbit

Bug Fixes

Improved layered-image handling when configuration data is removed but stale cached information remains.

Prevented layered images from being served when their associated configuration no longer exists.

Added safer fallback behavior when current configuration data cannot be retrieved.

Prevented configuration updates from unintentionally altering cached data or retaining outdated layered-image information.

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 · 2026-08-13T00:57:45Z

@umohnani8: The following tests 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/unit	`70e9f45`	link	true	`/test unit`
ci/prow/e2e-aws-ovn-upgrade	`70e9f45`	link	true	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-gcp-op-single-node	`70e9f45`	link	true	`/test e2e-gcp-op-single-node`
ci/prow/perfscale-control-plane-6nodes	`70e9f45`	link	false	`/test perfscale-control-plane-6nodes`
ci/prow/e2e-gcp-op-part2	`70e9f45`	link	true	`/test e2e-gcp-op-part2`
ci/prow/e2e-gcp-op-part1	`70e9f45`	link	true	`/test e2e-gcp-op-part1`
ci/prow/e2e-gcp-op-ocl-part2	`70e9f45`	link	true	`/test e2e-gcp-op-ocl-part2`
ci/prow/e2e-gcp-op-ocl-part1	`70e9f45`	link	true	`/test e2e-gcp-op-ocl-part1`

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.

openshift-ci Bot requested review from isabella-janssen and proietfb July 30, 2026 14:00

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2026

coderabbitai Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread pkg/server/cluster_server.go Outdated

umohnani8 force-pushed the OCPBUGS-99751-fix-stale-mcs-cache branch from 28918b0 to 70e9f45 Compare August 12, 2026 18:00

Uh oh!

Conversation

umohnani8 commented Jul 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

openshift-merge-bot Bot commented Jul 30, 2026

Uh oh!

openshift-ci-robot commented Jul 30, 2026

Uh oh!

umohnani8 commented Jul 30, 2026

Uh oh!

openshift-merge-bot Bot commented Jul 30, 2026

Uh oh!

coderabbitai Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Pre-merge checks failed

❌ Failed checks (1 error)

Uh oh!

openshift-ci-robot commented Jul 30, 2026

Summary by CodeRabbit

Uh oh!

openshift-ci Bot commented Jul 30, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

umohnani8 commented Jul 30, 2026

Uh oh!

sergiordlr commented Jul 31, 2026

Uh oh!

umohnani8 commented Aug 12, 2026

Uh oh!

openshift-merge-bot Bot commented Aug 12, 2026

Uh oh!

openshift-ci-robot commented Aug 12, 2026

Summary by CodeRabbit

Uh oh!

openshift-ci Bot commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

umohnani8 commented Jul 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading