fix: add ipv6 support to wekaclient

[assafgi]

No description provided.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[assafgi]

• fix: add ipv6 support to wekaclient #2678 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add 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]

Claude finished @assafgi's task in 3m 51s —— View job

---

Review: ipv6 support for WekaClient

• Read PR diff (git diff origin/main...HEAD)
• Trace Ipv6 consumers (pod env, runtime script, cluster path)
• Compare with WekaCluster/WekaContainer handling
• Check generated artifacts (doc/api_dump, CRDs)
• Post review

The propagation itself is correct and minimal: Spec.Ipv6 → WekaContainer.Spec.Ipv6 → IS_IPV6 env (internal/controllers/resources/pod.go:319) → get_single_device_ip() in charts/weka-operator/resources/weka_runtime.py:3776, matching what WekaCluster already does. Join-IP construction is already ipv6-safe (discovery.WrapIpv6Brackets, internal/services/discovery/discovery.go:345), so no change was needed there. Three things I'd address before merge.

---

1. Generated API docs weren't regenerated (should fix)

doc/api_dump/ is committed and is generated from the pkg/weka-k8s-api submodule, which this PR bumps. Both artifacts still lack the new field for WekaClientSpec:

• doc/api_dump/api-schema.json — Ipv6 appears only at line 3898 (WekaClusterSpec) and 5023 (WekaContainerSpec); the WekaClientSpec block (lines 3312–3575) has no entry.
• doc/api_dump/wekaclient.md — no ipv6 row, while wekacluster.md:84 and wekacontainer.md:80 have one.

Run make api-docs (or make manifests) with the submodule checked out and commit the result. Fix this →

While you're in the API repo: give the new field a doc comment (// use ipv6 for weka client networking configuration) so the CRD description and generated table aren't blank — WekaContainerSpec.Ipv6 has an empty description today and it isn't a pattern worth repeating.

2. ipv6 is now mutable but has no path to take effect

Adding Ipv6 to UpdatableClientSpec (client_reconciler_loop.go:1214, :1250) and to updateContainerIfChanged (:745-748) makes the field live-updatable, but IS_IPV6 is baked into the pod at creation time and there is no spec-diff → pod-recreation logic for client containers. Concretely, flipping spec.ipv6 on an existing WekaClient:

1. patches WekaContainer.Spec.Ipv6, sets changed = true, LastAppliedSpec advances;
2. the running pod keeps its old IS_IPV6 value, so nothing changes for the user;
3. at the next unrelated pod replacement (image upgrade, node reboot, deletePodIfUnschedulable), the client silently switches IP family and re-registers with a management IP from a different family than the one it joined with.

That deferred, invisible flip is the risky part. Two reasonable options: reject the change with a logged error the way the coresNum decrease guard does at client_reconciler_loop.go:723-733, or accept it and log/emit an event stating it applies only on next pod replacement. Note this also diverges from WekaCluster, where Ipv6 is set only at container creation (internal/controllers/factory/container_factory.go:141) and is never propagated on update — worth being deliberate about the difference. Fix this →

3. No validation of client vs. target-cluster IP family

If the target WekaCluster has ipv6: true and the WekaClient omits ipv6, the client resolves an ipv4 local address while receiving bracketed ipv6 join IPs from resolveJoinIps (:565) — a mismatch that surfaces as an opaque join/mount failure rather than a clear error. c.targetCluster is already available where the container is built (:502), so either defaulting from the cluster or a validation error/event would remove the whole class of misconfiguration. Since the field is a plain bool, unset and explicit false are indistinguishable; if you want inherit-from-cluster semantics you'd need *bool in the API type. A mismatch warning is the cheaper version. Fix this →

---

Notes

• The new field in UpdatableClientSpec changes the HashStruct result (:589), so every existing WekaClient will run one HandleSpecUpdates pass after the operator upgrade. Benign — Ipv6 defaults to false and existing containers hold false, so updateContainerIfChanged finds no diff on that field.
• charts/weka-operator/crds/ is empty/generated at build time, so no committed CRD needs updating here.
• No security or performance concerns in this change.
• Could not build or test: the pkg/weka-k8s-api submodule is not checked out in this environment (pkg/weka-k8s-api/ is empty), so go build ./... / make test can't run and I could not verify the type or JSON tag of the new WekaClientSpec.Ipv6 field directly — that part is inferred from the call sites and from the WekaCluster/WekaContainer equivalents.
  • Branch

