Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func TestGetCsiNodeDaemonSetHash_ChangesWithSelector(t *testing.T) {
}
}

// Pod NodeSelector and nodeAffinity are ANDed. Leaving NodeSelector populated would make the retain
// Pod NodeSelector and nodeAffinity are combined with AND. Leaving NodeSelector populated would make the retain
// term dead code and reproduce the deadlock exactly, so assert placement lives only in affinity.
func TestNewCsiNodeDaemonSet_PlacementOnlyInAffinity(t *testing.T) {
config.Config.Csi.WekafsImage = "test-csi-image"
Expand Down
4 changes: 4 additions & 0 deletions internal/pkg/domain/feature_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ type FeatureFlags struct {
SsdProxyIncludesDpdkMemory bool `json:"ssd_proxy_includes_dpdk_memory"` // 9
// flags 10, 11 are not used by the operator
WekaManagesNonIonodeAffinity bool `json:"weka_manages_non_ionode_affinity"` // 12
// 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
Comment on lines +19 to +22

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
108 changes: 78 additions & 30 deletions internal/services/weka.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,35 @@ type SmbwCluster struct {
Active bool `json:"active"`
}

// NfsInterfaceGroupPort mirrors one entry of the "ports" array of `weka nfs interface-group --json`.
// The two CLI generations identify the owning container differently, and both are accepted:
//
// wekactl (domain.FeatureFlags.WekactlAsDefault): "container": 13, "container_uid": "..."
// legacy python CLI: "host_id": "HostId<13>", "host_uid": "..."
//
// Container is a pointer so that a genuine container 13 is distinguishable from an absent field,
// which matters because container 0 is a valid id.
type NfsInterfaceGroupPort struct {
HostId string `json:"host_id"`
HostUid string `json:"host_uid"`
Port string `json:"port"`
Status string `json:"status"`
Container *int `json:"container"`
ContainerUid string `json:"container_uid"`
HostId string `json:"host_id"`
HostUid string `json:"host_uid"`
Port string `json:"port"`
Status string `json:"status"`
}

// 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)
}
Comment on lines +264 to 276

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type NfsInterfaceGroup struct {
Expand All @@ -258,11 +282,28 @@ type NfsInterfaceGroup struct {
Name string `json:"name"`
Ports []NfsInterfaceGroupPort `json:"ports"`
Status string `json:"status"`
SubnetMask string `json:"subnet_mask"`
Type string `json:"type"`
Uid string `json:"uid"`
}

// ContainerPorts returns the port device names the given weka container currently holds in the group.
// Any port whose owner cannot be resolved fails the call: an unattributable port may well be the
// caller's own, and treating it as somebody else's is what turns a missed removal into a wedge.
func (g *NfsInterfaceGroup) ContainerPorts(containerId int) ([]string, error) {
var ports []string
for i := range g.Ports {
port := &g.Ports[i]
owner, err := port.OwnerContainerId()
if err != nil {
return nil, errors.Wrapf(err, "cannot determine port ownership in interface group %s", g.Name)
}
if owner == containerId {
ports = append(ports, port.Port)
}
}
return ports, nil
}

type WekaUserResponse struct {
// OrgId int `json:"org_id"`
// PosixGid string `json:"posix_gid"`
Expand Down Expand Up @@ -1050,47 +1091,54 @@ func (c *CliWekaService) RemoveFromSmbwCluster(ctx context.Context, containerId
return nil
}

// EnsureNfsInterfaceGroupPorts ensures the NFS interface group has the specified ports for a container.
// It fetches current state and reconciles to desired state by adding missing ports and removing extra ones.
func (c *CliWekaService) EnsureNfsInterfaceGroupPorts(ctx context.Context, interfaceGroupName string, containerId int, targetInterfaces []string) error {
ctx, logger := instrumentation.CreateLogSpan(ctx, "EnsureNfsInterfaceGroupPorts")
defer logger.End()

executor, err := c.getExecutor(ctx)
if err != nil {
return err
}

containerIdStr := strconv.Itoa(containerId)
hostIdStr := fmt.Sprintf("HostId<%d>", containerId)
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
}
Comment on lines +1094 to +1117

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":

  1. interfaceGroups[0] is returned without checking Name == 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.
  2. len(interfaceGroups) > 1 is 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))

Fix this →


// EnsureNfsInterfaceGroupPorts ensures the NFS interface group has the specified ports for a container.
// It fetches current state and reconciles to desired state by adding missing ports and removing extra ones.
func (c *CliWekaService) EnsureNfsInterfaceGroupPorts(ctx context.Context, interfaceGroupName string, containerId int, targetInterfaces []string) error {
ctx, logger := instrumentation.CreateLogSpan(ctx, "EnsureNfsInterfaceGroupPorts")
defer logger.End()

executor, err := c.getExecutor(ctx)
if err != nil {
return err
}

containerIdStr := strconv.Itoa(containerId)

group, err := c.getNfsInterfaceGroup(ctx, executor, interfaceGroupName)
if err != nil {
return err
}

// Get current ports for this container
var currentInterfaces []string
for _, port := range interfaceGroups[0].Ports {
if port.HostId == hostIdStr {
currentInterfaces = append(currentInterfaces, port.Port)
}
currentInterfaces, err := group.ContainerPorts(containerId)
if err != nil {
logger.SetError(err, "Failed to read current NFS interface group ports", "containerId", containerId)
return err
}

// Calculate what needs to be added and removed
Expand All @@ -1106,7 +1154,7 @@ func (c *CliWekaService) EnsureNfsInterfaceGroupPorts(ctx context.Context, inter

// Add new interfaces first (before removing old ones to avoid service interruption)
for _, interfaceName := range toAdd {
cmd = []string{
cmd := []string{
"wekaauthcli", "nfs", "interface-group", "port", "add", interfaceGroupName, containerIdStr, interfaceName,
}
_, stderr, err := executor.ExecNamed(ctx, "AddNfsInterfaceGroupPort", cmd)
Expand Down Expand Up @@ -1147,7 +1195,7 @@ func (c *CliWekaService) EnsureNfsInterfaceGroupPorts(ctx context.Context, inter

// Remove interfaces that shouldn't be there
for _, interfaceName := range toRemove {
cmd = []string{
cmd := []string{
"wekaauthcli", "nfs", "interface-group", "port", "delete", "-f", interfaceGroupName, containerIdStr, interfaceName,
}
_, stderr, err := executor.ExecNamed(ctx, "RemoveNfsInterfaceGroupPort", cmd)
Expand Down
129 changes: 129 additions & 0 deletions internal/services/weka_nfs_interface_group_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package services

import (
"encoding/json"
"testing"
)

// wekactlInterfaceGroupJSON was captured verbatim from `weka nfs interface-group --name
// MgmtInterfaceGroup --json` on a live 6.0.0.6363 cluster (feature flag wekactl_as_default=true).
// Container 13 is mid-deletion, hence UNREACHABLE.
const wekactlInterfaceGroupJSON = `[
{
"allow_manage_gids": true,
"gateway": "255.255.255.255",
"ips": [],
"name": "MgmtInterfaceGroup",
"netmask": 32,
"ports": [
{"container_uid": "b4b129f6-88fa-7050-d2bd-6d7f7d5c7e9f", "container": 12, "port": "enp99s0f0np0", "status": "OK"},
{"container_uid": "8026f2bb-10f3-9e82-2882-97d0f26db15a", "container": 15, "port": "enp99s0f0np0", "status": "OK"},
{"container_uid": "fee78013-db99-4a9b-844e-5d3c5c2f67bc", "container": 13, "port": "enp99s0f0np0", "status": "UNREACHABLE"},
{"container_uid": "eef2d3e6-195a-81fb-db0d-2a96872dcbfe", "container": 14, "port": "enp99s0f0np0", "status": "OK"}
],
"status": "OK",
"tenant_ids": [],
"type": "NFS",
"uid": "dbab1da8-db5c-bc93-0f63-1460c1f08450"
}
]`

// legacyInterfaceGroupJSON is the pre-wekactl python CLI shape, which identifies the owner with a
// "HostId<N>" string instead of a numeric container.
const legacyInterfaceGroupJSON = `[
{
"name": "MgmtInterfaceGroup",
"ports": [
{"host_uid": "b4b129f6-88fa-7050-d2bd-6d7f7d5c7e9f", "host_id": "HostId<12>", "port": "enp99s0f0np0", "status": "OK"},
{"host_uid": "fee78013-db99-4a9b-844e-5d3c5c2f67bc", "host_id": "HostId<13>", "port": "enp99s0f0np0", "status": "UNREACHABLE"}
],
"status": "OK",
"type": "NFS"
}
]`

func parseGroup(t *testing.T, payload string) NfsInterfaceGroup {
t.Helper()
var groups []NfsInterfaceGroup
if err := json.Unmarshal([]byte(payload), &groups); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if len(groups) != 1 {
t.Fatalf("expected 1 interface group, got %d", len(groups))
}
return groups[0]
}

// TestContainerPortsAcrossCliGenerations pins the port-identity field of both CLI generations.
// A mismatch here does not fail loudly in production: it makes ContainerPorts return nothing, so
// RemoveFromNfs removes nothing yet still reports success, and container deletion then wedges
// forever on "HostId<N> is still part of an interface group" from the deactivate step.
func TestContainerPortsAcrossCliGenerations(t *testing.T) {
for _, tc := range []struct {
name string
payload string
}{
{"wekactl", wekactlInterfaceGroupJSON},
{"legacy python cli", legacyInterfaceGroupJSON},
} {
t.Run(tc.name, func(t *testing.T) {
group := parseGroup(t, tc.payload)

ports, err := group.ContainerPorts(13)
if err != nil {
t.Fatalf("ContainerPorts(13): %v", err)
}
if len(ports) != 1 || ports[0] != "enp99s0f0np0" {
t.Fatalf("container 13 must own enp99s0f0np0, got %v", ports)
}

// The port must be attributed to exactly one container, or removing it from 13 would
// also be attempted for its neighbours.
other, err := group.ContainerPorts(12)
if err != nil {
t.Fatalf("ContainerPorts(12): %v", err)
}
if len(other) != 1 {
t.Fatalf("container 12 must own exactly one port, got %v", other)
}

absent, err := group.ContainerPorts(99)
if err != nil {
t.Fatalf("ContainerPorts(99): %v", err)
}
if len(absent) != 0 {
t.Fatalf("container 99 owns no ports, got %v", absent)
}
})
}
}

// TestContainerPortsRejectsUnknownSchema locks in loud failure on a third schema. Returning an empty
// slice instead would reproduce the original wedge: nothing to remove, success reported, deletion
// stuck behind a deactivate that keeps failing.
func TestContainerPortsRejectsUnknownSchema(t *testing.T) {
group := parseGroup(t, `[{"name": "MgmtInterfaceGroup", "ports": [{"owner": 13, "port": "enp99s0f0np0"}]}]`)

if _, err := group.ContainerPorts(13); err == nil {
t.Fatal("a port with no recognizable owner field must fail, not read as unowned")
}
}

// TestContainerZeroIsDistinguishableFromAbsent guards the pointer on NfsInterfaceGroupPort.Container.
// With a plain int, container 0 and "field missing" are the same value, so a malformed 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")
}
}
Comment on lines +109 to +129

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 1 addition & 1 deletion pkg/util/hashes.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func checkForMaps(v reflect.Value) error {
}
}
}
case reflect.Ptr:
case reflect.Pointer:
if !v.IsNil() {
if err := checkForMaps(v.Elem()); err != nil {
return err
Expand Down
Loading