Merge pull request #55525 from miaoyq/fixes-55505

Automatic merge from submit-queue (batch tested with PRs 55254, 55525, 50108, 54674, 55263). 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>.

Correct clean up actions in e2e tests

**What this PR does / why we need it**:
Remove the duplicate "cleanup action" code in `test/e2e/e2e.go`, and use the clean up code in test/e2e/framework instead.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #55505 

**Special notes for your reviewer**:

**Release note**:

```release-note

```

test/e2e/apimachinery/BUILD

@@ -66,6 +66,7 @@ go_library(
         "//vendor/k8s.io/client-go/util/retry:go_default_library",
         "//vendor/k8s.io/client-go/util/workqueue:go_default_library",
         "//vendor/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1:go_default_library",
+        "//vendor/k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset:go_default_library",
         "//vendor/k8s.io/sample-apiserver/pkg/apis/wardle/v1alpha1:go_default_library",
     ],
 )

test/e2e/apimachinery/aggregator.go

@@ -35,7 +35,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/discovery"
+	clientset "k8s.io/client-go/kubernetes"
 	apiregistrationv1beta1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1"
+	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
 	rbacapi "k8s.io/kubernetes/pkg/apis/rbac"
 	utilversion "k8s.io/kubernetes/pkg/util/version"
 	"k8s.io/kubernetes/test/e2e/framework"
