CNTRLPLANE-3776: In-Place Config Refresh for NodePool Worker Nodes#2053
CNTRLPLANE-3776: In-Place Config Refresh for NodePool Worker Nodes#2053csrwng wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
csrwng
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@csrwng: This pull request references OCPSTRAT-3299 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. 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. |
csrwng
changed the title
muraee
left a comment
muraee
left a comment
There was a problem hiding this comment.
Review of the in-place config refresh enhancement. Five comments below — three substantive gaps, one API gap, one nit.
| 4. The config refresh controller in the HCCO detects the payload hash change | ||
| via updated annotations on Node/Machine objects. | ||
| 5. The controller computes the file-level diff between the current node config | ||
| and the target config. |
There was a problem hiding this comment.
Gap: How is the file-level diff computed?
This step says the controller "computes the file-level diff between the current node config and the target config" but doesn't explain where the "current" side comes from.
The target config is straightforward (token secret / user-data secret payload). But for the current config:
- For InPlace pools, MCD sets a
currentConfigannotation on the node — but that's just a hash, not the full ignition payload. You can't compute a file-level diff from a hash. - For Replace pools, there's no mechanism today for the HCCO to know what config a node is running.
The controller would need to either:
- Regenerate the old payload from the config version the node was built with — but old release images or MachineConfigs may have been garbage collected.
- Store the previous payload (ConfigMap, annotation, etc.) — extra storage, staleness risk.
- Read from the node via a privileged pod — adds a pre-flight step.
This is a fundamental implementation detail that should be explicitly specified.
There was a problem hiding this comment.
Addressed. The design has been revised so the controller no longer computes a file-level diff at all. Instead:
- The hash split classifies inputs (spec fields) into rollout hash vs payload hash at the NodePool controller level.
- The config refresh controller only detects that the payload hash changed while the rollout hash did not — this guarantees only refresh-eligible inputs changed.
- The controller sends the full updated ignition payload to MCD via the upgrade ConfigMap.
- MCD handles the file-level diff by comparing the desired config from the ConfigMap against the current config read from disk (
/etc/mcd-currentconfig.json) — this is MCD's existing behavior.
This sidesteps the "where does the current config come from" problem entirely — MCD already has it on disk. See the revised "Config Refresh Controller" and "Safety Invariant" sections.
|
|
||
| 1. If a Replace rollout is in progress for a NodePool, the config refresh | ||
| controller skips nodes that are pending replacement. | ||
| 2. Refresh annotations are separate from upgrade annotations, so the two |
There was a problem hiding this comment.
Gap: Interaction with the existing in-place upgrader needs more detail.
This section covers the Replace rollout case but doesn't address the InPlace upgrader interaction:
-
Simultaneous upgrade + refresh: If an InPlace-strategy node needs both a version upgrade (via the existing upgrader) and a config refresh, does the upgrade subsume the refresh? Do they race? Both would create MCD pods on the same node.
-
"Pending replacement" detection: The enhancement says "the config refresh controller skips nodes pending replacement," but MachineDeployment rollouts delete Machines — they don't annotate nodes as "pending." How does the controller know a node is about to be replaced?
The statement "Refresh annotations are separate from upgrade annotations, so the two mechanisms do not interfere" is not sufficient — separate annotations don't prevent two MCD pods from running on the same node simultaneously.
There was a problem hiding this comment.
Addressed. Added two new workflow sections:
"Interaction with concurrent Replace rollout" now specifies:
- Controller pauses refresh for the entire NodePool during active rollouts, detected via
MachineDeployment.status.updatedReplicas != status.replicasor Machines with deletion timestamps. - Controller checks for existing MCD pods on target nodes before creating new ones — prevents two MCD pods on the same node.
"Interaction with InPlace upgrade strategy" (new section) specifies:
- Version upgrades take precedence — the upgrade delivers the full config (including refresh-eligible changes), subsuming the refresh.
- Controller checks for active upgrade MCD pods before creating refresh MCD pods.
- After upgrade completes, controller re-evaluates whether refresh is still needed by comparing annotations.
| ] | ||
| ``` | ||
|
|
||
| The `no-reboot-paths` key contains a JSON array of objects, each specifying: |
There was a problem hiding this comment.
Security: threat model for no-reboot-paths ConfigMap tampering.
The no-reboot-paths key is effectively a privileged instruction to MCD: "don't reboot for these paths, execute these actions instead." If an attacker gains write access to the upgrade namespace/ConfigMap in the guest cluster, they could:
- Suppress reboots for changes that actually require them (e.g., adding a kernel arg path to
no-reboot-paths). - Inject arbitrary service restart/reload commands.
The enhancement should discuss:
- Who has write access to the
<pool>-upgradenamespace and its ConfigMaps in the guest cluster? - Should MCD independently validate paths against its own hardcoded reboot triggers (OS, kargs, FIPS, etc.) before honoring
no-reboot-paths, as a defense-in-depth measure? - This is especially relevant since the enhancement extends MCD pods to Replace-strategy pools for the first time, broadening the attack surface.
There was a problem hiding this comment.
Addressed. Added a "Security: no-reboot-paths Threat Model" subsection under Risks and Mitigations covering all three points:
-
Access control: The
<pool>-upgradenamespace uses restricted RBAC — only the HCCO service account has write access.cluster-adminaccess is expected and consistent with the existing threat model. -
Defense-in-depth in MCD: MCD independently validates hardcoded reboot triggers (
osUpdate,kargs,fips,kernelType,extensions) in themachineConfigDiffstruct before honoringno-reboot-paths. Even if an attacker adds a kernel arg path tono-reboot-paths, MCD still reboots because it detects the structural kargs change. -
Service name restriction: The
servicesfield is limited to a hardcoded set of valid service names in MCD (e.g.,crio,kubelet,update-ca-trust). Unrecognized names are rejected/ignored, preventing arbitrary service injection. -
Attack surface: Clarified that this does not broaden the MCD pod attack surface — the existing in-place upgrader already creates MCD pods using the same ConfigMap pattern. The
no-reboot-pathskey is additive and gated by MCD's defense-in-depth checks.
| 4. Changes that require a reboot are excluded from the refresh path and | ||
| deferred to the next Replace rollout. | ||
| 5. NodePool status reports the refresh state, indicating whether nodes are | ||
| up-to-date with the latest non-disruptive config or have a refresh pending. |
There was a problem hiding this comment.
Gap: NodePool status conditions for refresh state should be defined.
Goal #5 says "NodePool status reports the refresh state" and the GA graduation criteria include "NodePool status reporting for refresh state," but the enhancement doesn't define what conditions would be added.
At minimum, sketch the condition types (e.g., ConfigRefreshProgressing, ConfigRefreshDegraded, ConfigRefreshUpToDate), their semantics, and when they transition. This is an API contract that reviewers and downstream consumers need to evaluate.
There was a problem hiding this comment.
Addressed. Added a "NodePool Status Conditions" subsection under API Extensions defining three conditions:
ConfigRefreshProgressing:Truewhen MCD pods are running on nodes;messagereports progress (e.g., "Refreshing 2/5 nodes").ConfigRefreshComplete:Truewhen all nodes have been refreshed to the latest payload hash.ConfigRefreshDegraded:Truewhen refresh has failed on one or more nodes (MCD pod failure, timeout, post-apply action error);messageidentifies affected nodes and failure reason.
Also documented the 5 condition transition scenarios (new payload hash detected → progressing → per-node completion → all complete, failure case, post-Replace-rollout reset).
| | Change Category | File Path(s) | Post-Apply Action | | ||
| |----------------|-------------|-------------------| | ||
| | Management-side container image refs in static pod manifests | `/etc/kubernetes/manifests/kube-apiserver-proxy.yaml` | None | | ||
| | SSH authorized key changes | `passwd.users.sshAuthorizedKeys` | None | |
There was a problem hiding this comment.
nit: passwd.users.sshAuthorizedKeys is not a file path — it's an ignition passwd section entry. The allowlist is described elsewhere as mapping "file paths" to actions. Consider clarifying in the allowlist description that it covers both storage.files paths and specific ignition passwd section entries, so the safety gate logic accounts for both.
There was a problem hiding this comment.
Good catch — addressed. The "Eligible vs. Ineligible Changes" section now explicitly states:
The allowlist covers both
storage.filespaths and ignitionpasswdsection entries (e.g.,sshAuthorizedKeys).
The table entry for SSH keys now uses passwd.users[].sshAuthorizedKeys to make it clear this is an ignition passwd entry, not a file path. The "Built-in Path-to-Action Map" section (formerly "Built-in Allowlist") also references "all file paths (and ignition passwd entries)".
csrwng
changed the title
|
@csrwng: This pull request references CNTRLPLANE-3776 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
/jira refresh |
|
@csrwng: This pull request references CNTRLPLANE-3776 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
Introduces a config refresh mechanism (OCPSTRAT-3299) that delivers non-disruptive configuration changes to existing HyperShift worker nodes without node replacement, drain, or reboot. Uses the existing MCD pod pattern with an additional `no-reboot-paths` ConfigMap key to communicate safe file paths and post-apply actions to MCD. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
611db6c to
0b6a409
Compare
|
@csrwng: The following test 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. |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
I'm generally a bit hesitant on the current implementation, which has some nuances that I don't think it handles as well, plus it drifts between standalone and HCP behaviour quite a bit. Some initial questions/comments in line
| Rollout Control) introduces a hash split so that management-side changes no | ||
| longer trigger Replace rollouts. However, OCPSTRAT-3298's hash split only | ||
| covers management-side content (CPO-generated configs, HAProxy image refs, | ||
| etc.). User-driven HostedCluster spec changes — such as SSH key rotations, |
There was a problem hiding this comment.
A few notes as I read this (maybe covered later in the doc - will update)
-
This is (looks like deliberately) different behaviour compared to the MCO, where we don't split the actual content, but instead just calculate the diff in the MCD when upgrading and compare against user+system provided NDPs. I think we should consider whether it's worth matching those behaviours so we're not introducing additional drift between MCO and Hypershift.
-
It seems to imply that the use cases covered by 3298 could use this mechanism as well, just that those changes would get written but not take effect until a spec-change-reboot? Or would changes covered by 3298 cause disruption directly (e.g. static pod definitions that would cause restarts)
-
Can the user specify a "only allow non-disruptive diffs" mode? If the hypershift controller sees multiple changes (some disruptive, some not) how does it determine whether/how to apply those? Does that happen in parallel or are they all done by the main disruptive path?
-
Some of the use cases here also have varying levels of disruption to the cluster in the MCO. Are you planning on implementing different mechanisms to handle different updates?
-
The MCO has two paths: a cert writer and a MCD updater. The cert writer isn't implemented in Hypershift at all so those certs, despite not being part of machineconfig objects anymore, are rotated live. Do you plan on implementing that as well?
There was a problem hiding this comment.
I see a Mixed changes section below that answers the second half of 3, and I guess the first half of 3 is "no"
| 3. The mechanism guarantees no node reboots by classifying inputs into rollout | ||
| hash (reboot-requiring) and payload hash (refresh-eligible) categories, with | ||
| defense-in-depth validation in MCD. | ||
| 4. Changes that require a reboot are excluded from the refresh path and |
There was a problem hiding this comment.
(as asked above) how does this deferral get determined?
| 2. Changing the behavior of the InPlace upgrade strategy for full version | ||
| upgrades. The config refresh is a complementary mechanism for non-disruptive | ||
| changes, not a replacement for the existing upgrade lifecycle. | ||
| 3. User-facing API changes to NodePool spec. The config refresh mechanism is |
There was a problem hiding this comment.
The MCO does allow user NDPs, it sounds like that wouldn't be the case for Hypershift?
| type, extensions, OS image. | ||
| - **Payload-only inputs** (changes eligible for in-place refresh): | ||
| `spec.sshKey`, `spec.pullSecret`, `spec.additionalTrustBundle`, | ||
| `spec.imageContentSources`, `spec.configuration.proxy`, |
There was a problem hiding this comment.
Hmm, is imageContentSources ICSP? Or something else entirely?
I thought we switched over to IDMS/ITMS generally speaking (and there's additional logic to these changes in the MCO, not always disruptionless)
| - **Rollout hash inputs** (changes trigger node replacement): release image, | ||
| NodePool `.config` (user MachineConfigs), FIPS mode, kernel arguments, kernel | ||
| type, extensions, OS image. | ||
| - **Payload-only inputs** (changes eligible for in-place refresh): |
There was a problem hiding this comment.
(As asked above) do we plan on handling certificate refreshes?
| configuration (including both spec-driven and refresh-eligible changes), | ||
| so no post-rollout refresh is needed for those nodes. | ||
|
|
||
| #### Interaction with InPlace upgrade strategy |
There was a problem hiding this comment.
Does the inplace upgrader/replace upgrader "wait for a refresh to finish"? It sounds so far that the guard is one directional - the refresh controller waits for the an ongoing upgrade, but if it's already refreshing (and is slow) I think there's a possibility of conflict the other direction.
|
|
||
| #### ConfigMap `no-reboot-paths` Key | ||
|
|
||
| Today, the in-place upgrader creates a ConfigMap in the guest cluster with two |
There was a problem hiding this comment.
Some of these at least are shared with the MCO. Would you be able to take the base MCO list and build on it instead of completely redefining?
| - `action`: Post-apply action type (`None`, `Restart`, `Reload`) | ||
| - `services`: List of services to restart/reload (when action is not `None`) | ||
|
|
||
| Paths already in MCD's legacy safe list (e.g., `/var/lib/kubelet/config.json`, |
There was a problem hiding this comment.
I forget if that's the case, but I think we should depend on the nodedisruptionpolicy list for these so they're always up to date.
Also, many certificate paths are handled differently, I think /var/lib/kubelet/config.json is maybe one of them?
| 4. **Create ConfigMap**: Creates the upgrade ConfigMap in the guest cluster with | ||
| the full ignition payload (`config`), hash (`hash`), and the | ||
| `no-reboot-paths` key containing entries for all refresh-eligible file paths | ||
| and their post-apply actions. The `no-reboot-paths` list is a static, |
There was a problem hiding this comment.
(also asked above) so if we do see a diff with some refresh paths and some non-refresh paths, do we provide the MCD with just the refresh paths? Or we just do the "full reboot upgrade" workflow here?
| unexpected full-fleet node replacements from management-side image bumps — and | ||
| would leave no path to deliver non-disruptive changes without node replacement. | ||
|
|
||
| ### Node Disruption Policies (MachineConfiguration resource) |
There was a problem hiding this comment.
Even if we don't use the API, I think it would be best if we can share some of the logic so it's not a completely separate implementation as designed right now
4 participants
Summary
Enhancement proposal for OCPSTRAT-3299 — In-Place Config Refresh for NodePool Worker Nodes.
no-reboot-pathsConfigMap key to communicate safe file paths and their post-apply actions to MCD, requiring a small targeted change to MCD's HyperShift code path.Test plan
make lint)🤖 Generated with Claude Code