[Copilot]

Pull request overview

This PR adds IPv6 support propagation for WekaClient by wiring the spec.ipv6 setting into the generated WekaContainer spec and ensuring it is reconciled on updates, aligning client container behavior with existing IPv6 handling in the container/pod rendering path.

Changes:

• Propagate wekaClient.Spec.Ipv6 into the client WekaContainerSpec during container creation.
• Extend UpdatableClientSpec to include Ipv6 so it participates in reconciliation diffs.
• Update updateContainerIfChanged to detect and apply Ipv6 changes to existing client containers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[graphite-app]

Graphite Automations

"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (07/27/26)

2 reviewers were added to this PR based on Anton Bykov's automation.

[claude]

Claude finished @assafgi's task in 37s —— View job

---

Review: ipv6 support for WekaClient — no changes since last review

• Compare current diff against previously reviewed state
• Re-check whether pkg/weka-k8s-api submodule is now available for verification

The PR content is byte-identical to what I reviewed earlier in this thread: still one commit (09359953), internal/controllers/wekaclient/client_reconciler_loop.go at the same blob (bfc53a09), and the same submodule bump (b55581c1 → 75b00826). The synchronize event appears to be a no-content push (Graphite restack / re-trigger), so I'm exiting early rather than repeating the review.

pkg/weka-k8s-api/ is still not checked out in this environment, so I still can't build, run make test, or verify the new WekaClientSpec.Ipv6 field directly.

Recap of the three open items from my earlier review

1. Stale generated API docs — doc/api_dump/api-schema.json has Ipv6 only under WekaClusterSpec / WekaContainerSpec, not WekaClientSpec, and doc/api_dump/wekaclient.md has no ipv6 row (cf. wekacluster.md:84). Run make api-docs and commit.
2. ipv6 is mutable but has no path to take effect — IS_IPV6 is baked in at pod creation (internal/controllers/resources/pod.go:319), and nothing recreates the client pod on spec change, so a flip applies silently at the next unrelated pod replacement. Either reject the change (like the coresNum-decrease guard at client_reconciler_loop.go:723-733) or log/emit an event saying it takes effect on next pod replacement.
3. No client-vs-cluster IP family validation — a WekaClient with ipv6: false against an ipv6: true WekaCluster gets bracketed ipv6 join IPs from resolveJoinIps (:565) while configuring ipv4, surfacing as an opaque join/mount failure.

• Branch •

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[claude]

Claude finished @assafgi's task in 2m 45s —— View job

---

Review: ipv6 support for WekaClient — re-review

