From 94207352b923bbdb3940dfb63c785ae94026a0a3 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 6 Mar 2018 09:27:55 +0100 Subject: [PATCH] UPSTREAM: 60490: Volume deletion should be idempotent Contains parts of #59756 with isAWSErrorVolumeNotFound() and describeVolumes() changes. --- .../pkg/cloudprovider/providers/aws/aws.go | 32 ++++++-- .../kubernetes/test/e2e/framework/pv_util.go | 22 +++++- .../test/e2e/storage/volume_provisioning.go | 75 +++++++++++++++++++ 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/aws/aws.go b/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/aws/aws.go index 29f5da785bec..0d84d96da67a 100644 --- a/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/aws/aws.go +++ b/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/aws/aws.go @@ -809,7 +809,7 @@ func (s *awsSdkEC2) DescribeVolumes(request *ec2.DescribeVolumesInput) ([]*ec2.V if err != nil { recordAwsMetric("describe_volume", 0, err) - return nil, fmt.Errorf("error listing AWS volumes: %q", err) + return nil, err } results = append(results, response.Volumes...) @@ -1670,6 +1670,20 @@ func newAWSDisk(aws *Cloud, name KubernetesVolumeID) (*awsDisk, error) { return disk, nil } +// Helper function for describeVolume callers. Tries to retype given error to AWS error +// and returns true in case the AWS error is "InvalidVolume.NotFound", false otherwise +func isAWSErrorVolumeNotFound(err error) bool { + if err != nil { + if awsError, ok := err.(awserr.Error); ok { + // https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html + if awsError.Code() == "InvalidVolume.NotFound" { + return true + } + } + } + return false +} + // Gets the full information about this volume from the EC2 API func (d *awsDisk) describeVolume() (*ec2.Volume, error) { volumeID := d.awsID @@ -1680,13 +1694,13 @@ func (d *awsDisk) describeVolume() (*ec2.Volume, error) { volumes, err := d.ec2.DescribeVolumes(request) if err != nil { - return nil, fmt.Errorf("error querying ec2 for volume %q: %q", volumeID, err) + return nil, err } if len(volumes) == 0 { - return nil, fmt.Errorf("no volumes found for volume %q", volumeID) + return nil, fmt.Errorf("no volumes found") } if len(volumes) > 1 { - return nil, fmt.Errorf("multiple volumes found for volume %q", volumeID) + return nil, fmt.Errorf("multiple volumes found") } return volumes[0], nil } @@ -1839,10 +1853,10 @@ func (d *awsDisk) deleteVolume() (bool, error) { request := &ec2.DeleteVolumeInput{VolumeId: d.awsID.awsString()} _, err := d.ec2.DeleteVolume(request) if err != nil { + if isAWSErrorVolumeNotFound(err) { + return false, nil + } if awsError, ok := err.(awserr.Error); ok { - if awsError.Code() == "InvalidVolume.NotFound" { - return false, nil - } if awsError.Code() == "VolumeInUse" { return false, volume.NewDeletedVolumeInUseError(err.Error()) } @@ -2208,6 +2222,10 @@ func (c *Cloud) DeleteDisk(volumeName KubernetesVolumeID) (bool, error) { } available, err := c.checkIfAvailable(awsDisk, "deleting", "") if err != nil { + if isAWSErrorVolumeNotFound(err) { + glog.V(2).Infof("Volume %s not found when deleting it, assuming it's deleted", awsDisk.awsID) + return false, nil + } glog.Error(err) } diff --git a/vendor/k8s.io/kubernetes/test/e2e/framework/pv_util.go b/vendor/k8s.io/kubernetes/test/e2e/framework/pv_util.go index f39b1e6d5e4e..24e824661112 100644 --- a/vendor/k8s.io/kubernetes/test/e2e/framework/pv_util.go +++ b/vendor/k8s.io/kubernetes/test/e2e/framework/pv_util.go @@ -25,6 +25,7 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/golang/glog" . "github.com/onsi/ginkgo" "google.golang.org/api/googleapi" "k8s.io/api/core/v1" @@ -674,6 +675,22 @@ func DeletePDWithRetry(diskName string) error { return fmt.Errorf("unable to delete PD %q: %v", diskName, err) } +func newAWSClient(zone string) *ec2.EC2 { + var cfg *aws.Config + + if zone == "" { + zone = TestContext.CloudConfig.Zone + } + if zone == "" { + glog.Warning("No AWS zone configured!") + cfg = nil + } else { + region := zone[:len(zone)-1] + cfg = &aws.Config{Region: aws.String(region)} + } + return ec2.New(session.New(), cfg) +} + func createPD(zone string) (string, error) { if zone == "" { zone = TestContext.CloudConfig.Zone @@ -694,8 +711,7 @@ func createPD(zone string) (string, error) { } return pdName, nil } else if TestContext.Provider == "aws" { - client := ec2.New(session.New()) - + client := newAWSClient(zone) request := &ec2.CreateVolumeInput{} request.AvailabilityZone = aws.String(zone) request.Size = aws.Int64(10) @@ -747,7 +763,7 @@ func deletePD(pdName string) error { } return err } else if TestContext.Provider == "aws" { - client := ec2.New(session.New()) + client := newAWSClient("") tokens := strings.Split(pdName, "/") awsVolumeID := tokens[len(tokens)-1] diff --git a/vendor/k8s.io/kubernetes/test/e2e/storage/volume_provisioning.go b/vendor/k8s.io/kubernetes/test/e2e/storage/volume_provisioning.go index 38d1b2579302..6da3527d4c8b 100644 --- a/vendor/k8s.io/kubernetes/test/e2e/storage/volume_provisioning.go +++ b/vendor/k8s.io/kubernetes/test/e2e/storage/volume_provisioning.go @@ -30,6 +30,7 @@ import ( apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/api/core/v1" @@ -590,6 +591,80 @@ var _ = SIGDescribe("Dynamic Provisioning", func() { } framework.Logf("0 PersistentVolumes remain.") }) + + It("deletion should be idempotent", func() { + // This test ensures that deletion of a volume is idempotent. + // It creates a PV with Retain policy, deletes underlying AWS / GCE + // volume and changes the reclaim policy to Delete. + // PV controller should delete the PV even though the underlying volume + // is already deleted. + framework.SkipUnlessProviderIs("gce", "gke", "aws") + By("creating PD") + diskName, err := framework.CreatePDWithRetry() + framework.ExpectNoError(err) + + By("creating PV") + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "volume-idempotent-delete-", + }, + Spec: v1.PersistentVolumeSpec{ + // Use Retain to keep the PV, the test will change it to Delete + // when the time comes. + PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimRetain, + AccessModes: []v1.PersistentVolumeAccessMode{ + v1.ReadWriteOnce, + }, + Capacity: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse("1Gi"), + }, + // PV is bound to non-existing PVC, so it's reclaim policy is + // executed immediately + ClaimRef: &v1.ObjectReference{ + Kind: "PersistentVolumeClaim", + APIVersion: "v1", + UID: types.UID("01234567890"), + Namespace: ns, + Name: "dummy-claim-name", + }, + }, + } + switch framework.TestContext.Provider { + case "aws": + pv.Spec.PersistentVolumeSource = v1.PersistentVolumeSource{ + AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{ + VolumeID: diskName, + }, + } + case "gce", "gke": + pv.Spec.PersistentVolumeSource = v1.PersistentVolumeSource{ + GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{ + PDName: diskName, + }, + } + } + pv, err = c.CoreV1().PersistentVolumes().Create(pv) + framework.ExpectNoError(err) + + By("waiting for the PV to get Released") + err = framework.WaitForPersistentVolumePhase(v1.VolumeReleased, c, pv.Name, 2*time.Second, framework.PVReclaimingTimeout) + framework.ExpectNoError(err) + + By("deleting the PD") + err = framework.DeletePVSource(&pv.Spec.PersistentVolumeSource) + framework.ExpectNoError(err) + + By("changing the PV reclaim policy") + pv, err = c.CoreV1().PersistentVolumes().Get(pv.Name, metav1.GetOptions{}) + framework.ExpectNoError(err) + pv.Spec.PersistentVolumeReclaimPolicy = v1.PersistentVolumeReclaimDelete + pv, err = c.CoreV1().PersistentVolumes().Update(pv) + framework.ExpectNoError(err) + + By("waiting for the PV to get deleted") + err = framework.WaitForPersistentVolumeDeleted(c, pv.Name, 5*time.Second, framework.PVDeletingTimeout) + Expect(err).NotTo(HaveOccurred()) + }) }) Describe("DynamicProvisioner External", func() {