@@ -47,9 +49,21 @@ import (
 var serverAggregatorVersion = utilversion.MustParseSemantic("v1.7.0")
 
 var _ = SIGDescribe("Aggregator", func() {
+	var ns string
+	var c clientset.Interface
+	var aggrclient *aggregatorclient.Clientset
 	f := framework.NewDefaultFramework("aggregator")
 	framework.AddCleanupAction(func() {
-		cleanTest(f)
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(ns) > 0 {
+			cleanTest(c, aggrclient, ns)
+		}
+	})
+
+	BeforeEach(func() {
+		c = f.ClientSet
+		ns = f.Namespace.Name
+		aggrclient = f.AggregatorClient
 	})
 
 	It("Should be able to support the 1.7 Sample API Server using the current Aggregator", func() {
@@ -62,13 +76,10 @@ var _ = SIGDescribe("Aggregator", func() {
 	})
 })
 
-func cleanTest(f *framework.Framework) {
+func cleanTest(client clientset.Interface, aggrclient *aggregatorclient.Clientset, namespace string) {
 	// delete the APIService first to avoid causing discovery errors
-	aggrclient := f.AggregatorClient
 	_ = aggrclient.ApiregistrationV1beta1().APIServices().Delete("v1alpha1.wardle.k8s.io", nil)
 
-	namespace := f.Namespace.Name
-	client := f.ClientSet
 	_ = client.ExtensionsV1beta1().Deployments(namespace).Delete("sample-apiserver", nil)
 	_ = client.CoreV1().Secrets(namespace).Delete("sample-apiserver-secret", nil)
 	_ = client.CoreV1().Services(namespace).Delete("sample-api", nil)
@@ -415,7 +426,7 @@ func TestSampleAPIServer(f *framework.Framework, image string) {
 		framework.Failf("failed to get back the correct deleted flunders list %v from the dynamic client", unstructuredList)
 	}
 
-	cleanTest(f)
+	cleanTest(client, aggrclient, namespace)
 }
 
 func validateErrorWithDebugInfo(f *framework.Framework, err error, pods *v1.PodList, msg string, fields ...interface{}) {

test/e2e/apimachinery/webhook.go

@@ -71,12 +71,20 @@ var serverWebhookVersion = utilversion.MustParseSemantic("v1.8.0")
 
 var _ = SIGDescribe("AdmissionWebhook", func() {
 	var context *certContext
+	var ns string
+	var c clientset.Interface
 	f := framework.NewDefaultFramework("webhook")
 	framework.AddCleanupAction(func() {
-		cleanWebhookTest(f)
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(ns) > 0 {
+			cleanWebhookTest(c, ns)
+		}
 	})
 
 	BeforeEach(func() {
+		c = f.ClientSet
+		ns = f.Namespace.Name
+
 		// Make sure the relevant provider supports admission webhook
 		framework.SkipUnlessServerVersionGTE(serverWebhookVersion, f.ClientSet.Discovery())
 		framework.SkipUnlessProviderIs("gce", "gke", "local")
@@ -563,11 +571,8 @@ func updateConfigMap(c clientset.Interface, ns, name string, update updateConfig
 	return cm, pollErr
 }
 
-func cleanWebhookTest(f *framework.Framework) {
+func cleanWebhookTest(client clientset.Interface, namespaceName string) {
-	client := f.ClientSet
 	_ = client.AdmissionregistrationV1alpha1().ValidatingWebhookConfigurations().Delete(webhookConfigName, nil)
-	_ = client.AdmissionregistrationV1alpha1().ValidatingWebhookConfigurations().Delete(crdWebhookConfigName, nil)
-	namespaceName := f.Namespace.Name
 	_ = client.CoreV1().Services(namespaceName).Delete(serviceName, nil)
 	_ = client.ExtensionsV1beta1().Deployments(namespaceName).Delete(deploymentName, nil)
 	_ = client.CoreV1().Secrets(namespaceName).Delete(secretName, nil)

test/e2e/e2e.go

@@ -21,7 +21,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
-	"sync"
 	"testing"
 	"time"
 
@@ -249,55 +248,13 @@ var _ = ginkgo.SynchronizedBeforeSuite(func() []byte {
 	}
 })
 
-type CleanupActionHandle *int
-
-var cleanupActionsLock sync.Mutex
-var cleanupActions = map[CleanupActionHandle]func(){}
-
-// 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().
-func AddCleanupAction(fn func()) CleanupActionHandle {
-	p := CleanupActionHandle(new(int))
-	cleanupActionsLock.Lock()
-	defer cleanupActionsLock.Unlock()
-	cleanupActions[p] = fn
-	return p
-}
-
-// RemoveCleanupAction removes a function that was installed by
-// AddCleanupAction.
-func RemoveCleanupAction(p CleanupActionHandle) {
-	cleanupActionsLock.Lock()
-	defer cleanupActionsLock.Unlock()
-	delete(cleanupActions, p)
-}
-
-// RunCleanupActions runs all functions installed by AddCleanupAction.  It does
-// not remove them (see RemoveCleanupAction) but it does run unlocked, so they
-// may remove themselves.
-func RunCleanupActions() {
-	list := []func(){}
-	func() {
-		cleanupActionsLock.Lock()
-		defer cleanupActionsLock.Unlock()
-		for _, fn := range cleanupActions {
-			list = append(list, fn)
-		}
-	}()
-	// Run unlocked.
-	for _, fn := range list {
-		fn()
-	}
-}
-
 // Similar to SynchornizedBeforeSuite, we want to run some operations only once (such as collecting cluster logs).
 // Here, the order of functions is reversed; first, the function which runs everywhere,
 // and then the function that only runs on the first Ginkgo node.
 var _ = ginkgo.SynchronizedAfterSuite(func() {
 	// Run on all Ginkgo nodes
 	framework.Logf("Running AfterSuite actions on all node")
-	RunCleanupActions()
+	framework.RunCleanupActions()
 }, func() {
 	// Run only Ginkgo on node 1
 	framework.Logf("Running AfterSuite actions on node 1")

test/e2e/storage/persistent_volumes-vsphere.go

@@ -131,7 +131,8 @@ var _ = SIGDescribe("PersistentVolumes:vsphere", func() {
 		3. Delete Volume (vmdk)
 	*/
 	framework.AddCleanupAction(func() {
-		if len(volumePath) > 0 {
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(ns) > 0 && len(volumePath) > 0 {
 			framework.ExpectNoError(waitForVSphereDiskToDetach(vsp, volumePath, node))
 			vsp.DeleteVolume(volumePath)
 		}

test/e2e/storage/vsphere_scale.go

@@ -114,9 +114,12 @@ var _ = SIGDescribe("vcp at scale [Feature:vsphere] ", func() {
 		Remove labels from all the nodes
 	*/
 	framework.AddCleanupAction(func() {
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(namespace) > 0 {
 			for _, node := range nodes.Items {
 				framework.RemoveLabelOffNode(client, node.Name, NodeLabelKey)
 			}
+		}
 	})
 
 	It("vsphere scale tests", func() {

test/e2e/storage/vsphere_volume_diskformat.go

@@ -73,7 +73,7 @@ var _ = SIGDescribe("Volume Disk Format [Feature:vsphere]", func() {
 			framework.Failf("Unable to find ready and schedulable Node")
 		}
 		if !isNodeLabeled {
-			nodeLabelValue := "vsphere_e2e_" + string(uuid.NewUUID())
+			nodeLabelValue = "vsphere_e2e_" + string(uuid.NewUUID())
 			nodeKeyValueLabel = make(map[string]string)
 			nodeKeyValueLabel["vsphere_e2e_label"] = nodeLabelValue
 			framework.AddOrUpdateLabelOnNode(client, nodeName, "vsphere_e2e_label", nodeLabelValue)
@@ -81,7 +81,8 @@ var _ = SIGDescribe("Volume Disk Format [Feature:vsphere]", func() {
 		}
 	})
 	framework.AddCleanupAction(func() {
-		if len(nodeLabelValue) > 0 {
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(namespace) > 0 && len(nodeLabelValue) > 0 {
 			framework.RemoveLabelOffNode(client, nodeName, "vsphere_e2e_label")
 		}
 	})

test/e2e/storage/vsphere_volume_placement.go

@@ -77,12 +77,15 @@ var _ = SIGDescribe("Volume Placement", func() {
 		2. Delete VMDK volume
 	*/
 	framework.AddCleanupAction(func() {
+		// Cleanup actions will be called even when the tests are skipped and leaves namespace unset.
+		if len(ns) > 0 {
 			if len(node1KeyValueLabel) > 0 {
 				framework.RemoveLabelOffNode(c, node1Name, "vsphere_e2e_label")
 			}
 			if len(node2KeyValueLabel) > 0 {
 				framework.RemoveLabelOffNode(c, node2Name, "vsphere_e2e_label")
 			}
+		}
 	})
 	/*
 		Steps