From 38c0ce75c3da3f301b1214fb7569d0fe8dc2d625 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Mon, 26 Feb 2018 12:03:20 +0100 Subject: [PATCH 1/2] Volume deletion should be idempotent - Describe* calls should return aws.Error so caller can handle individual errors. aws.Error already has enough context ("InvalidVolume.NotFound: The volume 'vol-0a06cc096e989c5a2' does not exist") - Deletion of already deleted volume should succeed. --- pkg/cloudprovider/providers/aws/aws.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 6eb9f3dc02..394550e43c 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/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...) @@ -1891,10 +1891,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()) } @@ -2266,6 +2266,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) } From 97fae903a6f6f0f94a7637b9acdecc7284bf20e4 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 27 Feb 2018 12:26:26 +0100 Subject: [PATCH 2/2] Add e2e test for deletion --- test/e2e/framework/pv_util.go | 22 +++++++- test/e2e/storage/volume_provisioning.go | 75 +++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/test/e2e/framework/pv_util.go b/test/e2e/framework/pv_util.go index ee866b248b..296eb8e8a3 100644 --- a/test/e2e/framework/pv_util.go +++ b/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" @@ -678,6 +679,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 @@ -698,8 +715,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) @@ -751,7 +767,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/test/e2e/storage/volume_provisioning.go b/test/e2e/storage/volume_provisioning.go index 4193d86b37..97cbaa72c7 100644 --- a/test/e2e/storage/volume_provisioning.go +++ b/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" @@ -591,6 +592,80 @@ var _ = utils.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() {