Skip to content

Commit cd02e75

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#71509 from cofyc/fix71438
Fix device mountable volume names in DSW
2 parents be5a1fb + 67552a8 commit cd02e75

File tree

5 files changed

+266
-12
lines changed

5 files changed

+266
-12
lines changed

‎pkg/kubelet/volumemanager/cache/desired_state_of_world.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,12 @@ func (dsw *desiredStateOfWorld) AddPodToVolume(
203203

204204
var volumeName v1.UniqueVolumeName
205205

206-
// The unique volume name used depends on whether the volume is attachable
206+
// The unique volume name used depends on whether the volume is attachable/device-mountable
207207
// or not.
208208
attachable := dsw.isAttachableVolume(volumeSpec)
209-
if attachable {
210-
// For attachable volumes, use the unique volume name as reported by
209+
deviceMountable := dsw.isDeviceMountableVolume(volumeSpec)
210+
if attachable || deviceMountable {
211+
// For attachable/device-mountable volumes, use the unique volume name as reported by
211212
// the plugin.
212213
volumeName, err =
213214
util.GetUniqueVolumeNameFromSpec(volumePlugin, volumeSpec)
@@ -219,13 +220,11 @@ func (dsw *desiredStateOfWorld) AddPodToVolume(
219220
err)
220221
}
221222
} else {
222-
// For non-attachable volumes, generate a unique name based on the pod
223+
// For non-attachable and non-device-mountable volumes, generate a unique name based on the pod
223224
// namespace and name and the name of the volume within the pod.
224-
volumeName = util.GetUniqueVolumeNameForNonAttachableVolume(podName, volumePlugin, volumeSpec)
225+
volumeName = util.GetUniqueVolumeNameFromSpecWithPod(podName, volumePlugin, volumeSpec)
225226
}
226227

227-
deviceMountable := dsw.isDeviceMountableVolume(volumeSpec)
228-
229228
if _, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists {
230229
dsw.volumesToMount[volumeName] = volumeToMount{
231230
volumeName: volumeName,

‎pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,168 @@ func Test_AddPodToVolume_Positive_ExistingPodExistingVolume(t *testing.T) {
117117
verifyVolumeExistsWithSpecNameInVolumeDsw(t, podName, volumeSpec.Name(), dsw)
118118
}
119119

120+
// Call AddPodToVolume() on different pods for different kinds of volumes
121+
// Verities generated names are same for different pods if volume is device mountable or attachable
122+
// Verities generated names are different for different pods if volume is not device mountble and attachable
123+
func Test_AddPodToVolume_Positive_NamesForDifferentPodsAndDifferentVolumes(t *testing.T) {
124+
// Arrange
125+
fakeVolumeHost := volumetesting.NewFakeVolumeHost(
126+
"", /* rootDir */
127+
nil, /* kubeClient */
128+
nil, /* plugins */
129+
)
130+
plugins := []volume.VolumePlugin{
131+
&volumetesting.FakeBasicVolumePlugin{
132+
Plugin: volumetesting.FakeVolumePlugin{
133+
PluginName: "basic",
134+
},
135+
},
136+
&volumetesting.FakeDeviceMountableVolumePlugin{
137+
FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{
138+
Plugin: volumetesting.FakeVolumePlugin{
139+
PluginName: "device-mountable",
140+
},
141+
},
142+
},
143+
&volumetesting.FakeAttachableVolumePlugin{
144+
FakeDeviceMountableVolumePlugin: volumetesting.FakeDeviceMountableVolumePlugin{
145+
FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{
146+
Plugin: volumetesting.FakeVolumePlugin{
147+
PluginName: "attachable",
148+
},
149+
},
150+
},
151+
},
152+
}
153+
volumePluginMgr := volume.VolumePluginMgr{}
154+
volumePluginMgr.InitPlugins(plugins, nil /* prober */, fakeVolumeHost)
155+
dsw := NewDesiredStateOfWorld(&volumePluginMgr)
156+
157+
testcases := map[string]struct {
158+
pod1 *v1.Pod
159+
pod2 *v1.Pod
160+
same bool
161+
}{
162+
"basic": {
163+
&v1.Pod{
164+
ObjectMeta: metav1.ObjectMeta{
165+
Name: "pod1",
166+
UID: "pod1uid",
167+
},
168+
Spec: v1.PodSpec{
169+
Volumes: []v1.Volume{
170+
{
171+
Name: "basic",
172+
VolumeSource: v1.VolumeSource{},
173+
},
174+
},
175+
},
176+
},
177+
&v1.Pod{
178+
ObjectMeta: metav1.ObjectMeta{
179+
Name: "pod2",
180+
UID: "pod2uid",
181+
},
182+
Spec: v1.PodSpec{
183+
Volumes: []v1.Volume{
184+
{
185+
Name: "basic",
186+
VolumeSource: v1.VolumeSource{},
187+
},
188+
},
189+
},
190+
},
191+
false,
192+
},
193+
"device-mountable": {
194+
&v1.Pod{
195+
ObjectMeta: metav1.ObjectMeta{
196+
Name: "pod1",
197+
UID: "pod1uid",
198+
},
199+
Spec: v1.PodSpec{
200+
Volumes: []v1.Volume{
201+
{
202+
Name: "device-mountable",
203+
VolumeSource: v1.VolumeSource{},
204+
},
205+
},
206+
},
207+
},
208+
&v1.Pod{
209+
ObjectMeta: metav1.ObjectMeta{
210+
Name: "pod2",
211+
UID: "pod2uid",
212+
},
213+
Spec: v1.PodSpec{
214+
Volumes: []v1.Volume{
215+
{
216+
Name: "device-mountable",
217+
VolumeSource: v1.VolumeSource{},
218+
},
219+
},
220+
},
221+
},
222+
true,
223+
},
224+
"attachable": {
225+
&v1.Pod{
226+
ObjectMeta: metav1.ObjectMeta{
227+
Name: "pod1",
228+
UID: "pod1uid",
229+
},
230+
Spec: v1.PodSpec{
231+
Volumes: []v1.Volume{
232+
{
233+
Name: "attachable",
234+
VolumeSource: v1.VolumeSource{},
235+
},
236+
},
237+
},
238+
},
239+
&v1.Pod{
240+
ObjectMeta: metav1.ObjectMeta{
241+
Name: "pod2",
242+
UID: "pod2uid",
243+
},
244+
Spec: v1.PodSpec{
245+
Volumes: []v1.Volume{
246+
{
247+
Name: "attachable",
248+
VolumeSource: v1.VolumeSource{},
249+
},
250+
},
251+
},
252+
},
253+
true,
254+
},
255+
}
256+
257+
// Act & Assert
258+
for name, v := range testcases {
259+
volumeSpec1 := &volume.Spec{Volume: &v.pod1.Spec.Volumes[0]}
260+
volumeSpec2 := &volume.Spec{Volume: &v.pod2.Spec.Volumes[0]}
261+
generatedVolumeName1, err1 := dsw.AddPodToVolume(util.GetUniquePodName(v.pod1), v.pod1, volumeSpec1, volumeSpec1.Name(), "")
262+
generatedVolumeName2, err2 := dsw.AddPodToVolume(util.GetUniquePodName(v.pod2), v.pod2, volumeSpec2, volumeSpec2.Name(), "")
263+
if err1 != nil {
264+
t.Fatalf("test %q: AddPodToVolume failed. Expected: <no error> Actual: <%v>", name, err1)
265+
}
266+
if err2 != nil {
267+
t.Fatalf("test %q: AddPodToVolume failed. Expected: <no error> Actual: <%v>", name, err2)
268+
}
269+
if v.same {
270+
if generatedVolumeName1 != generatedVolumeName2 {
271+
t.Fatalf("test %q: AddPodToVolume should generate same names, but got %q != %q", name, generatedVolumeName1, generatedVolumeName2)
272+
}
273+
} else {
274+
if generatedVolumeName1 == generatedVolumeName2 {
275+
t.Fatalf("test %q: AddPodToVolume should generate different names, but got %q == %q", name, generatedVolumeName1, generatedVolumeName2)
276+
}
277+
}
278+
}
279+
280+
}
281+
120282
// Populates data struct with a new volume/pod
121283
// Calls DeletePodFromVolume() to removes the pod
122284
// Verifies newly added pod/volume are deleted

‎pkg/kubelet/volumemanager/reconciler/reconciler.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,10 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume,
455455
if err != nil {
456456
return nil, err
457457
}
458+
deviceMountablePlugin, err := rc.volumePluginMgr.FindDeviceMountablePluginByName(volume.pluginName)
459+
if err != nil {
460+
return nil, err
461+
}
458462

459463
// Create pod object
460464
pod := &v1.Pod{
@@ -480,13 +484,13 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume,
480484
}
481485

482486
var uniqueVolumeName v1.UniqueVolumeName
483-
if attachablePlugin != nil {
487+
if attachablePlugin != nil || deviceMountablePlugin != nil {
484488
uniqueVolumeName, err = util.GetUniqueVolumeNameFromSpec(plugin, volumeSpec)
485489
if err != nil {
486490
return nil, err
487491
}
488492
} else {
489-
uniqueVolumeName = util.GetUniqueVolumeNameForNonAttachableVolume(volume.podName, plugin, volumeSpec)
493+
uniqueVolumeName = util.GetUniqueVolumeNameFromSpecWithPod(volume.podName, plugin, volumeSpec)
490494
}
491495
// Check existence of mount point for filesystem volume or symbolic link for block volume
492496
isExist, checkErr := rc.operationExecutor.CheckVolumeExistenceOperation(volumeSpec, volume.mountPath, volumeSpec.Name(), rc.mounter, uniqueVolumeName, volume.podName, pod.UID, attachablePlugin)

‎pkg/volume/testing/testing.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,94 @@ func (plugin *FakeVolumePlugin) VolumeLimitKey(spec *Spec) string {
504504
return plugin.LimitKey
505505
}
506506

507+
// FakeBasicVolumePlugin implements a basic volume plugin. It wrappers on
508+
// FakeVolumePlugin but implements VolumePlugin interface only.
509+
// It is useful to test logic involving plugin interfaces.
510+
type FakeBasicVolumePlugin struct {
511+
Plugin FakeVolumePlugin
512+
}
513+
514+
func (f *FakeBasicVolumePlugin) GetPluginName() string {
515+
return f.Plugin.GetPluginName()
516+
}
517+
518+
func (f *FakeBasicVolumePlugin) GetVolumeName(spec *Spec) (string, error) {
519+
return f.Plugin.GetVolumeName(spec)
520+
}
521+
522+
// CanSupport tests whether the plugin supports a given volume specification by
523+
// testing volume spec name begins with plugin name or not.
524+
// This is useful to choose plugin by volume in testing.
525+
func (f *FakeBasicVolumePlugin) CanSupport(spec *Spec) bool {
526+
return strings.HasPrefix(spec.Name(), f.GetPluginName())
527+
}
528+
529+
func (f *FakeBasicVolumePlugin) ConstructVolumeSpec(ame, mountPath string) (*Spec, error) {
530+
return f.Plugin.ConstructVolumeSpec(ame, mountPath)
531+
}
532+
533+
func (f *FakeBasicVolumePlugin) Init(ost VolumeHost) error {
534+
return f.Plugin.Init(ost)
535+
}
536+
537+
func (f *FakeBasicVolumePlugin) NewMounter(spec *Spec, pod *v1.Pod, opts VolumeOptions) (Mounter, error) {
538+
return f.Plugin.NewMounter(spec, pod, opts)
539+
}
540+
541+
func (f *FakeBasicVolumePlugin) NewUnmounter(volName string, podUID types.UID) (Unmounter, error) {
542+
return f.Plugin.NewUnmounter(volName, podUID)
543+
}
544+
545+
func (f *FakeBasicVolumePlugin) RequiresRemount() bool {
546+
return f.Plugin.RequiresRemount()
547+
}
548+
549+
func (f *FakeBasicVolumePlugin) SupportsBulkVolumeVerification() bool {
550+
return f.Plugin.SupportsBulkVolumeVerification()
551+
}
552+
553+
func (f *FakeBasicVolumePlugin) SupportsMountOption() bool {
554+
return f.Plugin.SupportsMountOption()
555+
}
556+
557+
var _ VolumePlugin = &FakeBasicVolumePlugin{}
558+
559+
// FakeDeviceMountableVolumePlugin implements an device mountable plugin based on FakeBasicVolumePlugin.
560+
type FakeDeviceMountableVolumePlugin struct {
561+
FakeBasicVolumePlugin
562+
}
563+
564+
func (f *FakeDeviceMountableVolumePlugin) NewDeviceMounter() (DeviceMounter, error) {
565+
return f.Plugin.NewDeviceMounter()
566+
}
567+
568+
func (f *FakeDeviceMountableVolumePlugin) NewDeviceUnmounter() (DeviceUnmounter, error) {
569+
return f.Plugin.NewDeviceUnmounter()
570+
}
571+
572+
func (f *FakeDeviceMountableVolumePlugin) GetDeviceMountRefs(deviceMountPath string) ([]string, error) {
573+
return f.Plugin.GetDeviceMountRefs(deviceMountPath)
574+
}
575+
576+
var _ VolumePlugin = &FakeDeviceMountableVolumePlugin{}
577+
var _ DeviceMountableVolumePlugin = &FakeDeviceMountableVolumePlugin{}
578+
579+
// FakeAttachableVolumePlugin implements an attachable plugin based on FakeDeviceMountableVolumePlugin.
580+
type FakeAttachableVolumePlugin struct {
581+
FakeDeviceMountableVolumePlugin
582+
}
583+
584+
func (f *FakeAttachableVolumePlugin) NewAttacher() (Attacher, error) {
585+
return f.Plugin.NewAttacher()
586+
}
587+
588+
func (f *FakeAttachableVolumePlugin) NewDetacher() (Detacher, error) {
589+
return f.Plugin.NewDetacher()
590+
}
591+
592+
var _ VolumePlugin = &FakeAttachableVolumePlugin{}
593+
var _ AttachableVolumePlugin = &FakeAttachableVolumePlugin{}
594+
507595
type FakeFileVolumePlugin struct {
508596
}
509597

‎pkg/volume/util/util.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -825,9 +825,10 @@ func GetUniqueVolumeName(pluginName, volumeName string) v1.UniqueVolumeName {
825825
return v1.UniqueVolumeName(fmt.Sprintf("%s/%s", pluginName, volumeName))
826826
}
827827

828-
// GetUniqueVolumeNameForNonAttachableVolume returns the unique volume name
829-
// for a non-attachable volume.
830-
func GetUniqueVolumeNameForNonAttachableVolume(
828+
// GetUniqueVolumeNameFromSpecWithPod returns a unique volume name with pod
829+
// name included. This is useful to generate different names for different pods
830+
// on same volume.
831+
func GetUniqueVolumeNameFromSpecWithPod(
831832
podName types.UniquePodName, volumePlugin volume.VolumePlugin, volumeSpec *volume.Spec) v1.UniqueVolumeName {
832833
return v1.UniqueVolumeName(
833834
fmt.Sprintf("%s/%v-%s", volumePlugin.GetPluginName(), podName, volumeSpec.Name()))

0 commit comments

Comments
 (0)