fix: support new wekactl interface group response - #2798
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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 @assafgi's task in 3m 0s —— View job Review:
|
| # | Where | Severity | Issue |
|---|---|---|---|
| 1 | weka.go:1093-1116 |
medium | Extraction missed its second caller: EnsureNfsIpRanges (weka.go:1428-1458) still has the identical duplicated block. Also interfaceGroups[0] is taken without matching on Name. |
| 2 | weka.go:264-276 |
medium | "container": null decodes to nil and is indistinguishable from an absent field — one orphaned port would fail EnsureNfsInterfaceGroupPorts for every container in the group, re-creating the wedge via the delete flow. Error message also interpolates a possibly-empty p.Port. |
| 3 | feature_flags.go:19-22 |
nit | WekactlAsDefault has no Go consumer; worth saying so in the comment. |
| 4 | weka_nfs_interface_group_test.go |
nit | No "container": null case to pin the decision above. |
Broader risk worth a look on a wekactl cluster (out of scope for this PR)
If wekactl changed port identity in nfs interface-group, the same switch plausibly touches the other HostId<N>-shaped contracts the operator depends on. None are covered by this PR or its tests:
ListS3ClusterContainers(weka.go:1268) parses["HostId<0>", ...]and hard-errors on any other shape.ListSmbwClusterContainers(weka.go:991) parsessambaHoststhe same way.- Several control-flow decisions match on stderr text containing
HostId<N>:"Unrecognized host ID HostId<%d>"(weka.go:828,weka.go:1074),"Host HostId<%d> not found"(weka.go:1647,weka.go:1758). A go-CLI reword turns each of these idempotency guards back into a hard failure. - Same class: the
"is not part of group"match in the port-removal loop (weka.go:1204) — the idempotency escape hatch for the exact code path this PR touches.
Not blocking, but a quick pass over these against a wekactl_as_default=true cluster would be worth more than the fix here in isolation.
Not verified
I could not run go build, go vet, or go test in this environment — those commands are outside the allowed tool set for this job, so the review is static-analysis only. The new tests are pure encoding/json decode assertions with no external dependencies, so they should be cheap for CI to confirm.
• 09-06-fix_support_new_wekactl_interface_group_response
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/06/26)2 reviewers were added to this PR based on Anton Bykov's automation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func (c *CliWekaService) getNfsInterfaceGroup(ctx context.Context, executor podexec.Exec, interfaceGroupName string) (*NfsInterfaceGroup, error) { | ||
| logger := instrumentation.CurrentSpanLogger(ctx) | ||
|
|
||
| // Fetch current interface group configuration | ||
| cmd := []string{ | ||
| "weka", "nfs", "interface-group", "--name", interfaceGroupName, "--json", | ||
| } | ||
| stdout, stderr, err := executor.ExecNamed(ctx, "GetNfsInterfaceGroup", cmd) | ||
| if err != nil { | ||
| logger.SetError(err, "Failed to get NFS interface group", "interfaceGroup", interfaceGroupName, "stderr", stderr.String()) | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| // Parse the response | ||
| var interfaceGroups []NfsInterfaceGroup | ||
| if parseErr := json.Unmarshal(stdout.Bytes(), &interfaceGroups); parseErr != nil { | ||
| logger.SetError(parseErr, "Failed to parse NFS interface group JSON", "stdout", stdout.String()) | ||
| return parseErr | ||
| return nil, parseErr | ||
| } | ||
|
|
||
| if len(interfaceGroups) == 0 { | ||
| return errors.Errorf("NFS interface group %s not found", interfaceGroupName) | ||
| return nil, errors.Errorf("NFS interface group %s not found", interfaceGroupName) | ||
| } | ||
|
|
||
| return &interfaceGroups[0], nil | ||
| } |
There was a problem hiding this comment.
The extraction missed its second caller. EnsureNfsIpRanges (weka.go:1428–1458, unchanged by this PR) still contains a verbatim copy of this exact fetch → json.Unmarshal([]NfsInterfaceGroup) → len == 0 → [0] block, down to the same "GetNfsInterfaceGroup" span name. Now that the helper exists, that copy should call it, otherwise the next schema change has two places to fix and only one of them is covered by the new test.
Separately, two robustness gaps in the helper itself — both relevant precisely because the premise of this PR is "the CLI changed behaviour under us":
interfaceGroups[0]is returned without checkingName == interfaceGroupName. If a wekactl generation ever ignores/renames--name, this silently reconciles ports against the wrong interface group — a much worse failure than the one being fixed here, and a silent one.len(interfaceGroups) > 1is discarded without a word in the log.
Both are cheap to close by selecting the match by name:
for i := range interfaceGroups {
if interfaceGroups[i].Name == interfaceGroupName {
return &interfaceGroups[i], nil
}
}
return nil, errors.Errorf("NFS interface group %s not found (got %d groups)", interfaceGroupName, len(interfaceGroups))
| // OwnerContainerId resolves the weka container owning this port under either CLI schema. | ||
| // It errors rather than defaulting, so an unrecognized schema cannot be silently read as | ||
| // "this port belongs to nobody" — that reading previously let port removal no-op and wedge | ||
| // container deletion behind a failing deactivate. | ||
| func (p NfsInterfaceGroupPort) OwnerContainerId() (int, error) { | ||
| if p.Container != nil { | ||
| return *p.Container, nil | ||
| } | ||
| if p.HostId != "" { | ||
| return resources.HostIdToContainerId(p.HostId) | ||
| } | ||
| return 0, errors.Errorf("interface group port %q has neither a 'container' nor a 'host_id' field", p.Port) | ||
| } |
There was a problem hiding this comment.
The fail-loud choice is the right default and the rationale in the comment is convincing. Two things worth pinning down before merge:
"container": null would be indistinguishable from an absent field. Go decodes an explicit JSON null into a *int as nil, so an orphaned port (owner container already removed) that wekactl renders as "container": null with no host_id lands in the error branch. Because ContainerPorts fails the whole group scan, one such orphan makes EnsureNfsInterfaceGroupPorts fail for every container in the group — including the RemoveFromNfs step of flow_deleting_state.go:138. That reproduces exactly the wedge this PR is fixing, just triggered by a different payload. If you can confirm wekactl never emits a null/absent container for a live port, a one-line note here would be enough; if you can't, consider skipping ports that resolve to "no owner at all" only when they also carry no uid, and erroring on genuinely unrecognized shapes.
Diagnosability: the error interpolates p.Port, which is itself empty on a badly-shaped entry, yielding interface group port "" has neither a 'container' nor a 'host_id' field. Including ContainerUid/HostUid (at least one is populated in both schemas) would make the message actionable from logs alone.
| // flag 13 is not used by the operator | ||
| // WekactlAsDefault means the go CLI (wekactl) backs the `weka` command. It changes some JSON | ||
| // schemas, notably `weka nfs interface-group` port identity — see NfsInterfaceGroupPort. | ||
| WekactlAsDefault bool `json:"wekactl_as_default"` // 14 |
There was a problem hiding this comment.
WekactlAsDefault is never read anywhere in Go — the only Go reference is the doc comment on NfsInterfaceGroupPort. That's consistent with the design (the decoder accepts both schemas unconditionally, which is the more robust choice than branching on a flag), but it means the field exists purely so the flag decodes and so the comment has something to point at.
That's fine to keep, just make it explicit in the comment that the operator does not branch on this flag today, so the next reader doesn't go hunting for the consumer or assume the schema handling is gated on it.
| } | ||
| } | ||
|
|
||
| // TestContainerZeroIsDistinguishableFromAbsent guards the pointer on NfsInterfaceGroupPort.Container. | ||
| // With a plain int, container 0 and "field missing" are the same value, so an unparseable payload | ||
| // would silently claim to own container 0's ports. | ||
| func TestContainerZeroIsDistinguishableFromAbsent(t *testing.T) { | ||
| group := parseGroup(t, `[{"name": "g", "ports": [{"container": 0, "port": "eth0"}]}]`) | ||
| ports, err := group.ContainerPorts(0) | ||
| if err != nil { | ||
| t.Fatalf("ContainerPorts(0): %v", err) | ||
| } | ||
| if len(ports) != 1 { | ||
| t.Fatalf("container 0 must own eth0, got %v", ports) | ||
| } | ||
|
|
||
| missing := parseGroup(t, `[{"name": "g", "ports": [{"port": "eth0"}]}]`) | ||
| if _, err := missing.ContainerPorts(0); err == nil { | ||
| t.Fatal("an absent container field must not be read as container 0") | ||
| } | ||
| } |
There was a problem hiding this comment.
Good coverage — the container-0 / absent-field distinction is exactly the trap the *int exists for, and pinning it with a test is the right call.
One gap: there's no case for {"container": null, "port": "eth0"}. That decodes identically to the absent-field case, so today it errors — which may or may not be what you want for an orphaned port (see the note on OwnerContainerId). Adding it either way pins the decision rather than leaving it to encoding/json semantics that a future reader has to re-derive.
a1b3530 to
3b65d50
Compare
3b65d50 to
ce9a482
Compare
Merge activity
|
2 participants
No description provided.