feat: switch nodeaffinity from required to prefer for OLM#133
Closed
bhpratt wants to merge 3 commits into
Closed
Conversation
Member
|
Hi @bhpratt thanks for the PR we will evaluate this change and get back to you |
…cheduling Amend some details for nodeAffinity preferredDuringSchedulingIgnoredDuringExecution
Amend more details to use nodeAffinity preferredDuringSchedulingIgnoredDuringExecution
Member
2 tasks
spraveenio
added a commit
that referenced
this pull request
Jul 11, 2026
…readiness (#599) * fix(kmm): gate ContainerStatusUnknown pod deletion on node readiness (#1595) * fix(kmm): gate ContainerStatusUnknown pod deletion on node readiness When a KMM build pod hits ContainerStatusUnknown due to a NodeNotReady event, deleting it immediately causes KMM to spawn a replacement build pod before the kubelet's volume registry has re-synced. The replacement then fails with FailedMount on the Dockerfile ConfigMap (the ConfigMap exists in etcd but the kubelet's in-memory volume plugin registry is stale from the network partition window). Gate the deletion on the node's current Ready condition. If the node is still NotReady, skip deletion — the pod will transition to Failed once the node recovers, firing another Update event at which point deletion proceeds and KMM's replacement starts cleanly. Observed in CI job 32149765 (test_driver_upgrade_cycle[30.30], asrock-126-b3-1b, ubuntu-22.04, kernel 6.8.0-100-generic). Plan: docs-internal/knowledge/plans/2026-07-10-kmm-build-pod-node-readiness-gate.md Co-Authored-By: Claude <noreply@anthropic.com> * fix(kmm): gate ContainerStatusUnknown pod deletion on node readiness Co-Authored-By: Claude <noreply@anthropic.com> * fix(kmm): address copilot review comments - Scope node-readiness check to KMM builder pods only (pod-type=builder) - Add node name to node GET error log - Use structured logging for delete failure Co-Authored-By: Claude <noreply@anthropic.com> * fix(kmm): correct build pod-type label value from "builder" to "build" The build-pod gate and the pre-existing hasExpectedPodLabel watch predicate both matched kmm.node.kubernetes.io/pod-type == "builder". The deployed KMM (ROCm fork, release-v1.5.0) labels build pods pod-type=build (PodTypeBuild in internal/utils/podhelper.go); "builder" never matched, so both the node-readiness gate and the original auto-remove-unknown-build-pod feature (PR #133) were dead code against ROCm KMM. Correct both sites to "build" via shared local constants. KMM's constant lives in an internal, non-importable package, so the value is duplicated. Addresses review feedback from spraveenio on PR #1595. Co-Authored-By: Claude <noreply@anthropic.com> * fix(kmm): extend ContainerStatusUnknown node-readiness gate to sign pods Sign pods share KMM's recreate-if-missing lifecycle (internal/sign/pod/manager.go Sync respawns a missing sign pod exactly as the build manager does) and are real in this deployment via the ImageSign secure-boot spec. Extend the gate and the watch predicate to admit pod-type in {build, sign} via a shared isKMMBuildOrSignPod helper. Worker (workerMgr) pods are intentionally excluded: they mount HostPath volumes, not the Dockerfile ConfigMap, so the FailedMount race does not apply, and they have a dedicated lifecycle path respawned by our own workermgr reconcile. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit f25b3614fe194aa5498af3f9bd7a7b16a91ec1c1) * Delete docs-internal/knowledge/plans/2026-07-10-kmm-build-pod-node-readiness-gate.md --------- Co-authored-by: Yan Sun <Yan.Sun3@amd.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Praveen Kumar Shanmugam <58961022+spraveenio@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is related to: #129
There are a few ways to address this issue, but here is one idea - to switch nodeaffinity from Required to Preferred.
I did not update the Helm charts, as those appear to have a different flow (and do have a path to override nodeAffinity). However, I can add those to this PR as well.