Merge pull request #41386 from shashidharatd/federation-service-e2e-2

Automatic merge from submit-queue (batch tested with PRs 41299, 41325, 41386, 41329, 41418)

Fix resource leak in federation e2e tests and another issue

**What this PR does / why we need it**:
The cleanup after federation service e2e tests is not effective as this function cleanupServiceShardsAndProviderResources is getting called with empty string for namespace ("nsName") because the nsName variable is getting redefined.

Another issue is we are prematurely exiting the Poll in waitForServiceOrFail and the error check is incorrect.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #
Fixing the 2 issues mentioned above.

**Special notes for your reviewer**:

**Release note**:
`NONE`

cc @madhusudancs @kubernetes/sig-federation-bugs

test/e2e_federation/federated-service.go

@@ -92,7 +92,7 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 			clusters, primaryClusterName = getRegisteredClusters(UserAgentName, f)
 		})
 
-		Describe("service creation", func() {
+		Describe("Federated Service", func() {
 			var (
 				service *v1.Service
 				nsName  string
@@ -100,7 +100,7 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 
 			BeforeEach(func() {
 				fedframework.SkipUnlessFederated(f.ClientSet)
-				// Placeholder
+				nsName = f.FederationNamespace.Name
 			})
 
 			AfterEach(func() {
@@ -116,8 +116,6 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 
 			It("should create matching services in underlying clusters", func() {
 				fedframework.SkipUnlessFederated(f.ClientSet)
-
-				nsName = f.FederationNamespace.Name
 				service = createServiceOrFail(f.FederationClientset, nsName, FederatedServiceName)
 				defer func() { // Cleanup
 					By(fmt.Sprintf("Deleting service %q in namespace %q", service.Name, nsName))
@@ -129,7 +127,6 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 
 			It("should be deleted from underlying clusters when OrphanDependents is false", func() {
 				fedframework.SkipUnlessFederated(f.ClientSet)
-				nsName := f.FederationNamespace.Name
 				orphanDependents := false
 				verifyCascadingDeletionForService(f.FederationClientset, clusters, &orphanDependents, nsName)
 				By(fmt.Sprintf("Verified that services were deleted from underlying clusters"))
@@ -137,7 +134,6 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 
 			It("should not be deleted from underlying clusters when OrphanDependents is true", func() {
 				fedframework.SkipUnlessFederated(f.ClientSet)
-				nsName := f.FederationNamespace.Name
 				orphanDependents := true
 				verifyCascadingDeletionForService(f.FederationClientset, clusters, &orphanDependents, nsName)
 				By(fmt.Sprintf("Verified that services were not deleted from underlying clusters"))
@@ -145,7 +141,6 @@ var _ = framework.KubeDescribe("Federated Services [Feature:Federation]", func()
 
 			It("should not be deleted from underlying clusters when OrphanDependents is nil", func() {
 				fedframework.SkipUnlessFederated(f.ClientSet)
-				nsName := f.FederationNamespace.Name
 				verifyCascadingDeletionForService(f.FederationClientset, clusters, nil, nsName)
 				By(fmt.Sprintf("Verified that services were not deleted from underlying clusters"))
 			})

test/e2e_federation/federation-util.go

@@ -217,7 +217,6 @@ func getRegisteredClusters(userAgentName string, f *fedframework.Framework) (map
 */
 func waitForServiceOrFail(clientset *kubeclientset.Clientset, namespace string, service *v1.Service, present bool, timeout time.Duration) {
 	By(fmt.Sprintf("Fetching a federated service shard of service %q in namespace %q from cluster", service.Name, namespace))
-	var clusterService *v1.Service
 	err := wait.PollImmediate(framework.Poll, timeout, func() (bool, error) {
 		clusterService, err := clientset.Services(namespace).Get(service.Name, metav1.GetOptions{})
 		if (!present) && errors.IsNotFound(err) { // We want it gone, and it's gone.
@@ -225,17 +224,16 @@ func waitForServiceOrFail(clientset *kubeclientset.Clientset, namespace string,
 			return true, nil // Success
 		}
 		if present && err == nil { // We want it present, and the Get succeeded, so we're all good.
-			By(fmt.Sprintf("Success: shard of federated service %q in namespace %q in cluster is present", service.Name, namespace))
+			if equivalent(*clusterService, *service) {
-			return true, nil // Success
+				By(fmt.Sprintf("Success: shard of federated service %q in namespace %q in cluster is present", service.Name, namespace))
+				return true, nil // Success
+			}
+			return false, nil
 		}
 		By(fmt.Sprintf("Service %q in namespace %q in cluster.  Found: %v, waiting for Found: %v, trying again in %s (err=%v)", service.Name, namespace, clusterService != nil && err == nil, present, framework.Poll, err))
 		return false, nil
 	})
 	framework.ExpectNoError(err, "Failed to verify service %q in namespace %q in cluster: Present=%v", service.Name, namespace, present)
-
-	if present && clusterService != nil {
-		Expect(equivalent(*clusterService, *service))
-	}
 }
 
 /*