Skip to content

Commit

Permalink
Merge pull request kubernetes#26801 from saad-ali/mountUnmountRedesign
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue

Kubelet Volume Attach/Detach/Mount/Unmount Redesign

This PR redesigns the Volume Attach/Detach/Mount/Unmount in Kubelet as proposed in kubernetes#21931

```release-note
A new volume manager was introduced in kubelet that synchronizes volume mount/unmount (and attach/detach, if attach/detach controller is not enabled).

This eliminates the race conditions between the pod creation loop and the orphaned volumes loops. It also removes the unmount/detach from the `syncPod()` path so volume clean up never blocks the `syncPod` loop.
```
  • Loading branch information
k8s-merge-robot authored Jun 15, 2016
2 parents fe134f2 + 9e40746 commit 679f70b
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 43 deletions.
58 changes: 26 additions & 32 deletions pkg/volume/cinder/attacher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"time"

"github.com/golang/glog"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/util/exec"
"k8s.io/kubernetes/pkg/util/mount"
"k8s.io/kubernetes/pkg/volume"
Expand All @@ -46,17 +45,12 @@ func (plugin *cinderPlugin) NewAttacher() (volume.Attacher, error) {
return &cinderDiskAttacher{host: plugin.host}, nil
}

func (plugin *cinderPlugin) GetDeviceName(spec *volume.Spec) (string, error) {
volumeSource, _ := getVolumeSource(spec)
if volumeSource == nil {
return "", fmt.Errorf("Spec does not reference a Cinder volume type")
func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) error {
volumeSource, _, err := getVolumeSource(spec)
if err != nil {
return err
}

return volumeSource.VolumeID, nil
}

func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) error {
volumeSource, _ := getVolumeSource(spec)
volumeID := volumeSource.VolumeID

cloud, err := getCloudProvider(attacher.host.GetCloudProvider())
Expand Down Expand Up @@ -101,7 +95,12 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, timeout tim
if err != nil {
return "", err
}
volumeSource, _ := getVolumeSource(spec)

volumeSource, _, err := getVolumeSource(spec)
if err != nil {
return "", err
}

volumeID := volumeSource.VolumeID
instanceid, err := cloud.InstanceID()
if err != nil {
Expand Down Expand Up @@ -150,13 +149,19 @@ func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, timeout tim
}
}

