Merge pull request #60490 from jsafrane/fix-aws-delete

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

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.


**Release note**:


Fixes: #60778

```release-note
NONE
```

/sig storage
/sig aws

/assign @justinsb @gnufied

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)
 	}
 

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]

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() {