diff --git a/docs/keps/20200619-federated-resource-status.md b/docs/keps/20200619-federated-resource-status.md index fc99f298ff..f8108c01ad 100644 --- a/docs/keps/20200619-federated-resource-status.md +++ b/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`. diff --git a/pkg/apis/core/v1beta1/federatedtypeconfig_types.go b/pkg/apis/core/v1beta1/federatedtypeconfig_types.go index 0f9cc0c405..30288fdbe2 100644 --- a/pkg/apis/core/v1beta1/federatedtypeconfig_types.go +++ b/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 diff --git a/pkg/controller/federatedtypeconfig/controller.go b/pkg/controller/federatedtypeconfig/controller.go index 0653ecc394..990aa64963 100644 --- a/pkg/controller/federatedtypeconfig/controller.go +++ b/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() { diff --git a/pkg/controller/sync/controller.go b/pkg/controller/sync/controller.go index d46f1d48bf..f5d0106bb7 100644 --- a/pkg/controller/sync/controller.go +++ b/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 diff --git a/pkg/controller/sync/status/status.go b/pkg/controller/sync/status/status.go index 7d5724392b..f0fb2e1d54 100644 --- a/pkg/controller/sync/status/status.go +++ b/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 } } diff --git a/pkg/features/features.go b/pkg/features/features.go index 3f0f2b624a..df1075fb89 100644 --- a/pkg/features/features.go +++ b/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}, } diff --git a/test/common/crudtester.go b/test/common/crudtester.go index 4757e5f6f9..83d493c697 100644 --- a/test/common/crudtester.go +++ b/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 { diff --git a/test/e2e/framework/controller.go b/test/e2e/framework/controller.go index eea3bb4ee8..3f3db32b7c 100644 --- a/test/e2e/framework/controller.go +++ b/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()