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

fix issue in converting aws volume id from mount paths #36840

Merged
merged 2 commits into from
Nov 18, 2016
Merged

fix issue in converting aws volume id from mount paths #36840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d3e44e
fix issue in converting aws volume id from mount paths
jingxu97 Nov 15, 2016
1b89c79
Update aws_ebs.go
jingxu97 Nov 17, 2016
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
4 changes: 2 additions & 2 deletions pkg/controller/volume/attachdetach/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,11 @@ func (rc *reconciler) reconcile() {
if rc.actualStateOfWorld.VolumeNodeExists(
volumeToAttach.VolumeName, volumeToAttach.NodeName) {
// Volume/Node exists, touch it to reset detachRequestedTime
glog.V(1).Infof("Volume %q/Node %q is attached--touching.", volumeToAttach.VolumeName, volumeToAttach.NodeName)
glog.V(5).Infof("Volume %q/Node %q is attached--touching.", volumeToAttach.VolumeName, volumeToAttach.NodeName)
Copy link
Member

Choose a reason for hiding this comment

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

Why lower the priority on these messages? Are they happening very frequently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it prints out log for every reconciler loop

rc.actualStateOfWorld.ResetDetachRequestTime(volumeToAttach.VolumeName, volumeToAttach.NodeName)
} else {
// Volume/Node doesn't exist, spawn a goroutine to attach it
glog.V(1).Infof("Attempting to start AttachVolume for volume %q to node %q", volumeToAttach.VolumeName, volumeToAttach.NodeName)
glog.V(5).Infof("Attempting to start AttachVolume for volume %q to node %q", volumeToAttach.VolumeName, volumeToAttach.NodeName)
err := rc.attacherDetacher.AttachVolume(volumeToAttach.VolumeToAttach, rc.actualStateOfWorld)
if err == nil {
glog.Infof("Started AttachVolume for volume %q to node %q", volumeToAttach.VolumeName, volumeToAttach.NodeName)
Expand Down
13 changes: 10 additions & 3 deletions pkg/util/mount/mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import (

const (
// Default mount command if mounter path is not specified
defaultMountCommand = "mount"
defaultMountCommand = "mount"
MountsInGlobalPDPath = "mounts"
)

type Interface interface {
Expand Down Expand Up @@ -189,9 +190,15 @@ func getDeviceNameFromMount(mounter Interface, mountPath, pluginDir string) (str
glog.V(4).Infof("Directory %s is not mounted", mountPath)
return "", fmt.Errorf("directory %s is not mounted", mountPath)
}
basemountPath := path.Join(pluginDir, MountsInGlobalPDPath)
for _, ref := range refs {
if strings.HasPrefix(ref, pluginDir) {
return path.Base(ref), nil
if strings.HasPrefix(ref, basemountPath) {
volumeID, err := filepath.Rel(basemountPath, ref)
if err != nil {
glog.Errorf("Failed to get volume id from mount %s - %v", mountPath, err)
return "", err
}
return volumeID, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should return a struct with the mount information. I see there's another overload which returns the ref count, so it's difficult to know exactly where this is used by pure grep. But this is my go-to trick (when in doubt, create a type!); not sure it's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I plan to change the function name to avoid the confusion in another PR.

}

Expand Down
33 changes: 30 additions & 3 deletions pkg/volume/aws_ebs/aws_ebs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ var _ volume.ProvisionableVolumePlugin = &awsElasticBlockStorePlugin{}

const (
awsElasticBlockStorePluginName = "kubernetes.io/aws-ebs"
awsURLNamePrefix = "aws://"
)

func getPath(uid types.UID, volName string, host volume.VolumeHost) string {
Expand Down Expand Up @@ -189,10 +190,36 @@ func getVolumeSource(
func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath string) (*volume.Spec, error) {
mounter := plugin.host.GetMounter()
pluginDir := plugin.host.GetPluginDir(plugin.GetPluginName())
sourceName, err := mounter.GetDeviceNameFromMount(mountPath, pluginDir)
volumeID, err := mounter.GetDeviceNameFromMount(mountPath, pluginDir)
if err != nil {
return nil, err
}
// This is a workaround to fix the issue in converting aws volume id from globalPDPath
// There are three aws volume id formats and their volumeID from GetDeviceNameFromMount() are:
// aws:///vol-1234 (aws/vol-1234)
// aws://us-east-1/vol-1234 (aws/us-east-1/vol-1234)
// vol-1234 (vol-1234)
// This code is for converting volume id to aws style volume id for the first two cases.
sourceName := volumeID
if strings.HasPrefix(volumeID, "aws/") {
names := strings.Split(volumeID, "/")
length := len(names)
if length < 2 || length > 3 {
return nil, fmt.Errorf("Failed to get AWS volume id from mount path %q: invalid volume name format %q", mountPath, volumeID)
}
volName := names[length-1]
if !strings.HasPrefix(volName, "vol-") {
return nil, fmt.Errorf("Invalid volume name format for AWS volume (%q) retrieved from mount path %q", volName, mountPath)
}
if length == 2 {
sourceName = awsURLNamePrefix + "" + "/" + volName // empty zone label
}
if length == 3 {
sourceName = awsURLNamePrefix + names[1] + "/" + volName // names[1] is the zone label
}
glog.V(4).Infof("Convert aws volume name from %q to %q ", volumeID, sourceName)
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks right.

If I'm going to nit-pick, it might be safer to split the string, and then glog.Warning if it doesn't match one of the forms we expect.

i.e. we expect either aws/vol-12345678 or aws/us-east-1b/vol-12345678, but we should log if it doesn't look like that.

Bonus: code might be easier to understand also!


awsVolume := &api.Volume{
Name: volName,
VolumeSource: api.VolumeSource{
Expand Down Expand Up @@ -324,12 +351,12 @@ func makeGlobalPDPath(host volume.VolumeHost, volumeID aws.KubernetesVolumeID) s
// Clean up the URI to be more fs-friendly
name := string(volumeID)
name = strings.Replace(name, "://", "/", -1)
return path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), "mounts", name)
return path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), mount.MountsInGlobalPDPath, name)
}

// Reverses the mapping done in makeGlobalPDPath
func getVolumeIDFromGlobalMount(host volume.VolumeHost, globalPath string) (string, error) {
basePath := path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), "mounts")
basePath := path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), mount.MountsInGlobalPDPath)
rel, err := filepath.Rel(basePath, globalPath)
if err != nil {
glog.Errorf("Failed to get volume id from global mount %s - %v", globalPath, err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/azure_dd/azure_dd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ func (b *azureDiskMounter) SetUpAt(dir string, fsGroup *int64) error {
}

func makeGlobalPDPath(host volume.VolumeHost, volume string) string {
return path.Join(host.GetPluginDir(azureDataDiskPluginName), "mounts", volume)
return path.Join(host.GetPluginDir(azureDataDiskPluginName), mount.MountsInGlobalPDPath, volume)
}

func (azure *azureDisk) GetPath() string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/cinder/cinder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func (b *cinderVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
}

func makeGlobalPDName(host volume.VolumeHost, devName string) string {
return path.Join(host.GetPluginDir(cinderVolumePluginName), "mounts", devName)
return path.Join(host.GetPluginDir(cinderVolumePluginName), mount.MountsInGlobalPDPath, devName)
}

func (cd *cinderVolume) GetPath() string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/gce_pd/gce_pd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ func (b *gcePersistentDiskMounter) SetUpAt(dir string, fsGroup *int64) error {
}

func makeGlobalPDName(host volume.VolumeHost, devName string) string {
return path.Join(host.GetPluginDir(gcePersistentDiskPluginName), "mounts", devName)
return path.Join(host.GetPluginDir(gcePersistentDiskPluginName), mount.MountsInGlobalPDPath, devName)
}

func (b *gcePersistentDiskMounter) GetPath() string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/photon_pd/photon_pd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func (c *photonPersistentDiskUnmounter) TearDownAt(dir string) error {
}

func makeGlobalPDPath(host volume.VolumeHost, devName string) string {
return path.Join(host.GetPluginDir(photonPersistentDiskPluginName), "mounts", devName)
return path.Join(host.GetPluginDir(photonPersistentDiskPluginName), mount.MountsInGlobalPDPath, devName)
}

func (ppd *photonPersistentDisk) GetPath() string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/volume/vsphere_volume/vsphere_volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func (v *vsphereVolumeUnmounter) TearDownAt(dir string) error {
}

func makeGlobalPDPath(host volume.VolumeHost, devName string) string {
return path.Join(host.GetPluginDir(vsphereVolumePluginName), "mounts", devName)
return path.Join(host.GetPluginDir(vsphereVolumePluginName), mount.MountsInGlobalPDPath, devName)
}

func (vv *vsphereVolume) GetPath() string {
Expand Down