diff --git a/pkg/cloudprovider/providers/aws/BUILD b/pkg/cloudprovider/providers/aws/BUILD index 22560ed247c..29e7062722d 100644 --- a/pkg/cloudprovider/providers/aws/BUILD +++ b/pkg/cloudprovider/providers/aws/BUILD @@ -21,6 +21,7 @@ go_library( "log_handler.go", "retry_handler.go", "sets_ippermissions.go", + "volumes.go", ], tags = ["automanaged"], deps = [ diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 4a686355e28..3bea88ab5b8 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -21,7 +21,6 @@ import ( "fmt" "io" "net" - "net/url" "regexp" "strconv" "strings" @@ -299,31 +298,31 @@ type Volumes interface { // Attach the disk to the node with the specified NodeName // nodeName can be empty to mean "the instance on which we are running" // Returns the device (e.g. /dev/xvdf) where we attached the volume - AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) + AttachDisk(diskName KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) // Detach the disk from the node with the specified NodeName // nodeName can be empty to mean "the instance on which we are running" // Returns the device where the volume was attached - DetachDisk(diskName string, nodeName types.NodeName) (string, error) + DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName) (string, error) // Create a volume with the specified options - CreateDisk(volumeOptions *VolumeOptions) (volumeName string, err error) + CreateDisk(volumeOptions *VolumeOptions) (volumeName KubernetesVolumeID, err error) // Delete the specified volume // Returns true iff the volume was deleted // If the was not found, returns (false, nil) - DeleteDisk(volumeName string) (bool, error) + DeleteDisk(volumeName KubernetesVolumeID) (bool, error) // Get labels to apply to volume on creation - GetVolumeLabels(volumeName string) (map[string]string, error) + GetVolumeLabels(volumeName KubernetesVolumeID) (map[string]string, error) // Get volume's disk path from volume name // return the device path where the volume is attached - GetDiskPath(volumeName string) (string, error) + GetDiskPath(volumeName KubernetesVolumeID) (string, error) // Check if the volume is already attached to the node with the specified NodeName - DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error) + DiskIsAttached(diskName KubernetesVolumeID, nodeName types.NodeName) (bool, error) // Check if a list of volumes are attached to the node with the specified NodeName - DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) + DisksAreAttached(diskNames []KubernetesVolumeID, nodeName types.NodeName) (map[KubernetesVolumeID]bool, error) } // InstanceGroups is an interface for managing cloud-managed instance groups / autoscaling instance groups @@ -365,7 +364,7 @@ type Cloud struct { // attached, to avoid a race condition where we assign a device mapping // and then get a second request before we attach the volume attachingMutex sync.Mutex - attaching map[types.NodeName]map[mountDevice]string + attaching map[types.NodeName]map[mountDevice]awsVolumeID } var _ Volumes = &Cloud{} @@ -797,7 +796,7 @@ func newAWSCloud(config io.Reader, awsServices Services) (*Cloud, error) { cfg: cfg, region: regionName, - attaching: make(map[types.NodeName]map[mountDevice]string), + attaching: make(map[types.NodeName]map[mountDevice]awsVolumeID), } selfAWSInstance, err := awsCloud.buildSelfAWSInstance() @@ -1168,7 +1167,7 @@ func (i *awsInstance) describeInstance() (*ec2.Instance, error) { // Gets the mountDevice already assigned to the volume, or assigns an unused mountDevice. // If the volume is already assigned, this will return the existing mountDevice with alreadyAttached=true. // Otherwise the mountDevice is assigned by finding the first available mountDevice, and it is returned with alreadyAttached=false. -func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (assigned mountDevice, alreadyAttached bool, err error) { +func (c *Cloud) getMountDevice(i *awsInstance, volumeID awsVolumeID, assign bool) (assigned mountDevice, alreadyAttached bool, err error) { instanceType := i.getInstanceType() if instanceType == nil { return "", false, fmt.Errorf("could not get instance type for instance: %s", i.awsID) @@ -1178,7 +1177,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as if err != nil { return "", false, err } - deviceMappings := map[mountDevice]string{} + deviceMappings := map[mountDevice]awsVolumeID{} for _, blockDevice := range info.BlockDeviceMappings { name := aws.StringValue(blockDevice.DeviceName) if strings.HasPrefix(name, "/dev/sd") { @@ -1190,7 +1189,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as if len(name) < 1 || len(name) > 2 { glog.Warningf("Unexpected EBS DeviceName: %q", aws.StringValue(blockDevice.DeviceName)) } - deviceMappings[mountDevice(name)] = aws.StringValue(blockDevice.Ebs.VolumeId) + deviceMappings[mountDevice(name)] = awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId)) } // We lock to prevent concurrent mounts from conflicting @@ -1236,7 +1235,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as attaching := c.attaching[i.nodeName] if attaching == nil { - attaching = make(map[mountDevice]string) + attaching = make(map[mountDevice]awsVolumeID) c.attaching[i.nodeName] = attaching } attaching[chosen] = volumeID @@ -1247,7 +1246,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as // endAttaching removes the entry from the "attachments in progress" map // It returns true if it was found (and removed), false otherwise -func (c *Cloud) endAttaching(i *awsInstance, volumeID string, mountDevice mountDevice) bool { +func (c *Cloud) endAttaching(i *awsInstance, volumeID awsVolumeID, mountDevice mountDevice) bool { c.attachingMutex.Lock() defer c.attachingMutex.Unlock() @@ -1272,44 +1271,16 @@ type awsDisk struct { ec2 EC2 // Name in k8s - name string + name KubernetesVolumeID // id in AWS - awsID string + awsID awsVolumeID } -func newAWSDisk(aws *Cloud, name string) (*awsDisk, error) { - // name looks like aws://availability-zone/id - - // The original idea of the URL-style name was to put the AZ into the - // host, so we could find the AZ immediately from the name without - // querying the API. But it turns out we don't actually need it for - // multi-AZ clusters, as we put the AZ into the labels on the PV instead. - // However, if in future we want to support multi-AZ cluster - // volume-awareness without using PersistentVolumes, we likely will - // want the AZ in the host. - - if !strings.HasPrefix(name, "aws://") { - name = "aws://" + "" + "/" + name - } - url, err := url.Parse(name) +func newAWSDisk(aws *Cloud, name KubernetesVolumeID) (*awsDisk, error) { + awsID, err := name.mapToAWSVolumeID() if err != nil { - // TODO: Maybe we should pass a URL into the Volume functions - return nil, fmt.Errorf("Invalid disk name (%s): %v", name, err) + return nil, err } - if url.Scheme != "aws" { - return nil, fmt.Errorf("Invalid scheme for AWS volume (%s)", name) - } - - awsID := url.Path - if len(awsID) > 1 && awsID[0] == '/' { - awsID = awsID[1:] - } - - // TODO: Regex match? - if strings.Contains(awsID, "/") || !strings.HasPrefix(awsID, "vol-") { - return nil, fmt.Errorf("Invalid format for AWS volume (%s)", name) - } - disk := &awsDisk{ec2: aws.ec2, name: name, awsID: awsID} return disk, nil } @@ -1319,7 +1290,7 @@ func (d *awsDisk) describeVolume() (*ec2.Volume, error) { volumeID := d.awsID request := &ec2.DescribeVolumesInput{ - VolumeIds: []*string{&volumeID}, + VolumeIds: []*string{volumeID.awsString()}, } volumes, err := d.ec2.DescribeVolumes(request) @@ -1400,7 +1371,7 @@ func (d *awsDisk) waitForAttachmentStatus(status string) (*ec2.VolumeAttachment, // Deletes the EBS disk func (d *awsDisk) deleteVolume() (bool, error) { - request := &ec2.DeleteVolumeInput{VolumeId: aws.String(d.awsID)} + request := &ec2.DeleteVolumeInput{VolumeId: d.awsID.awsString()} _, err := d.ec2.DeleteVolume(request) if err != nil { if awsError, ok := err.(awserr.Error); ok { @@ -1460,7 +1431,7 @@ func (c *Cloud) getAwsInstance(nodeName types.NodeName) (*awsInstance, error) { } // AttachDisk implements Volumes.AttachDisk -func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) { +func (c *Cloud) AttachDisk(diskName KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) { disk, err := newAWSDisk(c, diskName) if err != nil { return "", err @@ -1508,7 +1479,7 @@ func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bo request := &ec2.AttachVolumeInput{ Device: aws.String(ec2Device), InstanceId: aws.String(awsInstance.awsID), - VolumeId: aws.String(disk.awsID), + VolumeId: disk.awsID.awsString(), } attachResponse, err := c.ec2.AttachVolume(request) @@ -1547,7 +1518,7 @@ func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bo } // DetachDisk implements Volumes.DetachDisk -func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, error) { +func (c *Cloud) DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName) (string, error) { disk, err := newAWSDisk(c, diskName) if err != nil { return "", err @@ -1579,7 +1550,7 @@ func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, er request := ec2.DetachVolumeInput{ InstanceId: &awsInstance.awsID, - VolumeId: &disk.awsID, + VolumeId: disk.awsID.awsString(), } response, err := c.ec2.DetachVolume(&request) @@ -1610,7 +1581,7 @@ func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, er } // CreateDisk implements Volumes.CreateDisk -func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) { +func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (KubernetesVolumeID, error) { allZones, err := c.getAllZones() if err != nil { return "", fmt.Errorf("error querying for all zones: %v", err) @@ -1668,10 +1639,11 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) { return "", err } - az := orEmpty(response.AvailabilityZone) - awsID := orEmpty(response.VolumeId) - - volumeName := "aws://" + az + "/" + awsID + awsID := awsVolumeID(aws.StringValue(response.VolumeId)) + if awsID == "" { + return "", fmt.Errorf("VolumeID was not returned by CreateVolume") + } + volumeName := KubernetesVolumeID("aws://" + aws.StringValue(response.AvailabilityZone) + "/" + string(awsID)) // apply tags tags := make(map[string]string) @@ -1684,7 +1656,7 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) { } if len(tags) != 0 { - if err := c.createTags(awsID, tags); err != nil { + if err := c.createTags(string(awsID), tags); err != nil { // delete the volume and hope it succeeds _, delerr := c.DeleteDisk(volumeName) if delerr != nil { @@ -1698,7 +1670,7 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) { } // DeleteDisk implements Volumes.DeleteDisk -func (c *Cloud) DeleteDisk(volumeName string) (bool, error) { +func (c *Cloud) DeleteDisk(volumeName KubernetesVolumeID) (bool, error) { awsDisk, err := newAWSDisk(c, volumeName) if err != nil { return false, err @@ -1707,7 +1679,7 @@ func (c *Cloud) DeleteDisk(volumeName string) (bool, error) { } // GetVolumeLabels implements Volumes.GetVolumeLabels -func (c *Cloud) GetVolumeLabels(volumeName string) (map[string]string, error) { +func (c *Cloud) GetVolumeLabels(volumeName KubernetesVolumeID) (map[string]string, error) { awsDisk, err := newAWSDisk(c, volumeName) if err != nil { return nil, err @@ -1733,7 +1705,7 @@ func (c *Cloud) GetVolumeLabels(volumeName string) (map[string]string, error) { } // GetDiskPath implements Volumes.GetDiskPath -func (c *Cloud) GetDiskPath(volumeName string) (string, error) { +func (c *Cloud) GetDiskPath(volumeName KubernetesVolumeID) (string, error) { awsDisk, err := newAWSDisk(c, volumeName) if err != nil { return "", err @@ -1749,7 +1721,7 @@ func (c *Cloud) GetDiskPath(volumeName string) (string, error) { } // DiskIsAttached implements Volumes.DiskIsAttached -func (c *Cloud) DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error) { +func (c *Cloud) DiskIsAttached(diskName KubernetesVolumeID, nodeName types.NodeName) (bool, error) { awsInstance, err := c.getAwsInstance(nodeName) if err != nil { if err == cloudprovider.InstanceNotFound { @@ -1764,22 +1736,33 @@ func (c *Cloud) DiskIsAttached(diskName string, nodeName types.NodeName) (bool, return false, err } + diskID, err := diskName.mapToAWSVolumeID() + if err != nil { + return false, fmt.Errorf("error mapping volume spec %q to aws id: %v", diskName, err) + } + info, err := awsInstance.describeInstance() if err != nil { return false, err } for _, blockDevice := range info.BlockDeviceMappings { - name := aws.StringValue(blockDevice.Ebs.VolumeId) - if name == diskName { + id := awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId)) + if id == diskID { return true, nil } } return false, nil } -func (c *Cloud) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) { - attached := make(map[string]bool) +func (c *Cloud) DisksAreAttached(diskNames []KubernetesVolumeID, nodeName types.NodeName) (map[KubernetesVolumeID]bool, error) { + idToDiskName := make(map[awsVolumeID]KubernetesVolumeID) + attached := make(map[KubernetesVolumeID]bool) for _, diskName := range diskNames { + volumeID, err := diskName.mapToAWSVolumeID() + if err != nil { + return nil, fmt.Errorf("error mapping volume spec %q to aws id: %v", diskName, err) + } + idToDiskName[volumeID] = diskName attached[diskName] = false } awsInstance, err := c.getAwsInstance(nodeName) @@ -1800,12 +1783,11 @@ func (c *Cloud) DisksAreAttached(diskNames []string, nodeName types.NodeName) (m return attached, err } for _, blockDevice := range info.BlockDeviceMappings { - volumeId := aws.StringValue(blockDevice.Ebs.VolumeId) - for _, diskName := range diskNames { - if volumeId == diskName { - // Disk is still attached to node - attached[diskName] = true - } + volumeID := awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId)) + diskName, found := idToDiskName[volumeID] + if found { + // Disk is still attached to node + attached[diskName] = true } } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 0c000dbb845..7a9c99bcc42 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1163,16 +1163,16 @@ func TestGetVolumeLabels(t *testing.T) { awsServices := NewFakeAWSServices() c, err := newAWSCloud(strings.NewReader("[global]"), awsServices) assert.Nil(t, err, "Error building aws cloud: %v", err) - volumeId := aws.String("vol-VolumeId") - expectedVolumeRequest := &ec2.DescribeVolumesInput{VolumeIds: []*string{volumeId}} + volumeId := awsVolumeID("vol-VolumeId") + expectedVolumeRequest := &ec2.DescribeVolumesInput{VolumeIds: []*string{volumeId.awsString()}} awsServices.ec2.On("DescribeVolumes", expectedVolumeRequest).Return([]*ec2.Volume{ { - VolumeId: volumeId, + VolumeId: volumeId.awsString(), AvailabilityZone: aws.String("us-east-1a"), }, }) - labels, err := c.GetVolumeLabels(*volumeId) + labels, err := c.GetVolumeLabels(KubernetesVolumeID("aws:///" + string(volumeId))) assert.Nil(t, err, "Error creating Volume %v", err) assert.Equal(t, map[string]string{ diff --git a/pkg/cloudprovider/providers/aws/volumes.go b/pkg/cloudprovider/providers/aws/volumes.go new file mode 100644 index 00000000000..f0ed233892e --- /dev/null +++ b/pkg/cloudprovider/providers/aws/volumes.go @@ -0,0 +1,83 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package aws + +import ( + "fmt" + "github.com/aws/aws-sdk-go/aws" + "net/url" + "strings" +) + +// awsVolumeID represents the ID of the volume in the AWS API, e.g. vol-12345678a +// The "traditional" format is "vol-12345678" +// A new longer format is also being introduced: "vol-12345678abcdef01" +// We should not assume anything about the length or format, though it seems +// reasonable to assume that volumes will continue to start with "vol-". +type awsVolumeID string + +func (i awsVolumeID) awsString() *string { + return aws.String(string(i)) +} + +// KubernetesVolumeID represents the id for a volume in the kubernetes API; +// a few forms are recognized: +// * aws:/// +// * aws:/// +// * +type KubernetesVolumeID string + +// mapToAWSVolumeID extracts the awsVolumeID from the KubernetesVolumeID +func (name KubernetesVolumeID) mapToAWSVolumeID() (awsVolumeID, error) { + // name looks like aws://availability-zone/awsVolumeId + + // The original idea of the URL-style name was to put the AZ into the + // host, so we could find the AZ immediately from the name without + // querying the API. But it turns out we don't actually need it for + // multi-AZ clusters, as we put the AZ into the labels on the PV instead. + // However, if in future we want to support multi-AZ cluster + // volume-awareness without using PersistentVolumes, we likely will + // want the AZ in the host. + + s := string(name) + + if !strings.HasPrefix(s, "aws://") { + // Assume a bare aws volume id (vol-1234...) + // Build a URL with an empty host (AZ) + s = "aws://" + "" + "/" + s + } + url, err := url.Parse(s) + if err != nil { + // TODO: Maybe we should pass a URL into the Volume functions + return "", fmt.Errorf("Invalid disk name (%s): %v", name, err) + } + if url.Scheme != "aws" { + return "", fmt.Errorf("Invalid scheme for AWS volume (%s)", name) + } + + awsID := url.Path + awsID = strings.Trim(awsID, "/") + + // We sanity check the resulting volume; the two known formats are + // vol-12345678 and vol-12345678abcdef01 + // TODO: Regex match? + if strings.Contains(awsID, "/") || !strings.HasPrefix(awsID, "vol-") { + return "", fmt.Errorf("Invalid format for AWS volume (%s)", name) + } + + return awsVolumeID(awsID), nil +} diff --git a/pkg/volume/aws_ebs/attacher.go b/pkg/volume/aws_ebs/attacher.go index 59e2566a8f3..7af31011b58 100644 --- a/pkg/volume/aws_ebs/attacher.go +++ b/pkg/volume/aws_ebs/attacher.go @@ -64,7 +64,7 @@ func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, nodeName return "", err } - volumeID := volumeSource.VolumeID + volumeID := aws.KubernetesVolumeID(volumeSource.VolumeID) // awsCloud.AttachDisk checks if disk is already attached to node and // succeeds in that case, so no need to do that separately. @@ -79,8 +79,8 @@ func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, nodeName func (attacher *awsElasticBlockStoreAttacher) VolumesAreAttached(specs []*volume.Spec, nodeName types.NodeName) (map[*volume.Spec]bool, error) { volumesAttachedCheck := make(map[*volume.Spec]bool) - volumeSpecMap := make(map[string]*volume.Spec) - volumeIDList := []string{} + volumeSpecMap := make(map[aws.KubernetesVolumeID]*volume.Spec) + volumeIDList := []aws.KubernetesVolumeID{} for _, spec := range specs { volumeSource, _, err := getVolumeSource(spec) if err != nil { @@ -88,9 +88,10 @@ func (attacher *awsElasticBlockStoreAttacher) VolumesAreAttached(specs []*volume continue } - volumeIDList = append(volumeIDList, volumeSource.VolumeID) + name := aws.KubernetesVolumeID(volumeSource.VolumeID) + volumeIDList = append(volumeIDList, name) volumesAttachedCheck[spec] = true - volumeSpecMap[volumeSource.VolumeID] = spec + volumeSpecMap[name] = spec } attachedResult, err := attacher.awsVolumes.DisksAreAttached(volumeIDList, nodeName) if err != nil { @@ -163,7 +164,7 @@ func (attacher *awsElasticBlockStoreAttacher) GetDeviceMountPath( return "", err } - return makeGlobalPDPath(attacher.host, volumeSource.VolumeID), nil + return makeGlobalPDPath(attacher.host, aws.KubernetesVolumeID(volumeSource.VolumeID)), nil } // FIXME: this method can be further pruned. @@ -221,7 +222,7 @@ func (plugin *awsElasticBlockStorePlugin) NewDetacher() (volume.Detacher, error) } func (detacher *awsElasticBlockStoreDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error { - volumeID := path.Base(deviceMountPath) + volumeID := aws.KubernetesVolumeID(path.Base(deviceMountPath)) attached, err := detacher.awsVolumes.DiskIsAttached(volumeID, nodeName) if err != nil { diff --git a/pkg/volume/aws_ebs/attacher_test.go b/pkg/volume/aws_ebs/attacher_test.go index d3b8cf98567..9b2d780ab2a 100644 --- a/pkg/volume/aws_ebs/attacher_test.go +++ b/pkg/volume/aws_ebs/attacher_test.go @@ -29,37 +29,37 @@ import ( "k8s.io/kubernetes/pkg/types" ) -func TestGetDeviceName_Volume(t *testing.T) { +func TestGetVolumeName_Volume(t *testing.T) { plugin := newPlugin() - name := "my-aws-volume" + name := aws.KubernetesVolumeID("my-aws-volume") spec := createVolSpec(name, false) - deviceName, err := plugin.GetVolumeName(spec) + volumeName, err := plugin.GetVolumeName(spec) if err != nil { - t.Errorf("GetDeviceName error: %v", err) + t.Errorf("GetVolumeName error: %v", err) } - if deviceName != name { - t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName) + if volumeName != string(name) { + t.Errorf("GetVolumeName error: expected %s, got %s", name, volumeName) } } -func TestGetDeviceName_PersistentVolume(t *testing.T) { +func TestGetVolumeName_PersistentVolume(t *testing.T) { plugin := newPlugin() - name := "my-aws-pv" + name := aws.KubernetesVolumeID("my-aws-pv") spec := createPVSpec(name, true) - deviceName, err := plugin.GetVolumeName(spec) + volumeName, err := plugin.GetVolumeName(spec) if err != nil { - t.Errorf("GetDeviceName error: %v", err) + t.Errorf("GetVolumeName error: %v", err) } - if deviceName != name { - t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName) + if volumeName != string(name) { + t.Errorf("GetVolumeName error: expected %s, got %s", name, volumeName) } } // One testcase for TestAttachDetach table test below type testcase struct { - name string + name aws.KubernetesVolumeID // For fake AWS: attach attachCall detach detachCall @@ -74,7 +74,7 @@ type testcase struct { } func TestAttachDetach(t *testing.T) { - diskName := "disk" + diskName := aws.KubernetesVolumeID("disk") nodeName := types.NodeName("instance") readOnly := false spec := createVolSpec(diskName, readOnly) @@ -111,7 +111,8 @@ func TestAttachDetach(t *testing.T) { detach: detachCall{diskName, nodeName, "/dev/sda", nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) - return "", detacher.Detach(diskName, nodeName) + mountPath := "/mnt/" + string(diskName) + return "", detacher.Detach(mountPath, nodeName) }, }, @@ -121,7 +122,8 @@ func TestAttachDetach(t *testing.T) { diskIsAttached: diskIsAttachedCall{diskName, nodeName, false, nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) - return "", detacher.Detach(diskName, nodeName) + mountPath := "/mnt/" + string(diskName) + return "", detacher.Detach(mountPath, nodeName) }, }, @@ -132,7 +134,8 @@ func TestAttachDetach(t *testing.T) { detach: detachCall{diskName, nodeName, "/dev/sda", nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) - return "", detacher.Detach(diskName, nodeName) + mountPath := "/mnt/" + string(diskName) + return "", detacher.Detach(mountPath, nodeName) }, }, @@ -143,7 +146,8 @@ func TestAttachDetach(t *testing.T) { detach: detachCall{diskName, nodeName, "", detachError}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) - return "", detacher.Detach(diskName, nodeName) + mountPath := "/mnt/" + string(diskName) + return "", detacher.Detach(mountPath, nodeName) }, expectedError: detachError, }, @@ -185,12 +189,12 @@ func newDetacher(testcase *testcase) *awsElasticBlockStoreDetacher { } } -func createVolSpec(name string, readOnly bool) *volume.Spec { +func createVolSpec(name aws.KubernetesVolumeID, readOnly bool) *volume.Spec { return &volume.Spec{ Volume: &api.Volume{ VolumeSource: api.VolumeSource{ AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ - VolumeID: name, + VolumeID: string(name), ReadOnly: readOnly, }, }, @@ -198,13 +202,13 @@ func createVolSpec(name string, readOnly bool) *volume.Spec { } } -func createPVSpec(name string, readOnly bool) *volume.Spec { +func createPVSpec(name aws.KubernetesVolumeID, readOnly bool) *volume.Spec { return &volume.Spec{ PersistentVolume: &api.PersistentVolume{ Spec: api.PersistentVolumeSpec{ PersistentVolumeSource: api.PersistentVolumeSource{ AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ - VolumeID: name, + VolumeID: string(name), ReadOnly: readOnly, }, }, @@ -216,7 +220,7 @@ func createPVSpec(name string, readOnly bool) *volume.Spec { // Fake AWS implementation type attachCall struct { - diskName string + diskName aws.KubernetesVolumeID nodeName types.NodeName readOnly bool retDeviceName string @@ -224,20 +228,20 @@ type attachCall struct { } type detachCall struct { - diskName string + diskName aws.KubernetesVolumeID nodeName types.NodeName retDeviceName string ret error } type diskIsAttachedCall struct { - diskName string + diskName aws.KubernetesVolumeID nodeName types.NodeName isAttached bool ret error } -func (testcase *testcase) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) { +func (testcase *testcase) AttachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) { expected := &testcase.attach if expected.diskName == "" && expected.nodeName == "" { @@ -267,7 +271,7 @@ func (testcase *testcase) AttachDisk(diskName string, nodeName types.NodeName, r return expected.retDeviceName, expected.ret } -func (testcase *testcase) DetachDisk(diskName string, nodeName types.NodeName) (string, error) { +func (testcase *testcase) DetachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) { expected := &testcase.detach if expected.diskName == "" && expected.nodeName == "" { @@ -292,7 +296,7 @@ func (testcase *testcase) DetachDisk(diskName string, nodeName types.NodeName) ( return expected.retDeviceName, expected.ret } -func (testcase *testcase) DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error) { +func (testcase *testcase) DiskIsAttached(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) { expected := &testcase.diskIsAttached if expected.diskName == "" && expected.nodeName == "" { @@ -317,22 +321,22 @@ func (testcase *testcase) DiskIsAttached(diskName string, nodeName types.NodeNam return expected.isAttached, expected.ret } -func (testcase *testcase) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) { +func (testcase *testcase) DisksAreAttached(diskNames []aws.KubernetesVolumeID, nodeName types.NodeName) (map[aws.KubernetesVolumeID]bool, error) { return nil, errors.New("Not implemented") } -func (testcase *testcase) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName string, err error) { +func (testcase *testcase) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName aws.KubernetesVolumeID, err error) { return "", errors.New("Not implemented") } -func (testcase *testcase) DeleteDisk(volumeName string) (bool, error) { +func (testcase *testcase) DeleteDisk(volumeName aws.KubernetesVolumeID) (bool, error) { return false, errors.New("Not implemented") } -func (testcase *testcase) GetVolumeLabels(volumeName string) (map[string]string, error) { +func (testcase *testcase) GetVolumeLabels(volumeName aws.KubernetesVolumeID) (map[string]string, error) { return map[string]string{}, errors.New("Not implemented") } -func (testcase *testcase) GetDiskPath(volumeName string) (string, error) { +func (testcase *testcase) GetDiskPath(volumeName aws.KubernetesVolumeID) (string, error) { return "", errors.New("Not implemented") } diff --git a/pkg/volume/aws_ebs/aws_ebs.go b/pkg/volume/aws_ebs/aws_ebs.go index 8cf704334be..494a14e7733 100644 --- a/pkg/volume/aws_ebs/aws_ebs.go +++ b/pkg/volume/aws_ebs/aws_ebs.go @@ -27,6 +27,7 @@ import ( "github.com/golang/glog" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/resource" + "k8s.io/kubernetes/pkg/cloudprovider/providers/aws" "k8s.io/kubernetes/pkg/types" "k8s.io/kubernetes/pkg/util/exec" "k8s.io/kubernetes/pkg/util/mount" @@ -102,7 +103,7 @@ func (plugin *awsElasticBlockStorePlugin) newMounterInternal(spec *volume.Spec, return nil, err } - volumeID := ebs.VolumeID + volumeID := aws.KubernetesVolumeID(ebs.VolumeID) fsType := ebs.FSType partition := "" if ebs.Partition != 0 { @@ -153,7 +154,7 @@ func (plugin *awsElasticBlockStorePlugin) newDeleterInternal(spec *volume.Spec, return &awsElasticBlockStoreDeleter{ awsElasticBlockStore: &awsElasticBlockStore{ volName: spec.Name(), - volumeID: spec.PersistentVolume.Spec.AWSElasticBlockStore.VolumeID, + volumeID: aws.KubernetesVolumeID(spec.PersistentVolume.Spec.AWSElasticBlockStore.VolumeID), manager: manager, plugin: plugin, }}, nil @@ -205,7 +206,7 @@ func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath // Abstract interface to PD operations. type ebsManager interface { - CreateVolume(provisioner *awsElasticBlockStoreProvisioner) (volumeID string, volumeSizeGB int, labels map[string]string, err error) + CreateVolume(provisioner *awsElasticBlockStoreProvisioner) (volumeID aws.KubernetesVolumeID, volumeSizeGB int, labels map[string]string, err error) // Deletes a volume DeleteVolume(deleter *awsElasticBlockStoreDeleter) error } @@ -216,7 +217,7 @@ type awsElasticBlockStore struct { volName string podUID types.UID // Unique id of the PD, used to find the disk resource in the provider. - volumeID string + volumeID aws.KubernetesVolumeID // Specifies the partition to mount partition string // Utility interface that provides API calls to the provider to attach/detach disks. @@ -312,9 +313,9 @@ func (b *awsElasticBlockStoreMounter) SetUpAt(dir string, fsGroup *int64) error return nil } -func makeGlobalPDPath(host volume.VolumeHost, volumeID string) string { +func makeGlobalPDPath(host volume.VolumeHost, volumeID aws.KubernetesVolumeID) string { // Clean up the URI to be more fs-friendly - name := volumeID + name := string(volumeID) name = strings.Replace(name, "://", "/", -1) return path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), "mounts", name) } @@ -432,7 +433,7 @@ func (c *awsElasticBlockStoreProvisioner) Provision() (*api.PersistentVolume, er }, PersistentVolumeSource: api.PersistentVolumeSource{ AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ - VolumeID: volumeID, + VolumeID: string(volumeID), FSType: "ext4", Partition: 0, ReadOnly: false, diff --git a/pkg/volume/aws_ebs/aws_ebs_test.go b/pkg/volume/aws_ebs/aws_ebs_test.go index 4173eef9731..9de017bf647 100644 --- a/pkg/volume/aws_ebs/aws_ebs_test.go +++ b/pkg/volume/aws_ebs/aws_ebs_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake" + "k8s.io/kubernetes/pkg/cloudprovider/providers/aws" "k8s.io/kubernetes/pkg/types" "k8s.io/kubernetes/pkg/util/mount" utiltesting "k8s.io/kubernetes/pkg/util/testing" @@ -91,7 +92,7 @@ type fakePDManager struct { // TODO(jonesdl) To fully test this, we could create a loopback device // and mount that instead. -func (fake *fakePDManager) CreateVolume(c *awsElasticBlockStoreProvisioner) (volumeID string, volumeSizeGB int, labels map[string]string, err error) { +func (fake *fakePDManager) CreateVolume(c *awsElasticBlockStoreProvisioner) (volumeID aws.KubernetesVolumeID, volumeSizeGB int, labels map[string]string, err error) { labels = make(map[string]string) labels["fakepdmanager"] = "yes" return "test-aws-volume-name", 100, labels, nil diff --git a/pkg/volume/aws_ebs/aws_util.go b/pkg/volume/aws_ebs/aws_util.go index 1d2ded0063b..74c0e38f44f 100644 --- a/pkg/volume/aws_ebs/aws_util.go +++ b/pkg/volume/aws_ebs/aws_util.go @@ -65,7 +65,7 @@ func (util *AWSDiskUtil) DeleteVolume(d *awsElasticBlockStoreDeleter) error { // CreateVolume creates an AWS EBS volume. // Returns: volumeID, volumeSizeGB, labels, error -func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (string, int, map[string]string, error) { +func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (aws.KubernetesVolumeID, int, map[string]string, error) { cloud, err := getCloudProvider(c.awsElasticBlockStore.plugin.host.GetCloudProvider()) if err != nil { return "", 0, nil, err diff --git a/pkg/volume/gce_pd/attacher.go b/pkg/volume/gce_pd/attacher.go index 190e27acaff..052da91503e 100644 --- a/pkg/volume/gce_pd/attacher.go +++ b/pkg/volume/gce_pd/attacher.go @@ -242,7 +242,7 @@ func (plugin *gcePersistentDiskPlugin) NewDetacher() (volume.Detacher, error) { // attached to the specified node. If the volume is not attached, it succeeds // (returns nil). If it is attached, Detach issues a call to the GCE cloud // provider to attach it. -// Callers are responsible for retryinging on failure. +// Callers are responsible for retrying on failure. // Callers are responsible for thread safety between concurrent attach and detach // operations. func (detacher *gcePersistentDiskDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error { diff --git a/plugin/pkg/admission/persistentvolume/label/admission.go b/plugin/pkg/admission/persistentvolume/label/admission.go index a2c92ebc87d..e4fb246acb5 100644 --- a/plugin/pkg/admission/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/persistentvolume/label/admission.go @@ -118,7 +118,8 @@ func (l *persistentVolumeLabel) findAWSEBSLabels(volume *api.PersistentVolume) ( // TODO: GetVolumeLabels is actually a method on the Volumes interface // If that gets standardized we can refactor to reduce code duplication - labels, err := ebsVolumes.GetVolumeLabels(volume.Spec.AWSElasticBlockStore.VolumeID) + spec := aws.KubernetesVolumeID(volume.Spec.AWSElasticBlockStore.VolumeID) + labels, err := ebsVolumes.GetVolumeLabels(spec) if err != nil { return nil, err } diff --git a/plugin/pkg/admission/persistentvolume/label/admission_test.go b/plugin/pkg/admission/persistentvolume/label/admission_test.go index edd33b34bf6..01d2bc34c7c 100644 --- a/plugin/pkg/admission/persistentvolume/label/admission_test.go +++ b/plugin/pkg/admission/persistentvolume/label/admission_test.go @@ -34,35 +34,35 @@ type mockVolumes struct { var _ aws.Volumes = &mockVolumes{} -func (v *mockVolumes) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) { +func (v *mockVolumes) AttachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) { return "", fmt.Errorf("not implemented") } -func (v *mockVolumes) DetachDisk(diskName string, nodeName types.NodeName) (string, error) { +func (v *mockVolumes) DetachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) { return "", fmt.Errorf("not implemented") } -func (v *mockVolumes) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName string, err error) { +func (v *mockVolumes) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName aws.KubernetesVolumeID, err error) { return "", fmt.Errorf("not implemented") } -func (v *mockVolumes) DeleteDisk(volumeName string) (bool, error) { +func (v *mockVolumes) DeleteDisk(volumeName aws.KubernetesVolumeID) (bool, error) { return false, fmt.Errorf("not implemented") } -func (v *mockVolumes) GetVolumeLabels(volumeName string) (map[string]string, error) { +func (v *mockVolumes) GetVolumeLabels(volumeName aws.KubernetesVolumeID) (map[string]string, error) { return v.volumeLabels, v.volumeLabelsError } -func (c *mockVolumes) GetDiskPath(volumeName string) (string, error) { +func (c *mockVolumes) GetDiskPath(volumeName aws.KubernetesVolumeID) (string, error) { return "", fmt.Errorf("not implemented") } -func (c *mockVolumes) DiskIsAttached(volumeName string, nodeName types.NodeName) (bool, error) { +func (c *mockVolumes) DiskIsAttached(volumeName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) { return false, fmt.Errorf("not implemented") } -func (c *mockVolumes) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) { +func (c *mockVolumes) DisksAreAttached(diskNames []aws.KubernetesVolumeID, nodeName types.NodeName) (map[aws.KubernetesVolumeID]bool, error) { return nil, fmt.Errorf("not implemented") }