From 82786ff72059f0f788623d8161a4e153bc11a057 Mon Sep 17 00:00:00 2001 From: Matthew Wong Date: Tue, 8 Oct 2019 09:57:38 -0700 Subject: [PATCH] Fix AWS block volume reconstruction to be like file --- pkg/volume/awsebs/aws_ebs.go | 38 +++----------------- pkg/volume/awsebs/aws_ebs_block.go | 35 +++++++----------- pkg/volume/awsebs/aws_ebs_block_test.go | 12 +++++-- pkg/volume/awsebs/aws_ebs_test.go | 33 +++++++++++++++++ pkg/volume/awsebs/aws_util.go | 48 +++++++++++++++++++++++++ 5 files changed, 108 insertions(+), 58 deletions(-) diff --git a/pkg/volume/awsebs/aws_ebs.go b/pkg/volume/awsebs/aws_ebs.go index 5fc5c095712..4885d991cef 100644 --- a/pkg/volume/awsebs/aws_ebs.go +++ b/pkg/volume/awsebs/aws_ebs.go @@ -263,41 +263,13 @@ func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath 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 - } - klog.V(4).Infof("Convert aws volume name from %q to %q ", volumeID, sourceName) + volumeID, err = formatVolumeID(volumeID) + if err != nil { + return nil, fmt.Errorf("failed to get AWS volume id from mount path %q: %v", mountPath, err) } - awsVolume := &v1.Volume{ - Name: volName, - VolumeSource: v1.VolumeSource{ - AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{ - VolumeID: sourceName, - }, - }, - } - return volume.NewSpecFromVolume(awsVolume), nil + file := v1.PersistentVolumeFilesystem + return newAWSVolumeSpec(volName, volumeID, file), nil } func (plugin *awsElasticBlockStorePlugin) RequiresFSResize() bool { diff --git a/pkg/volume/awsebs/aws_ebs_block.go b/pkg/volume/awsebs/aws_ebs_block.go index 9511393c5a5..71bc11c98b1 100644 --- a/pkg/volume/awsebs/aws_ebs_block.go +++ b/pkg/volume/awsebs/aws_ebs_block.go @@ -25,7 +25,6 @@ import ( "strings" v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/klog" "k8s.io/kubernetes/pkg/util/mount" @@ -52,37 +51,27 @@ func (plugin *awsElasticBlockStorePlugin) ConstructBlockVolumeSpec(podUID types. return nil, fmt.Errorf("failed to get volume plugin information from globalMapPathUUID: %v", globalMapPathUUID) } - return getVolumeSpecFromGlobalMapPath(volumeName, globalMapPath) + return plugin.getVolumeSpecFromGlobalMapPath(volumeName, globalMapPath) } -func getVolumeSpecFromGlobalMapPath(volumeName string, globalMapPath string) (*volume.Spec, error) { +func (plugin *awsElasticBlockStorePlugin) getVolumeSpecFromGlobalMapPath(volumeName string, globalMapPath string) (*volume.Spec, error) { // Get volume spec information from globalMapPath // globalMapPath example: // plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumeID} // plugins/kubernetes.io/aws-ebs/volumeDevices/vol-XXXXXX - vID := filepath.Base(globalMapPath) - if len(vID) <= 1 { - return nil, fmt.Errorf("failed to get volumeID from global path=%s", globalMapPath) + pluginDir := plugin.host.GetVolumeDevicePluginDir(awsElasticBlockStorePluginName) + if !strings.HasPrefix(globalMapPath, pluginDir) { + return nil, fmt.Errorf("volume symlink %s is not in global plugin directory", globalMapPath) } - if !strings.Contains(vID, "vol-") { - return nil, fmt.Errorf("failed to get volumeID from global path=%s, invalid volumeID format = %s", globalMapPath, vID) - } - block := v1.PersistentVolumeBlock - awsVolume := &v1.PersistentVolume{ - ObjectMeta: metav1.ObjectMeta{ - Name: volumeName, - }, - Spec: v1.PersistentVolumeSpec{ - PersistentVolumeSource: v1.PersistentVolumeSource{ - AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{ - VolumeID: vID, - }, - }, - VolumeMode: &block, - }, + fullVolumeID := strings.TrimPrefix(globalMapPath, pluginDir) // /vol-XXXXXX + fullVolumeID = strings.TrimLeft(fullVolumeID, "/") // vol-XXXXXX + vID, err := formatVolumeID(fullVolumeID) + if err != nil { + return nil, fmt.Errorf("failed to get AWS volume id from map path %q: %v", globalMapPath, err) } - return volume.NewSpecFromPersistentVolume(awsVolume, true), nil + block := v1.PersistentVolumeBlock + return newAWSVolumeSpec(volumeName, vID, block), nil } // NewBlockVolumeMapper creates a new volume.BlockVolumeMapper from an API specification. diff --git a/pkg/volume/awsebs/aws_ebs_block_test.go b/pkg/volume/awsebs/aws_ebs_block_test.go index f2a49456b78..6a92638ec36 100644 --- a/pkg/volume/awsebs/aws_ebs_block_test.go +++ b/pkg/volume/awsebs/aws_ebs_block_test.go @@ -51,14 +51,22 @@ func TestGetVolumeSpecFromGlobalMapPath(t *testing.T) { expectedGlobalPath := filepath.Join(tmpVDir, testGlobalPath) + plugMgr := volume.VolumePluginMgr{} + plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, volumetest.NewFakeVolumeHost(tmpVDir, nil, nil)) + plug, err := plugMgr.FindMapperPluginByName(awsElasticBlockStorePluginName) + if err != nil { + os.RemoveAll(tmpVDir) + t.Fatalf("Can't find the plugin by name: %q", awsElasticBlockStorePluginName) + } + //Bad Path - badspec, err := getVolumeSpecFromGlobalMapPath("", "") + badspec, err := plug.(*awsElasticBlockStorePlugin).getVolumeSpecFromGlobalMapPath("", "") if badspec != nil || err == nil { t.Fatalf("Expected not to get spec from GlobalMapPath but did") } // Good Path - spec, err := getVolumeSpecFromGlobalMapPath("myVolume", expectedGlobalPath) + spec, err := plug.(*awsElasticBlockStorePlugin).getVolumeSpecFromGlobalMapPath("myVolume", expectedGlobalPath) if spec == nil || err != nil { t.Fatalf("Failed to get spec from GlobalMapPath: %v", err) } diff --git a/pkg/volume/awsebs/aws_ebs_test.go b/pkg/volume/awsebs/aws_ebs_test.go index bddaadaf861..e10f5273362 100644 --- a/pkg/volume/awsebs/aws_ebs_test.go +++ b/pkg/volume/awsebs/aws_ebs_test.go @@ -414,3 +414,36 @@ func TestGetCandidateZone(t *testing.T) { assert.Equal(t, test.expectedZones, zones) } } + +func TestFormatVolumeID(t *testing.T) { + tests := []struct { + volumeIDFromPath string + expectedVolumeID string + }{ + { + "aws/vol-1234", + "aws:///vol-1234", + }, + { + "aws:/vol-1234", + "aws:///vol-1234", + }, + { + "aws/us-east-1/vol-1234", + "aws://us-east-1/vol-1234", + }, + { + "aws:/us-east-1/vol-1234", + "aws://us-east-1/vol-1234", + }, + { + "vol-1234", + "vol-1234", + }, + } + for _, test := range tests { + volumeID, err := formatVolumeID(test.volumeIDFromPath) + assert.Nil(t, err) + assert.Equal(t, test.expectedVolumeID, volumeID, test.volumeIDFromPath) + } +} diff --git a/pkg/volume/awsebs/aws_util.go b/pkg/volume/awsebs/aws_util.go index 2f69d54f9b9..148881aa98f 100644 --- a/pkg/volume/awsebs/aws_util.go +++ b/pkg/volume/awsebs/aws_util.go @@ -30,6 +30,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" cloudprovider "k8s.io/cloud-provider" volumehelpers "k8s.io/cloud-provider/volume/helpers" @@ -287,3 +288,50 @@ func findNvmeVolume(findName string) (device string, err error) { return resolved, nil } + +func formatVolumeID(volumeID string) (string, error) { + // This is a workaround to fix the issue in converting aws volume id from globalPDPath and globalMapPath + // There are three formats for AWSEBSVolumeSource.VolumeID and they are stored on disk in paths like so: + // VolumeID mountPath mapPath + // aws:///vol-1234 aws/vol-1234 aws:/vol-1234 + // aws://us-east-1/vol-1234 aws/us-east-1/vol-1234 aws:/us-east-1/vol-1234 + // vol-1234 vol-1234 vol-1234 + // This code is for converting volume ids from paths back to AWS style VolumeIDs + sourceName := volumeID + if strings.HasPrefix(volumeID, "aws/") || strings.HasPrefix(volumeID, "aws:/") { + names := strings.Split(volumeID, "/") + length := len(names) + if length < 2 || length > 3 { + return "", fmt.Errorf("invalid volume name format %q", volumeID) + } + volName := names[length-1] + if !strings.HasPrefix(volName, "vol-") { + return "", fmt.Errorf("Invalid volume name format for AWS volume (%q)", volName) + } + if length == 2 { + sourceName = awsURLNamePrefix + "" + "/" + volName // empty zone label + } + if length == 3 { + sourceName = awsURLNamePrefix + names[1] + "/" + volName // names[1] is the zone label + } + klog.V(4).Infof("Convert aws volume name from %q to %q ", volumeID, sourceName) + } + return sourceName, nil +} + +func newAWSVolumeSpec(volumeName, volumeID string, mode v1.PersistentVolumeMode) *volume.Spec { + awsVolume := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: volumeName, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{ + VolumeID: volumeID, + }, + }, + VolumeMode: &mode, + }, + } + return volume.NewSpecFromPersistentVolume(awsVolume, false) +}