func (attacher *cinderDiskAttacher) GetDeviceMountPath(spec *volume.Spec) string {
volumeSource, _ := getVolumeSource(spec)
return makeGlobalPDName(attacher.host, volumeSource.VolumeID)
func (attacher *cinderDiskAttacher) GetDeviceMountPath(
spec *volume.Spec) (string, error) {
volumeSource, _, err := getVolumeSource(spec)
if err != nil {
return "", err
}

return makeGlobalPDName(attacher.host, volumeSource.VolumeID), nil
}

// FIXME: this method can be further pruned.
func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string, mounter mount.Interface) error {
func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string) error {
mounter := attacher.host.GetMounter()
notMnt, err := mounter.IsLikelyNotMountPoint(deviceMountPath)
if err != nil {
if os.IsNotExist(err) {
Expand All @@ -169,7 +174,10 @@ func (attacher *cinderDiskAttacher) MountDevice(spec *volume.Spec, devicePath st
}
}

volumeSource, readOnly := getVolumeSource(spec)
volumeSource, readOnly, err := getVolumeSource(spec)
if err != nil {
return err
}

options := []string{}
if readOnly {
Expand Down Expand Up @@ -254,7 +262,8 @@ func (detacher *cinderDiskDetacher) WaitForDetach(devicePath string, timeout tim
}
}

func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string, mounter mount.Interface) error {
func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error {
mounter := detacher.host.GetMounter()
volume := path.Base(deviceMountPath)
if err := unmountPDAndRemoveGlobalPath(deviceMountPath, mounter); err != nil {
glog.Errorf("Error unmounting %q: %v", volume, err)
Expand All @@ -263,21 +272,6 @@ func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string, mounte
return nil
}

func getVolumeSource(spec *volume.Spec) (*api.CinderVolumeSource, bool) {
var readOnly bool
var volumeSource *api.CinderVolumeSource

if spec.Volume != nil && spec.Volume.Cinder != nil {
volumeSource = spec.Volume.Cinder
readOnly = volumeSource.ReadOnly
} else {
volumeSource = spec.PersistentVolume.Spec.Cinder
readOnly = spec.ReadOnly
}

return volumeSource, readOnly
}

// Checks if the specified path exists
func pathExists(path string) (bool, error) {
_, err := os.Stat(path)
Expand Down
35 changes: 28 additions & 7 deletions pkg/volume/cinder/cinder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,27 @@ func (plugin *cinderPlugin) Init(host volume.VolumeHost) error {
return nil
}

func (plugin *cinderPlugin) Name() string {
func (plugin *cinderPlugin) GetPluginName() string {
return cinderVolumePluginName
}

func (plugin *cinderPlugin) GetVolumeName(spec *volume.Spec) (string, error) {
volumeSource, _, err := getVolumeSource(spec)
if err != nil {
return "", err
}

return volumeSource.VolumeID, nil
}

func (plugin *cinderPlugin) CanSupport(spec *volume.Spec) bool {
return (spec.Volume != nil && spec.Volume.Cinder != nil) || (spec.PersistentVolume != nil && spec.PersistentVolume.Spec.Cinder != nil)
}

func (plugin *cinderPlugin) RequiresRemount() bool {
return false
}

func (plugin *cinderPlugin) GetAccessModes() []api.PersistentVolumeAccessMode {
return []api.PersistentVolumeAccessMode{
api.ReadWriteOnce,
Expand All @@ -93,16 +106,13 @@ func (plugin *cinderPlugin) NewMounter(spec *volume.Spec, pod *api.Pod, _ volume
}

func (plugin *cinderPlugin) newMounterInternal(spec *volume.Spec, podUID types.UID, manager cdManager, mounter mount.Interface) (volume.Mounter, error) {
var cinder *api.CinderVolumeSource
if spec.Volume != nil && spec.Volume.Cinder != nil {
cinder = spec.Volume.Cinder
} else {
cinder = spec.PersistentVolume.Spec.Cinder
cinder, readOnly, err := getVolumeSource(spec)
if err != nil {
return nil, err
}

pdName := cinder.VolumeID
fsType := cinder.FSType
readOnly := cinder.ReadOnly

return &cinderVolumeMounter{
cinderVolume: &cinderVolume{
Expand Down Expand Up @@ -458,3 +468,14 @@ func (c *cinderVolumeProvisioner) Provision() (*api.PersistentVolume, error) {
}
return pv, nil
}

func getVolumeSource(spec *volume.Spec) (*api.CinderVolumeSource, bool, error) {
if spec.Volume != nil && spec.Volume.Cinder != nil {
return spec.Volume.Cinder, spec.Volume.Cinder.ReadOnly, nil
} else if spec.PersistentVolume != nil &&
spec.PersistentVolume.Spec.Cinder != nil {
return spec.PersistentVolume.Spec.Cinder, spec.ReadOnly, nil
}

return nil, false, fmt.Errorf("Spec does not reference a Cinder volume type")
}
8 changes: 4 additions & 4 deletions pkg/volume/cinder/cinder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ func TestCanSupport(t *testing.T) {
}
defer os.RemoveAll(tmpDir)
plugMgr := volume.VolumePluginMgr{}
plugMgr.InitPlugins(ProbeVolumePlugins(), volumetest.NewFakeVolumeHost(tmpDir, nil, nil))
plugMgr.InitPlugins(ProbeVolumePlugins(), volumetest.NewFakeVolumeHost(tmpDir, nil, nil, "" /* rootContext */))

plug, err := plugMgr.FindPluginByName("kubernetes.io/cinder")
if err != nil {
t.Errorf("Can't find the plugin by name")
}
if plug.Name() != "kubernetes.io/cinder" {
t.Errorf("Wrong name: %s", plug.Name())
if plug.GetPluginName() != "kubernetes.io/cinder" {
t.Errorf("Wrong name: %s", plug.GetPluginName())
}
if !plug.CanSupport(&volume.Spec{Volume: &api.Volume{VolumeSource: api.VolumeSource{Cinder: &api.CinderVolumeSource{}}}}) {
t.Errorf("Expected true")
Expand Down Expand Up @@ -135,7 +135,7 @@ func TestPlugin(t *testing.T) {
}
defer os.RemoveAll(tmpDir)
plugMgr := volume.VolumePluginMgr{}
plugMgr.InitPlugins(ProbeVolumePlugins(), volumetest.NewFakeVolumeHost(tmpDir, nil, nil))
plugMgr.InitPlugins(ProbeVolumePlugins(), volumetest.NewFakeVolumeHost(tmpDir, nil, nil, "" /* rootContext */))

plug, err := plugMgr.FindPluginByName("kubernetes.io/cinder")
if err != nil {
Expand Down

0 comments on commit 679f70b

Please sign in to comment.