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

Jump to bottom

Delegate map operation to BlockVolumeMapper plugin #64094

Merged
merged 1 commit into from
Jun 5, 2018
Merged

Delegate map operation to BlockVolumeMapper plugin #64094

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/kubelet/kubelet_volumes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,11 @@ func (f *stubBlockVolume) GetPodDeviceMapPath() (string, string) {
func (f *stubBlockVolume) SetUpDevice() (string, error) {
return "", nil
}

func (f stubBlockVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return nil
}

func (f *stubBlockVolume) TearDownDevice(mapPath string, devicePath string) error {
return nil
}
32 changes: 8 additions & 24 deletions pkg/kubelet/volumemanager/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,12 +509,8 @@ func Test_Run_Positive_VolumeAttachAndMap(t *testing.T) {
1 /* expectedAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount(
1 /* expectedWaitForAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount(
1 /* expectedGetGlobalMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount(
1 /* expectedPodDeviceMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount(
1 /* expectedSetUpDeviceCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount(
1 /* expectedGetMapDeviceCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin))

Expand Down Expand Up @@ -601,12 +597,8 @@ func Test_Run_Positive_BlockVolumeMapControllerAttachEnabled(t *testing.T) {
assert.NoError(t, volumetesting.VerifyZeroAttachCalls(fakePlugin))
assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount(
1 /* expectedWaitForAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount(
1 /* expectedGetGlobalMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount(
1 /* expectedPodDeviceMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount(
1 /* expectedSetUpCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount(
1 /* expectedGetMapDeviceCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin))

Expand Down Expand Up @@ -692,12 +684,8 @@ func Test_Run_Positive_BlockVolumeAttachMapUnmapDetach(t *testing.T) {
1 /* expectedAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount(
1 /* expectedWaitForAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount(
1 /* expectedGetGlobalMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount(
1 /* expectedPodDeviceMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount(
1 /* expectedSetUpCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount(
1 /* expectedGetMapDeviceCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin))

Expand Down Expand Up @@ -796,12 +784,8 @@ func Test_Run_Positive_VolumeUnmapControllerAttachEnabled(t *testing.T) {
assert.NoError(t, volumetesting.VerifyZeroAttachCalls(fakePlugin))
assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount(
1 /* expectedWaitForAttachCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetGlobalMapPathCallCount(
1 /* expectedGetGlobalMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetPodDeviceMapPathCallCount(
1 /* expectedPodDeviceMapPathCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifySetUpDeviceCallCount(
1 /* expectedSetUpCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyGetMapDeviceCallCount(
1 /* expectedGetMapDeviceCallCount */, fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroTearDownDeviceCallCount(fakePlugin))
assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin))

Expand Down
5 changes: 5 additions & 0 deletions pkg/volume/aws_ebs/aws_ebs_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/kubernetes/pkg/util/mount"
kstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/volume"
"k8s.io/kubernetes/pkg/volume/util"
"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
)

Expand Down Expand Up @@ -155,6 +156,10 @@ func (b *awsElasticBlockStoreMapper) SetUpDevice() (string, error) {
return "", nil
}

func (b *awsElasticBlockStoreMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

// GetGlobalMapPath returns global map path and error
// path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID
// plugins/kubernetes.io/aws-ebs/volumeDevices/vol-XXXXXX
Expand Down
5 changes: 5 additions & 0 deletions pkg/volume/azure_dd/azure_dd_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"k8s.io/kubernetes/pkg/util/mount"
kstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/volume"
"k8s.io/kubernetes/pkg/volume/util"
"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
)

Expand Down Expand Up @@ -137,6 +138,10 @@ func (b *azureDataDiskMapper) SetUpDevice() (string, error) {
return "", nil
}

