Federated Status Implementation for federated types #247

shashidharatd · 2018-09-04T16:43:06Z

This pr implements a generic federated status for federated types by extending the sync controller. The federated status is a list of resource status in member cluster, as outlined in the first alternative in issue #246

Later the controllers depending upon the status in member clusters can make use of this without monitoring the resource in individual clusters.

/cc @onyiny-ang @pmorie @marun @kubernetes-sigs/federation-wg

k8s-ci-robot · 2018-09-04T16:43:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashidharatd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [shashidharatd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

marun · 2018-09-07T18:03:17Z

@shashidharatd Am I missing something, or is the entirety of status being collected? I wasn't aware that chatty fields like conditions were useful.

gyliu513 · 2018-09-10T02:11:34Z

pkg/apis/core/v1alpha1/federatedingress_types.go

+	ClusterStatuses []FederatedIngressClusterStatus `json:"clusterStatuses,omitempty"`
+}
+
+// FederatedDeploymentClusterStatus is the observed status for a named cluster


s/FederatedDeploymentClusterStatus/FederatedIngressClusterStatus

oops, fixed. Thanks!

gyliu513 · 2018-09-10T02:16:38Z

pkg/apis/core/v1alpha1/zz_generated.kubebuilder.go

@@ -303,7 +303,6 @@ var (
 			Scope: "Namespaced",
 			Validation: &v1beta1.CustomResourceValidation{
 				OpenAPIV3Schema: &v1beta1.JSONSchemaProps{
-					Type: "object",


I think this change will not needed if the PR #250 got merged before this PR.

Agree. if it lands before. i will remove it.

gyliu513 · 2018-09-10T02:27:45Z

pkg/apis/core/typeconfig/interface.go

@@ -31,4 +31,5 @@ type Interface interface {
 	GetPlacement() metav1.APIResource
 	GetOverride() *metav1.APIResource
 	GetOverridePath() []string
+	GetCollectStatus() bool


Can you move this commit ahead of 3f94daf

its already in the same order as you asked (if you look in git). github may have messed up the order of commits).

gyliu513 · 2018-09-10T02:32:54Z

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go

@@ -89,6 +89,35 @@ type FederatedTypeConfig struct {
 	Status FederatedTypeConfigStatus `json:"status,omitempty"`
 }

+// FederatedResource is a generic resource representing any federated type


What is the target of this resource type? Does the intend is want to handle arbitrary resource types?

yes, as the comment suggest FederatedResource type generically represent any arbitrary federated type.

I see, then what is the relationship between this and #60 ? Why put this commit in this PR? With this generic resource type, do we still need other resource types? Like FederatedDeployment, FederatedIngress etc?

The sync controller is already capable of handling propogation of arbitrary types. here is the e2e test case which does that, https://github.com/kubernetes-sigs/federation-v2/blob/master/test/e2e/crud.go#L89 and there is no need to define an API for that. #60 is some additional feature to automatically create template, override & placement CRDs for a federated type.

The type FederatedResource which i defined in this PR represents a generic federated type which is used by the controller to Marshall/Unmarshall Json. We still need specific federated types defined like FederatedDeployment, FederatedIngress etc.. because they help validating user inputs and offer type safety.

Thanks @shashidharatd , clear now. But still have more question, I think we do not have placement policy for generic resources, what is the plan for this?

shashidharatd · 2018-09-10T04:24:17Z

@shashidharatd Am I missing something, or is the entirety of status being collected? I wasn't aware that chatty fields like conditions were useful.

@marun, the entire status is being collected. the purpose here was to be a generic solution. I agree that fields like conditions are not useful. overtime these fields will be obsoleted from kubernetes types itself as inferred from kubernetes/kubernetes#50798, and we may need not do any special handling. currently too many of the kubernetes controller may have moved from updating this field.
The stats collection for federated types is optional and can be switched off by configuring in federated typeconfig.

Alternatively, Should we drop conditions in status if found in status? wdyt?

font · 2018-09-11T04:37:05Z

test/common/crudtester.go

+				statusFound := false
+				for _, resourceClusterStatus := range resource.Status.ClusterStatuses {
+					if resourceClusterStatus.ClusterName == clusterName {
+						statusFound = true


font · 2018-09-11T04:49:46Z

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go

@@ -89,6 +89,35 @@ type FederatedTypeConfig struct {
 	Status FederatedTypeConfigStatus `json:"status,omitempty"`
 }

+// FederatedResource is a generic resource representing any federated type
+// +k8s:deepcopy-gen=false
+type FederatedResource struct {


I'm not sure I understand the need for this generic FederatedResource API type. I don't see it being persisted; it looks like it's just used to capture the status into this generic object and then just copied into the specified Federated<Type> resource for persistence. Why not work directly with the status for each individual Federated<Type> resource that you've added in this PR? Am I missing something?

Agree with your argument. Updated accordingly. PTAL. Thanks!

gyliu513

LGTM

marun · 2018-09-13T15:24:16Z

I'm reviewing this.

shashidharatd · 2018-09-13T16:39:07Z

I'm reviewing this.

cool, waiting for your inputs. Thanks !

marun

I think this PR makes sense if the target use case is user visibility into member cluster state of federated resources. If the goal is to provide status for consumption by other controllers, though, I have concerns with having the sync controller collect status.

If the scheduling controllers are rewritten to consume status written by the sync controller they effectively have a dependency on push propagation. What about pull propagation? Similarly, if the dns record controllers were rewritten to consume status written by the sync controller, what is a user wanted to deploy without propagation of any kind (a use case that has been previously discussed)?

If status were collected via a controller other than sync, though, it could complicate version tracking since propagated versions need to be maintained in response to status updates. That could suggest storing status separately from templates (i.e. in status-specific resources).

cc: @pmorie

marun · 2018-09-13T15:18:48Z

pkg/controller/sync/controller.go

+		newTemplate.Object["metadata"] = oldTemplate.Object["metadata"]
+		newTemplate.Object["spec"] = oldTemplate.Object["spec"]
+
+		newTemplate, err := s.templateClient.Resources(newTemplate.GetNamespace()).Update(newTemplate)


(No action required) Interesting, I didn't realize until now that the dynamic client doesn't have separate Update and UpdateStatus methods.

marun · 2018-09-13T15:33:03Z

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go

@@ -89,6 +92,19 @@ type FederatedTypeConfig struct {
 	Status FederatedTypeConfigStatus `json:"status,omitempty"`
 }

+// FederatedResourceStatus defines status of generic federated type
+// +k8s:deepcopy-gen=false
+type FederatedResourceStatus struct {


Why does this non-api type belong in this file, or in the api path at all?

marun · 2018-09-13T15:41:32Z

pkg/controller/sync/controller.go

+				status, ok, err := unstructured.NestedMap(clusterObj.Object, "status")
+				if err != nil || !ok {
+					wrappedErr := fmt.Errorf("Failed to get status of cluster resource object %s %q for cluster %q: %v", targetKind, key, clusterName, err)
+					runtime.HandleError(wrappedErr)


Why is status still collected for the cluster if it can't be retrieved successfully?

marun · 2018-09-13T15:54:43Z

pkg/controller/sync/controller.go

+			return federatedResourceStatus.ClusterStatuses[i].ClusterName < federatedResourceStatus.ClusterStatuses[j].ClusterName
+		})
+		newTemplate := &unstructured.Unstructured{make(map[string]interface{})}
+		newTemplate.Object["status"] = federatedResourceStatus


Why is it desirable to return a template wrapper instead of just the status slice?

marun · 2018-09-13T15:58:39Z

pkg/controller/sync/controller.go

+		if err != nil {
+			glog.Errorf("Error updating federated type status: %v", err)
+		}
+		return newTemplate, err


Why is it safe to return newTemplate if an error occurred?

marun · 2018-09-13T15:59:15Z

pkg/controller/sync/controller.go

+					wrappedErr := fmt.Errorf("Failed to get status of cluster resource object %s %q for cluster %q: %v", targetKind, key, clusterName, err)
+					runtime.HandleError(wrappedErr)
+				}
+				resourceClusterStatus := fedv1a1.ResourceClusterStatus{ClusterName: clusterName, Status: status}


Why is it desirable to collect status in this arbitrary type instead of a map?

marun · 2018-09-13T16:28:35Z

pkg/controller/sync/controller.go

@@ -167,6 +171,7 @@ func newFederationSyncController(typeConfig typeconfig.Interface, kubeConfig *re
 		templateClient:          templateClient,
 		pendingVersionUpdates:   sets.String{},
 		fedNamespace:            fedNamespace,
+		collectStatus:           typeConfig.GetCollectStatus(),


Why is it desirable to have this is a first-class field vs just calling the method on the typeconfig?

marun · 2018-09-13T16:38:06Z

pkg/controller/sync/controller.go

@@ -638,7 +643,18 @@ func (s *FederationSyncController) syncToClusters(selectedClusters, unselectedCl
 	updatedClusterVersions, operationErrors := s.updater.Update(operations)

 	defer func() {
-		err = updatePropagatedVersion(s.typeConfig, s.fedClient, updatedClusterVersions, template, override,
+		if s.collectStatus {


Status and propagated version will not be written if there are no operations (as per [1]). I'm assuming this is an oversight, since status could change even if no changes need to be propagated. Consider adding a test that validates this corner case.

1: https://github.com/kubernetes-sigs/federation-v2/pull/247/files#diff-e8c07a7dd1e59adabc5ec582f6b8fd37R638

marun · 2018-09-13T16:39:59Z

pkg/controller/sync/controller.go

@@ -638,7 +643,18 @@ func (s *FederationSyncController) syncToClusters(selectedClusters, unselectedCl
 	updatedClusterVersions, operationErrors := s.updater.Update(operations)

 	defer func() {
-		err = updatePropagatedVersion(s.typeConfig, s.fedClient, updatedClusterVersions, template, override,
+		if s.collectStatus {
+			newTemplate, err = s.UpdateFederatedStatus(template, newTemplate)


Consider using a more descriptive name than newTemplate (e.g. updatedTemplate or statusUpdatedTemplate).

shashidharatd · 2018-09-26T15:58:32Z

Closing this in favour of #291

k8s-ci-robot requested review from marun, onyiny-ang and pmorie September 4, 2018 16:43

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2018

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2018

gyliu513 reviewed Sep 10, 2018

View reviewed changes

shashidharatd force-pushed the status branch from af0c37d to 37e10e0 Compare September 10, 2018 04:25

font reviewed Sep 11, 2018

View reviewed changes

shashidharatd force-pushed the status branch from 37e10e0 to 87ab586 Compare September 11, 2018 15:56

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2018

irfanurrehman added this to the v0.0.2 milestone Sep 12, 2018

gyliu513 reviewed Sep 13, 2018

View reviewed changes

shashidharatd force-pushed the status branch from 87ab586 to b91d99c Compare September 13, 2018 16:28

marun suggested changes Sep 13, 2018

View reviewed changes

shashidharatd added 5 commits September 19, 2018 14:54

Add an option field to collect status in federated typeconfig

c653518

Add status for federated types

546f902

Updated generated code

36221a8

Update sync controller to collect status from member cluster

54db102

Add e2e test for checking federated status

fe97176

shashidharatd force-pushed the status branch from b91d99c to fe97176 Compare September 19, 2018 14:32

shashidharatd mentioned this pull request Sep 26, 2018

Generic status for federated types #291

Merged

shashidharatd closed this Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Federated Status Implementation for federated types #247

Federated Status Implementation for federated types #247

shashidharatd commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

marun commented Sep 7, 2018 •

edited

Loading

gyliu513 Sep 10, 2018

shashidharatd Sep 10, 2018

gyliu513 Sep 10, 2018

shashidharatd Sep 10, 2018

gyliu513 Sep 10, 2018

shashidharatd Sep 10, 2018

gyliu513 Sep 10, 2018

shashidharatd Sep 10, 2018

gyliu513 Sep 10, 2018

shashidharatd Sep 10, 2018

gyliu513 Sep 10, 2018

shashidharatd commented Sep 10, 2018

font Sep 11, 2018

shashidharatd Sep 11, 2018

font Sep 11, 2018

shashidharatd Sep 11, 2018

gyliu513 left a comment

marun commented Sep 13, 2018

shashidharatd commented Sep 13, 2018

marun left a comment •

edited

Loading

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

marun Sep 13, 2018

shashidharatd commented Sep 26, 2018

Federated Status Implementation for federated types #247

Federated Status Implementation for federated types #247

Conversation

shashidharatd commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

marun commented Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyliu513 left a comment

Choose a reason for hiding this comment

marun commented Sep 13, 2018

shashidharatd commented Sep 13, 2018

marun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd commented Sep 26, 2018

marun commented Sep 7, 2018 •

edited

Loading

marun left a comment •

edited

Loading