Skip to content

Commit 1bbc39b

Browse files
andyzhangxandyzhangx
committed
fix: set mount path as uuid in Create/Delete Volume
field rename
1 parent daed573 commit 1bbc39b

File tree

2 files changed

+92
-51
lines changed

2 files changed

+92
-51
lines changed

‎pkg/smb/controllerserver.go‎

Lines changed: 37 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ type smbVolume struct {
3939
// Volume id
4040
id string
4141
// Address of the SMB server.
42-
sourceField string
42+
source string
4343
// Subdirectory of the SMB server to create volumes under
4444
subDir string
4545
// size of volume
@@ -51,7 +51,7 @@ type smbVolume struct {
5151
// Ordering of elements in the CSI volume id.
5252
// ID is of the form {server}/{subDir}.
5353
const (
54-
idsourceField = iota
54+
idSource = iota
5555
idSubDir
5656
idUUID
5757
totalIDElements // Always last
@@ -106,7 +106,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
106106
}()
107107
// Create subdirectory under base-dir
108108
// TODO: revisit permissions
109-
internalVolumePath := d.getInternalVolumePath(smbVol)
109+
internalVolumePath := getInternalVolumePath(d.workingMountDir, smbVol)
110110
if err = os.Mkdir(internalVolumePath, 0777); err != nil && !os.IsExist(err) {
111111
return nil, status.Errorf(codes.Internal, "failed to make subdirectory: %v", err.Error())
112112
}
@@ -145,10 +145,6 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
145145

146146
secrets := req.GetSecrets()
147147
deleteSubDir := len(secrets) > 0
148-
if len(smbVol.uuid) > 0 {
149-
klog.V(2).Infof("existing subDir(%s) is provided, skip subdirectory deletion", smbVol.subDir)
150-
deleteSubDir = false
151-
}
152148
if !deleteSubDir {
153149
options := strings.Split(mountOptions, ",")
154150
if hasGuestMountOptions(options) {
@@ -169,7 +165,7 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
169165
}()
170166

171167
// Delete subdirectory under base-dir
172-
internalVolumePath := d.getInternalVolumePath(smbVol)
168+
internalVolumePath := getInternalVolumePath(d.workingMountDir, smbVol)
173169
klog.V(2).Infof("Removing subdirectory at %v", internalVolumePath)
174170
if err = os.RemoveAll(internalVolumePath); err != nil {
175171
return nil, status.Errorf(codes.Internal, "failed to delete subdirectory: %v", err.Error())
@@ -244,23 +240,9 @@ func (d *Driver) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsReques
244240
return nil, status.Error(codes.Unimplemented, "")
245241
}
246242

247-
// Given a smbVolume, return a CSI volume id
248-
func getVolumeIDFromSmbVol(vol *smbVolume) string {
249-
idElements := make([]string, totalIDElements)
250-
idElements[idsourceField] = strings.Trim(vol.sourceField, "/")
251-
idElements[idSubDir] = strings.Trim(vol.subDir, "/")
252-
idElements[idUUID] = vol.uuid
253-
return strings.Join(idElements, separator)
254-
}
255-
256-
// Get working directory for CreateVolume and DeleteVolume
257-
func (d *Driver) getInternalMountPath(vol *smbVolume) string {
258-
return filepath.Join(d.workingMountDir, vol.subDir)
259-
}
260-
261243
// Mount smb server at base-dir
262244
func (d *Driver) internalMount(ctx context.Context, vol *smbVolume, volCap *csi.VolumeCapability, secrets map[string]string) error {
263-
stagingPath := d.getInternalMountPath(vol)
245+
stagingPath := getInternalMountPath(d.workingMountDir, vol)
264246

265247
if volCap == nil {
266248
volCap = &csi.VolumeCapability{
@@ -270,11 +252,11 @@ func (d *Driver) internalMount(ctx context.Context, vol *smbVolume, volCap *csi.
270252
}
271253
}
272254

273-
klog.V(4).Infof("internally mounting %v at %v", sourceField, stagingPath)
255+
klog.V(4).Infof("internally mounting %v at %v", vol.source, stagingPath)
274256
_, err := d.NodeStageVolume(ctx, &csi.NodeStageVolumeRequest{
275257
StagingTargetPath: stagingPath,
276258
VolumeContext: map[string]string{
277-
sourceField: vol.sourceField,
259+
sourceField: vol.source,
278260
},
279261
VolumeCapability: volCap,
280262
VolumeId: vol.id,
@@ -285,17 +267,38 @@ func (d *Driver) internalMount(ctx context.Context, vol *smbVolume, volCap *csi.
285267

286268
// Unmount smb server at base-dir
287269
func (d *Driver) internalUnmount(ctx context.Context, vol *smbVolume) error {
288-
targetPath := d.getInternalMountPath(vol)
270+
targetPath := getInternalMountPath(d.workingMountDir, vol)
289271

290272
// Unmount smb server at base-dir
291273
klog.V(4).Infof("internally unmounting %v", targetPath)
292274
_, err := d.NodeUnstageVolume(ctx, &csi.NodeUnstageVolumeRequest{
293275
VolumeId: vol.id,
294-
StagingTargetPath: d.getInternalMountPath(vol),
276+
StagingTargetPath: targetPath,
295277
})
296278
return err
297279
}
298280

281+
// Given a smbVolume, return a CSI volume id
282+
func getVolumeIDFromSmbVol(vol *smbVolume) string {
283+
idElements := make([]string, totalIDElements)
284+
idElements[idSource] = strings.Trim(vol.source, "/")
285+
idElements[idSubDir] = strings.Trim(vol.subDir, "/")
286+
idElements[idUUID] = vol.uuid
287+
return strings.Join(idElements, separator)
288+
}
289+
290+
// getInternalMountPath: get working directory for CreateVolume and DeleteVolume
291+
func getInternalMountPath(workingMountDir string, vol *smbVolume) string {
292+
if vol == nil {
293+
return ""
294+
}
295+
mountDir := vol.uuid
296+
if vol.uuid == "" {
297+
mountDir = vol.subDir
298+
}
299+
return filepath.Join(workingMountDir, mountDir)
300+
}
301+
299302
// Convert VolumeCreate parameters to an smbVolume
300303
func newSMBVolume(name string, size int64, params map[string]string) (*smbVolume, error) {
301304
var source, subDir string
@@ -317,8 +320,8 @@ func newSMBVolume(name string, size int64, params map[string]string) (*smbVolume
317320
}
318321

319322
vol := &smbVolume{
320-
sourceField: source,
321-
size: size,
323+
source: source,
324+
size: size,
322325
}
323326
if subDir == "" {
324327
// use pv name by default if not specified
@@ -339,8 +342,8 @@ func newSMBVolume(name string, size int64, params map[string]string) (*smbVolume
339342
// CreateVolume calls in parallel and they may use the same underlying share.
340343
// Instead of refcounting how many CreateVolume calls are using the same
341344
// share, it's simpler to just do a mount per request.
342-
func (d *Driver) getInternalVolumePath(vol *smbVolume) string {
343-
return filepath.Join(d.getInternalMountPath(vol), vol.subDir)
345+
func getInternalVolumePath(workingMountDir string, vol *smbVolume) string {
346+
return filepath.Join(getInternalMountPath(workingMountDir, vol), vol.subDir)
344347
}
345348

346349
// Convert into smbVolume into a csi.Volume
@@ -366,9 +369,9 @@ func getSmbVolFromID(id string) (*smbVolume, error) {
366369
source = "//" + source
367370
}
368371
vol := &smbVolume{
369-
id: id,
370-
sourceField: source,
371-
subDir: segments[1],
372+
id: id,
373+
source: source,
374+
subDir: segments[1],
372375
}
373376
if len(segments) >= 3 {
374377
vol.uuid = segments[2]

‎pkg/smb/controllerserver_test.go‎

Lines changed: 55 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ func TestGetSmbVolFromID(t *testing.T) {
500500
smbVolume, err := getSmbVolFromID(test.volumeID)
501501

502502
if !test.expectErr {
503-
assert.Equal(t, smbVolume.sourceField, test.source)
503+
assert.Equal(t, smbVolume.source, test.source)
504504
assert.Equal(t, smbVolume.subDir, test.subDir)
505505
assert.Equal(t, smbVolume.uuid, test.uuid)
506506
assert.Nil(t, err)
@@ -520,24 +520,24 @@ func TestGetVolumeIDFromSmbVol(t *testing.T) {
520520
{
521521
desc: "volume without uuid",
522522
vol: &smbVolume{
523-
sourceField: "//smb-server.default.svc.cluster.local/share",
524-
subDir: "subdir",
523+
source: "//smb-server.default.svc.cluster.local/share",
524+
subDir: "subdir",
525525
},
526526
result: "smb-server.default.svc.cluster.local/share#subdir#",
527527
},
528528
{
529529
desc: "volume with uuid",
530530
vol: &smbVolume{
531-
sourceField: "//smb-server.default.svc.cluster.local/share",
532-
subDir: "subdir",
533-
uuid: "uuid",
531+
source: "//smb-server.default.svc.cluster.local/share",
532+
subDir: "subdir",
533+
uuid: "uuid",
534534
},
535535
result: "smb-server.default.svc.cluster.local/share#subdir#uuid",
536536
},
537537
{
538538
desc: "volume without subdir",
539539
vol: &smbVolume{
540-
sourceField: "//smb-server.default.svc.cluster.local/share",
540+
source: "//smb-server.default.svc.cluster.local/share",
541541
},
542542
result: "smb-server.default.svc.cluster.local/share##",
543543
},
@@ -549,6 +549,44 @@ func TestGetVolumeIDFromSmbVol(t *testing.T) {
549549
}
550550
}
551551

552+
func TestGetInternalMountPath(t *testing.T) {
553+
cases := []struct {
554+
desc string
555+
workingMountDir string
556+
vol *smbVolume
557+
result string
558+
}{
559+
{
560+
desc: "nil volume",
561+
workingMountDir: "/tmp",
562+
result: "",
563+
},
564+
{
565+
desc: "uuid not empty",
566+
workingMountDir: "/tmp",
567+
vol: &smbVolume{
568+
subDir: "subdir",
569+
uuid: "uuid",
570+
},
571+
result: filepath.Join("/tmp", "uuid"),
572+
},
573+
{
574+
desc: "uuid empty",
575+
workingMountDir: "/tmp",
576+
vol: &smbVolume{
577+
subDir: "subdir",
578+
uuid: "",
579+
},
580+
result: filepath.Join("/tmp", "subdir"),
581+
},
582+
}
583+
584+
for _, test := range cases {
585+
path := getInternalMountPath(test.workingMountDir, test.vol)
586+
assert.Equal(t, path, test.result)
587+
}
588+
}
589+
552590
func TestNewSMBVolume(t *testing.T) {
553591
cases := []struct {
554592
desc string
@@ -567,11 +605,11 @@ func TestNewSMBVolume(t *testing.T) {
567605
"subDir": "subdir",
568606
},
569607
expectVol: &smbVolume{
570-
id: "smb-server.default.svc.cluster.local/share#subdir#pv-name",
571-
sourceField: "//smb-server.default.svc.cluster.local/share",
572-
subDir: "subdir",
573-
size: 100,
574-
uuid: "pv-name",
608+
id: "smb-server.default.svc.cluster.local/share#subdir#pv-name",
609+
source: "//smb-server.default.svc.cluster.local/share",
610+
subDir: "subdir",
611+
size: 100,
612+
uuid: "pv-name",
575613
},
576614
},
577615
{
@@ -582,11 +620,11 @@ func TestNewSMBVolume(t *testing.T) {
582620
"source": "//smb-server.default.svc.cluster.local/share",
583621
},
584622
expectVol: &smbVolume{
585-
id: "smb-server.default.svc.cluster.local/share#pv-name#",
586-
sourceField: "//smb-server.default.svc.cluster.local/share",
587-
subDir: "pv-name",
588-
size: 200,
589-
uuid: "",
623+
id: "smb-server.default.svc.cluster.local/share#pv-name#",
624+
source: "//smb-server.default.svc.cluster.local/share",
625+
subDir: "pv-name",
626+
size: 200,
627+
uuid: "",
590628
},
591629
},
592630
{

0 commit comments

Comments
 (0)