[NO-ISSUE] Wait for pull-secret rollout to MCP master before boot returns ready - #37
[NO-ISSUE] Wait for pull-secret rollout to MCP master before boot returns ready#37eliorerz wants to merge 1 commit into
Conversation
recert writes kubelet's config.json straight to disk instead of through MCO, so machine-config-daemon can boot believing the rendered-master MachineConfig's content doesn't match what's on disk and latch Degraded. Until MachineConfigPool master reports Updated, the node's kubelet may still be running with the stale/empty authfile baked into the snapshot, so anything that pulls an authenticated image right after boot (metal3, marketplace catalogs, etc.) can race that rollout and fail. Force a resync and wait for MCP master to report Updated, with the same stuck-Degraded self-heal used by infra/netris/roles/restore-snapshot for this exact recert side effect, before cmd_boot reports the cluster ready. Signed-off-by: Elior Erez <eerez@redhat.com>
Walkthrough
ChangesPull-secret rollout during boot
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_boot
participant machine-config-daemon
participant MCP_master
participant node
cmd_boot->>machine-config-daemon: force resynchronization
cmd_boot->>MCP_master: poll Updated condition
MCP_master-->>cmd_boot: return rollout status
cmd_boot->>node: clear and restate annotations
cmd_boot->>machine-config-daemon: delete daemon pods
cmd_boot->>MCP_master: poll Updated condition again
MCP_master-->>cmd_boot: return final rollout status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@cluster-tool`:
- Around line 1126-1128: Update the timeout text in the pull-secret rollout
handling near the synced check to report the actual maximum wait of 25 minutes
instead of 30 minutes, while preserving the existing exit behavior and message
context.
- Line 1216: Update the completion message in the operator-waiting flow around
print_step to remove the tautological len(items)/len(items) count, replacing it
with concise wording that reports completion without redundant totals.
- Around line 1109-1119: In the unsynced recovery block, remove the `env.run_vm`
node lookup and use the already-known `vm_name` directly as the node identifier
in the patch command. Keep the existing patch and machine-config-daemon pod
deletion behavior unchanged.
- Around line 1077-1079: Remove the redundant kc_path and oc_vm assignments in
the “Waiting for pull-secret rollout” section of cmd_boot, reusing the identical
variables already initialized earlier in the same function. Preserve the
existing command execution behavior and single source of truth for the
kubeconfig path.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70b5dad8-e59d-4894-a789-29daf41d3e2b
📒 Files selected for processing (2)
cluster-tooltest_cluster_tool.py
| print_step(7, total, "Waiting for pull-secret rollout") | ||
| kc_path = "/etc/kubernetes/static-pod-resources/kube-apiserver-certs/secrets/node-kubeconfigs/lb-ext.kubeconfig" | ||
| oc_vm = f"sudo oc --kubeconfig={kc_path} --insecure-skip-tls-verify" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Redundant kc_path/oc_vm redefinition.
These are already defined identically a few lines earlier (lines 1066-1067) in the same function scope and remain valid here. Recomputing them is dead duplication that can drift if the kubeconfig path ever changes (it's now defined 3 times in cmd_boot).
♻️ Proposed fix
print_step(7, total, "Waiting for pull-secret rollout")
- kc_path = "/etc/kubernetes/static-pod-resources/kube-apiserver-certs/secrets/node-kubeconfigs/lb-ext.kubeconfig"
- oc_vm = f"sudo oc --kubeconfig={kc_path} --insecure-skip-tls-verify"
-
# recert writes kubelet's config.json straight to disk instead of📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print_step(7, total, "Waiting for pull-secret rollout") | |
| kc_path = "/etc/kubernetes/static-pod-resources/kube-apiserver-certs/secrets/node-kubeconfigs/lb-ext.kubeconfig" | |
| oc_vm = f"sudo oc --kubeconfig={kc_path} --insecure-skip-tls-verify" | |
| print_step(7, total, "Waiting for pull-secret rollout") |
🤖 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 `@cluster-tool` around lines 1077 - 1079, Remove the redundant kc_path and
oc_vm assignments in the “Waiting for pull-secret rollout” section of cmd_boot,
reusing the identical variables already initialized earlier in the same
function. Preserve the existing command execution behavior and single source of
truth for the kubeconfig path.
| if not synced: | ||
| r = env.run_vm(vm_ip, f"{oc_vm} get nodes -o jsonpath='{{.items[0].metadata.name}}'", identity=key_path) | ||
| node_name = r.stdout.strip() | ||
| env.run_vm( | ||
| vm_ip, | ||
| f"{oc_vm} patch node {node_name} --type merge --patch " | ||
| f"'{{\"metadata\":{{\"annotations\":{{\"machineconfiguration.openshift.io/reason\":\"\"," | ||
| f"\"machineconfiguration.openshift.io/state\":\"Done\"}}}}}}'", | ||
| identity=key_path, | ||
| ) | ||
| env.run_vm(vm_ip, f"{oc_vm} delete pod -n openshift-machine-config-operator -l k8s-app=machine-config-daemon", identity=key_path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unnecessary, fragile API call to look up the node name; vm_name is already known.
node_name is fetched via oc get nodes -o jsonpath=... with the default check=True. If this single call fails transiently (plausible right after forcing an MCD resync, which can itself disrupt the API briefly on SNO), the whole self-heal path aborts and the entire clone gets rolled back for what could be a transient blip — an unguarded external call on a recovery path.
This lookup is also redundant: vm_name (test-infra-cluster-{clone_id}-master-0, set at line 892) is explicitly set as the node's hostname earlier via hostnamectl set-hostname in the recert fix-up script (line 1030), so it's already known and guaranteed to match. Using it directly removes both the fragility and the extra round trip.
🔧 Proposed fix
if not synced:
- r = env.run_vm(vm_ip, f"{oc_vm} get nodes -o jsonpath='{{.items[0].metadata.name}}'", identity=key_path)
- node_name = r.stdout.strip()
env.run_vm(
vm_ip,
- f"{oc_vm} patch node {node_name} --type merge --patch "
+ f"{oc_vm} patch node {vm_name} --type merge --patch "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not synced: | |
| r = env.run_vm(vm_ip, f"{oc_vm} get nodes -o jsonpath='{{.items[0].metadata.name}}'", identity=key_path) | |
| node_name = r.stdout.strip() | |
| env.run_vm( | |
| vm_ip, | |
| f"{oc_vm} patch node {node_name} --type merge --patch " | |
| f"'{{\"metadata\":{{\"annotations\":{{\"machineconfiguration.openshift.io/reason\":\"\"," | |
| f"\"machineconfiguration.openshift.io/state\":\"Done\"}}}}}}'", | |
| identity=key_path, | |
| ) | |
| env.run_vm(vm_ip, f"{oc_vm} delete pod -n openshift-machine-config-operator -l k8s-app=machine-config-daemon", identity=key_path) | |
| if not synced: | |
| env.run_vm( | |
| vm_ip, | |
| f"{oc_vm} patch node {vm_name} --type merge --patch " | |
| f"'{{\"metadata\":{{\"annotations\":{{\"machineconfiguration.openshift.io/reason\":\"\"," | |
| f"\"machineconfiguration.openshift.io/state\":\"Done\"}}}}}}'", | |
| identity=key_path, | |
| ) | |
| env.run_vm(vm_ip, f"{oc_vm} delete pod -n openshift-machine-config-operator -l k8s-app=machine-config-daemon", identity=key_path) |
🤖 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 `@cluster-tool` around lines 1109 - 1119, In the unsynced recovery block,
remove the `env.run_vm` node lookup and use the already-known `vm_name` directly
as the node identifier in the patch command. Keep the existing patch and
machine-config-daemon pod deletion behavior unchanged.
| if not synced: | ||
| sys.exit("MachineConfigPool 'master' not Updated after pull-secret rollout (30 minutes).") | ||
| print_step(7, total, "Waiting for pull-secret rollout", "done") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Timeout message overstates the actual wait.
The exit message says "(30 minutes)" but the actual worst case is 90×10s + 60×10s = 1500s = 25 minutes. This can mislead on-call responders diagnosing a stuck boot.
📝 Proposed fix
- sys.exit("MachineConfigPool 'master' not Updated after pull-secret rollout (30 minutes).")
+ sys.exit("MachineConfigPool 'master' not Updated after pull-secret rollout (25 minutes).")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not synced: | |
| sys.exit("MachineConfigPool 'master' not Updated after pull-secret rollout (30 minutes).") | |
| print_step(7, total, "Waiting for pull-secret rollout", "done") | |
| if not synced: | |
| sys.exit("MachineConfigPool 'master' not Updated after pull-secret rollout (25 minutes).") | |
| print_step(7, total, "Waiting for pull-secret rollout", "done") |
🤖 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 `@cluster-tool` around lines 1126 - 1128, Update the timeout text in the
pull-secret rollout handling near the synced check to report the actual maximum
wait of 25 minutes instead of 30 minutes, while preserving the existing exit
behavior and message context.
| detail = "\n".join(f" - {u}" for u in last_unhealthy) | ||
| sys.exit(f"Operators not healthy after 20 minutes.\nUnhealthy operators:\n{detail}") | ||
| print_step(8, total, "Waiting for operators", f"done ({len(items)}/{len(items)})") | ||
| print_step(9, total, "Waiting for operators", f"done ({len(items)}/{len(items)})") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tautological (N/N) in the completion message.
The preceding loop only breaks when last_unhealthy is empty (i.e., all items are already healthy), so len(items)}/{len(items)} always prints an identical numerator and denominator, e.g. done (3/3). It conveys no real signal beyond the total count.
📝 Proposed fix
- print_step(9, total, "Waiting for operators", f"done ({len(items)}/{len(items)})")
+ print_step(9, total, "Waiting for operators", f"done ({len(items)} operators)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print_step(9, total, "Waiting for operators", f"done ({len(items)}/{len(items)})") | |
| print_step(9, total, "Waiting for operators", f"done ({len(items)} operators)") |
🤖 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 `@cluster-tool` at line 1216, Update the completion message in the
operator-waiting flow around print_step to remove the tautological
len(items)/len(items) count, replacing it with concise wording that reports
completion without redundant totals.
eliorerz
changed the title
1 participant
Why
After #33 (caching the etcd image at snapshot time), the golden snapshot's
/var/lib/kubelet/config.jsonends up with an empty authfile instead of a stale-but-present one.cmd_bootalready pushes a fresh pull-secret intoopenshift-config/pull-secretonce the API is healthy, but nothing waits for MCO to actually roll that out to the node beforecmd_bootreports the cluster ready.That rollout takes several minutes, and recert makes it worse: it writes kubelet's
config.jsonstraight to disk instead of through MCO, so machine-config-daemon can boot believing the rendered-master MachineConfig doesn't match what's on disk and latchDegraded.Anything that pulls an authenticated image right after
cmd_bootreturns — metal3/ironic in BMaaS, marketplace catalog sources in CaaS-full-install — can race this and fail withunauthorized, eventually succeeding after backoff retries, but sometimes outliving the caller's own readiness timeout (seen on osac-3 and osac-9 withoc wait --timeout=600son the metal3 pod).infra/netris/roles/restore-snapshotin osac-test-infra already hit this exact recert side effect and fixed it: force an MCD resync, wait formcp/masterto reportUpdated=True, and self-heal (clear stuck degraded annotations, restart machine-config-daemon) if it doesn't. This PR ports that same fix intocmd_bootso every caller gets it for free instead of patching each workflow's timeout individually.What
cmd_bootbetween "Waiting for cluster health" and "Configuring access": force/run/machine-config-daemon-force, pollmcp/masterforUpdated=True(90 x 10s), and if still not synced, clear the stuck node annotation + restart machine-config-daemon and poll again (60 x 10s) before giving up.cmd_boot's step count 9 → 10 and renumbered accordingly.Test plan
python3 -m pytest test_cluster_tool.py -q— 161 passed (was 158; added 3 new tests), same 11 pre-existing failures unrelated to this change (root/NetworkManager-permission-dependent tests that fail identically onmainin a sandboxed environment).TestPullSecretRolloutcovers ordering (resync forced after pull-secret set, before "Configuring access"), the self-heal path, and the give-up-after-timeout path.Summary by CodeRabbit
New Features
Bug Fixes