Centralize cleanup of created resources

This change revamps the way resources (like volumes and now also
snapshots) are managed in tests with regards to cleaning up. Instead of
putting the onus of cleaning up on the test author, we extend Cleanup to
automatically (un-)register resources as they are being used.

Cleanup now exposes a single API that implements both ControllerClient
and NodeClient to make it easier for all garbage collection-worthy
requests to be funnelled through the new API. The way this is
implemented in Cleanup is by embedding both ControllerClient and
NodeClient, and proxying to the actual methods before registering
cleanup tasks and returning the results.

Consequently, we can throw away large chunks of cleanup test code and
unify all {Controller,Node}Client access to the Cleanup variable. In
essence, this makes it much easier to do the right thing as a test
author since each existing Describe context will provide a single
interaction point to the CSI APIs only.

For frequently used resource creation operations, we also provide Must*
equivalents that fail the test if the results are unexpected. This makes
our test code even more streamlined by DRYing out the number of
assertions called.

List of other changes:

cleanup.go:
- Key volume and snapshot objects by ID instead of name. We have a few
  tests that omit or reuse the name, which makes it impossible to do
  automatic cleanup. Not printing the name of the resource as we clean
  up is a small price we have to pay for this adjustment, though.
- Fail tests when cleanup operations error out, except when we see error
  codes indicating that the resource is already cleaned up.
- Rename DeleteVolumes to Cleanup.
- Provide convenience method MustCreateSnapshotFromVolumeRequest to
  create a sourcing volume and a snapshot in one go.

controller.go, node.go:
- Change all tests to use the API exposed by Cleanup only. (That is, do
  not offer ControllerClient and NodeClient directly anymore.)
- Register Cleanup.Cleanup in AfterEach where missing.
- Drop cleanup steps from various tests as this is now being taken care
  of by Cleanup.
- Use Must* equivalents were applicable.
- Use HaveLen to simplify length assertions.
- Make order of Cleanup variable initialization consistent.
- Minor cosmetic improvements.

Author: timoreimann

pkg/sanity/cleanup.go

@@ -18,12 +18,18 @@ package sanity
 
 import (
 	"context"
+	"fmt"
 	"log"
 	"sync"
 
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/container-storage-interface/spec/lib/go/csi"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
 )
 
 // VolumeInfo keeps track of the information needed to delete a volume.
@@ -36,110 +42,284 @@ type VolumeInfo struct {
 	VolumeID string
 }
 
