issue-1588: fix parameters validation/error codes in csi driver (#1790)

issue: #1588

1. add parameters validation to NodeGetVolumeStats
2. return AlreadyExists error code if volume exists
3. update error messages
4. enable skipped csi sanity tests

cloud/blockstore/public/sdk/go/client/client.go

@@ -1,6 +1,9 @@
 package client
 
 import (
+	"errors"
+	"strings"
+
 	protos "github.com/ydb-platform/nbs/cloud/blockstore/public/api/protos"
 	"golang.org/x/net/context"
 )
@@ -237,6 +240,27 @@ func (client *Client) RefreshEndpoint(
 
 ////////////////////////////////////////////////////////////////////////////////
 
+func IsDiskNotFoundError(e error) bool {
+	var clientErr *ClientError
+	if errors.As(e, &clientErr) {
+		if clientErr.Facility() == FACILITY_SCHEMESHARD {
+			// TODO: remove support for PathDoesNotExist.
+			if clientErr.Status() == 2 {
+				return true
+			}
+
+			// Hack for NBS-3162.
+			if strings.Contains(clientErr.Error(), "Another drop in progress") {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 func NewClient(
 	grpcOpts *GrpcClientOpts,
 	durableOpts *DurableClientOpts,

cloud/blockstore/tests/csi_driver/test.py

@@ -338,13 +338,7 @@ def test_csi_sanity_nbs_backend():
         params_file = Path(os.getcwd()) / "params.yaml"
         params_file.write_text(f"backend: {backend}")
 
-        skipTests = ["should fail when requesting to create a volume with already existing name and different capacity",
-                     "should fail when the node does not exist",
-                     # below are disabled NodeGetVolumeStats tests: NBSNEBIUS-390
-                     "should fail when no volume id is provided",
-                     "should fail when no volume path is provided",
-                     "should fail when volume does not exist on the specified path",
-                     "should fail when volume is not found"]
+        skipTests = ["should fail when the node does not exist"]
 
         args = [CSI_SANITY_BINARY_PATH,
                 "-csi.endpoint",

cloud/blockstore/tools/.gitignore

@@ -46,6 +46,8 @@ nbd/blockstore-nbd
 testing/build-fresh-blob/build-fresh-blob
 testing/chaos-monkey/chaos-monkey
 testing/chaos-monkey/tests/tests
+testing/csi-sanity/bin/cloud
+testing/csi-sanity/bin/csi-sanity
 testing/eternal_tests/checkpoint-validator/bin/checkpoint-validator
 testing/eternal_tests/checkpoint-validator/lib/ut/lib-ut
 testing/eternal_tests/eternal-load/bin/eternal-load
@@ -67,5 +69,3 @@ testing/plugintest/blockstore-plugintest
 testing/rdma-test/rdma-test
 testing/stable-plugin/yandex-cloud-blockstore-plugin_*.deb
 testing/verify-test/verify-test
-testing/csi-sanity/bin/cloud
-testing/csi-sanity/bin/csi-sanity

cloud/blockstore/tools/csi_driver/internal/driver/controller.go

@@ -125,12 +125,25 @@ func (c *nbsServerControllerService) CreateVolume(
 	var err error
 	if parameters["backend"] == "nfs" {
 		err = c.createFileStore(ctx, req.Name, requiredBytes)
+		// TODO (issues/464): return codes.AlreadyExists if volume exists
 	} else {
 		err = c.createDisk(ctx, req.Name, requiredBytes, parameters)
+		if err != nil {
+			describeVolumeRequest := &nbsapi.TDescribeVolumeRequest{
+				DiskId: req.Name,
+			}
+			_, describeVolumeErr := c.nbsClient.DescribeVolume(
+				ctx,
+				describeVolumeRequest)
+			if describeVolumeErr == nil {
+				return nil, status.Errorf(
+					codes.AlreadyExists,
+					"Failed to create volume: %w", describeVolumeErr)
+			}
+		}
 	}
 
 	if err != nil {
-		// TODO (issues/464): return codes.AlreadyExists if volume exists
 		return nil, status.Errorf(
 			codes.Internal, "Failed to create volume: %w", err)
 	}
@@ -243,9 +256,18 @@ func (c *nbsServerControllerService) ControllerPublishVolume(
 			"VolumeCapability missing in ControllerPublishVolumeRequest")
 	}
 
-	if !c.doesVolumeExist(ctx, req.VolumeId) {
+	describeVolumeRequest := &nbsapi.TDescribeVolumeRequest{
+		DiskId: req.VolumeId,
+	}
+	_, err := c.nbsClient.DescribeVolume(ctx, describeVolumeRequest)
+	if err != nil {
+		if nbsclient.IsDiskNotFoundError(err) {
+			return nil, status.Errorf(
+				codes.NotFound, "Volume %q does not exist", req.VolumeId)
+		}
+
 		return nil, status.Errorf(
-			codes.NotFound, "Volume %q does not exist", req.VolumeId)
+			codes.Internal, "Failed to publish volume: %w", err)
 	}
 
 	// TODO (issues/464): check if req.NodeId exists in the cluster
@@ -287,9 +309,18 @@ func (c *nbsServerControllerService) ValidateVolumeCapabilities(
 			"VolumeCapabilities missing in ValidateVolumeCapabilitiesRequest")
 	}
 
-	if !c.doesVolumeExist(ctx, req.VolumeId) {
+	describeVolumeRequest := &nbsapi.TDescribeVolumeRequest{
+		DiskId: req.VolumeId,
+	}
+	_, err := c.nbsClient.DescribeVolume(ctx, describeVolumeRequest)
+	if err != nil {
+		if nbsclient.IsDiskNotFoundError(err) {
+			return nil, status.Errorf(
+				codes.NotFound, "Volume %q does not exist", req.VolumeId)
+		}
+
 		return nil, status.Errorf(
-			codes.NotFound, "Volume %q does not exist", req.VolumeId)
+			codes.Internal, "Failed to validate volume capabilities: %w", err)
 	}
 
 	return &csi.ValidateVolumeCapabilitiesResponse{}, nil
@@ -304,20 +335,3 @@ func (c *nbsServerControllerService) ControllerGetCapabilities(
 		Capabilities: nbsServerControllerServiceCapabilities,
 	}, nil
 }
-
-func (c *nbsServerControllerService) doesVolumeExist(
-	ctx context.Context,
-	volumeId string) bool {
-
-	describeVolumeRequest := &nbsapi.TDescribeVolumeRequest{
-		DiskId: volumeId,
-	}
-
-	_, err := c.nbsClient.DescribeVolume(ctx, describeVolumeRequest)
-	if err != nil {
-		// TODO (issues/464): check error code
-		return false
-	}
-
-	return true
-}

cloud/blockstore/tools/csi_driver/internal/driver/node.go

@@ -129,17 +129,17 @@ func (s *nodeService) NodeStageVolume(
 	if req.VolumeId == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"VolumeId missing in NodeStageVolumeRequest")
+			"VolumeId is missing in NodeStageVolumeRequest")
 	}
 	if req.StagingTargetPath == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"StagingTargetPath missing in NodeStageVolumeRequest")
+			"StagingTargetPath is missing in NodeStageVolumeRequest")
 	}
 	if req.VolumeCapability == nil {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"VolumeCapability missing in NodeStageVolumeRequest")
+			"VolumeCapability is missing in NodeStageVolumeRequest")
 	}
 
 	return &csi.NodeStageVolumeResponse{}, nil
