Skip to content

Commit fa9b36d

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#76151 from vladimirvivien/plugin-CanAttach-return-error
Ability for volume AttachablePlugin.CanAttach() to return both bool and error
2 parents 65dc445 + cfafde9 commit fa9b36d

File tree

16 files changed

+131
-43
lines changed

16 files changed

+131
-43
lines changed

‎pkg/controller/volume/attachdetach/testing/testvolumespec.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,8 @@ func (plugin *TestPlugin) NewDetacher() (volume.Detacher, error) {
308308
return &detacher, nil
309309
}
310310

311-
func (plugin *TestPlugin) CanAttach(spec *volume.Spec) bool {
312-
return true
311+
func (plugin *TestPlugin) CanAttach(spec *volume.Spec) (bool, error) {
312+
return true, nil
313313
}
314314

315315
func (plugin *TestPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/awsebs/attacher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,8 +277,8 @@ func (detacher *awsElasticBlockStoreDetacher) UnmountDevice(deviceMountPath stri
277277
return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
278278
}
279279

280-
func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) bool {
281-
return true
280+
func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) (bool, error) {
281+
return true, nil
282282
}
283283

284284
func (plugin *awsElasticBlockStorePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/azure_dd/azure_dd.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,8 +238,8 @@ func (plugin *azureDataDiskPlugin) NewDetacher() (volume.Detacher, error) {
238238
}, nil
239239
}
240240

241-
func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) bool {
242-
return true
241+
func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) {
242+
return true, nil
243243
}
244244

245245
func (plugin *azureDataDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/cinder/attacher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,8 @@ func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error
406406
return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
407407
}
408408

409-
func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) bool {
410-
return true
409+
func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) (bool, error) {
410+
return true, nil
411411
}
412412