func (b *azureDataDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

// GetGlobalMapPath returns global map path and error
// path: plugins/kubernetes.io/{PluginName}/volumeDevices/volumeID
// plugins/kubernetes.io/azure-disk/volumeDevices/vol-XXXXXX
Expand Down
4 changes: 4 additions & 0 deletions pkg/volume/fc/fc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,10 @@ func (b *fcDiskMapper) SetUpDevice() (string, error) {
return "", nil
}

func (b *fcDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

type fcDiskUnmapper struct {
*fcDisk
deviceUtil util.DeviceUtil
Expand Down
5 changes: 5 additions & 0 deletions pkg/volume/gce_pd/gce_pd_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/kubernetes/pkg/util/mount"
kstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/volume"
"k8s.io/kubernetes/pkg/volume/util"
"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
)

Expand Down Expand Up @@ -151,6 +152,10 @@ func (b *gcePersistentDiskMapper) SetUpDevice() (string, error) {
return "", nil
}

func (b *gcePersistentDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

// GetGlobalMapPath returns global map path and error
// path: plugins/kubernetes.io/{PluginName}/volumeDevices/pdName
func (pd *gcePersistentDisk) GetGlobalMapPath(spec *volume.Spec) (string, error) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/volume/iscsi/iscsi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,10 @@ func (b *iscsiDiskMapper) SetUpDevice() (string, error) {
return "", nil
}

func (b *iscsiDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return ioutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

type iscsiDiskUnmapper struct {
*iscsiDisk
exec mount.Exec
Expand Down
4 changes: 4 additions & 0 deletions pkg/volume/local/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ func (m *localVolumeMapper) SetUpDevice() (string, error) {
return globalPath, nil
}

func (m *localVolumeMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return util.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

// localVolumeUnmapper implements the BlockVolumeUnmapper interface for local volumes.
type localVolumeUnmapper struct {
*localVolume
Expand Down
4 changes: 4 additions & 0 deletions pkg/volume/rbd/rbd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,10 @@ func (rbd *rbdDiskMapper) SetUpDevice() (string, error) {
return "", nil
}

func (rbd *rbdDiskMapper) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error {
return volutil.MapBlockVolume(devicePath, globalMapPath, volumeMapPath, volumeMapName, podUID)
}

func (rbd *rbd) rbdGlobalMapPath(spec *volume.Spec) (string, error) {
mon, err := getVolumeSourceMonitors(spec)
if err != nil {
Expand Down
34 changes: 34 additions & 0 deletions pkg/volume/testing/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ type FakeVolume struct {
GetDeviceMountPathCallCount int
SetUpDeviceCallCount int
TearDownDeviceCallCount int
MapDeviceCallCount int
GlobalMapPathCallCount int
PodDeviceMapPathCallCount int
}
Expand Down Expand Up @@ -642,6 +643,21 @@ func (fv *FakeVolume) GetTearDownDeviceCallCount() int {
return fv.TearDownDeviceCallCount
}

// Block volume support
func (fv *FakeVolume) MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, pod types.UID) error {
fv.Lock()
defer fv.Unlock()
fv.MapDeviceCallCount++
return nil
}

// Block volume support
func (fv *FakeVolume) GetMapDeviceCallCount() int {
fv.RLock()
defer fv.RUnlock()
return fv.MapDeviceCallCount
}

func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error) {
fv.Lock()
defer fv.Unlock()
Expand Down Expand Up @@ -1121,6 +1137,24 @@ func VerifyGetPodDeviceMapPathCallCount(
expectedPodDeviceMapPathCallCount)
}

// VerifyGetMapDeviceCallCount ensures that at least one of the Mappers for this
// plugin has the expectedMapDeviceCallCount number of calls. Otherwise it
// returns an error.
func VerifyGetMapDeviceCallCount(
expectedMapDeviceCallCount int,
fakeVolumePlugin *FakeVolumePlugin) error {
for _, mapper := range fakeVolumePlugin.GetBlockVolumeMapper() {
actualCallCount := mapper.GetMapDeviceCallCount()
if actualCallCount >= expectedMapDeviceCallCount {
return nil
}
}

return fmt.Errorf(
"No Mapper have expected MapdDeviceCallCount. Expected: <%v>.",
expectedMapDeviceCallCount)
}

// GetTestVolumePluginMgr creates, initializes, and returns a test volume plugin
// manager and fake volume plugin using a fake volume host.
func GetTestVolumePluginMgr(
Expand Down
1 change: 1 addition & 0 deletions pkg/volume/util/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ go_library(
"//pkg/util/mount:go_default_library",
"//pkg/volume:go_default_library",
"//pkg/volume/util/types:go_default_library",
"//pkg/volume/util/volumepathhandler:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
Expand Down
50 changes: 23 additions & 27 deletions pkg/volume/util/operationexecutor/operation_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,37 +856,19 @@ func (og *operationGenerator) GenerateMapVolumeFunc(
// On failure, return error. Caller will log and retry.
return volumeToMount.GenerateError("MapVolume.SetUp failed", mapErr)
}
// Update devicePath for none attachable plugin case
if len(devicePath) == 0 {
if len(pluginDevicePath) != 0 {
devicePath = pluginDevicePath
} else {
return volumeToMount.GenerateError("MapVolume failed", fmt.Errorf("Device path of the volume is empty"))
}
}
// Update actual state of world to reflect volume is globally mounted
markDeviceMappedErr := actualStateOfWorld.MarkDeviceAsMounted(
volumeToMount.VolumeName, devicePath, globalMapPath)
if markDeviceMappedErr != nil {
// On failure, return error. Caller will log and retry.
return volumeToMount.GenerateError("MapVolume.MarkDeviceAsMounted failed", markDeviceMappedErr)
}

mapErr = og.blkUtil.MapDevice(devicePath, globalMapPath, string(volumeToMount.Pod.UID))
if mapErr != nil {
// On failure, return error. Caller will log and retry.
return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr)
// if pluginDevicePath is provided, assume attacher may not provide device
// or attachment flow uses SetupDevice to get device path
if len(pluginDevicePath) != 0 {
devicePath = pluginDevicePath
}
if len(devicePath) == 0 {
return volumeToMount.GenerateError("MapVolume failed", fmt.Errorf("Device path of the volume is empty"))
Copy link
Member

Choose a reason for hiding this comment

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

devicePath comes from volumeAttacher.WaitForAttach(...) or blockVolumeMapper.SetUpDevice(). Is that correct? Will all plugins return devicePath in one of those functions? Will CSI?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original code assumes that devicePath comes from either a volumeAttacher.WaitForAttach(...) or from blockVolumeMapper.SetupDevice() . If neither one return a path, code errors out. So yes, it is a assume that a blockVolumeMapper will return a deviceMap.

Since CSI does not return a devicePath with WaitForAttach, it uses SetupDevice to return the global map path (which is the path sent to the external driver for NodeStage call).

Copy link
Member

Choose a reason for hiding this comment

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

Ack!

}

// Device mapping for global map path succeeded
simpleMsg, detailedMsg := volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("globalMapPath %q", globalMapPath))
verbosity := glog.Level(4)
og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg)
glog.V(verbosity).Infof(detailedMsg)

// Map device to pod device map path under the given pod directory using symbolic link
// Map device to global and pod device map path
volumeMapPath, volName := blockVolumeMapper.GetPodDeviceMapPath()
mapErr = og.blkUtil.MapDevice(devicePath, volumeMapPath, volName)
mapErr = blockVolumeMapper.MapDevice(devicePath, globalMapPath, volumeMapPath, volName, volumeToMount.Pod.UID)
if mapErr != nil {
// On failure, return error. Caller will log and retry.
return volumeToMount.GenerateError("MapVolume.MapDevice failed", mapErr)
Expand All @@ -901,6 +883,20 @@ func (og *operationGenerator) GenerateMapVolumeFunc(
return volumeToMount.GenerateError("MapVolume.AttachFileDevice failed", err)
}

// Update actual state of world to reflect volume is globally mounted
markDeviceMappedErr := actualStateOfWorld.MarkDeviceAsMounted(
volumeToMount.VolumeName, devicePath, globalMapPath)
if markDeviceMappedErr != nil {
// On failure, return error. Caller will log and retry.
return volumeToMount.GenerateError("MapVolume.MarkDeviceAsMounted failed", markDeviceMappedErr)
}

// Device mapping for global map path succeeded
simpleMsg, detailedMsg := volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("globalMapPath %q", globalMapPath))
verbosity := glog.Level(4)
og.recorder.Eventf(volumeToMount.Pod, v1.EventTypeNormal, kevents.SuccessfulMountVolume, simpleMsg)
glog.V(verbosity).Infof(detailedMsg)

// Device mapping for pod device map path succeeded
simpleMsg, detailedMsg = volumeToMount.GenerateMsg("MapVolume.MapDevice succeeded", fmt.Sprintf("volumeMapPath %q", volumeMapPath))
verbosity = glog.Level(1)
Expand Down
28 changes: 28 additions & 0 deletions pkg/volume/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"k8s.io/apimachinery/pkg/api/resource"
utypes "k8s.io/apimachinery/pkg/types"
"k8s.io/kubernetes/pkg/volume/util/types"
"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
)

const (
Expand Down Expand Up @@ -741,3 +742,30 @@ func MakeAbsolutePath(goos, path string) string {
// Otherwise, add 'c:\'
return "c:\\" + path
}

// MapBlockVolume is a utility function to provide a common way of mounting
// block device path for a specified volume and pod. This function should be
// called by volume plugins that implements volume.BlockVolumeMapper.Map() method.
func MapBlockVolume(
devicePath,
globalMapPath,
podVolumeMapPath,
volumeMapName string,
podUID utypes.UID,
) error {
blkUtil := volumepathhandler.NewBlockVolumePathHandler()

// map devicePath to global node path
mapErr := blkUtil.MapDevice(devicePath, globalMapPath, string(podUID))
if mapErr != nil {
return mapErr
}

// map devicePath to pod volume path
mapErr = blkUtil.MapDevice(devicePath, podVolumeMapPath, volumeMapName)
if mapErr != nil {
return mapErr
}

return nil
}
3 changes: 3 additions & 0 deletions pkg/volume/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ type BlockVolumeMapper interface {
// at attacher.Attach() and attacher.WaitForAttach().
// This may be called more than once, so implementations must be idempotent.
SetUpDevice() (string, error)

// Map maps the block device path for the specified spec and pod.
Copy link
Member

Choose a reason for hiding this comment

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

Do you need a symmetric UnmapVolume call too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, i thought about that too. But there is an unmapper.Teardown() method defined where custom code could go.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do any similar moving around of common logic?

MapDevice(devicePath, globalMapPath, volumeMapPath, volumeMapName string, podUID types.UID) error
}

// BlockVolumeUnmapper interface provides methods to cleanup/unmap the volumes.
Expand Down