Merge pull request #37353 from nikhiljindal/serviceDelete

Automatic merge from submit-queue

Stop deleting underlying services when federation service is deleted

Fixes https://github.com/kubernetes/kubernetes/issues/36799

Fixing federation service controller to not delete services from underlying clusters when federated service is deleted.
None of the federation controller should do this unless explicitly asked by the user using DeleteOptions. This is the only federation controller that does that.

cc @kubernetes/sig-cluster-federation @madhusudancs

```release-note
federation service controller: stop deleting services from underlying clusters when federated service is deleted.
```

federation/pkg/federation-controller/service/servicecontroller.go

@@ -387,41 +387,6 @@ func (s *ServiceController) updateFederationService(key string, cachedService *c
 	return nil, !retryable
 }
 
-func (s *ServiceController) deleteFederationService(cachedService *cachedService) (error, bool) {
-	// handle available clusters one by one
-	var hasErr bool
-	for clusterName, cluster := range s.clusterCache.clientMap {
-		err := s.deleteClusterService(clusterName, cachedService, cluster.clientset)
-		if err != nil {
-			hasErr = true
-		} else if err := s.ensureDnsRecords(clusterName, cachedService); err != nil {
-			hasErr = true
-		}
-	}
-	if hasErr {
-		// detail error has been dumpped inside the loop
-		return fmt.Errorf("Service %s/%s was not successfully updated to all clusters", cachedService.lastState.Namespace, cachedService.lastState.Name), retryable
-	}
-	return nil, !retryable
-}
-
-func (s *ServiceController) deleteClusterService(clusterName string, cachedService *cachedService, clientset *kubeclientset.Clientset) error {
-	service := cachedService.lastState
-	glog.V(4).Infof("Deleting service %s/%s from cluster %s", service.Namespace, service.Name, clusterName)
-	var err error
-	for i := 0; i < clientRetryCount; i++ {
-		err = clientset.Core().Services(service.Namespace).Delete(service.Name, &v1.DeleteOptions{})
-		if err == nil || errors.IsNotFound(err) {
-			glog.V(4).Infof("Service %s/%s deleted from cluster %s", service.Namespace, service.Name, clusterName)
-			delete(cachedService.endpointMap, clusterName)
-			return nil
-		}
-		time.Sleep(cachedService.nextRetryDelay())
-	}
-	glog.V(4).Infof("Failed to delete service %s/%s from cluster %s, %+v", service.Namespace, service.Name, clusterName, err)
-	return err
-}
-
 func (s *ServiceController) ensureClusterService(cachedService *cachedService, clusterName string, service *v1.Service, client *kubeclientset.Clientset) error {
 	var err error
 	var needUpdate bool
@@ -925,31 +890,6 @@ func (s *ServiceController) processServiceUpdate(cachedService *cachedService, s
 // we should retry in that Duration.
 func (s *ServiceController) processServiceDeletion(key string) (error, time.Duration) {
 	glog.V(2).Infof("Process service deletion for %v", key)
-	cachedService, ok := s.serviceCache.get(key)
-	if !ok {
-		return fmt.Errorf("Service %s not in cache even though the watcher thought it was. Ignoring the deletion.", key), doNotRetry
-	}
-	service := cachedService.lastState
-	cachedService.rwlock.Lock()
-	defer cachedService.rwlock.Unlock()
-	s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingDNSRecord", "Deleting DNS Records")
-	// TODO should we delete dns info here or wait for endpoint changes? prefer here
-	// or we do nothing for service deletion
-	//err := s.dns.balancer.EnsureLoadBalancerDeleted(service)
-	err, retry := s.deleteFederationService(cachedService)
-	if err != nil {
-		message := "Error occurs when deleting federation service"
-		if retry {
-			message += " (will retry): "
-		} else {
-			message += " (will not retry): "
-		}
-		s.eventRecorder.Event(service, v1.EventTypeWarning, "DeletingDNSRecordFailed", message)
-		return err, cachedService.nextRetryDelay()
-	}
-	s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletedDNSRecord", "Deleted DNS Records")
 	s.serviceCache.delete(key)
-
-	cachedService.resetRetryDelay()
 	return nil, doNotRetry
 }

test/e2e/federated-service.go

@@ -113,6 +113,27 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() {
 				}()
 				waitForServiceShardsOrFail(nsName, service, clusters)
 			})
+
+			It("should not be deleted from underlying clusters when it is deleted", func() {
+				framework.SkipUnlessFederated(f.ClientSet)
+				nsName = f.FederationNamespace.Name
+				service = createServiceOrFail(f.FederationClientset_1_5, nsName, FederatedServiceName)
+				By(fmt.Sprintf("Successfully created federated service %q in namespace %q. Waiting for shards to appear in underlying clusters", service.Name, nsName))
+
+				waitForServiceShardsOrFail(nsName, service, clusters)
+
+				By(fmt.Sprintf("Deleting service %s", service.Name))
+				err := f.FederationClientset_1_5.Services(nsName).Delete(service.Name, &v1.DeleteOptions{})
+				framework.ExpectNoError(err, "Error deleting service %q in namespace %q", service.Name, service.Namespace)
+				By(fmt.Sprintf("Deletion of service %q in namespace %q succeeded.", service.Name, nsName))
+				By(fmt.Sprintf("Verifying that services in underlying clusters are not deleted"))
+				for clusterName, clusterClientset := range clusters {
+					_, err := clusterClientset.Core().Services(service.Namespace).Get(service.Name)
+					if err != nil {
+						framework.Failf("Unexpected error in fetching service %s in cluster %s, %s", service.Name, clusterName, err)
+					}
+				}
+			})
 		})
 
 		var _ = Describe("DNS", func() {
@@ -192,6 +213,15 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() {
 				for i, DNSName := range svcDNSNames {
 					discoverService(f, DNSName, true, "federated-service-e2e-discovery-pod-"+strconv.Itoa(i))
 				}
+				By("Verified that DNS rules are working as expected")
+
+				By("Deleting the service to verify that DNS rules still work")
+				err := f.FederationClientset_1_5.Services(nsName).Delete(FederatedServiceName, &v1.DeleteOptions{})
+				framework.ExpectNoError(err, "Error deleting service %q in namespace %q", service.Name, service.Namespace)
+				for i, DNSName := range svcDNSNames {
+					discoverService(f, DNSName, true, "federated-service-e2e-discovery-pod-"+strconv.Itoa(i))
+				}
+				By("Verified that deleting the service does not affect DNS records")
 			})
 
 			Context("non-local federated service", func() {