From e48ee9ecc24c64c00d0bc8b46fd27a7497c0aa7a Mon Sep 17 00:00:00 2001 From: Anton Myagkov Date: Thu, 15 Aug 2024 14:40:04 +0200 Subject: [PATCH] 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 --- .../blockstore/public/sdk/go/client/client.go | 24 +++++++ cloud/blockstore/tests/csi_driver/test.py | 8 +-- cloud/blockstore/tools/.gitignore | 4 +- .../csi_driver/internal/driver/controller.go | 58 ++++++++++------- .../tools/csi_driver/internal/driver/node.go | 65 +++++++++++++++---- .../csi_driver/internal/driver/node_test.go | 3 + .../internal/pkg/clients/nbs/client.go | 17 +---- 7 files changed, 119 insertions(+), 60 deletions(-) diff --git a/cloud/blockstore/public/sdk/go/client/client.go b/cloud/blockstore/public/sdk/go/client/client.go index 394c6ee58e3..f26dcf483a7 100644 --- a/cloud/blockstore/public/sdk/go/client/client.go +++ b/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, diff --git a/cloud/blockstore/tests/csi_driver/test.py b/cloud/blockstore/tests/csi_driver/test.py index dc40fd59993..8855d22ca3a 100644 --- a/cloud/blockstore/tests/csi_driver/test.py +++ b/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", diff --git a/cloud/blockstore/tools/.gitignore b/cloud/blockstore/tools/.gitignore index 44c7166446e..a58218608e1 100644 --- a/cloud/blockstore/tools/.gitignore +++ b/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 diff --git a/cloud/blockstore/tools/csi_driver/internal/driver/controller.go b/cloud/blockstore/tools/csi_driver/internal/driver/controller.go index 1aca84d14b7..9a3a7e037af 100644 --- a/cloud/blockstore/tools/csi_driver/internal/driver/controller.go +++ b/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 -} diff --git a/cloud/blockstore/tools/csi_driver/internal/driver/node.go b/cloud/blockstore/tools/csi_driver/internal/driver/node.go index 53293339738..e77024ac9e6 100644 --- a/cloud/blockstore/tools/csi_driver/internal/driver/node.go +++ b/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 } diff --git a/cloud/blockstore/tools/csi_driver/internal/driver/node_test.go b/cloud/blockstore/tools/csi_driver/internal/driver/node_test.go index 634ce27985d..fb2afcb33bf 100644 --- a/cloud/blockstore/tools/csi_driver/internal/driver/node_test.go +++ b/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, diff --git a/cloud/disk_manager/internal/pkg/clients/nbs/client.go b/cloud/disk_manager/internal/pkg/clients/nbs/client.go index a4b24ec297f..e31df1fbbb1 100644 --- a/cloud/disk_manager/internal/pkg/clients/nbs/client.go +++ b/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 {