Merge pull request #40911 from nikhiljindal/finalizer1

Automatic merge from submit-queue (batch tested with PRs 40906, 40924, 40938, 40902, 40911)

federation: Updating deletion helper to add both finalizers in a single update

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

cc @mwielgus @csbell

federation/pkg/federation-controller/daemonset/daemonset_controller.go

@@ -239,20 +239,20 @@ func (daemonsetcontroller *DaemonSetController) removeFinalizerFunc(obj pkgrunti
 	return daemonset, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a daemonset.
-func (daemonsetcontroller *DaemonSetController) addFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (daemonsetcontroller *DaemonSetController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	daemonset := obj.(*extensionsv1.DaemonSet)
-	daemonset.ObjectMeta.Finalizers = append(daemonset.ObjectMeta.Finalizers, finalizer)
+	daemonset.ObjectMeta.Finalizers = append(daemonset.ObjectMeta.Finalizers, finalizers...)
 	daemonset, err := daemonsetcontroller.federatedApiClient.Extensions().DaemonSets(daemonset.Namespace).Update(daemonset)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to daemonset %s: %v", finalizer, daemonset.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to daemonset %s: %v", finalizers, daemonset.Name, err)
 	}
 	return daemonset, nil
 }
 
 func (daemonsetcontroller *DaemonSetController) Run(stopChan <-chan struct{}) {
-	glog.V(1).Infof("Starting daemonset controllr")
+	glog.V(1).Infof("Starting daemonset controller")
 	go daemonsetcontroller.daemonsetInformerController.Run(stopChan)
 
 	glog.V(1).Infof("Starting daemonset federated informer")

federation/pkg/federation-controller/daemonset/daemonset_controller_test.go

@@ -97,10 +97,9 @@ func TestDaemonSetController(t *testing.T) {
 	// Test add federated daemonset.
 	daemonsetWatch.Add(&daemonset1)
 
-	// There should be 2 updates to add both the finalizers.
+	// There should be an update to add both the finalizers.
 	updatedDaemonSet := GetDaemonSetFromChan(daemonsetUpdateChan)
 	assert.True(t, daemonsetController.hasFinalizerFunc(updatedDaemonSet, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	updatedDaemonSet = GetDaemonSetFromChan(daemonsetUpdateChan)
 	assert.True(t, daemonsetController.hasFinalizerFunc(updatedDaemonSet, metav1.FinalizerOrphan))
 	daemonset1 = *updatedDaemonSet
 

federation/pkg/federation-controller/deployment/deploymentcontroller.go

@@ -256,14 +256,14 @@ func (fdc *DeploymentController) removeFinalizerFunc(obj runtime.Object, finaliz
 	return deployment, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a deployment.
-func (fdc *DeploymentController) addFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (fdc *DeploymentController) addFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	deployment := obj.(*extensionsv1.Deployment)
-	deployment.ObjectMeta.Finalizers = append(deployment.ObjectMeta.Finalizers, finalizer)
+	deployment.ObjectMeta.Finalizers = append(deployment.ObjectMeta.Finalizers, finalizers...)
 	deployment, err := fdc.fedClient.Extensions().Deployments(deployment.Namespace).Update(deployment)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to deployment %s: %v", finalizer, deployment.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to deployment %s: %v", finalizers, deployment.Name, err)
 	}
 	return deployment, nil
 }
@@ -420,8 +420,10 @@ func (fdc *DeploymentController) schedule(fd *extensionsv1.Deployment, clusters
 	if fdPref != nil { // create a new planner if user specified a preference
 		plannerToBeUsed = planner.NewPlanner(fdPref)
 	}
-
+	replicas := int64(0)
-	replicas := int64(*fd.Spec.Replicas)
+	if fd.Spec.Replicas != nil {
+		replicas = int64(*fd.Spec.Replicas)
+	}
 	var clusterNames []string
 	for _, cluster := range clusters {
 		clusterNames = append(clusterNames, cluster.Name)

federation/pkg/federation-controller/deployment/deploymentcontroller_test.go

@@ -85,6 +85,9 @@ func TestDeploymentController(t *testing.T) {
 	cluster2 := NewCluster("cluster2", apiv1.ConditionTrue)
 
 	fakeClient := &fakefedclientset.Clientset{}
+	// Add an update reactor on fake client to return the desired updated object.
+	// This is a hack to workaround https://github.com/kubernetes/kubernetes/issues/40939.
+	AddFakeUpdateReactor("deployments", &fakeClient.Fake)
 	RegisterFakeList("clusters", &fakeClient.Fake, &fedv1.ClusterList{Items: []fedv1.Cluster{*cluster1}})
 	deploymentsWatch := RegisterFakeWatch("deployments", &fakeClient.Fake)
 	clusterWatch := RegisterFakeWatch("clusters", &fakeClient.Fake)

federation/pkg/federation-controller/ingress/ingress_controller.go

@@ -339,14 +339,14 @@ func (ic *IngressController) removeFinalizerFunc(obj pkgruntime.Object, finalize
 	return ingress, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a ingress.
-func (ic *IngressController) addFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (ic *IngressController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	ingress := obj.(*extensionsv1beta1.Ingress)
-	ingress.ObjectMeta.Finalizers = append(ingress.ObjectMeta.Finalizers, finalizer)
+	ingress.ObjectMeta.Finalizers = append(ingress.ObjectMeta.Finalizers, finalizers...)
 	ingress, err := ic.federatedApiClient.Extensions().Ingresses(ingress.Namespace).Update(ingress)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to ingress %s: %v", finalizer, ingress.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to ingress %s: %v", finalizers, ingress.Name, err)
 	}
 	return ingress, nil
 }

federation/pkg/federation-controller/ingress/ingress_controller_test.go

@@ -140,10 +140,9 @@ func TestIngressController(t *testing.T) {
 	assert.Equal(t, cluster.ObjectMeta.Annotations[uidAnnotationKey], cfg1.Data[uidKey])
 
 	t.Logf("Checking that appropriate finalizers are added")
-	// There should be 2 updates to add both the finalizers.
+	// There should be an update to add both the finalizers.
 	updatedIngress := GetIngressFromChan(t, fedIngressUpdateChan)
 	assert.True(t, ingressController.hasFinalizerFunc(updatedIngress, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	updatedIngress = GetIngressFromChan(t, fedIngressUpdateChan)
 	assert.True(t, ingressController.hasFinalizerFunc(updatedIngress, metav1.FinalizerOrphan), fmt.Sprintf("ingress does not have the orphan finalizer: %v", updatedIngress))
 	fedIngress = *updatedIngress
 

federation/pkg/federation-controller/namespace/namespace_controller.go

@@ -219,14 +219,14 @@ func (nc *NamespaceController) removeFinalizerFunc(obj runtime.Object, finalizer
 	return namespace, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a namespace.
-func (nc *NamespaceController) addFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (nc *NamespaceController) addFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	namespace := obj.(*apiv1.Namespace)
-	namespace.ObjectMeta.Finalizers = append(namespace.ObjectMeta.Finalizers, finalizer)
+	namespace.ObjectMeta.Finalizers = append(namespace.ObjectMeta.Finalizers, finalizers...)
 	namespace, err := nc.federatedApiClient.Core().Namespaces().Finalize(namespace)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to namespace %s: %v", finalizer, namespace.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to namespace %s: %v", finalizers, namespace.Name, err)
 	}
 	return namespace, nil
 }

federation/pkg/federation-controller/replicaset/replicasetcontroller.go

@@ -260,14 +260,14 @@ func (frsc *ReplicaSetController) removeFinalizerFunc(obj runtime.Object, finali
 	return replicaset, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a replicaset.
-func (frsc *ReplicaSetController) addFinalizerFunc(obj runtime.Object, finalizer string) (runtime.Object, error) {
+func (frsc *ReplicaSetController) addFinalizerFunc(obj runtime.Object, finalizers []string) (runtime.Object, error) {
 	replicaset := obj.(*extensionsv1.ReplicaSet)
-	replicaset.ObjectMeta.Finalizers = append(replicaset.ObjectMeta.Finalizers, finalizer)
+	replicaset.ObjectMeta.Finalizers = append(replicaset.ObjectMeta.Finalizers, finalizers...)
 	replicaset, err := frsc.fedClient.Extensions().ReplicaSets(replicaset.Namespace).Update(replicaset)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to replicaset %s: %v", finalizer, replicaset.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to replicaset %s: %v", finalizers, replicaset.Name, err)
 	}
 	return replicaset, nil
 }

federation/pkg/federation-controller/secret/secret_controller.go

@@ -220,14 +220,14 @@ func (secretcontroller *SecretController) removeFinalizerFunc(obj pkgruntime.Obj
 	return secret, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a secret.
-func (secretcontroller *SecretController) addFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (secretcontroller *SecretController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	secret := obj.(*apiv1.Secret)
-	secret.ObjectMeta.Finalizers = append(secret.ObjectMeta.Finalizers, finalizer)
+	secret.ObjectMeta.Finalizers = append(secret.ObjectMeta.Finalizers, finalizers...)
 	secret, err := secretcontroller.federatedApiClient.Core().Secrets(secret.Namespace).Update(secret)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to secret %s: %v", finalizer, secret.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to secret %s: %v", finalizers, secret.Name, err)
 	}
 	return secret, nil
 }

federation/pkg/federation-controller/secret/secret_controller_test.go

@@ -97,10 +97,9 @@ func TestSecretController(t *testing.T) {
 
 	// Test add federated secret.
 	secretWatch.Add(&secret1)
-	// There should be 2 updates to add both the finalizers.
+	// There should be an update to add both the finalizers.
 	updatedSecret := GetSecretFromChan(secretUpdateChan)
 	assert.True(t, secretController.hasFinalizerFunc(updatedSecret, deletionhelper.FinalizerDeleteFromUnderlyingClusters))
-	updatedSecret = GetSecretFromChan(secretUpdateChan)
 	assert.True(t, secretController.hasFinalizerFunc(updatedSecret, metav1.FinalizerOrphan))
 	secret1 = *updatedSecret
 

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

@@ -345,14 +345,14 @@ func (s *ServiceController) removeFinalizerFunc(obj pkgruntime.Object, finalizer
 	return service, nil
 }
 
-// Adds the given finalizer to the given objects ObjectMeta.
+// Adds the given finalizers to the given objects ObjectMeta.
 // Assumes that the given object is a service.
-func (s *ServiceController) addFinalizerFunc(obj pkgruntime.Object, finalizer string) (pkgruntime.Object, error) {
+func (s *ServiceController) addFinalizerFunc(obj pkgruntime.Object, finalizers []string) (pkgruntime.Object, error) {
 	service := obj.(*v1.Service)
-	service.ObjectMeta.Finalizers = append(service.ObjectMeta.Finalizers, finalizer)
+	service.ObjectMeta.Finalizers = append(service.ObjectMeta.Finalizers, finalizers...)
 	service, err := s.federationClient.Core().Services(service.Namespace).Update(service)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add finalizer %s to service %s: %v", finalizer, service.Name, err)
+		return nil, fmt.Errorf("failed to add finalizers %v to service %s: %v", finalizers, service.Name, err)
 	}
 	return service, nil
 }

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

@@ -46,7 +46,7 @@ const (
 
 type HasFinalizerFunc func(runtime.Object, string) bool
 type RemoveFinalizerFunc func(runtime.Object, string) (runtime.Object, error)
-type AddFinalizerFunc func(runtime.Object, string) (runtime.Object, error)
+type AddFinalizerFunc func(runtime.Object, []string) (runtime.Object, error)
 type ObjNameFunc func(runtime.Object) string
 
 type DeletionHelper struct {
@@ -89,19 +89,16 @@ func NewDeletionHelper(
 // This method should be called before creating objects in underlying clusters.
 func (dh *DeletionHelper) EnsureFinalizers(obj runtime.Object) (
 	runtime.Object, error) {
+	finalizers := []string{}
 	if !dh.hasFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters) {
-		glog.V(2).Infof("Adding finalizer %s to %s", FinalizerDeleteFromUnderlyingClusters, dh.objNameFunc(obj))
+		finalizers = append(finalizers, FinalizerDeleteFromUnderlyingClusters)
-		obj, err := dh.addFinalizerFunc(obj, FinalizerDeleteFromUnderlyingClusters)
-		if err != nil {
-			return obj, err
-		}
 	}
 	if !dh.hasFinalizerFunc(obj, metav1.FinalizerOrphan) {
-		glog.V(2).Infof("Adding finalizer %s to %s", metav1.FinalizerOrphan, dh.objNameFunc(obj))
+		finalizers = append(finalizers, metav1.FinalizerOrphan)
-		obj, err := dh.addFinalizerFunc(obj, metav1.FinalizerOrphan)
+	}
-		if err != nil {
+	if len(finalizers) != 0 {
-			return obj, err
+		glog.V(2).Infof("Adding finalizers %v to %s", finalizers, dh.objNameFunc(obj))
-		}
+		return dh.addFinalizerFunc(obj, finalizers)
 	}
 	return obj, nil
 }

federation/pkg/federation-controller/util/test/test_helper.go

@@ -226,6 +226,18 @@ func RegisterFakeCopyOnUpdate(resource string, client *core.Fake, watcher *Watch
 	return objChan
 }
 
+// Adds an update reactor to the given fake client.
+// The reactor just returns the object passed to update action.
+// This is used as a hack to workaround https://github.com/kubernetes/kubernetes/issues/40939.
+// Without this, all update actions using fake client return empty objects.
+func AddFakeUpdateReactor(resource string, client *core.Fake) {
+	client.AddReactor("update", resource, func(action core.Action) (bool, runtime.Object, error) {
+		updateAction := action.(core.UpdateAction)
+		originalObj := updateAction.GetObject()
+		return true, originalObj, nil
+	})
+}
+
 // GetObjectFromChan tries to get an api object from the given channel
 // within a reasonable time.
 func GetObjectFromChan(c chan runtime.Object) runtime.Object {