@@ -154,12 +154,12 @@ func (s *nodeService) NodeUnstageVolume(
 	if req.VolumeId == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"VolumeId missing in NodeUnstageVolumeRequest")
+			"VolumeId is missing in NodeUnstageVolumeRequest")
 	}
 	if req.StagingTargetPath == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"StagingTargetPath missing in NodeUnstageVolumeRequest")
+			"StagingTargetPath is missing in NodeUnstageVolumeRequest")
 	}
 
 	return &csi.NodeUnstageVolumeResponse{}, nil
@@ -179,27 +179,27 @@ func (s *nodeService) NodePublishVolume(
 	if req.StagingTargetPath == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"StagingTargetPath missing in NodePublishVolumeRequest")
+			"StagingTargetPath is missing in NodePublishVolumeRequest")
 	}
 	if req.TargetPath == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"TargetPath missing in NodePublishVolumeRequest")
+			"TargetPath is missing in NodePublishVolumeRequest")
 	}
 	if req.VolumeCapability == nil {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"VolumeCapability missing in NodePublishVolumeRequest")
+			"VolumeCapability is missing in NodePublishVolumeRequest")
 	}
 	if req.VolumeContext == nil {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"VolumeContext missing in NodePublishVolumeRequest")
+			"VolumeContext is missing in NodePublishVolumeRequest")
 	}
 
 	if s.getPodId(req) == "" {
 		return nil, s.statusError(codes.Internal,
-			"podUID missing in NodePublishVolumeRequest.VolumeContext")
+			"podUID is missing in NodePublishVolumeRequest.VolumeContext")
 	}
 
 	var err error