-// Cleanup keeps track of resources, in particular volumes, which need
-// to be freed when testing is done. All methods can be called concurrently.
+// Cleanup keeps track of resources, in particular volumes and snapshots, that
+// need to be freed when testing is done. It implements both ControllerClient
+// and NodeClient and should be used as the only interaction point to either
+// APIs. That way, Cleanup can ensure that resources are marked for cleanup as
+// necessary.
+// All methods can be called concurrently.
 type Cleanup struct {
-	Context                    *TestContext
-	ControllerClient           csi.ControllerClient
-	NodeClient                 csi.NodeClient
+	Context *TestContext
+	// ControllerClient is meant for struct-internal use only
+	csi.ControllerClient
+	// NodeClient is meant for struct-internal use only
+	csi.NodeClient
 	ControllerPublishSupported bool
 	NodeStageSupported         bool
 
-	// Maps from volume name to the node ID for which the volume
-	// is published and the volume ID.
-	volumes map[string]VolumeInfo
-	mutex   sync.Mutex
+	// mutex protects access to the volumes and snapshots maps.
+	mutex sync.Mutex
+	// volumes maps from volume IDs to VolumeInfo structs and records if a given
+	// volume must be cleaned up.
+	volumes map[string]*VolumeInfo
+	// snapshots is keyed by snapshot IDs and records if a given snapshot must
+	// be cleaned up.
+	snapshots map[string]bool
+}
+
+// ControllerClient interface wrappers
+
+// CreateVolume proxies to a Controller service implementation and registers the
+// volume for cleanup.
+func (cl *Cleanup) CreateVolume(ctx context.Context, in *csi.CreateVolumeRequest, _ ...grpc.CallOption) (*csi.CreateVolumeResponse, error) {
+	return cl.createVolume(ctx, in)
+}
+
+// DeleteVolume proxies to a Controller service implementation and unregisters
+// the volume from cleanup.
+func (cl *Cleanup) DeleteVolume(ctx context.Context, in *csi.DeleteVolumeRequest, _ ...grpc.CallOption) (*csi.DeleteVolumeResponse, error) {
+	return cl.deleteVolume(ctx, in)
+}
+
+// ControllerPublishVolume proxies to a Controller service implementation and
+// adds the node ID to the corresponding volume for cleanup.
+func (cl *Cleanup) ControllerPublishVolume(ctx context.Context, in *csi.ControllerPublishVolumeRequest, _ ...grpc.CallOption) (*csi.ControllerPublishVolumeResponse, error) {
+	return cl.controllerPublishVolume(ctx, in)
+}
+
+// CreateSnapshot proxies to a Controller service implementation and registers
+// the snapshot for cleanup.
+func (cl *Cleanup) CreateSnapshot(ctx context.Context, in *csi.CreateSnapshotRequest, _ ...grpc.CallOption) (*csi.CreateSnapshotResponse, error) {
+	return cl.createSnapshot(ctx, in)
+}
+
+// DeleteSnapshot proxies to a Controller service implementation and unregisters
+// the snapshot from cleanup.
+func (cl *Cleanup) DeleteSnapshot(ctx context.Context, in *csi.DeleteSnapshotRequest, _ ...grpc.CallOption) (*csi.DeleteSnapshotResponse, error) {
+	return cl.deleteSnapshot(ctx, in)
+}
+
+// MustCreateVolume is like CreateVolume but asserts that the volume was
+// successfully created.
+func (cl *Cleanup) MustCreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) *csi.CreateVolumeResponse {
+	vol, err := cl.createVolume(ctx, req)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(vol).NotTo(BeNil())
+	Expect(vol.GetVolume()).NotTo(BeNil())
+	Expect(vol.GetVolume().GetVolumeId()).NotTo(BeEmpty())
+	return vol
+}
+
+func (cl *Cleanup) createVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
+	vol, err := cl.ControllerClient.CreateVolume(ctx, req)
+	if err == nil && vol != nil && vol.GetVolume().GetVolumeId() != "" {
+		cl.registerVolume(VolumeInfo{VolumeID: vol.GetVolume().GetVolumeId()})
+	}
+	return vol, err
 }
 
-// RegisterVolume adds or updates an entry for the volume with the
-// given name.
-func (cl *Cleanup) RegisterVolume(name string, info VolumeInfo) {
+func (cl *Cleanup) deleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {
+	vol, err := cl.ControllerClient.DeleteVolume(ctx, req)
+	if err == nil {
+		cl.unregisterVolume(req.VolumeId)
+	}
+	return vol, err
+}
+
+// MustControllerPublishVolume is like ControllerPublishVolume but asserts that
+// the volume was successfully controller-published.
+func (cl *Cleanup) MustControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) *csi.ControllerPublishVolumeResponse {
+	conpubvol, err := cl.controllerPublishVolume(ctx, req)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(conpubvol).NotTo(BeNil())
+	return conpubvol
+}
+
+func (cl *Cleanup) controllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) {
+	conpubvol, err := cl.ControllerClient.ControllerPublishVolume(ctx, req)
+	if err == nil && req.VolumeId != "" && req.NodeId != "" {
+		cl.registerVolume(VolumeInfo{VolumeID: req.VolumeId, NodeID: req.NodeId})
+	}
+	return conpubvol, err
+}
+
+// registerVolume adds or updates an entry for given volume.
+func (cl *Cleanup) registerVolume(info VolumeInfo) {
+	Expect(info).NotTo(BeNil())
+	Expect(info.VolumeID).NotTo(BeEmpty())
 	cl.mutex.Lock()
 	defer cl.mutex.Unlock()
 	if cl.volumes == nil {
-		cl.volumes = make(map[string]VolumeInfo)
+		cl.volumes = make(map[string]*VolumeInfo)
 	}
-	cl.volumes[name] = info
+	cl.volumes[info.VolumeID] = &info
 }
 
