fix: recreate discovery containers when their owner's spec changes (OP-358) - #2752
fix: recreate discovery containers when their owner's spec changes (OP-358)#2752caspx wants to merge 1 commit into
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. |
Reviewing PR #2752
|
|
Claude finished @caspx's task in 4m 48s —— View job Review: PR #2752 — discovery container spec drift
VerdictThe core fix is sound and the diagnosis is right. I traced the failure mode and it holds up: with a broken image the container never sets I also checked the thrash risk specifically — nothing in the repo mutates a discovery container's 5 inline comments posted. Nothing blocking; the delete race is the one I'd fix before merge. Findings
Plus test-coverage gaps (no end-to-end recreate assertion; tests use On the
|
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/18/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
There was a problem hiding this comment.
Pull request overview
This PR improves reconciliation of per-node discovery WekaContainer objects by detecting owner-spec drift and triggering a delete/recreate cycle, preventing stale weka-dsc-* containers (e.g., with old images) from being reused indefinitely.
Changes:
- Add spec-drift detection (
image,imagePullSecret,tolerations,serviceAccountName) for existing discovery containers and recreate when drift is detected (delete +WaitError, no same-pass create). - Gate drift enforcement to only the controller-owner (
metav1.IsControlledBy) to avoid multiple owners fighting over the shared per-node singleton. - Add unit tests covering matching/no-op, spec drift delete+wait, foreign-owner no-op, terminating wait, and missing-create paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/controllers/operations/discover_node.go |
Detects drift on existing discovery containers and coordinates safe delete/recreate behavior with owner gating. |
internal/controllers/operations/discover_node_test.go |
Adds unit tests for drift/ownership/termination/missing-container branches in EnsureContainers. |
.ainav/operations/index.md |
Updates navigation docs to reflect new drift/recreate behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // recreate on a later pass — a same-name Create would fail while the old container is terminating | ||
| if err := o.DeleteContainers(ctx); err != nil { |
There was a problem hiding this comment.
Delete is not UID-guarded — can delete a container someone else just recreated.
DeleteContainers does a bare o.client.Delete(ctx, o.container). o.container was read back in the GetContainer step, and between then and here another owner on the same node can legitimately delete + recreate weka-dsc-<node> (that is exactly the flow this PR introduces on the drift path). We'd then delete the new, correctly-specced object, and the peer owner — which is now the controller-owner — would just re-create it. Net effect is an extra discovery restart and a wasted reconcile cycle.
Since the object was read at a known ResourceVersion/UID, the delete can be made a no-op when the object underneath changed:
if err := o.client.Delete(ctx, o.container, client.Preconditions{UID: &o.container.UID}); err != nil && !apierrors.IsNotFound(err) && !apierrors.IsConflict(err) {
return err
}Simplest version: add a UID-preconditioned delete here rather than changing DeleteContainers, since the other two call sites (FinishOnExistingInfo, DeleteOnFinish) genuinely want an unconditional cleanup.
| if o.container != nil { | ||
| return nil | ||
| if o.container.GetDeletionTimestamp() != nil { | ||
| return lifecycle.NewWaitError(fmt.Errorf("discovery container %s is being deleted", o.container.Name)) | ||
| } | ||
| // the discovery container is a shared per-node singleton; only its controller-owner | ||
| // enforces spec drift, otherwise owners with different specs delete each other's container in a loop | ||
| if !metav1.IsControlledBy(o.container, o.ownerRef) { | ||
| return nil | ||
| } | ||
| if !o.isContainerSpecChanged() { | ||
| return nil | ||
| } | ||
| // recreate on a later pass — a same-name Create would fail while the old container is terminating | ||
| if err := o.DeleteContainers(ctx); err != nil { | ||
| return err | ||
| } | ||
| return lifecycle.NewWaitError(fmt.Errorf("discovery container spec changed, recreating")) | ||
| } |
There was a problem hiding this comment.
No logging on any of the three new branches. The rest of this file uses instrumentation.CurrentSpanLogger(ctx) (see GetNode, Enrich), and deleting a shared per-node singleton is exactly the kind of action you want in a trace when debugging why discovery restarted. Two spots in particular:
- The drift delete — log the specific field(s) that differ (old vs new image is the OP-358 signal).
- The
!metav1.IsControlledBybail — this is the branch where a foreign-owned stale container is silently accepted forever. Without a log line, the OP-358 symptom (weka-dsc-*stuck on a broken image) reappears with zero diagnostic, because the owner that could fix it never runs and the owner that notices says nothing.
A logger.Debug/Info on each keeps the multi-owner deferral observable.
Related: isContainerSpecChanged returns a bare bool, so the caller has no way to say what drifted. Returning (bool, string) or a small []string of changed fields would make both the log message and the WaitError text actionable.
| // isContainerSpecChanged reports whether the existing discovery container no longer matches the owner-derived spec | ||
| func (o *DiscoverNodeOperation) isContainerSpecChanged() bool { |
There was a problem hiding this comment.
Drift set is narrower than what EnsureContainers actually writes. The create path below (lines 210-230) also sets Labels from util2.MergeMaps(o.ownerRef.GetLabels(), labels), but isContainerSpecChanged only covers Image / ImagePullSecret / ServiceAccountName / Tolerations. Owner label changes therefore never trigger a recreate, so the container's labels drift permanently from the owner's.
Low severity (discovery-container labels are mostly cosmetic/selection metadata), but it means the function name over-promises. Either add the label comparison — util2.NewHashableMap(...).Equals(...), the idiom used at internal/controllers/wekaclient/client_reconciler_loop.go:769 — or rename to something like isContainerRecreateRequired and add a comment stating labels are deliberately excluded so the omission reads as intentional.
The NodeAffinity/Mode omissions are fine — both are derived from the container name, which is the lookup key.
|
|
||
| "github.com/pkg/errors" | ||
| "github.com/weka/go-steps-engine/lifecycle" | ||
| k8sutil "github.com/weka/weka-k8s-api/util" |
There was a problem hiding this comment.
Import is out of alphabetical order within the group — github.com/weka/weka-k8s-api/util sorts after github.com/weka/go-weka-observability/instrumentation. gofmt won't flag it, but goimports/gci will if either is in the lint config.
| k8sutil "github.com/weka/weka-k8s-api/util" | |
| "github.com/weka/go-weka-observability/instrumentation" | |
| weka "github.com/weka/weka-k8s-api/api/v1alpha1" | |
| k8sutil "github.com/weka/weka-k8s-api/util" |
(and drop the now-duplicated instrumentation / weka lines below)
| op.container = container | ||
|
|
||
| err := op.EnsureContainers(context.Background()) | ||
| waitErr := &lifecycle.WaitError{} |
There was a problem hiding this comment.
Style nit: the repo idiom for this is var waitErr *lifecycle.WaitError (see sign_drives_test.go:220 and funcs_aws_termination_lifecycle_test.go:228). &lifecycle.WaitError{} allocates a throwaway struct and makes the errors.As target a **WaitError — it still works, but it diverges from every other WaitError assertion in the package.
Same at line 192.
| waitErr := &lifecycle.WaitError{} | |
| var waitErr *lifecycle.WaitError |
|
|
||
| func TestEnsureContainers_CreatesWhenMissing(t *testing.T) { | ||
| scheme := newDiscoverNodeTestScheme(t) | ||
| op := newDiscoverNodeTestOp(scheme) | ||
|
|
||
| if err := op.EnsureContainers(context.Background()); err != nil { | ||
| t.Fatalf("expected nil error, got %v", err) | ||
| } | ||
| if op.container == nil { | ||
| t.Fatal("container must be created") | ||
| } | ||
| if op.container.Spec.Image != op.image { | ||
| t.Errorf("created with image %q, want %q", op.container.Spec.Image, op.image) | ||
| } | ||
| if op.container.Spec.ServiceAccountName != op.serviceAccount { | ||
| t.Errorf("created with serviceAccount %q, want %q", op.container.Spec.ServiceAccountName, op.serviceAccount) | ||
| } | ||
| } |
There was a problem hiding this comment.
Two coverage gaps worth closing, since they're the parts that actually encode OP-358:
-
No end-to-end recreate test. Every test stops at the
WaitError. The behaviour the ticket is about is "next pass creates it with the new image" — worth a test that callsEnsureContainers(drift → delete → WaitError), clearsop.container, calls it again, and asserts the persisted object now hasop.image. That's what proves delete-then-recreate-on-a-later-pass actually converges rather than just deleting. -
The owner in these tests is a
*weka.WekaClient, but production always passes a*weka.WekaContainer—NewDiscoverNodeOperationis called from exactly one place,internal/controllers/wekacontainer/funcs_getters.go:365, withr.container. That distinction is the whole premise of theIsControlledBygate: every WekaContainer scheduled on a node is a candidate owner of the sameweka-dsc-<node>, so the "two owners fight" scenario is container-vs-container, not client-vs-builder. Using*weka.WekaContainerasownerRefwould make the tests model the real contention.
Merge activity
|
TL;DR
Discovery WekaContainers now track their owner's spec — a stale (e.g. wrong-image)
weka-dsc-*container is deleted and recreated instead of being reused forever.What changed?
DiscoverNodeOperation.EnsureContainersno longer early-returns on an existing discovery container: if the container's spec (image, imagePullSecret, tolerations, serviceAccountName) no longer matches the owner-derived desired spec, it is deleted and recreated on a subsequent pass (delete +WaitError, neversame-pass create, since the old object may still be terminating).
metav1.IsControlledBy) —weka-dsc-<node>is a shared per-node singleton, and without this gate two owners with different specs (e.g. a client container and a drivers-builder) delete each other's container in a ~2s loop.EnsureContainers.Why make this change?
Discovery containers were created once and never reconciled against the current owner spec, so after an image fix they kept the broken image indefinitely - Fixes OP-358.