@@ -250,12 +250,12 @@ func (s *nodeService) NodeUnpublishVolume(
 	if req.VolumeId == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"Volume ID missing in NodeUnpublishVolumeRequest")
+			"Volume ID is missing in NodeUnpublishVolumeRequest")
 	}
 	if req.TargetPath == "" {
 		return nil, s.statusError(
 			codes.InvalidArgument,
-			"Target Path missing in NodeUnpublishVolumeRequest")
+			"Target Path is missing in NodeUnpublishVolumeRequest")
 	}
 
 	if err := s.nodeUnpublishVolume(ctx, req); err != nil {
@@ -692,12 +692,51 @@ func (s *nodeService) NodeGetVolumeStats(
 	req *csi.NodeGetVolumeStatsRequest) (
 	*csi.NodeGetVolumeStatsResponse, error) {
 
+	if req.VolumeId == "" {
+		return nil, s.statusError(
+			codes.InvalidArgument,
+			"VolumeId is missing in NodeGetVolumeStatsRequest")
+	}
+
+	if req.VolumePath == "" {
+		return nil, s.statusError(
+			codes.InvalidArgument,
+			"VolumePath is missing in NodeGetVolumeStatsRequest")
+	}
+
 	if s.vmMode {
 		return nil, fmt.Errorf("NodeGetVolumeStats is not supported in vmMode")
 	}
 
+	if s.nbsClient == nil {
+		return nil, fmt.Errorf("NBS client is not available")
+	}
+
+	describeVolumeRequest := &nbsapi.TDescribeVolumeRequest{
+		DiskId: req.VolumeId,
+	}
+
+	_, err := s.nbsClient.DescribeVolume(ctx, describeVolumeRequest)
+	if err != nil {
+		if nbsclient.IsDiskNotFoundError(err) {
+			return nil, s.statusError(
+				codes.NotFound,
+				"Volume is not found")
+		}
+		return nil, s.statusErrorf(
+			codes.Internal,
+			"Failed to get volume stats: %v", err)
+	}
+
+	_, err = s.mounter.IsMountPoint(req.VolumePath)
+	if err != nil {
+		return nil, s.statusError(
+			codes.NotFound,
+			"Volume does not exist on the specified path")
+	}
+
 	var stat unix.Statfs_t
-	err := unix.Statfs(req.VolumePath, &stat)
+	err = unix.Statfs(req.VolumePath, &stat)
 	if err != nil {
 		return nil, err
 	}

cloud/blockstore/tools/csi_driver/internal/driver/node_test.go

@@ -471,6 +471,9 @@ func TestGetVolumeStatCapabilitiesWithoutVmMode(t *testing.T) {
 		})
 	assert.NotEqual(t, -1, capabilityIndex)
 
+	nbsClient.On("DescribeVolume", ctx, &nbs.TDescribeVolumeRequest{DiskId: diskID}).Return(&nbs.TDescribeVolumeResponse{}, nil)
+	mounter.On("IsMountPoint", targetPath).Return(true, nil)
+
 	stat, err := nodeService.NodeGetVolumeStats(ctx, &csi.NodeGetVolumeStatsRequest{
 		VolumeId:   diskID,
 		VolumePath: targetPath,

cloud/disk_manager/internal/pkg/clients/nbs/client.go

@@ -356,22 +356,7 @@ func isAbortedError(e error) bool {
 }
 
 func IsDiskNotFoundError(e error) bool {
-	var clientErr *nbs_client.ClientError
-	if errors.As(e, &clientErr) {
-		if clientErr.Facility() == nbs_client.FACILITY_SCHEMESHARD {
-			// TODO: remove support for PathDoesNotExist.
-			if clientErr.Status() == 2 {
-				return true
-			}
-
-			// Hack for NBS-3162.
-			if strings.Contains(clientErr.Error(), "Another drop in progress") {
-				return true
-			}
-		}
-	}
-
-	return false
+	return nbs_client.IsDiskNotFoundError(e)
 }
 
 func IsNotFoundError(e error) bool {