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

Fix mountOptions not applied during node stage and expand volume #375

Merged
merged 8 commits into from
Nov 12, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/actions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
- name: Checkout the code
uses: actions/checkout@v4
- name: Run the formatter, linter, and vetter
uses: dell/common-github-actions/go-code-formatter-linter-vetter@main
uses: dell/common-github-actions/go-code-formatter-vetter@main
with:
directories: ./...
test:
Expand Down
6 changes: 3 additions & 3 deletions pkg/array/array.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ func GetPowerStoreArrays(fs fs.Interface, filePath string) (map[string]*PowerSto
// It will do that by querying default powerstore array passed as one of the arguments
func ParseVolumeID(ctx context.Context, volumeHandle string,
defaultArray *PowerStoreArray, /*legacy support*/
cap *csi.VolumeCapability, /*legacy support*/
vc *csi.VolumeCapability, /*legacy support*/
) (localVolumeID, arrayID, protocol, remoteVolumeID, remoteArrayID string, e error) {
if volumeHandle == "" {
return "", "", "", "", "", status.Errorf(codes.FailedPrecondition,
Expand All @@ -305,8 +305,8 @@ func ParseVolumeID(ctx context.Context, volumeHandle string,
arrayID = defaultArray.GetGlobalID()

// If we have volume capability in request we can check FsType
if cap != nil && cap.GetMount() != nil {
if cap.GetMount().GetFsType() == "nfs" {
if vc != nil && vc.GetMount() != nil {
if vc.GetMount().GetFsType() == "nfs" {
protocol = "nfs"
} else {
protocol = "scsi"
Expand Down
13 changes: 11 additions & 2 deletions pkg/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ func SetLogFields(ctx context.Context, fields log.Fields) context.Context {

// RandomString returns a random string of specified length.
// String is generated by using crypto/rand.
func RandomString(len int) string {
b := make([]byte, len)
func RandomString(length int) string {
b := make([]byte, length)
_, err := rand.Read(b)
if err != nil {
log.Errorf("Can't generate random string; error = %v", err)
Expand Down Expand Up @@ -525,3 +525,12 @@ func ReachableEndPoint(endpoint string) bool {
}
return true
}

func GetMountFlags(vc *csi.VolumeCapability) []string {
if vc != nil {
if mount := vc.GetMount(); mount != nil {
return mount.GetMountFlags()
}
}
return nil
}
48 changes: 48 additions & 0 deletions pkg/common/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,3 +439,51 @@ func TestReachableEndPoint(t *testing.T) {
})
}
}

func TestGetMountFlags(t *testing.T) {
tests := []struct {
name string
vc *csi.VolumeCapability
expected []string
}{
{
name: "Nil VolumeCapability",
vc: nil,
expected: nil,
},
{
name: "Nil Mount",
vc: &csi.VolumeCapability{},
expected: nil,
},
{
name: "With Mount Flags",
vc: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Mount{
Mount: &csi.VolumeCapability_MountVolume{
MountFlags: []string{"ro", "noexec"},
},
},
},
expected: []string{"ro", "noexec"},
},
{
name: "Empty Mount Flags",
vc: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Mount{
Mount: &csi.VolumeCapability_MountVolume{
MountFlags: []string{},
},
},
},
expected: []string{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := common.GetMountFlags(tt.vc)
assert.Equal(t, tt.expected, result)
})
}
}
4 changes: 2 additions & 2 deletions pkg/common/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ func (fs *Fs) OpenFile(name string, flag int, perm os.FileMode) (*os.File, error
}

// WriteString is a wrapper of file.WriteString
func (fs *Fs) WriteString(file *os.File, string string) (int, error) {
return file.WriteString(string) // #nosec G304
func (fs *Fs) WriteString(file *os.File, str string) (int, error) {
return file.WriteString(str) // #nosec G304
}

// Create is a wrapper of os.Create
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1104,11 +1104,11 @@ func cacheMaximumVolumeSize(key string, value int64) {

// ControllerGetCapabilities returns list of capabilities that are supported by the driver.
func (s *Service) ControllerGetCapabilities(_ context.Context, _ *csi.ControllerGetCapabilitiesRequest) (*csi.ControllerGetCapabilitiesResponse, error) {
newCap := func(cap csi.ControllerServiceCapability_RPC_Type) *csi.ControllerServiceCapability {
newCap := func(capability csi.ControllerServiceCapability_RPC_Type) *csi.ControllerServiceCapability {
return &csi.ControllerServiceCapability{
Type: &csi.ControllerServiceCapability_Rpc{
Rpc: &csi.ControllerServiceCapability_RPC{
Type: cap,
Type: capability,
},
},
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/node/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,8 @@ func deleteMapping(volID, tmpDir string, fs fs.Interface) error {
return err
}

func isBlock(cap *csi.VolumeCapability) bool {
_, isBlock := cap.GetAccessType().(*csi.VolumeCapability_Block)
func isBlock(vc *csi.VolumeCapability) bool {
_, isBlock := vc.GetAccessType().(*csi.VolumeCapability_Block)
return isBlock
}

Expand Down
8 changes: 5 additions & 3 deletions pkg/node/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,9 @@ func (s *Service) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandVolum
return nil, status.Error(codes.Internal,
fmt.Sprintf("Failed to find mount info for (%s) with error (%s)", vol.Name, err.Error()))
}
err = s.Fs.GetUtil().Mount(ctx, disklocation, targetmount, "")

mntFlags := common.GetMountFlags(req.GetVolumeCapability())
err = s.Fs.GetUtil().Mount(ctx, disklocation, targetmount, "", mntFlags...)
if err != nil {
return nil, status.Error(codes.Internal,
fmt.Sprintf("Failed to find mount info for (%s) with error (%s)", vol.Name, err.Error()))
Expand Down Expand Up @@ -1072,11 +1074,11 @@ func (s *Service) nodeExpandRawBlockVolume(ctx context.Context, volumeWWN string

// NodeGetCapabilities returns supported features by the node service
func (s *Service) NodeGetCapabilities(_ context.Context, _ *csi.NodeGetCapabilitiesRequest) (*csi.NodeGetCapabilitiesResponse, error) {
newCap := func(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeServiceCapability {
newCap := func(capability csi.NodeServiceCapability_RPC_Type) *csi.NodeServiceCapability {
return &csi.NodeServiceCapability{
Type: &csi.NodeServiceCapability_Rpc{
Rpc: &csi.NodeServiceCapability_RPC{
Type: cap,
Type: capability,
},
},
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/node/publisher.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"context"

"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/dell/csi-powerstore/v2/pkg/common"
"github.com/dell/csi-powerstore/v2/pkg/common/fs"
log "github.com/sirupsen/logrus"
"google.golang.org/grpc/codes"
Expand All @@ -31,7 +32,7 @@ import (
// VolumePublisher allows to node publish a volume
type VolumePublisher interface {
Publish(ctx context.Context, logFields log.Fields, fs fs.Interface,
cap *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error)
vc *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error)
}

// SCSIPublisher implementation of NodeVolumePublisher for SCSI based (FC, iSCSI) volumes
Expand All @@ -40,7 +41,7 @@ type SCSIPublisher struct {
}

// Publish publishes volume as either raw block or mount by mounting it to the target path
func (sp *SCSIPublisher) Publish(ctx context.Context, logFields log.Fields, fs fs.Interface, cap *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error) {
func (sp *SCSIPublisher) Publish(ctx context.Context, logFields log.Fields, fs fs.Interface, vc *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error) {
published, err := isAlreadyPublished(ctx, targetPath, getRWModeString(isRO), fs)
if err != nil {
return nil, err
Expand All @@ -51,9 +52,9 @@ func (sp *SCSIPublisher) Publish(ctx context.Context, logFields log.Fields, fs f
}

if sp.isBlock {
return sp.publishBlock(ctx, logFields, fs, cap, isRO, targetPath, stagingPath)
return sp.publishBlock(ctx, logFields, fs, vc, isRO, targetPath, stagingPath)
}
return sp.publishMount(ctx, logFields, fs, cap, isRO, targetPath, stagingPath)
return sp.publishMount(ctx, logFields, fs, vc, isRO, targetPath, stagingPath)
}

func (sp *SCSIPublisher) publishBlock(ctx context.Context, logFields log.Fields, fs fs.Interface, _ *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error) {
Expand All @@ -78,21 +79,21 @@ func (sp *SCSIPublisher) publishBlock(ctx context.Context, logFields log.Fields,
return &csi.NodePublishVolumeResponse{}, nil
}

func (sp *SCSIPublisher) publishMount(ctx context.Context, logFields log.Fields, fs fs.Interface, cap *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error) {
if cap.GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER {
func (sp *SCSIPublisher) publishMount(ctx context.Context, logFields log.Fields, fs fs.Interface, vc *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string) (*csi.NodePublishVolumeResponse, error) {
if vc.GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER {
// MULTI_WRITER not supported for mount volumes
return nil, status.Error(codes.Unimplemented, "Mount volumes do not support AccessMode MULTI_NODE_MULTI_WRITER")
}

if cap.GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY {
if vc.GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY {
// Warning in case of MULTI_NODE_READER_ONLY for mount volumes
log.Warningf("Mount volume with the AccessMode ReadOnlyMany")
}

var opts []string
mountCap := cap.GetMount()
mountCap := vc.GetMount()
mountFsType := mountCap.GetFsType()
mntFlags := mountCap.GetMountFlags()
mntFlags := common.GetMountFlags(vc)
if mountFsType == "xfs" {
mntFlags = append(mntFlags, "nouuid")
}
Expand Down Expand Up @@ -150,7 +151,7 @@ type NFSPublisher struct{}

// Publish publishes nfs volume by mounting it to the target path
func (np *NFSPublisher) Publish(ctx context.Context, logFields log.Fields, fs fs.Interface,
cap *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string,
vc *csi.VolumeCapability, isRO bool, targetPath string, stagingPath string,
) (*csi.NodePublishVolumeResponse, error) {
published, err := isAlreadyPublished(ctx, targetPath, getRWModeString(isRO), fs)
if err != nil {
Expand All @@ -167,8 +168,7 @@ func (np *NFSPublisher) Publish(ctx context.Context, logFields log.Fields, fs fs
}
log.WithFields(logFields).Info("target path successfully created")

mountCap := cap.GetMount()
mntFlags := mountCap.GetMountFlags()
mntFlags := common.GetMountFlags(vc)

if isRO {
mntFlags = append(mntFlags, "ro")
Expand Down
6 changes: 4 additions & 2 deletions pkg/node/stager.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ func (s *SCSIStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest,
}
log.WithFields(logFields).Info("target path successfully created")

if err := fs.GetUtil().BindMount(ctx, devicePath, stagingPath); err != nil {
mntFlags := common.GetMountFlags(req.GetVolumeCapability())
if err := fs.GetUtil().BindMount(ctx, devicePath, stagingPath, mntFlags...); err != nil {
return nil, status.Errorf(codes.Internal,
"error bind disk %s to target path: %s", devicePath, err.Error())
}
Expand Down Expand Up @@ -177,7 +178,8 @@ func (n *NFSStager) Stage(ctx context.Context, req *csi.NodeStageVolumeRequest,
}
log.WithFields(logFields).Info("stage path successfully created")

if err := fs.GetUtil().Mount(ctx, nfsExport, stagingPath, ""); err != nil {
mntFlags := common.GetMountFlags(req.GetVolumeCapability())
if err := fs.GetUtil().Mount(ctx, nfsExport, stagingPath, "", mntFlags...); err != nil {
return nil, status.Errorf(codes.Internal,
"error mount nfs share %s to target path: %s", nfsExport, err.Error())
}
Expand Down