chore: code polishing

docs/keps/20200619-federated-resource-status.md

@@ -375,11 +375,12 @@ By raw status, the system understand to show the status of all the federated res
 This can have an impact in the performance, so this should be configured with precautions.
 
 **Note that**, this proposed solution includes additional flags to the kubefed control-plane
-components to avoid blowing out the control-plane due to frequent and concurrent API calls to
-update the status of the federated resources.
+components to limit the calls per second to the API, which could cause performance degradations
+to the control-plane when updating the status of the federated resources.
 
 In addition to the `conditions` field, this new feature adds a new property `remoteStatus` that holds
-the status of the created resource created in the kubefed cluster.
+the status of the created resource created in the kubefed cluster.The `remoteStatus` is an unstructured
+object due to the unknown structure of the status of the target resources.
 To enable the collection of the status of federated resources, a new feature gate, disabled by default, is added to the current list of features.
 Likewise the user has to explicitly enable the collection at `FederatedTypeConfig` level. To do so, the `FederatedTypeConfig.statusCollection` field
 has to be set to `Enabled`.

pkg/apis/core/v1beta1/federatedtypeconfig_types.go

@@ -208,7 +208,7 @@ func (f *FederatedTypeConfig) GetFederatedType() metav1.APIResource {
 }
 
 // TODO (hectorj2f): It should get deprecated once we move to the new status approach
