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.

Sign up for GitHub

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

support volume partition #824

Merged
merged 1 commit into from
Apr 9, 2021
Merged

support volume partition #824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions pkg/driver/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ const (
DevicePathKey = "devicePath"
)

// constants of keys in VolumeContext
const (
// VolumeAttributePartition represents key for partition config in VolumeContext
// this represents the partition number on a device used to mount
VolumeAttributePartition = "partition"
)

// constants of disk partition suffix
const (
diskPartitionSuffix = ""
nvmeDiskPartitionSuffix = "p"
)

// constants of keys in volume parameters
const (
// VolumeTypeKey represents key for volume type
Expand Down
17 changes: 17 additions & 0 deletions pkg/driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,23 @@ func isValidVolumeCapabilities(volCaps []*csi.VolumeCapability) bool {
return foundAll
}

func isValidVolumeContext(volContext map[string]string) bool {
//There could be multiple volume attributes in the volumeContext map
//Validate here case by case
if partition, ok := volContext[VolumeAttributePartition]; ok {
partitionInt, err := strconv.ParseInt(partition, 10, 64)
if err != nil {
klog.Errorf("failed to parse partition %s as int", partition)
return false
}
if partitionInt < 0 {
klog.Errorf("invalid partition config, partition = %s", partition)
return false
}
}
return true
}

func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {
klog.V(4).Infof("CreateSnapshot: called with args %+v", req)
snapshotName := req.GetName()
Expand Down
30 changes: 28 additions & 2 deletions pkg/driver/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol
if !isValidVolumeCapabilities([]*csi.VolumeCapability{volCap}) {
return nil, status.Error(codes.InvalidArgument, "Volume capability not supported")
}
volumeContext := req.GetVolumeContext()
if isValidVolumeContext := isValidVolumeContext(volumeContext); !isValidVolumeContext {
return nil, status.Error(codes.InvalidArgument, "Volume Attribute is not valid")
}

// If the access type is block, do nothing for stage
switch volCap.GetAccessType().(type) {
Expand Down Expand Up @@ -158,7 +162,15 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol
return nil, status.Error(codes.InvalidArgument, "Device path not provided")
}

source, err := d.findDevicePath(devicePath, volumeID)
partition := ""
if part, ok := volumeContext[VolumeAttributePartition]; ok {
if part != "0" {
partition = part
} else {
klog.Warningf("NodeStageVolume: invalid partition config, will ignore. partition = %v", part)
}
}
source, err := d.findDevicePath(devicePath, volumeID, partition)
if err != nil {
return nil, status.Errorf(codes.Internal, "Failed to find device path %s. %v", devicePath, err)
}
Expand Down Expand Up @@ -516,12 +528,26 @@ func (d *nodeService) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoReque
func (d *nodeService) nodePublishVolumeForBlock(req *csi.NodePublishVolumeRequest, mountOptions []string) error {
target := req.GetTargetPath()
volumeID := req.GetVolumeId()
volumeContext := req.GetVolumeContext()

devicePath, exists := req.PublishContext[DevicePathKey]
if !exists {
return status.Error(codes.InvalidArgument, "Device path not provided")
}
source, err := d.findDevicePath(devicePath, volumeID)
if isValidVolumeContext := isValidVolumeContext(volumeContext); !isValidVolumeContext {
return status.Error(codes.InvalidArgument, "Volume Attribute is invalid")
}

partition := ""
wongma7 marked this conversation as resolved.
Show resolved Hide resolved
if part, ok := req.GetVolumeContext()[VolumeAttributePartition]; ok {
if part != "0" {
partition = part
Copy link
Contributor

Choose a reason for hiding this comment

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

should we validate it? Check that it's an integer? and not something crazy/malicious with weird paths and whatnot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, parse to int makes sense to me, is there any special rule for windows partition validation? should that be same?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave it same, I'll test it out later.

I think I will make the windows findDevicePath implementation ignore the partition arg. From my understanding, the windows mount code will always mount the first partition so it doesn't matter what partition is set to. https://github.com/kubernetes-csi/csi-proxy/blob/d47b9ecde8f020dd9d5eb2df3e56bf2f10a3f4a3/internal/os/volume/api.go#L87 (similarly, fstype gets ignored in windows world)

} else {
klog.Warningf("NodePublishVolume: invalid partition config, will ignore. partition = %v", part)
}
}

source, err := d.findDevicePath(devicePath, volumeID, partition)
if err != nil {
return status.Errorf(codes.Internal, "Failed to find device path %s. %v", devicePath, err)
}
Expand Down
14 changes: 12 additions & 2 deletions pkg/driver/node_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,17 @@ import (
// findDevicePath finds path of device and verifies its existence
// if the device is not nvme, return the path directly
// if the device is nvme, finds and returns the nvme device path eg. /dev/nvme1n1
func (d *nodeService) findDevicePath(devicePath, volumeID string) (string, error) {
func (d *nodeService) findDevicePath(devicePath, volumeID string, partition string) (string, error) {
exists, err := d.mounter.PathExists(devicePath)
if err != nil {
return "", err
}

// If the path exists, assume it is not nvme device
if exists {
if partition != "" {
devicePath = devicePath + diskPartitionSuffix + partition
}
return devicePath, nil
}

Expand All @@ -49,7 +52,14 @@ func (d *nodeService) findDevicePath(devicePath, volumeID string) (string, error
// /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol0fab1d5e3f72a5e23
nvmeName := "nvme-Amazon_Elastic_Block_Store_" + strings.Replace(volumeID, "-", "", -1)

return findNvmeVolume(nvmeName)
nvmeDevicePath, err := findNvmeVolume(nvmeName)
if err != nil {
return "", err
}
if partition != "" {
nvmeDevicePath = nvmeDevicePath + nvmeDiskPartitionSuffix + partition
}
return nvmeDevicePath, nil
}

// findNvmeVolume looks for the nvme volume with the specified name
Expand Down
178 changes: 178 additions & 0 deletions pkg/driver/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ func TestNodeStageVolume(t *testing.T) {
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
}
stdVolContext = map[string]string{VolumeAttributePartition: "1"}
devicePathWithPartition = devicePath + "1"
)
testCases := []struct {
name string
Expand Down Expand Up @@ -241,6 +243,17 @@ func TestNodeStageVolume(t *testing.T) {
},
expectedCode: codes.InvalidArgument,
},
{
name: "fail invalid volumeContext",
request: &csi.NodeStageVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
StagingTargetPath: targetPath,
VolumeCapability: stdVolCap,
VolumeContext: map[string]string{VolumeAttributePartition: "partition1"},
VolumeId: "vol-test",
},
expectedCode: codes.InvalidArgument,
},
{
name: "success device already mounted at target",
request: &csi.NodeStageVolumeRequest{
Expand All @@ -260,6 +273,44 @@ func TestNodeStageVolume(t *testing.T) {
mockMounter.EXPECT().FormatAndMount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
},
},
{
name: "success with partition",
request: &csi.NodeStageVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
StagingTargetPath: targetPath,
VolumeCapability: stdVolCap,
VolumeContext: stdVolContext,
VolumeId: "vol-test",
},
expectMock: func(mockMounter mocks.MockMounter) {
gomock.InOrder(
mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(false, nil),
)
mockMounter.EXPECT().MakeDir(targetPath).Return(nil)
mockMounter.EXPECT().GetDeviceNameFromMount(targetPath).Return("", 1, nil)
mockMounter.EXPECT().FormatAndMount(gomock.Eq(devicePathWithPartition), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Any())
},
},
{
name: "success with invalid partition config, will ignore partition",
request: &csi.NodeStageVolumeRequest{
PublishContext: map[string]string{DevicePathKey: devicePath},
StagingTargetPath: targetPath,
VolumeCapability: stdVolCap,
VolumeContext: map[string]string{VolumeAttributePartition: "0"},
VolumeId: "vol-test",
},
expectMock: func(mockMounter mocks.MockMounter) {
gomock.InOrder(
mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
mockMounter.EXPECT().PathExists(gomock.Eq(targetPath)).Return(false, nil),
)
mockMounter.EXPECT().MakeDir(targetPath).Return(nil)
mockMounter.EXPECT().GetDeviceNameFromMount(targetPath).Return("", 1, nil)
mockMounter.EXPECT().FormatAndMount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(defaultFsType), gomock.Any())
},
},
{
name: "fail with in-flight request",
request: &csi.NodeStageVolumeRequest{
Expand Down Expand Up @@ -517,6 +568,8 @@ func TestNodePublishVolume(t *testing.T) {
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
}
stdVolContext := map[string]string{"partition": "1"}
devicePathWithPartition := devicePath + "1"
testCases := []struct {
name string
testFunc func(t *testing.T)
Expand Down Expand Up @@ -716,6 +769,131 @@ func TestNodePublishVolume(t *testing.T) {
}
},
},
{
name: "success normal with partition [raw block]",
testFunc: func(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(),
}

gomock.InOrder(
mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil),
)
mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil)
mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
mockMounter.EXPECT().Mount(gomock.Eq(devicePathWithPartition), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: "/dev/fake"},
StagingTargetPath: stagingTargetPath,
TargetPath: targetPath,
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Block{
Block: &csi.VolumeCapability_BlockVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
},
VolumeContext: stdVolContext,
VolumeId: "vol-test",
}

_, err := awsDriver.NodePublishVolume(context.TODO(), req)
if err != nil {
t.Fatalf("Expect no error but got: %v", err)
}
},
},
{
name: "success normal with invalid partition config, will ignore the config [raw block]",
testFunc: func(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(),
}

gomock.InOrder(
mockMounter.EXPECT().PathExists(gomock.Eq(devicePath)).Return(true, nil),
mockMounter.EXPECT().PathExists(gomock.Eq("/test")).Return(false, nil),
)
mockMounter.EXPECT().MakeDir(gomock.Eq("/test")).Return(nil)
mockMounter.EXPECT().MakeFile(targetPath).Return(nil)
mockMounter.EXPECT().Mount(gomock.Eq(devicePath), gomock.Eq(targetPath), gomock.Eq(""), gomock.Eq([]string{"bind"})).Return(nil)

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: "/dev/fake"},
StagingTargetPath: stagingTargetPath,
TargetPath: targetPath,
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Block{
Block: &csi.VolumeCapability_BlockVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
},
VolumeContext: map[string]string{VolumeAttributePartition: "0"},
VolumeId: "vol-test",
}

_, err := awsDriver.NodePublishVolume(context.TODO(), req)
if err != nil {
t.Fatalf("Expect no error but got: %v", err)
}
},
},
{
name: "Fail invalid volumeContext config [raw block]",
testFunc: func(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(),
}

req := &csi.NodePublishVolumeRequest{
PublishContext: map[string]string{DevicePathKey: "/dev/fake"},
StagingTargetPath: stagingTargetPath,
TargetPath: targetPath,
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Block{
Block: &csi.VolumeCapability_BlockVolume{},
},
AccessMode: &csi.VolumeCapability_AccessMode{
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
},
},
VolumeContext: map[string]string{VolumeAttributePartition: "partition1"},
VolumeId: "vol-test",
}

_, err := awsDriver.NodePublishVolume(context.TODO(), req)
expectErr(t, err, codes.InvalidArgument)
},
},
{
name: "fail no device path [raw block]",
testFunc: func(t *testing.T) {
Expand Down