Repository navigation
Conversation
eb5408a to
cffd987
Compare
| upgradeRequiredLabelValue = "upgrade-required" | ||
| // The device plugin's resource name varies with its naming strategy, so a pod | ||
| // holding a GPU is identified by the domain rather than by an enumeration. | ||
| amdResourceDomain = "amd.com/" |
There was a problem hiding this comment.
Making the predicate strategy-independent is the right call, and this const is the place that claims it. Worth noting the operator still won't accept the strategy this unblocks — the enumeration moved out of here but it lives in three other places.
internal/validator/specValidators.go:160 allow-lists the flag:
utils.ResourceNamingStrategyFlag: {utils.SingleStrategy, utils.MixedStrategy},backed by internal/utils.go:52-54. A DeviceConfig setting resource_naming_strategy: product fails ValidateDevicePluginSpec before any pod ever advertises the new name, so the broadened match here is unreachable for that strategy.
Two more sites:
api/v1alpha1/deviceconfig_types.go:477—supported flag values: {"resource_naming_strategy": {"single", "mixed"}}. Generated out to seven artifacts (config/crd/bases/,helm-charts-k8s/crds/,helm-charts-k8s/values.yaml, bothbundle/manifests/,config/manifests/bases/,hack/k8s-patch/metadata-patch/), sokubectl explainand the OLM bundle both report the new strategy as unsupported.internal/plugin/plugin.go:112-113— PF/VF passthrough force-defaults the flag to"mixed"when unset, using bare string literals rather than theutilsconstants, so a grep-driven update of those constants misses it.
ROCm/k8s-device-plugin#246 names the validator change as separate work, so none of this is news. Raising it because the description here reads "It stands alone — the same enumeration already misses nps2 and nps8 — but it is a prerequisite here rather than a follow-up", which suggests this PR is the operator-side blocker. After it merges, #246 still can't be driven through DeviceConfig.
Either worth naming the PR that closes the validator side, or folding it in — from here it looks like a constant, a slice entry, and a doc comment, though the generated artifacts mean a make manifests run too.
There was a problem hiding this comment.
You are right that this does not unblock the strategy on its own, and the wording you quoted oversold it. Opened #666 for the validator side rather than folding it in, and fixed the claim in both descriptions.
All four sites are in #666: the ProductStrategy constant, the allow-list entry, the CRD doc comment with its four generated artifacts, and the two hand-maintained values.yaml comments you did not list but which enumerate the same values. The internal/plugin/plugin.go bare literals are switched to the utils constants there too — no behaviour change, it still defaults to mixed, but a grep for the constants finds it now.
On make manifests: it also runs update-registry and update-version, which sed image tags and project versions across the tree. I ran controller-gen crd directly and hand-edited the rest, so the diff is eleven one-line changes instead of a regenerated tree. Flagging in case the project would rather take the churn.
The reason for splitting rather than folding, which is the part your comment does not have: the validator change cannot correctly merge until a plugin image carrying product ships. The operator's default is the floating rocm/k8s-device-plugin:latest (internal/plugin/plugin.go:55), so allow-listing the value early means a DeviceConfig can request a strategy the plugin exits at startup on. Folding it in would hold this PR behind ROCm/k8s-device-plugin#246, and this one fixes a bug that is live right now — nps2 and nps8 pods are not evicted during a driver upgrade today, with no new strategy involved. #666 is a draft until #246 merges and an image is published.
Also fixed here (c44e695): the description, commit message and test case names said model. #246 settled on product. Added a case for amd.com/gpu-mi250x_mi250 while I was in there, since that strategy can emit an underscore inside the name as well as in the partition style, and the predicate should not care either way.
cffd987 to
c44e695
Compare
…tion getPodsToDrainOrDelete decides which pods a driver upgrade must evict by testing each resource request against a built set of names: amd.com/gpu plus the cross product of five compute partition types and two memory ones. A pod requesting any other amd.com resource is not recognised as a GPU pod, and the only other arm is an explicit /dev/dri volumeMount, which ordinary GPU pods do not have -- device plugins inject devices via the Allocate response. The upgrade then proceeds without evicting the running workload. Two resource names already miss today: the memory partition list holds only nps1 and nps4, so nps2 and nps8 are unmatched. A third class arrives with the device plugin's product naming strategy (ROCm/k8s-device-plugin#246), where a whole-GPU request is amd.com/gpu-mi300x. Match the amd.com domain instead. Every name the device plugin advertises under every strategy lives there, so the predicate no longer has to track the strategies. Adds upgrademgr_test.go covering the six name shapes and the two negative cases; the positive spec fails against the enumeration on exactly the four names it misses. This fix stands alone and is correct against the device plugin as shipped today. It is necessary but not sufficient for the product strategy: ValidateDevicePluginSpec still rejects the flag value, which is a separate change that cannot land before ROCm#246 ships a plugin image carrying the strategy. Signed-off-by: Karl Isenberg <kisenber@amd.com>
c44e695 to
7134428
Compare
getPodsToDrainOrDeletedecides which pods a driver upgrade evicts by testing each resource request against a built set:amd.com/gpuplus the cross product of five compute partition types and two memory ones. A pod requesting any otheramd.comresource is not recognised as a GPU pod, and the second arm — an explicit/dev/drivolumeMount — does not catch it, because device plugins inject devices through theAllocateresponse rather than the pod spec. The upgrade then proceeds without evicting the running workload.Two names miss today: the memory list holds only
nps1andnps4, sonps2andnps8are unmatched. A third class arrives with the device plugin'sproductnaming strategy (k8s-device-plugin#246), where a whole GPU isamd.com/gpu-mi300x.Matching the
amd.com/domain covers every name the plugin advertises under every strategy, so the predicate stops having to track them.Scope
This fix stands alone and is correct against the device plugin as shipped today. The
nps2/nps8miss is live right now, with no new strategy involved, so this is not blocked on anything.It is necessary but not sufficient for the
productstrategy. The operator keeps its own copy of the accepted flag values, so aDeviceConfigsettingresource_naming_strategy: productfailsValidateDevicePluginSpecbefore any pod advertises the new name. That is #666, kept separate because it cannot correctly merge until a plugin image carrying the strategy ships — the operator's default is the floatingrocm/k8s-device-plugin:latest, so allow-listing the value early means aDeviceConfigcan request a strategy the plugin exits at startup on. Folding it in would hold this bug fix behind another repo's PR.Testing
go build ./...,go vet ./internal/...andgo test ./internal/...pass. The newupgrademgr_test.gocovers the six name shapes and two negatives; its positive spec fails against the enumeration on exactly the four names that miss:product-alternativesisamd.com/gpu-mi250x_mi250— underproduct, one device id that libdrm cannot resolve to a single SKU keeps both names joined by an underscore, so the strategy can emit one inside the product part of the name as well as in the partition style. The predicate should not care either way, and this pins that.Not exercised against a live driver upgrade.