413413
func (plugin *cinderPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/csi/csi_attacher_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,10 @@ func TestAttacherWithCSIDriver(t *testing.T) {
348348
csiAttacher := attacher.(*csiAttacher)
349349
spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
350350

351-
pluginCanAttach := plug.CanAttach(spec)
351+
pluginCanAttach, err := plug.CanAttach(spec)
352+
if err != nil {
353+
t.Fatalf("attacher.CanAttach failed: %s", err)
354+
}
352355
if pluginCanAttach != test.expectVolumeAttachment {
353356
t.Errorf("attacher.CanAttach does not match expected attachment status %t", test.expectVolumeAttachment)
354357
}
@@ -429,7 +432,10 @@ func TestAttacherWaitForVolumeAttachmentWithCSIDriver(t *testing.T) {
429432
csiAttacher := attacher.(*csiAttacher)
430433
spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
431434

432-
pluginCanAttach := plug.CanAttach(spec)
435+
pluginCanAttach, err := plug.CanAttach(spec)
436+
if err != nil {
437+
t.Fatalf("plugin.CanAttach test failed: %s", err)
438+
}
433439
if !pluginCanAttach {
434440
t.Log("plugin is not attachable")
435441
return

‎pkg/volume/csi/csi_plugin.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -581,34 +581,30 @@ func (p *csiPlugin) NewDetacher() (volume.Detacher, error) {
581581
}, nil
582582
}
583583

584-
// TODO change CanAttach to return error to propagate ability
585-
// to support Attachment or an error - see https://github.com/kubernetes/kubernetes/issues/74810
586-
func (p *csiPlugin) CanAttach(spec *volume.Spec) bool {
584+
func (p *csiPlugin) CanAttach(spec *volume.Spec) (bool, error) {
587585
driverMode, err := p.getDriverMode(spec)
588586
if err != nil {
589-
return false
587+
return false, err
590588
}
591589

592590
if driverMode == ephemeralDriverMode {
593-
klog.V(4).Info(log("driver ephemeral mode detected for spec %v", spec.Name))
594-
return false
591+
klog.V(5).Info(log("plugin.CanAttach = false, ephemeral mode detected for spec %v", spec.Name()))
592+
return false, nil
595593
}
596594

597595
pvSrc, err := getCSISourceFromSpec(spec)
598596
if err != nil {
599-
klog.Error(log("plugin.CanAttach failed to get info from spec: %s", err))
600-
return false
597+
return false, err
601598
}
602599

603600
driverName := pvSrc.Driver
604601

605602
skipAttach, err := p.skipAttach(driverName)
606603
if err != nil {
607-
klog.Error(log("plugin.CanAttach error when calling plugin.skipAttach for driver %s: %s", driverName, err))
608-
return false
604+
return false, err
609605
}
610606

611-
return !skipAttach
607+
return !skipAttach, nil
612608
}
613609

614610
// TODO (#75352) add proper logic to determine device moutability by inspecting the spec.

‎pkg/volume/csi/csi_plugin_test.go‎

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -880,6 +880,7 @@ func TestPluginCanAttach(t *testing.T) {
880880
driverName string
881881
spec *volume.Spec
882882
canAttach bool
883+
shouldFail bool
883884
}{
884885
{
885886
name: "non-attachable inline",
@@ -893,6 +894,19 @@ func TestPluginCanAttach(t *testing.T) {
893894
spec: volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true),
894895
canAttach: true,
895896
},
897+
{
898+
name: "incomplete spec",
899+
driverName: "attachable-pv",
900+
spec: &volume.Spec{ReadOnly: true},
901+
canAttach: false,
902+
shouldFail: true,
903+
},
904+
{
905+
name: "nil spec",
906+
driverName: "attachable-pv",
907+
canAttach: false,
908+
shouldFail: true,
909+
},
896910
}
897911

898912
for _, test := range tests {
@@ -902,10 +916,80 @@ func TestPluginCanAttach(t *testing.T) {
902916
plug, tmpDir := newTestPlugin(t, fakeCSIClient)
903917
defer os.RemoveAll(tmpDir)
904918

905-
pluginCanAttach := plug.CanAttach(test.spec)
919+
pluginCanAttach, err := plug.CanAttach(test.spec)
920+
if err != nil && !test.shouldFail {
921+
t.Fatalf("unexected plugin.CanAttach error: %s", err)
922+
}
906923
if pluginCanAttach != test.canAttach {
907924
t.Fatalf("expecting plugin.CanAttach %t got %t", test.canAttach, pluginCanAttach)
908-
return
925+
}
926+
})
927+
}
928+
}
929+
930+
func TestPluginFindAttachablePlugin(t *testing.T) {
931+
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIInlineVolume, true)()
932+
tests := []struct {
933+
name string
934+
driverName string
935+
spec *volume.Spec
936+
canAttach bool
937+
shouldFail bool
938+
}{
939+
{
940+
name: "non-attachable inline",
941+
driverName: "attachable-inline",
942+
spec: volume.NewSpecFromVolume(makeTestVol("test-vol", "attachable-inline")),
943+
canAttach: false,
944+
},
945+
{
946+
name: "attachable PV",
947+
driverName: "attachable-pv",
948+
spec: volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true),
949+
canAttach: true,
950+
},
951+
{
952+
name: "incomplete spec",
953+
driverName: "attachable-pv",
954+
spec: &volume.Spec{ReadOnly: true},
955+
canAttach: false,
956+
shouldFail: true,
957+
},
958+
{
959+
name: "nil spec",
960+
driverName: "attachable-pv",
961+
canAttach: false,
962+
shouldFail: true,
963+
},
964+
}
965+
966+
for _, test := range tests {
967+
t.Run(test.name, func(t *testing.T) {
968+
tmpDir, err := utiltesting.MkTmpdir("csi-test")
969+
if err != nil {
970+
t.Fatalf("can't create temp dir: %v", err)
971+
}
972+
defer os.RemoveAll(tmpDir)
973+
974+
client := fakeclient.NewSimpleClientset(getCSIDriver(test.driverName, nil, &test.canAttach))
975+
factory := informers.NewSharedInformerFactory(client, csiResyncPeriod)
976+
host := volumetest.NewFakeVolumeHostWithCSINodeName(
977+
tmpDir,
978+
client,
979+
nil,
980+
"fakeNode",
981+
factory.Storage().V1beta1().CSIDrivers().Lister(),
982+
)
983+
984+
plugMgr := &volume.VolumePluginMgr{}
985+
plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, host)
986+
987+
plugin, err := plugMgr.FindAttachablePluginBySpec(test.spec)
988+
if err != nil && !test.shouldFail {
989+
t.Fatalf("unexected error calling pluginMgr.FindAttachablePluginBySpec: %s", err)
990+
}
991+
if (plugin != nil) != test.canAttach {
992+
t.Fatal("expecting attachable plugin, but got nil")
909993
}
910994
})
911995
}

‎pkg/volume/fc/attacher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,8 @@ func (detacher *fcDetacher) UnmountDevice(deviceMountPath string) error {
175175
return nil
176176
}
177177

178-
func (plugin *fcPlugin) CanAttach(spec *volume.Spec) bool {
179-
return true
178+
func (plugin *fcPlugin) CanAttach(spec *volume.Spec) (bool, error) {
179+
return true, nil
180180
}
181181

182182
func (plugin *fcPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/flexvolume/plugin.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,8 @@ func (plugin *flexVolumeAttachablePlugin) NewDeviceUnmounter() (volume.DeviceUnm
256256
return plugin.NewDetacher()
257257
}
258258

259-
func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) bool {
260-
return true
259+
func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) (bool, error) {
260+
return true, nil
261261
}
262262

263263
func (plugin *flexVolumeAttachablePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

‎pkg/volume/gcepd/attacher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,8 @@ func (detacher *gcePersistentDiskDetacher) UnmountDevice(deviceMountPath string)
350350
return mount.CleanupMountPoint(deviceMountPath, detacher.host.GetMounter(gcePersistentDiskPluginName), false)
351351
}
352352

353-
func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) bool {
354-
return true
353+
func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) {
354+
return true, nil
355355
}
356356

357357
func (plugin *gcePersistentDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

0 commit comments

Comments
 (0)