-// MaybeRegisterVolume adds or updates an entry for the volume with
-// the given name if CreateVolume was successful.
-func (cl *Cleanup) MaybeRegisterVolume(name string, vol *csi.CreateVolumeResponse, err error) {
-	if err == nil && vol.GetVolume().GetVolumeId() != "" {
-		cl.RegisterVolume(name, VolumeInfo{VolumeID: vol.GetVolume().GetVolumeId()})
+// unregisterVolume removes the entry for the volume with the
+// given ID, thus preventing all cleanup operations for it.
+func (cl *Cleanup) unregisterVolume(id string) {
+	cl.mutex.Lock()
+	defer cl.mutex.Unlock()
+	cl.unregisterVolumeNoLock(id)
+}
+
+func (cl *Cleanup) unregisterVolumeNoLock(id string) {
+	Expect(id).NotTo(BeEmpty())
+	if cl.volumes != nil {
+		delete(cl.volumes, id)
 	}
 }
 
-// UnregisterVolume removes the entry for the volume with the
-// given name, thus preventing all cleanup operations for it.
-func (cl *Cleanup) UnregisterVolume(name string) {
+// MustCreateSnapshot is like CreateSnapshot but asserts that the snapshot was
+// successfully created.
+func (cl *Cleanup) MustCreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) *csi.CreateSnapshotResponse {
+	snap, err := cl.createSnapshot(ctx, req)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(snap).NotTo(BeNil())
+	verifySnapshotInfo(snap.GetSnapshot())
+	return snap
+}
+
+// MustCreateSnapshotFromVolumeRequest creates a volume from the given
+// CreateVolumeRequest and a snapshot subsequently. It registers the volume and
+// snapshot and asserts that both were created successfully.
+func (cl *Cleanup) MustCreateSnapshotFromVolumeRequest(ctx context.Context, req *csi.CreateVolumeRequest, snapshotName string) (*csi.CreateSnapshotResponse, *csi.CreateVolumeResponse) {
+	vol := cl.MustCreateVolume(ctx, req)
+	snap := cl.MustCreateSnapshot(ctx, MakeCreateSnapshotReq(cl.Context, snapshotName, vol.Volume.VolumeId))
+	return snap, vol
+}
+
+func (cl *Cleanup) createSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {
+	snap, err := cl.ControllerClient.CreateSnapshot(ctx, req)
+	if err == nil && snap.GetSnapshot().GetSnapshotId() != "" {
+		cl.registerSnapshot(snap.Snapshot.SnapshotId)
+	}
+	return snap, err
+}
+
+func (cl *Cleanup) deleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) {
+	snap, err := cl.ControllerClient.DeleteSnapshot(ctx, req)
+	if err == nil && req.SnapshotId != "" {
+		cl.unregisterSnapshot(req.SnapshotId)
+	}
+	return snap, err
+}
+
+func (cl *Cleanup) registerSnapshot(id string) {
 	cl.mutex.Lock()
 	defer cl.mutex.Unlock()
-	cl.unregisterVolume(name)
+	cl.registerSnapshotNoLock(id)
 }
-func (cl *Cleanup) unregisterVolume(name string) {
-	if cl.volumes != nil {
-		delete(cl.volumes, name)
+
+func (cl *Cleanup) registerSnapshotNoLock(id string) {
+	Expect(id).NotTo(BeEmpty())
+	if cl.snapshots == nil {
+		cl.snapshots = make(map[string]bool)
 	}
+	cl.snapshots[id] = true
 }
 
-// DeleteVolumes stops using the registered volumes and tries to delete all of them.
-func (cl *Cleanup) DeleteVolumes() {
+func (cl *Cleanup) unregisterSnapshot(id string) {
 	cl.mutex.Lock()
 	defer cl.mutex.Unlock()
-	if cl.volumes == nil {
-		return
+	cl.unregisterSnapshotNoLock(id)
+}
+
+func (cl *Cleanup) unregisterSnapshotNoLock(id string) {
+	Expect(id).NotTo(BeEmpty())
+	if cl.snapshots != nil {
+		delete(cl.snapshots, id)
 	}
-	logger := log.New(GinkgoWriter, "cleanup: ", 0)
+}
+
+// Cleanup calls unpublish methods as needed and deletes all volumes and
+// snapshots.
+func (cl *Cleanup) Cleanup() {
+	cl.mutex.Lock()
+	defer cl.mutex.Unlock()
 	ctx := context.Background()
 
-	for name, info := range cl.volumes {
-		logger.Printf("deleting %s = %s", name, info.VolumeID)
-		if _, err := cl.NodeClient.NodeUnpublishVolume(
-			ctx,
-			&csi.NodeUnpublishVolumeRequest{
-				VolumeId:   info.VolumeID,
-				TargetPath: cl.Context.TargetPath + "/target",
-			},
-		); err != nil {
-			logger.Printf("warning: NodeUnpublishVolume: %s", err)
-		}
+	cl.deleteVolumes(ctx)
+	cl.deleteSnapshots(ctx)
+}
+
+func (cl *Cleanup) deleteVolumes(ctx context.Context) {
+	logger := log.New(GinkgoWriter, "cleanup volumes: ", 0)
 
-		if cl.NodeStageSupported {
-			if _, err := cl.NodeClient.NodeUnstageVolume(
+	for volumeID, info := range cl.volumes {
+		logger.Printf("deleting %s", volumeID)
+		if cl.NodeClient != nil {
+			if _, err := cl.NodeUnpublishVolume(
 				ctx,
-				&csi.NodeUnstageVolumeRequest{
-					VolumeId:          info.VolumeID,
-					StagingTargetPath: cl.Context.StagingPath,
+				&csi.NodeUnpublishVolumeRequest{
+					VolumeId:   volumeID,
+					TargetPath: cl.Context.TargetPath + "/target",
 				},
 			); err != nil {
-				logger.Printf("warning: NodeUnstageVolume: %s", err)
+				if status.Code(err) != codes.NotFound {
+					Fail(fmt.Sprintf("NodeUnpublishVolume failed: %s", err))
+				}
+			}
+
+			if cl.NodeStageSupported {
+				if _, err := cl.NodeUnstageVolume(
+					ctx,
+					&csi.NodeUnstageVolumeRequest{
+						VolumeId:          volumeID,
+						StagingTargetPath: cl.Context.StagingPath,
+					},
+				); err != nil {
+					if status.Code(err) != codes.NotFound {
+						Fail(fmt.Sprintf("NodeUnstageVolume failed: %s", err))
+					}
+				}
 			}
 		}
 
 		if cl.ControllerPublishSupported && info.NodeID != "" {
 			if _, err := cl.ControllerClient.ControllerUnpublishVolume(
 				ctx,
 				&csi.ControllerUnpublishVolumeRequest{
-					VolumeId: info.VolumeID,
+					VolumeId: volumeID,
 					NodeId:   info.NodeID,
 					Secrets:  cl.Context.Secrets.ControllerUnpublishVolumeSecret,
 				},
 			); err != nil {
-				logger.Printf("warning: ControllerUnpublishVolume: %s", err)
+				Fail(fmt.Sprintf("ControllerUnpublishVolume failed: %s", err))
 			}
 		}
 
 		if _, err := cl.ControllerClient.DeleteVolume(
 			ctx,
 			&csi.DeleteVolumeRequest{
-				VolumeId: info.VolumeID,
+				VolumeId: volumeID,
 				Secrets:  cl.Context.Secrets.DeleteVolumeSecret,
 			},
 		); err != nil {
-			logger.Printf("error: DeleteVolume: %s", err)
+			if status.Code(err) != codes.NotFound {
+				Fail(fmt.Sprintf("DeleteVolume failed: %s", err))
+			}
 		}
 
-		cl.unregisterVolume(name)
+		cl.unregisterVolumeNoLock(volumeID)
+	}
+}
+
+func (cl *Cleanup) deleteSnapshots(ctx context.Context) {
+	logger := log.New(GinkgoWriter, "cleanup snapshots: ", 0)
+
+	for id := range cl.snapshots {
+		logger.Printf("deleting %s", id)
+		_, err := cl.ControllerClient.DeleteSnapshot(
+			ctx,
+			&csi.DeleteSnapshotRequest{
+				SnapshotId: id,
+				Secrets:    cl.Context.Secrets.DeleteSnapshotSecret,
+			},
+		)
+		if err != nil {
+			Fail(fmt.Sprintf("DeleteSnapshot failed: %s", err))
+		}
+		cl.unregisterSnapshotNoLock(id)
 	}
 }