fix: stage the cluster image weka CLI for drivers-builder - #2789
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
How to use the Graphite Merge QueueAdd the label main-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @rugggger's task in 6m 31s —— View job Code review: stage the cluster image weka CLI for drivers-builder
The core diagnosis is right and the fix is the correct shape: 1. The flags and the staged CLI can describe different images —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| builderFlags, ffErr := r.GetFeatureFlags(ctx) | ||
| if ffErr != nil { | ||
| return errors.Wrap(ffErr, "failed to get feature flags for drivers-builder") | ||
| } | ||
|
|
||
| payloadBytes, _ := json.Marshal(map[string]string{ //nolint:errcheck // error return value intentionally not checked | ||
| "targetImage": container.Spec.Image, | ||
| "cliImage": image, | ||
| "cliImage": drivers.GetBuilderCliImage(builderFlags, container.Spec.Image, image), |
There was a problem hiding this comment.
The flags and the CLI image can refer to two different images.
r.GetFeatureFlags(ctx) → operations.GetFeatureFlagsForContainer keys the lookup on container.Status.LastAppliedImage, falling back to Spec.Image only when it is empty (internal/controllers/operations/feature_flags.go:102-105). But the value handed to GetBuilderCliImage — and staged as cliImage/targetImage — is container.Spec.Image.
ensurePod is gated on PodNotSet (flow_active_state.go:319-324), so the one moment it runs for an existing drivers-builder is pod (re)creation — which is exactly when the two diverge during an image roll: Spec.Image is already the new cluster image while Status.LastAppliedImage still holds the old one (it is only advanced later, flow_active_state.go:1097).
Concrete failure: cluster upgraded from an image without WekaGetCopyLocalDriverFiles to one with it. Flags are resolved for the old image → flag false → cliImage = builder image → the driver extraction again runs the stale builder CLI against the new targetImage, i.e. the bug this PR is fixing, on the upgrade path. The mirror case is worse: old image has the flag, new one doesn't → we stage the new cluster image's CLI, which cannot copy driver files at all.
GetLoaderImageForNode avoids this by resolving flags for an explicit image (drivers.go:69). Suggest doing the same here — resolve for container.Spec.Image via operations.GetFeatureFlagsForImage with AdhocContainerParams{Image: container.Spec.Image, ...} — so the flag and the CLI it selects always describe one image.
Minor, same lines: builderFlags reads as "flags of the builder image", but these are (meant to be) the cluster image's flags. clusterFlags would be less misleading.
| builderFlags, ffErr := r.GetFeatureFlags(ctx) | ||
| if ffErr != nil { | ||
| return errors.Wrap(ffErr, "failed to get feature flags for drivers-builder") | ||
| } |
There was a problem hiding this comment.
A non-transient flag-fetch failure now permanently blocks driver builds.
Deferring on a cold cache is the stated intent and is fine for the WaitError case. But this returns every error, and the step carries OnFail: r.setErrorStatus (flow_active_state.go:319-324), so a hard failure (ad-hoc container can't be created/scheduled, image not pullable on that node, malformed feature_flags.json, operator-deployment lookup failure in GetAdhocContainer) means the drivers-builder pod is never created → drivers are never built → every driver consumer on that node stays stuck, with no self-healing path. Before this change that decision degraded to the builder image.
Consider splitting the two cases: propagate WaitError to defer, and on any other error log + fall back to builderImage (previous behavior). Cheap insurance for a step that sits on the critical path of all driver loading.
Two related things worth confirming rather than assuming:
-
Cold-cache deadlock risk on a driverless node.
GetFeatureFlagsOperationfalls back to creating an ad-hocadhoc-op-with-containerWekaContainer pinned to the builder's node (get_feature_flags.go:257-267). IfRequiresDrivers()is true for that mode, that pod waits for drivers on a node whose drivers are being built by the very builder we are blocking → deadlock. I couldn't check (pkg/weka-k8s-apiis an unpopulated submodule in this checkout). The cluster-creation path already spins up the same ad-hoc container early (steps_cluster_creation.go:233), which suggests it's driver-free — but this is the first caller that runs it behind a drivers-builder, so it's worth being sure. -
errors.WrapandWaitErrordetection.github.com/pkg/errorshas implementedUnwrap()since v0.9.0, soerrors.Asstill sees through it — matching the idiom infuncs_numa_dra_test.go:31. Only a concern if the engine type-asserts instead. The pre-existing SSDProxy branch just below (line 143-146) wraps identically, so this is consistent either way.
| // GetBuilderCliImage picks the image that supplies the weka CLI staged for the | ||
| // drivers-builder init containers. The extraction step bind-mounts over /opt/weka, | ||
| // where the cluster image keeps its CLI, so a CLI is always staged out of band; | ||
| // prefer the cluster image's own CLI when it can copy driver files itself. | ||
| func GetBuilderCliImage(flags *domain.FeatureFlags, image, builderImage string) string { | ||
| if flags != nil && flags.WekaGetCopyLocalDriverFiles { | ||
| // innovation cli --kernel-build-id etc. | ||
| return image | ||
| } | ||
|
|
||
| return builderImage | ||
| } |
There was a problem hiding this comment.
Two things here:
Parameter naming invites the exact bug being fixed. GetBuilderCliImage(flags, image, builderImage) takes two adjacent string params; swapping them compiles cleanly and silently restores the old "always use the builder CLI" behavior. Rename image → clusterImage to make the call site self-checking.
The policy now lives in two places. GetLoaderImageForNode (line 68-79) expresses the identical rule with a different fallback shape. Having them drift is how the loader and the builder end up staging different CLIs for the same image. Since the new helper is the pure form of the decision, the loader can just delegate:
func GetLoaderImageForNode(ctx context.Context, node *v1.Node, image string) string {
flags, err := services.GetFeatureFlags(ctx, image)
if err != nil {
flags = nil
}
return GetBuilderCliImage(flags, image, GetBuilderImageForNode(node))
}That also makes the differing failure semantics explicit (loader: cache-only, silent fallback; builder: populating fetch, defer) rather than implicit in two separate bodies.
| // drivers-builder init containers. The extraction step bind-mounts over /opt/weka, | ||
| // where the cluster image keeps its CLI, so a CLI is always staged out of band; | ||
| // prefer the cluster image's own CLI when it can copy driver files itself. | ||
| func GetBuilderCliImage(flags *domain.FeatureFlags, image, builderImage string) string { |
There was a problem hiding this comment.
GetBuilderCliImage is a pure function with three branches (nil flags / flag off / flag on) and internal/drivers/drivers_test.go already exists — a small table test would pin the flags == nil fallback in particular, since that's the branch that decides behavior whenever flag resolution is unavailable.
f4ae7fe to
7f22ad8
Compare
The copy-cli init container staged whatever `weka` resolved to in the builder image, so the driver extraction ran an old CLI pinned in builder-ubuntu*-v1 rather than the one shipped with the cluster image being built. Pick the CLI source via weka_get_copy_local_driver_files, the same flag the loader already keys on, and route both callers through one helper so the two cannot drift apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7f22ad8 to
f6d58f8
Compare
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/02/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
Merge activity
|
2 participants
The copy-cli init container staged whatever
wekaresolved to in thebuilder image, so the driver extraction ran an old CLI pinned in
builder-ubuntu*-v1 rather than the one shipped with the cluster image
being built. Pick the CLI source the same way the loader already does,
via weka_get_copy_local_driver_files, and resolve the flag through the
populating fetch so a cold cache defers pod creation instead of
silently falling back to the builder image.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com