From 708261e06c754ed9f757e1c91fd12e446857b702 Mon Sep 17 00:00:00 2001
From: Hemant Kumar <hekumar@redhat.com>
Date: Tue, 7 Apr 2020 19:19:34 -0400
Subject: [PATCH] Make AfterSuite hooks ordered

ginkgo has a weird bug that - AfterEach does not get called when
testsuite exits with certain kind of interrupt (Ctrl-C for example).
More info - https://github.com/onsi/ginkgo/issues/222

We workaround this issue in Kubernetes by adding a special hook into
AfterSuite call, but AfterSuite can not be used to peforms certain
kind of cleanup because it can race with AfterEach hook and
framework.AfterEach hook will set framework.ClientSet to nil.

This presents a problem in cleaning up CSI driver and testpods. This
PR removes cleanup of driver manifest via CleanupAction because that
is not safe and racy (such as f.ClientSet may disappear!) and makes
AfterSuite hooks run in a ordered fashion
---
 test/e2e/framework/cleanup.go    | 25 +++++++++++++++++++------
 test/e2e/storage/utils/create.go |  8 --------
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/test/e2e/framework/cleanup.go b/test/e2e/framework/cleanup.go
index 6c5097dc404..c1dc00b934a 100644
--- a/test/e2e/framework/cleanup.go
+++ b/test/e2e/framework/cleanup.go
@@ -16,22 +16,30 @@ limitations under the License.
 
 package framework
 
-import "sync"
+import (
+	"sync"
+)
 
 // CleanupActionHandle is an integer pointer type for handling cleanup action
 type CleanupActionHandle *int
+type cleanupFuncHandle struct {
+	actionHandle CleanupActionHandle
+	actionHook   func()
+}
 
 var cleanupActionsLock sync.Mutex
-var cleanupActions = map[CleanupActionHandle]func(){}
+var cleanupHookList = []cleanupFuncHandle{}
 
 // AddCleanupAction installs a function that will be called in the event of the
 // whole test being terminated.  This allows arbitrary pieces of the overall
 // test to hook into SynchronizedAfterSuite().
+// The hooks are called in last-in-first-out order.
 func AddCleanupAction(fn func()) CleanupActionHandle {
 	p := CleanupActionHandle(new(int))
 	cleanupActionsLock.Lock()
 	defer cleanupActionsLock.Unlock()
-	cleanupActions[p] = fn
+	c := cleanupFuncHandle{actionHandle: p, actionHook: fn}
+	cleanupHookList = append([]cleanupFuncHandle{c}, cleanupHookList...)
 	return p
 }
 
@@ -40,7 +48,12 @@ func AddCleanupAction(fn func()) CleanupActionHandle {
 func RemoveCleanupAction(p CleanupActionHandle) {
 	cleanupActionsLock.Lock()
 	defer cleanupActionsLock.Unlock()
-	delete(cleanupActions, p)
+	for i, item := range cleanupHookList {
+		if item.actionHandle == p {
+			cleanupHookList = append(cleanupHookList[:i], cleanupHookList[i+1:]...)
+			break
+		}
+	}
 }
 
 // RunCleanupActions runs all functions installed by AddCleanupAction.  It does
@@ -51,8 +64,8 @@ func RunCleanupActions() {
 	func() {
 		cleanupActionsLock.Lock()
 		defer cleanupActionsLock.Unlock()
-		for _, fn := range cleanupActions {
-			list = append(list, fn)
+		for _, p := range cleanupHookList {
+			list = append(list, p.actionHook)
 		}
 	}()
 	// Run unlocked.
diff --git a/test/e2e/storage/utils/create.go b/test/e2e/storage/utils/create.go
index 06daf34846c..691286475fd 100644
--- a/test/e2e/storage/utils/create.go
+++ b/test/e2e/storage/utils/create.go
@@ -141,14 +141,7 @@ func PatchItems(f *framework.Framework, items ...interface{}) error {
 // - only the latest stable API version for each item is supported
 func CreateItems(f *framework.Framework, items ...interface{}) (func(), error) {
 	var destructors []func() error
-	var cleanupHandle framework.CleanupActionHandle
 	cleanup := func() {
-		if cleanupHandle == nil {
-			// Already done.
-			return
-		}
-		framework.RemoveCleanupAction(cleanupHandle)
-
 		// TODO (?): use same logic as framework.go for determining
 		// whether we are expected to clean up? This would change the
 		// meaning of the -delete-namespace and -delete-namespace-on-failure
@@ -160,7 +153,6 @@ func CreateItems(f *framework.Framework, items ...interface{}) (func(), error) {
 			}
 		}
 	}
-	cleanupHandle = framework.AddCleanupAction(cleanup)
 
 	var result error
 	for _, item := range items {