• Diff current state vs. previously reviewed state
• Verify generated API docs (prior item Operator sdk #1)
• Re-check mutability / pod-recreation concern (prior item Run busybox pod as a privileged container #2)
• Re-check client-vs-cluster IP family validation (prior item minikube 1.32 #3)
• Post review

All three items from my earlier review were addressed. The design decision — make ipv6 create-time-only rather than live-updatable, and warn instead of block on a client/cluster mismatch — is the right call, and the comments explaining why at client_reconciler_loop.go:751-756 and :1253-1254 are genuinely good; they encode the reasoning where the next person will look for it.

Resolved:

1. ✅ doc/api_dump/api-schema.json:3377-3383 now carries Ipv6 inside the WekaClientSpec definition (3312–3581), with a real description, and wekaclient.md:49 has the ipv6 | bool row in matching field order. The submodule field also got the doc comment.
2. ✅ Ipv6 removed from both UpdatableClientSpec and updateContainerIfChanged; consistent with WekaCluster, and the spec hash is unchanged so no upgrade churn.
3. ✅ New validateClientClusterIpFamily step (:167-172, :962-989) runs after FetchTargetCluster and before EnsureClientsWekaContainers, warns without blocking. fmt/time are already imported and RecordEventThrottled (:923) matches the call.

Two issues in the new validation code, both consequences of items 2 and 3 interacting.

---

1. The warning compares spec-to-spec, so it goes quiet exactly when the user thinks they fixed it (should fix)

validateClientClusterIpFamily (:976) compares c.wekaClient.Spec.Ipv6 against c.targetCluster.Spec.Ipv6. But after change #2, spec.ipv6 no longer describes the running clients — the containers hold whatever value they were created with. The two drift, and the check follows the wrong one:

• False negative (the damaging one): cluster is ipv6: true, client was created with ipv6: false, warning fires correctly. User reads the event and sets spec.ipv6: true. The warning stops on the next reconcile — but no container was touched, every pod still runs IS_IPV6=false, and the joins still fail. The single diagnostic pointing at the actual cause is silenced by an edit that changed nothing.
• False positive: containers were created with ipv6: true against an ipv6: true cluster and are healthy; someone edits spec.ipv6: false. The warning fires about a mismatch that doesn't exist in the running system.

c.containers is already populated by getCurrentContainers (step at :94, well before this step at :167), so c.containers[i].Spec.Ipv6 is available here. Warning when an existing container's Ipv6 differs from the cluster — in addition to the spec check for the not-yet-created case — makes the signal track reality and, as a bonus, covers the silent-no-op in the note below. Fix this →

2. Throttle key is global, so only one mismatched client per 30s gets an event (should fix)

RecordEventThrottled keys the throttle on eventtype+reason alone (:924), and ThrottlingMap is shared across every reconcile of the controller (NewClientReconcileLoop, :56). So with three mismatched WekaClients, "Warning"+"ClientClusterIpv6Mismatch" is claimed by whichever reconciles first, and the other two record nothing for 30s — their users see a healthy-looking object with no event.

ValidateClientVersionCompatibility shares this key shape, but it gets away with it: it also returns a blocking lifecycle.NewWaitErrorWithDuration, so the condition surfaces in status regardless of whether the event landed. This new step returns nil, so the throttled event is the only user-facing signal — and it's the one that can be silently dropped. Including c.wekaClient.Name in the throttle key (or the reason) fixes it. Fix this →

---

Notes (non-blocking)

• Editing spec.ipv6 is now a completely silent no-op. With Ipv6 out of UpdatableClientSpec, the hash at :595 doesn't change, HandleSpecUpdates never runs, and nothing logs or events. Fixing item Operator sdk #1 above covers this. The stronger option is making it genuinely immutable via a CEL x-kubernetes-validations rule (self == oldSelf) on the API field — then the API server rejects the edit with a clear message at admission and there's no drift to detect. Worth considering given WekaCluster has the same latent gap. At minimum, the field's doc comment in the submodule (use ipv6 for weka client networking configuration) should say the value only applies at creation, since that comment is what users see in the CRD.
• The mismatch warning message and the accompanying comment are well-calibrated — explicitly calling out dual-stack and the bool-can't-distinguish-unset caveat, and saying "safe to ignore on dual-stack", is the right tone for a heuristic.
• validateClientClusterIpFamily and ValidateClientVersionCompatibility are now adjacent steps with an identical targetCluster != nil predicate; folding them into one step would be marginally tidier, but keeping them separate is also fine for step-level observability. No action needed.
• No security concerns. No performance concerns — the new step is a field comparison on already-fetched objects.
• Could not build, test, or lint: pkg/weka-k8s-api/ is still not checked out in this environment, so go build ./... and make test can't run; gofmt wasn't in the allowed tool set either. The type and JSON tag of WekaClientSpec.Ipv6 are inferred from the call sites and the regenerated api_dump output rather than verified against the source. Everything else above was verified by reading the checked-out files.
  • Branch

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[claude]

Claude finished @assafgi's task in 1m 21s —— View job

---

Review: ipv6 support for WekaClient — no changes since last review

• Diff current state against previously reviewed state
• Verify the two open items from the 07:21 review are still open
• Re-check whether pkg/weka-k8s-api is now checked out for build/test

The code is identical to the state I reviewed at 07:21. Same three-file diff (+56/−1), same submodule pointer (b55581c1 → 7ac7be61), and every line number I cited then still lands on the same content: the create-time-only comment at client_reconciler_loop.go:751-756, the UpdatableClientSpec exclusion note at :1253-1254, RecordEventThrottled at :923, validateClientClusterIpFamily at :962-989, api-schema.json:3377-3383, wekaclient.md:49. The push was a no-content amend (author date 09:22, committer date 13:37 — Graphite restack), so I'm exiting early rather than repeating the full review.

The three items from my first review remain resolved. The two items from the 07:21 review are still open — both are in validateClientClusterIpFamily, both non-blocking for correctness of the happy path:

Recap of the two open items

1. The mismatch warning compares spec-to-spec, so it goes quiet exactly when the user thinks they fixed it. :976 compares c.wekaClient.Spec.Ipv6 to c.targetCluster.Spec.Ipv6, but since Ipv6 is create-time only the spec can diverge from what the running containers hold. Cluster ipv6: true, client created with ipv6: false → warning fires; user sets spec.ipv6: true → warning stops, no container was touched, pods still run IS_IPV6=false, joins still fail. Also warns spuriously in the reverse case. c.containers is already populated by getCurrentContainers (step at :94), so also comparing existing containers' Spec.Ipv6 would make the signal track reality. Fix this →

2. Throttle key is global, so only one mismatched client per 30s gets an event. RecordEventThrottled keys on eventtype+reason (:923) and ThrottlingMap is shared across all reconciles (:56), so with three mismatched clients only the first to reconcile records anything. ValidateClientVersionCompatibility shares this key shape but also returns a blocking wait error, so the condition surfaces in status anyway; this step returns nil, making the throttled event the only user-facing signal. Including the client name/namespace in the throttle key fixes it. Fix this →

Also still open, and worth folding in since Copilot raised the same point independently on wekaclient.md:49 and api-schema.json:3382: the field description (use ipv6 for weka client networking configuration, from the submodule doc comment) doesn't say the value only applies at creation. That comment is what users see in the CRD, and it's the one place the create-time-only rule isn't documented — the two Go-side comments you added cover it well for readers of the controller, but not for users.

Could not build, test, or lint: pkg/weka-k8s-api/ is still empty in this environment, so go build ./... and make test can't run, and the type/JSON tag of WekaClientSpec.Ipv6 remains inferred from call sites and the regenerated api_dump rather than verified against source.

• Branch •

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[claude]

Claude finished @assafgi's task in 3m 2s —— View job

---

Review: ipv6 support for WekaClient — re-review

• Diff current state vs. previously reviewed state
• Verify the two open items from the 10:38 review
• Verify the new validateClientClusterIpFamily logic against getCurrentContainers / EnsureClientsWekaContainers
• Check throttling helper semantics and API doc regeneration
• Post review

The Go file grew from +56/−1 to +101/−9, so this is a real change, not a restack. Both open items are addressed, and the doc-comment gap Copilot and I both raised is closed too. The reasoning captured in the comment block at client_reconciler_loop.go:962-979 — why the check reads containers rather than spec, and why it warns instead of blocks — is the kind of thing that saves the next reader an hour.

Resolved:

1. ✅ Warning now tracks reality. validateClientClusterIpFamily (:1000-1007) inspects c.containers when non-empty and only falls back to the spec before any container exists. Verified c.containers is populated by getCurrentContainers (:353-360, step at :94), well before this step at :167. Branch 1 catches container-vs-cluster, branch 2 catches spec-vs-container — so editing spec.ipv6 no longer silences the signal, it changes which message you get.
2. ✅ Throttle is per-object. key = "ipv6-mismatch-<ns>/<name>" (:1017) instead of the global eventtype+reason used by RecordEventThrottled (:924). Gating the log on the same key also closes Copilot's log-spam point from the 07:24 review.
3. ✅ Docs say create-time only. api-schema.json:3377-3383 and wekaclient.md:49 both carry the expanded description, so the CRD users see states the constraint.

Two things left, both small.

---

1. The log line drops the reason and can show two identical values (should fix)

reason carries the diagnostic detail — which container, what its Ipv6 is — but the logger.Warn at :1027-1032 logs only clientSpecIpv6 and clusterIpv6, neither of which is what triggered branch 1. Concretely: cluster ipv6=true, spec.ipv6=true, one container created earlier with ipv6=false. Branch 1 fires correctly, the event message names the container — but the log reads:

WARN Client/cluster ipv6 mismatch client=c1 clientSpecIpv6=true targetCluster=cl clusterIpv6=true

Two identical values under a "mismatch" headline, and no mention of the container that's actually wrong. Operator logs and traces are usually where this gets debugged, and the event may already have aged out by then. Adding "reason", reason (or logging msg) makes the log self-contained. Fix this →

2. "changing it on an existing WekaClient has no effect" isn't quite true — scale-out picks up the new value (should fix, doc wording)

The new field description says a change has no effect. But EnsureClientsWekaContainers (:362-391) builds a container for any tolerated node that doesn't have one yet, via buildClientWekaContainer → Ipv6: wekaClient.Spec.Ipv6 (:511). So after editing spec.ipv6, existing containers keep the old family and any node that joins the client's set afterwards gets a container with the new one — a heterogeneous fleet, not a no-op.

The validation does catch this (the old containers trip branch 2, the new ones trip branch 1 if they now differ from the cluster), so it's detectable rather than silent — that's why this is wording, not a correctness bug. But the description is what users act on, and "no effect" invites exactly the edit that produces the split. Something like applied when each client container is first created; changing it does not affect existing containers, and containers created later will use the new value — recreate the WekaClient to switch IP family consistently is accurate. Same edit belongs in the submodule doc comment, since that's the source. Fix this →

---

Notes (non-blocking)

• ShouldRun without a matching SetNow — worth one glance. The other ShouldRun call site (:905-912) pairs it with c.ThrottlingMap.SetNow(key), but it also passes EnsureStepSuccess: true, which is presumably why. The new call omits both, exactly matching RecordEventThrottled (:924-929), so if the semantics are wrong here they're equally wrong in the existing helper — i.e. not a regression from this PR. I couldn't confirm it: pkg/go-steps-engine is not checked out in this environment and there's no module cache, so I could not read throttling.Throttler. If ShouldRun does not record the timestamp itself, the 30s throttle is a no-op and both the log and the event fire every reconcile.
• The throttle key is per-object but not per-reason, so when the mismatch transitions between branch 1 and branch 2 (which is precisely what a user edit does), the updated message waits out the remaining window. 30s, so barely noticeable — mentioning only because that transition is the moment a user is watching for feedback.
• Only the first offending container is reported; on a mixed fleet the message names one of several. Reasonable, and the message generalizes fine.
• validateClientClusterIpFamily re-checks c.targetCluster == nil (:983) although the step predicate at :170 already guarantees it. Harmless and defensive; no action.
• The UpdatableClientSpec diff is mostly gofmt realignment from the added comment — no field semantics changed, and the hash is unaffected, so no upgrade churn.
• No security concerns. No performance concerns: field comparisons over already-fetched objects, throttled log/event.
• Could not build, test, or lint. pkg/weka-k8s-api/ and pkg/go-steps-engine/ are both empty in this checkout and there's no populated module cache, so go build ./..., make test, and gofmt can't run. The type and JSON tag of WekaClientSpec.Ipv6, and WekaContainerSpec.Ipv6 on the container side, are inferred from the call sites and the regenerated api_dump rather than verified against source. Everything else above was read directly from the checked-out files.

• Branch •

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/controllers/wekaclient/client_reconciler_loop.go:996

• When multiple client containers exist, this loop stops at the first container that differs from spec.ipv6 even if a later container actually mismatches the target cluster’s ipv6 setting. That can produce a less actionable/misleading warning (spec-vs-container) while the real problem is container-vs-cluster, which is what blocks join/mount.

		for _, container := range c.containers {
			if container.Spec.Ipv6 != clusterIpv6 {
				reason = fmt.Sprintf("client container %q runs ipv6=%t but target cluster %q has ipv6=%t; recreate the WekaClient to switch IP family",
					container.Name, container.Spec.Ipv6, c.targetCluster.Name, clusterIpv6)
			} else if c.wekaClient.Spec.Ipv6 != container.Spec.Ipv6 {