Repository navigation
Conversation
getManifestObjects logs the error from getDriverAdditionalConfigs and carries
on. Every other fallible step in the same loop returns it: getDriverSpec,
getGDSSpec, getGDRCopySpec, renderManifestObjects and
handleDefaultImagesInObjects. Because the call assigns into
renderData.AdditionalConfigs, the failure also leaves that field nil, and the
DaemonSet template guards every use of it with
{{- if and .AdditionalConfigs .AdditionalConfigs.VolumeMounts }}
so the driver renders with none of the volumes the user asked for: no custom
package repo, no custom CA certificates, no kernel module parameters and no
vGPU licensing config. getDriverAdditionalConfigs only ever returns nil with
the error, so there is no partial result worth keeping.
The reconcile reports success, the NVIDIADriver goes Ready once the pods come
up, and the only trace is one log line. Nothing reaches the CR status or its
conditions. The ClusterPolicy path does not behave this way:
transformDriverContainer returns the same failure to its caller.
TestGetManifestObjectsAdditionalConfigsErrorIsLogged pinned the old behaviour,
down to a comment saying the driver is still deployed "silently without the
repo config the user asked for". It came in with NVIDIA#2658, which added unit tests
across internal/state, so it recorded what the code did rather than what it
should do. Renamed and flipped to assert the error.
Signed-off-by: Dmitry Voropaev <dy.voropaev@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Missing driver configuration now fails reconciliation instead of allowing incomplete manifests to be applied. The inspected caller stops before applying objects, and no actionable merge risk remains.
Comment |
|
/ok to test 13acf3e |
kvalliyurnatt
left a comment
There was a problem hiding this comment.
LGTM ! Thanks for the fix @v0ropaev
Description
stateDriver.getManifestObjectslogs the error fromgetDriverAdditionalConfigsand carries on (internal/state/driver.go:333):Every other fallible step in the same loop returns instead:
getDriverSpec,getGDSSpec,getGDRCopySpec,renderManifestObjects,handleDefaultImagesInObjects.Two things follow from carrying on. The call assigns into
renderData.AdditionalConfigs, so the failure leaves that field nil, andmanifests/state-driver/0500_daemonset.yamlguards every use of it with{{- if and .AdditionalConfigs .AdditionalConfigs.VolumeMounts }}and.AdditionalConfigs.Volumes. The driver therefore renders with none of the volumes the user configured: no custom package repository, no custom CA certificates, no kernel module parameters, no vGPU licensing config. AndgetDriverAdditionalConfigsonly ever returnsnilalongside its error, so nothing partial is being salvaged.What a user sees: they set
spec.repoConfig.name(orcertConfig,kernelModuleConfig, a vGPUlicensingConfig) on anNVIDIADriver, the ConfigMap is missing or misnamed or the node's distro is not inRepoConfigPathMap, and the reconcile reports success. TheNVIDIADrivergoes Ready once the pods come up, and the driver runs against the default repository. The only trace is one log line; nothing reaches the CR status or its conditions. A driver built against the wrong package repository is not a milder failure than a reconcile error, it is a harder one to find.The ClusterPolicy path for the same inputs does not behave this way.
transformDriverContainerincontrollers/object_controls.goreturnsfmt.Errorf("ERROR: failed to create ConfigMap VolumeMounts for custom repo config: %v", err)to its caller.Checklist
make lint)make validate-generated-assets)make validate-modules)make lintfails here, with threestaticcheckSA4023 findings aroundresolveHostNvidiaSMIincmd/nvidia-validator/main.go. It fails the same way on an untouched tree and has nothing to do with this change, so I left it alone.Testing
TestGetManifestObjectsAdditionalConfigsErrorIsLoggedpinned the old behaviour, with a comment reading "The driver is still deployed, silently without the repo config the user asked for". It arrived in #2658, which added unit tests acrossinternal/state, so it recorded what the code does rather than what it should do. I renamed it toTestGetManifestObjectsAdditionalConfigsErrorand flipped the assertions, which keeps the same fixture: one GPU node, anNVIDIADriverwhosespec.repoConfig.namepoints at a ConfigMap that is not in the fake client.With only the test change applied and
driver.gountouched:and with the fix,
go test ./internal/state/is ok.go build ./...,go vet ./internal/state/andgofmt -lare clean, andgo test ./...passes everywhere excepttests/e2e, which needs a cluster and fails identically on an untouched tree.No GPU or cluster was used. The whole path is covered by the fake client.