From 068f68dec467bcdc7fa1851c22268e858c1b22ff Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Thu, 30 Jul 2026 10:52:00 +0200 Subject: [PATCH 1/2] fix(csi): populate VolumeCondition in NodeGetVolumeStats The driver advertises RPC_VOLUME_CONDITION in NodeGetCapabilities but never populated the VolumeCondition field, so external-health-monitor had no signal to mark a volume abnormal and trigger automated cleanup of stale VolumeAttachments after a stale/disconnected NVMe-oF mount. Check the stashed devicePath up front (catches the case where a cached stat/statfs on the mount still looks healthy), and turn any subsequent stat/statfs/block-size error into Abnormal: true instead of an opaque gRPC Internal error the sidecar can't interpret. Co-Authored-By: Claude Sonnet 5 --- csi-driver/pkg/spdk/nodeserver.go | 42 ++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/csi-driver/pkg/spdk/nodeserver.go b/csi-driver/pkg/spdk/nodeserver.go index 4fa32754d..ecc64d22e 100644 --- a/csi-driver/pkg/spdk/nodeserver.go +++ b/csi-driver/pkg/spdk/nodeserver.go @@ -179,18 +179,47 @@ func (ns *nodeServer) NodeGetVolumeStats( return nil, status.Error(codes.InvalidArgument, "volume_path is required") } + // IsMountPoint/stat on volumePath can still succeed even after the backing NVMe-oF + // device is gone (see backingBlockDeviceGone), so check the stashed device path + // directly rather than relying on volumePath alone. + if stagingPath := req.GetStagingTargetPath(); stagingPath != "" { + if vc, err := util.LookupVolumeContext(stagingPath); err == nil { + if devicePath := vc["devicePath"]; devicePath != "" && !deviceExists(devicePath) { + return &csi.NodeGetVolumeStatsResponse{ + VolumeCondition: &csi.VolumeCondition{ + Abnormal: true, + Message: fmt.Sprintf("NVMe device %s not found; volume has disconnected", devicePath), + }, + }, nil + } + } + } + st, err := os.Stat(volumePath) if err != nil { if os.IsNotExist(err) { return nil, status.Error(codes.NotFound, "volume_path not found") } - return nil, status.Errorf(codes.Internal, "stat volume_path %q: %v", volumePath, err) + // Any other stat error (EIO, ENOTCONN) means the mount is unhealthy. + return &csi.NodeGetVolumeStatsResponse{ + VolumeCondition: &csi.VolumeCondition{ + Abnormal: true, + Message: fmt.Sprintf("stat %q failed: %v", volumePath, err), + }, + }, nil } if st.IsDir() { var s unix.Statfs_t if err := unix.Statfs(volumePath, &s); err != nil { - return nil, status.Errorf(codes.Internal, "statfs %q: %v", volumePath, err) + // statfs failing on a mounted directory means the backing device has + // gone away (stale NVMe/TCP mount surfaces as EIO or ENOTCONN here). + return &csi.NodeGetVolumeStatsResponse{ + VolumeCondition: &csi.VolumeCondition{ + Abnormal: true, + Message: fmt.Sprintf("statfs %q failed: %v", volumePath, err), + }, + }, nil } // Compute in uint64 (Bsize is int64 on Linux but uint32 on darwin; the block @@ -225,12 +254,18 @@ func (ns *nodeServer) NodeGetVolumeStats( Available: availInodes, }, }, + VolumeCondition: &csi.VolumeCondition{Abnormal: false}, }, nil } sizeBytes, err := getBlockSizeBytes(volumePath) if err != nil { - return nil, status.Errorf(codes.Internal, "get block size for %q: %v", volumePath, err) + return &csi.NodeGetVolumeStatsResponse{ + VolumeCondition: &csi.VolumeCondition{ + Abnormal: true, + Message: fmt.Sprintf("get block size for %q failed: %v", volumePath, err), + }, + }, nil } return &csi.NodeGetVolumeStatsResponse{ @@ -242,6 +277,7 @@ func (ns *nodeServer) NodeGetVolumeStats( Available: int64(sizeBytes), }, }, + VolumeCondition: &csi.VolumeCondition{Abnormal: false}, }, nil } From b733712df09998c58dea07e9e1904591659c3a50 Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Thu, 30 Jul 2026 14:57:08 +0200 Subject: [PATCH 2/2] test(csi): add unit tests for NodeGetVolumeStats VolumeCondition reporting Covers the healthy-directory, disconnected-device, connected-device, missing-stash, and block-size-error paths. NodeGetVolumeStats touches none of nodeServer's fields, so these run against the zero-value receiver with real temp files/dirs -- no mounter mocking needed. Verified these fail against the pre-fix implementation (3 of 7 cases), confirming they pin the actual behavior change rather than passing trivially. Co-Authored-By: Claude Sonnet 5 --- .../pkg/spdk/nodeserver_volumestats_test.go | 195 ++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 csi-driver/pkg/spdk/nodeserver_volumestats_test.go diff --git a/csi-driver/pkg/spdk/nodeserver_volumestats_test.go b/csi-driver/pkg/spdk/nodeserver_volumestats_test.go new file mode 100644 index 000000000..a4d2003f6 --- /dev/null +++ b/csi-driver/pkg/spdk/nodeserver_volumestats_test.go @@ -0,0 +1,195 @@ +/* +Copyright (c) Arm Limited and Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package spdk + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/container-storage-interface/spec/lib/go/csi" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/spdk/spdk-csi/pkg/util" +) + +// NodeGetVolumeStats touches none of nodeServer's fields, so the zero value is +// a valid receiver for these tests -- no mounter/kubeClient/driver setup needed. + +const testVolumeID = "vol" + +func TestNodeGetVolumeStats_MissingArgs(t *testing.T) { + ns := &nodeServer{} + ctx := context.Background() + + req := &csi.NodeGetVolumeStatsRequest{VolumePath: "/tmp"} + if _, err := ns.NodeGetVolumeStats(ctx, req); status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument for missing volume_id, got %v", err) + } + req = &csi.NodeGetVolumeStatsRequest{VolumeId: testVolumeID} + if _, err := ns.NodeGetVolumeStats(ctx, req); status.Code(err) != codes.InvalidArgument { + t.Fatalf("expected InvalidArgument for missing volume_path, got %v", err) + } +} + +func TestNodeGetVolumeStats_VolumePathMissing(t *testing.T) { + ns := &nodeServer{} + + _, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: filepath.Join(t.TempDir(), "does-not-exist"), + }) + if status.Code(err) != codes.NotFound { + t.Fatalf("expected NotFound, got %v", err) + } +} + +func TestNodeGetVolumeStats_HealthyDirectoryReportsNormal(t *testing.T) { + ns := &nodeServer{} + + resp, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: t.TempDir(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cond := resp.GetVolumeCondition(); cond == nil || cond.GetAbnormal() { + t.Fatalf("expected Abnormal=false for a healthy mount, got %+v", cond) + } +} + +// TestNodeGetVolumeStats_DisconnectedDeviceReportsAbnormal is the core regression +// test for the bug this fix addresses: a cached stat/statfs on the staging +// directory can still succeed even after the backing NVMe-oF device is gone, so +// NodeGetVolumeStats must cross-check the stashed devicePath independently. +func TestNodeGetVolumeStats_DisconnectedDeviceReportsAbnormal(t *testing.T) { + ns := &nodeServer{} + dir := t.TempDir() + + stagingParentPath := filepath.Join(dir, "staging") + if err := os.MkdirAll(stagingParentPath, 0o755); err != nil { + t.Fatalf("mkdir staging parent: %v", err) + } + volumePath := filepath.Join(dir, "mount") + if err := os.MkdirAll(volumePath, 0o755); err != nil { + t.Fatalf("mkdir volume path: %v", err) + } + + missingDevice := filepath.Join(dir, "nvme-disconnected") + if err := util.StashVolumeContext(map[string]string{"devicePath": missingDevice}, stagingParentPath); err != nil { + t.Fatalf("stash volume context: %v", err) + } + + resp, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: volumePath, + StagingTargetPath: stagingParentPath, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cond := resp.GetVolumeCondition() + if cond == nil || !cond.GetAbnormal() { + t.Fatalf("expected Abnormal=true when the stashed device is gone, got %+v", cond) + } + if !strings.Contains(cond.GetMessage(), missingDevice) { + t.Fatalf("expected message to reference the missing device path, got %q", cond.GetMessage()) + } +} + +func TestNodeGetVolumeStats_ConnectedDeviceStillReportsNormal(t *testing.T) { + ns := &nodeServer{} + dir := t.TempDir() + + stagingParentPath := filepath.Join(dir, "staging") + if err := os.MkdirAll(stagingParentPath, 0o755); err != nil { + t.Fatalf("mkdir staging parent: %v", err) + } + volumePath := filepath.Join(dir, "mount") + if err := os.MkdirAll(volumePath, 0o755); err != nil { + t.Fatalf("mkdir volume path: %v", err) + } + + existingDevice := filepath.Join(dir, "nvme-connected") + if err := os.WriteFile(existingDevice, []byte("x"), 0o600); err != nil { + t.Fatalf("create fake device path: %v", err) + } + if err := util.StashVolumeContext(map[string]string{"devicePath": existingDevice}, stagingParentPath); err != nil { + t.Fatalf("stash volume context: %v", err) + } + + resp, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: volumePath, + StagingTargetPath: stagingParentPath, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cond := resp.GetVolumeCondition(); cond.GetAbnormal() { + t.Fatalf("expected Abnormal=false when the stashed device still exists, got %+v", cond) + } +} + +func TestNodeGetVolumeStats_NoStashSkipsDeviceCheck(t *testing.T) { + ns := &nodeServer{} + dir := t.TempDir() + + // No stash written at stagingParentPath (e.g. kubelet didn't pass + // staging_target_path, or nothing has been staged yet at this path). + // The device check must be skipped, not treated as abnormal. + resp, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: dir, + StagingTargetPath: filepath.Join(dir, "no-such-staging-path"), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cond := resp.GetVolumeCondition(); cond.GetAbnormal() { + t.Fatalf("expected Abnormal=false when no stash exists, got %+v", cond) + } +} + +// TestNodeGetVolumeStats_BlockSizeErrorReportsAbnormal exercises the non-directory +// (raw block volume) path: a regular file is not a block device, so the +// BLKGETSIZE64 ioctl legitimately fails, which must now surface as +// Abnormal: true instead of an opaque gRPC Internal error. +func TestNodeGetVolumeStats_BlockSizeErrorReportsAbnormal(t *testing.T) { + ns := &nodeServer{} + dir := t.TempDir() + + volumePath := filepath.Join(dir, "block-target") + if err := os.WriteFile(volumePath, []byte("not a real block device"), 0o600); err != nil { + t.Fatalf("create fake block target: %v", err) + } + + resp, err := ns.NodeGetVolumeStats(context.Background(), &csi.NodeGetVolumeStatsRequest{ + VolumeId: testVolumeID, + VolumePath: volumePath, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cond := resp.GetVolumeCondition(); cond == nil || !cond.GetAbnormal() { + t.Fatalf("expected Abnormal=true for a non-block-device volume path, got %+v", cond) + } +}