Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NodeExpandVolume no-op for raw block #695

Merged
Merged
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
45 changes: 41 additions & 4 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,47 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV
if len(volumeID) == 0 {
return nil, status.Error(codes.InvalidArgument, "Volume ID not provided")
}
volumePath := req.GetVolumePath()
if len(volumePath) == 0 {
return nil, status.Error(codes.InvalidArgument, "volume path must be provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

So prior to this PR, we don't have this requirement (at least here). We have to make sure this is not a breaking change. Did we verify that behavior stays the same when volumePath is absent?

Copy link
Contributor

Choose a reason for hiding this comment

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

volume_path is required, so we should be fine I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

volumeCapability := req.GetVolumeCapability()
// VolumeCapability is optional, if specified, use that as source of truth
if volumeCapability != nil {
caps := []*csi.VolumeCapability{volumeCapability}
if !isValidVolumeCapabilities(caps) {
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("VolumeCapability is invalid: %v", volumeCapability))
}

args := []string{"-o", "source", "--noheadings", "--target", req.GetVolumePath()}
if blk := volumeCapability.GetBlock(); blk != nil {
// Noop for Block NodeExpandVolume
klog.V(4).Infof("NodeExpandVolume called for %v at %s. Since it is a block device, ignoring...", volumeID, volumePath)
return &csi.NodeExpandVolumeResponse{}, nil
}
} else {
// VolumeCapability is nil, check if volumePath point to a block device
isBlock, err := d.IsBlockDevice(volumePath)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to determine device path for volumePath [%v]: %v", volumePath, err)
}
if isBlock {
// Skip resizing for Block NodeExpandVolume
bcap, err := d.getBlockSizeBytes(volumePath)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to get block capacity on path %s: %v", req.VolumePath, err)
}
klog.V(4).Infof("NodeExpandVolume called for %v at %s, since given volumePath is a block device, ignoring...", volumeID, volumePath)
return &csi.NodeExpandVolumeResponse{CapacityBytes: bcap}, nil
}
}

args := []string{"-o", "source", "--noheadings", "--target", volumePath}
output, err := d.mounter.Command("findmnt", args...).Output()
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not determine device path: %v", err)

}

devicePath := strings.TrimSpace(string(output))
if len(devicePath) == 0 {
return nil, status.Errorf(codes.Internal, "Could not get valid device for mount path: %q", req.GetVolumePath())
Expand All @@ -274,11 +307,15 @@ func (d *nodeService) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV
})

// TODO: lock per volume ID to have some idempotency
if _, err := r.Resize(devicePath, req.GetVolumePath()); err != nil {
if _, err := r.Resize(devicePath, volumePath); err != nil {
return nil, status.Errorf(codes.Internal, "Could not resize volume %q (%q): %v", volumeID, devicePath, err)
}

return &csi.NodeExpandVolumeResponse{}, nil
bcap, err := d.getBlockSizeBytes(devicePath)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to get block capacity on path %s: %v", req.VolumePath, err)
}
return &csi.NodeExpandVolumeResponse{CapacityBytes: bcap}, nil
}

func (d *nodeService) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) {
Expand Down
86 changes: 86 additions & 0 deletions pkg/driver/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1082,6 +1082,92 @@ func TestNodePublishVolume(t *testing.T) {
t.Run(tc.name, tc.testFunc)
}
}
func TestNodeExpandVolume(t *testing.T) {
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()

mockMetadata := mocks.NewMockMetadataService(mockCtl)
mockMounter := mocks.NewMockMounter(mockCtl)

awsDriver := &nodeService{
metadata: mockMetadata,
mounter: mockMounter,
inFlight: internal.NewInFlight(),
}

tests := []struct {
name string
request csi.NodeExpandVolumeRequest
expectResponseCode codes.Code
}{
{
name: "fail missing volumeId",
request: csi.NodeExpandVolumeRequest{},
expectResponseCode: codes.InvalidArgument,
},
{
name: "fail missing volumePath",
request: csi.NodeExpandVolumeRequest{
StagingTargetPath: "/testDevice/Path",
VolumeId: "test-volume-id",
},
expectResponseCode: codes.InvalidArgument,
},
{
name: "fail volume path not exist",
request: csi.NodeExpandVolumeRequest{
VolumePath: "./test",
VolumeId: "test-volume-id",
},
expectResponseCode: codes.Internal,
},
{
name: "Fail validate VolumeCapability",
request: csi.NodeExpandVolumeRequest{
VolumePath: "./test",
VolumeId: "test-volume-id",
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Block{
Block: &csi.VolumeCapability_BlockVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_UNKNOWN,
},
},
},
expectResponseCode: codes.InvalidArgument,
},
{
name: "Success [VolumeCapability is block]",
request: csi.NodeExpandVolumeRequest{
VolumePath: "./test",
VolumeId: "test-volume-id",
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Block{
Block: &csi.VolumeCapability_BlockVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use losetup and dd to check the other case of volume_path being a block device?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use losetup and dd to create loop device when setup testing but with no luck. It failed to find an available loop device when I ran it in linux instance.. This should not be a blocker as I tested the functionality e2e on the cluster. Testing code is here, I will follow up on this later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to approve to unblock you, but yeah let's follow up.

expectResponseCode: codes.OK,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
_, err := awsDriver.NodeExpandVolume(context.Background(), &test.request)
if err != nil {
if test.expectResponseCode != codes.OK {
expectErr(t, err, test.expectResponseCode)
} else {
t.Fatalf("Expect no error but got: %v", err)
}
}
})
}
}

func TestNodeUnpublishVolume(t *testing.T) {
targetPath := "/test/path"
Expand Down