Merge pull request #44066 from marun/fed-improve-e2e-setup

Automatic merge from submit-queue (batch tested with PRs 44097, 42772, 43880, 44031, 44066)

[Federation] Improve e2e test setup

This PR improves federation e2e test setup:

 - reuses e2e framework setup (``NewDefaultFramework``) instead of duplicating it
 - ensures ``FederationAfterEach`` is called if an error occurs in ``FederationBeforeEach`` (as per the [example](https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/framework.go#L161) of the e2e framework)
 - skips creation of a test namespace in the hosting cluster (not used for a federation e2e test)

cc: @kubernetes/sig-federation-pr-reviews @kubernetes/sig-testing-pr-reviews

test/e2e/framework/framework.go

@@ -64,7 +64,8 @@ type Framework struct {
 	StagingClient     *staging.Clientset
 	ClientPool        dynamic.ClientPool
 
-	Namespace                *v1.Namespace   // Every test has at least one namespace
+	SkipNamespaceCreation    bool            // Whether to skip creating a namespace
+	Namespace                *v1.Namespace   // Every test has at least one namespace unless creation is skipped
 	namespacesToDelete       []*v1.Namespace // Some tests have more than one.
 	NamespaceDeletionTimeout time.Duration
 
@@ -181,23 +182,26 @@ func (f *Framework) BeforeEach() {
 		f.ClientPool = dynamic.NewClientPool(config, api.Registry.RESTMapper(), dynamic.LegacyAPIPathResolverFunc)
 	}
 
-	By("Building a namespace api object")
+	if !f.SkipNamespaceCreation {
-	namespace, err := f.CreateNamespace(f.BaseName, map[string]string{
+		By("Building a namespace api object")
-		"e2e-framework": f.BaseName,
+		namespace, err := f.CreateNamespace(f.BaseName, map[string]string{
-	})
+			"e2e-framework": f.BaseName,
-	Expect(err).NotTo(HaveOccurred())
+		})
-
-	f.Namespace = namespace
-
-	if TestContext.VerifyServiceAccount {
-		By("Waiting for a default service account to be provisioned in namespace")
-		err = WaitForDefaultServiceAccountInNamespace(f.ClientSet, namespace.Name)
 		Expect(err).NotTo(HaveOccurred())
-	} else {
+
-		Logf("Skipping waiting for service account")
+		f.Namespace = namespace
+
+		if TestContext.VerifyServiceAccount {
+			By("Waiting for a default service account to be provisioned in namespace")
+			err = WaitForDefaultServiceAccountInNamespace(f.ClientSet, namespace.Name)
+			Expect(err).NotTo(HaveOccurred())
+		} else {
+			Logf("Skipping waiting for service account")
+		}
 	}
 
 	if TestContext.GatherKubeSystemResourceUsageData != "false" && TestContext.GatherKubeSystemResourceUsageData != "none" {
+		var err error
 		f.gatherer, err = NewResourceUsageGatherer(f.ClientSet, ResourceGathererOptions{
 			inKubemark: ProviderIs("kubemark"),
 			masterOnly: TestContext.GatherKubeSystemResourceUsageData == "master",
@@ -274,7 +278,9 @@ func (f *Framework) AfterEach() {
 	// Print events if the test failed.
 	if CurrentGinkgoTestDescription().Failed && TestContext.DumpLogsOnFailure {
 		// Pass both unversioned client and and versioned clientset, till we have removed all uses of the unversioned client.
-		DumpAllNamespaceInfo(f.ClientSet, f.Namespace.Name)
+		if !f.SkipNamespaceCreation {
+			DumpAllNamespaceInfo(f.ClientSet, f.Namespace.Name)
+		}
 		By(fmt.Sprintf("Dumping a list of prepulled images on each node"))
 		LogContainersInPodsWithLabels(f.ClientSet, metav1.NamespaceSystem, ImagePullerLabels, "image-puller", Logf)
 	}

test/e2e_federation/framework/framework.go

@@ -40,33 +40,40 @@ import (
 type Framework struct {
 	*framework.Framework
 
+	// To make sure that this framework cleans up after itself, no matter what,
+	// we install a Cleanup action before each test and clear it after.  If we
+	// should abort, the AfterSuite hook should run all Cleanup actions.
+	cleanupHandle framework.CleanupActionHandle
+
 	FederationClientset *federation_clientset.Clientset
 	FederationNamespace *v1.Namespace
 }
 
 func NewDefaultFederatedFramework(baseName string) *Framework {
-	options := framework.FrameworkOptions{
+	f := &Framework{}
-		ClientQPS:   20,
-		ClientBurst: 50,
-	}
 
-	f := &Framework{&framework.Framework{
+	// Register the federation cleanup before initializing the default
-		BaseName:                 baseName,
+	// e2e framework to ensure it gets called before the default
-		AddonResourceConstraints: make(map[string]framework.ResourceConstraint),
+	// framework's cleanup.
-		Options:                  options,
-		ClientSet:                nil,
-	}, nil, &v1.Namespace{}}
-
-	BeforeEach(f.BeforeEach)
-	BeforeEach(f.FederationBeforeEach)
 	AfterEach(f.FederationAfterEach)
-	AfterEach(f.AfterEach)
+
+	f.Framework = framework.NewDefaultFramework(baseName)
+	f.Framework.SkipNamespaceCreation = true
+
+	// Register the federation setup after initializing the default
+	// e2e framework to ensure it gets called after the default
+	// framework's setup.
+	BeforeEach(f.FederationBeforeEach)
 
 	return f
 }
 
 // FederationBeforeEach checks for federation apiserver is ready and makes a namespace.
 func (f *Framework) FederationBeforeEach() {
+	// The fact that we need this feels like a bug in ginkgo.
+	// https://github.com/onsi/ginkgo/issues/222
+	f.cleanupHandle = framework.AddCleanupAction(f.FederationAfterEach)
+
 	if f.FederationClientset == nil {
 		By("Creating a release 1.5 federation Clientset")
 		var err error
@@ -122,6 +129,8 @@ func (f *Framework) deleteFederationNs() {
 
 // FederationAfterEach deletes the namespace, after reading its events.
 func (f *Framework) FederationAfterEach() {
+	framework.RemoveCleanupAction(f.cleanupHandle)
+
 	// DeleteNamespace at the very end in defer, to avoid any
 	// expectation failures preventing deleting the namespace.
 	defer func() {