-// because the type is the same than the target type.
+// because the type is the same as the target type.
 func (f *FederatedTypeConfig) GetStatusType() *metav1.APIResource {
 	if f.Spec.StatusType == nil {
 		return nil

pkg/controller/federatedtypeconfig/controller.go

@@ -148,13 +148,10 @@ func (c *Controller) reconcile(qualifiedName util.QualifiedName) util.Reconcilia
 	corev1b1.SetFederatedTypeConfigDefaults(typeConfig)
 
 	syncEnabled := typeConfig.GetPropagationEnabled()
-	// TODO (hectorj2f): Disabled by default the collection of the resource status following
-	// the NEW approach.
-	statusControllerEnabled := c.isEnabledFederatedServiceStatusCollection(typeConfig)
-	if c.controllerConfig.RawResourceStatusCollection {
-		// If Feature RawResourceStatusCollection is enabled then disable the old mechanism
-		statusControllerEnabled = false
-	}
+	// NOTE (hectorj2f): RawResourceStatusCollection is a new feature and is
+	// Disabled by default. When RawResourceStatusCollection is enabled,
+	// the old mechanism to collect the service status of FederatedServices would be disabled.
+	statusControllerEnabled := !c.controllerConfig.RawResourceStatusCollection && c.isEnabledFederatedServiceStatusCollection(typeConfig)
 
 	limitedScope := c.controllerConfig.TargetNamespace != metav1.NamespaceAll
 	if limitedScope && syncEnabled && !typeConfig.GetNamespaced() {

pkg/controller/sync/controller.go

@@ -203,7 +203,7 @@ func (s *KubeFedSyncController) Run(stopChan <-chan struct{}) {
 // synced with the corresponding api server.
 func (s *KubeFedSyncController) isSynced() bool {
 	if !s.informer.ClustersSynced() {
-		klog.Infof("Cluster list not synced")
+		klog.V(2).Info("Cluster list not synced")
 		return false
 	}
 	if !s.fedAccessor.HasSynced() {
@@ -290,7 +290,7 @@ func (s *KubeFedSyncController) reconcile(qualifiedName util.QualifiedName) util
 func (s *KubeFedSyncController) syncToClusters(fedResource FederatedResource) util.ReconciliationStatus {
 	// Enable raw resource status collection if the statusCollection is enabled for that type
 	// and the feature is also enabled.
-	enableRawResourceStatusCollection := (s.typeConfig.GetStatusEnabled() && s.rawResourceStatusCollection)
+	enableRawResourceStatusCollection := s.typeConfig.GetStatusEnabled() && s.rawResourceStatusCollection
 
 	clusters, err := s.informer.GetClusters()
 	if err != nil {
@@ -423,7 +423,7 @@ func (s *KubeFedSyncController) setFederatedStatus(fedResource FederatedResource
 			klog.Infof("No status update necessary for %s %q", kind, name)
 			return true, nil
 		}
-		klog.Infof("Updating status of name '%v' and kind '%v'", name, kind)
+		klog.Infof("Updating status for %s %q", kind, name)
 		err := s.hostClusterClient.UpdateStatus(context.TODO(), obj)
 		if err == nil {
 			return true, nil

pkg/controller/sync/status/status.go

@@ -76,7 +76,6 @@ type GenericClusterStatus struct {
 	Name         string            `json:"name"`
 	Status       PropagationStatus `json:"status,omitempty"`
 	RemoteStatus interface{}       `json:"remoteStatus,omitempty"`
-	//	Conditions []*metav1.Condition `json:"conditions,omitempty"`
 }
 
 type GenericCondition struct {
@@ -194,11 +193,11 @@ func (s *GenericFederatedStatus) update(generation int64, reason AggregateReason
 	return statusUpdated
 }
 
-// setClusters sets the status.clusters slice from a propagation status
-// map. Returns a boolean indication of whether the status.clusters was
+// setClusters sets the status.clusters slice from propagation and resource status
+// maps. Returns a boolean indication of whether the status.clusters was
 // modified.
 func (s *GenericFederatedStatus) setClusters(statusMap PropagationStatusMap, resourceStatusMap map[string]interface{}) bool {
-	if !s.clustersDiffers(statusMap, resourceStatusMap) {
+	if !s.clustersDiffer(statusMap, resourceStatusMap) {
 		return false
 	}
 	s.Clusters = []GenericClusterStatus{}
@@ -213,9 +212,9 @@ func (s *GenericFederatedStatus) setClusters(statusMap PropagationStatusMap, res
 	return true
 }
 
-// clustersDiffers checks whether `status.clusters` differs from the
+// clustersDiffer checks whether `status.clusters` differs from the
 // given status map.
-func (s *GenericFederatedStatus) clustersDiffers(statusMap PropagationStatusMap, resourceStatusMap map[string]interface{}) bool {
+func (s *GenericFederatedStatus) clustersDiffer(statusMap PropagationStatusMap, resourceStatusMap map[string]interface{}) bool {
 	if len(s.Clusters) != len(statusMap) || len(s.Clusters) != len(resourceStatusMap) {
 		klog.Info("Clusters differs from the size")
 		return true
@@ -225,7 +224,7 @@ func (s *GenericFederatedStatus) clustersDiffers(statusMap PropagationStatusMap,
 			return true
 		}
 		if !reflect.DeepEqual(resourceStatusMap[status.Name], status.RemoteStatus) {
-			klog.Infof("Clusters differs resource status: %v VS %v", resourceStatusMap[status.Name], status.RemoteStatus)
+			klog.V(4).Infof("Clusters resource status differ: %v VS %v", resourceStatusMap[status.Name], status.RemoteStatus)
 			return true
 		}
 	}

pkg/features/features.go

@@ -75,5 +75,5 @@ var DefaultKubeFedFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec
 	PushReconciler:               {Default: true, PreRelease: featuregate.Beta},
 	CrossClusterServiceDiscovery: {Default: true, PreRelease: featuregate.Alpha},
 	FederatedIngress:             {Default: true, PreRelease: featuregate.Alpha},
-	RawResourceStatusCollection:  {Default: false, PreRelease: featuregate.Beta},
+	RawResourceStatusCollection:  {Default: false, PreRelease: featuregate.Alpha},
 }

test/common/crudtester.go

@@ -730,12 +730,6 @@ func (c *FederatedTypeCrudTester) CheckRemoteStatus(fedObject *unstructured.Unst
 		}
 
 		clusterClient := genericclient.NewForConfigOrDie(clusterConfig)
-		defer func() {
-			err := clusterClient.Delete(context.TODO(), labeledObj, labeledObj.GetNamespace(), labeledObj.GetName())
-			if err != nil {
-				c.tl.Fatalf("Unexpected error: %v", err)
-			}
-		}()
 
 		c.tl.Log("Checking that the resource has status")
 		var objStatus interface{}
@@ -766,6 +760,11 @@ func (c *FederatedTypeCrudTester) CheckRemoteStatus(fedObject *unstructured.Unst
 			c.tl.Fatal("Federated object remote status is empty")
 		}
 		c.tl.Logf("Show federated object remote status %v", objRemoteStatus)
+
+		err = clusterClient.Delete(context.TODO(), labeledObj, labeledObj.GetNamespace(), labeledObj.GetName())
+		if err != nil {
+			c.tl.Fatalf("Unexpected error deleting the labeled resource: %v", err)
+		}
 	}
 }
 
@@ -784,27 +783,24 @@ func (c *FederatedTypeCrudTester) getRemoteStatus(fedObject *unstructured.Unstru
 			return false, nil
 		}
 
-		if err == nil {
-			resource := &status.GenericFederatedResource{}
-			err := util.UnstructuredToInterface(fedObj, resource)
-			if err != nil {
-				return false, err
-			}
-			if resource.Status != nil {
-				for _, cluster := range resource.Status.Clusters {
-					c.tl.Logf("Current status of resource for cluster '%s' with value: %v", cluster.Name, resource.Status)
-					if cluster.Name == clusterName && cluster.Status == status.ClusterPropagationOK {
-						c.tl.Logf("resource remote status for cluster '%s': %v", cluster.Name, cluster.RemoteStatus)
-						if cluster.RemoteStatus != nil {
-							remoteStatusObj = cluster.RemoteStatus
-							return true, nil
-						}
+		resource := &status.GenericFederatedResource{}
+		err = util.UnstructuredToInterface(fedObj, resource)
+		if err != nil {
+			return false, err
+		}
+		if resource.Status != nil {
+			for _, cluster := range resource.Status.Clusters {
+				c.tl.Logf("Current status of resource for cluster '%s' with value: %v", cluster.Name, resource.Status)
+				if cluster.Name == clusterName && cluster.Status == status.ClusterPropagationOK {
+					c.tl.Logf("resource remote status for cluster '%s': %v", cluster.Name, cluster.RemoteStatus)
+					if cluster.RemoteStatus != nil {
+						remoteStatusObj = cluster.RemoteStatus
+						return true, nil
 					}
 				}
 			}
-			return false, nil
 		}
-		return false, err
+		return false, nil
 	})
 
 	if err != nil {

test/e2e/framework/controller.go

@@ -50,7 +50,7 @@ func NewSyncControllerFixture(tl common.TestLogger, controllerConfig *util.Contr
 	if err != nil {
 		tl.Fatalf("Error starting sync controller: %v", err)
 	}
-	// The status controller should be only enabled for services whenver the statusAPIResource is defined
+	// The status controller should be only enabled for services whenever the statusAPIResource is defined
 	if typeConfig.GetStatusEnabled() && typeConfig.GetTargetType().Kind == "Service" {
 		federatedAPIResource := typeConfig.GetFederatedType()
 		statusAPIResource := typeConfig.GetStatusType()