[Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. #42683

perotinus · 2017-03-07T23:01:21Z

Address part of #42324.

Adds annotations to all Federation objects created by kubefed.

k8s-ci-robot · 2017-03-07T23:01:22Z

Hi @perotinus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-03-07T23:01:27Z

This change is

nikhiljindal · 2017-04-03T23:00:01Z

pkg/kubectl/cluster.go

@@ -30,6 +31,8 @@ import (
 type ClusterGeneratorV1Beta1 struct {
 	// Name of the cluster context (required)
 	Name string
+	// Name of the federation this cluster is joining (required)


Adding a new required param breaks existing scripts. Is this really required?

I dont see any value of adding this annotation on cluster objects. Running kubectl get clusters <cluster-name> -o yaml will not give me a reusable yaml file that I can use with another federation.

IIUC the original issue was because we store some federation objects in host cluster and since the host cluster can be hosting multiple federation objects it makes sense to add these annotation there.
Cluster objects are already stored in federation control plane, so skip the annotation for clusters?

Hmm. Do you mean that you can't use an old version of kubefed with a newer federation? Or vice versa? Can you give an example of what would fail that we need to continue supporting?

On a side note, do we have a grid for which versions of kubefed, the FCP, the host cluster Kubernetes version and the joined cluster versions we need to support, in new federation and upgrade cases? I know that we want to maintain backwards compatibility, but I'm not sure I really understand what sort of compatibility we're trying to maintain.

Adding the annotation does give you the ability to get all objects related to a particular federation, rather than having to get all the annotated objects and then the cluster objects separately.

That all said, I'm happy to remove the cluster annotations. I just want to make sure I understand the benefit.

On a side note, do we have a grid for which versions of kubefed, the FCP, the host cluster Kubernetes version and the joined cluster versions we need to support, in new federation and upgrade cases? I know that we want to maintain backwards compatibility, but I'm not sure I really understand what sort of compatibility we're trying to maintain.

We have neither concretely defined nor documented that right now. I guess that will be one of the important work items in taking federation to GA.

But having said that, I think @nikhiljindal's comment holds. I did not think about that before.

Adding the annotation does give you the ability to get all objects related to a particular federation, rather than having to get all the annotated objects and then the cluster objects separately.

You need to do that anyway. You need to get all the annotated objects from the host cluster and then separately get the cluster objects from the federation API server irrespective of the annotations. Even if your objects are annotated, I believe you still need to get each type of object separately and filter for objects that contain the annotation you care about. Our API machinery does not allow getting objects with a given annotation across different API types.

@nikhiljindal, having said all that, I don't think adding the annotation here is that dangerous. This required param is an internal detail and @perotinus is making it mandatory to establish the contract between the provider and the client. The client here is the kubefed join implementation. Users who run the kubefed join command don't have to care about this. So it is not going to break them. It is just that the implementation of kubefed join should provide these annotation values which it does.

To clarify, the code here is kubectl and not kubefed. We have defined policies for kubectl version skew support that we cant break.

Ah, I see. kubectl vs kubefed makes a big difference.

@nikhiljindal @madhusudancs Are we agreed on removing the annotation from the cluster objects, and so reverting the changes to the ClusterGenerator?

Ah, I see. kubectl vs kubefed makes a big difference.

FWIW, we do need to be strict about not breaking kubefed as well (now that its beta)

@nikhiljindal @madhusudancs Are we agreed on removing the annotation from the cluster objects, and so reverting the changes to the ClusterGenerator?

Yes I dont see much advantage of adding the annotation but to be clear, reverting the change in ClusterGenerator and adding annotation to cluster object when its created via kubefed are different independent points. kubefed should not be using ClusterGenerator. Its providing the yaml I believe.

kubefed should not be using ClusterGenerator? That seems like a separate issue. Is it being tracked somewhere?

I'll revert all of the code that adds the annotation to the cluster objects later today unless someone objects. We can always add it back later.

OK, code removed.

Just to close this discussion, I am summarizing our offline discussion here:

To clarify, the code here is kubectl and not kubefed. We have defined policies for kubectl version skew support that we cant break.

ClusterGenerator is not kubectl code, it is kubefed code in the sense that kubefed is the only client. It sits in kubectl package to reuse some of the machinery there.

FWIW, we do need to be strict about not breaking kubefed as well (now that its beta)

Agreed. But this doesn't break kubefed too. It is just changing the internal detail.

kubefed should not be using ClusterGenerator. Its providing the yaml I believe.

kubefed is using ClusterGenerator. In fact, it is the only consumer of that interface. It uses the kubectl generator machinery to simplify a number of other things (read: reinvent the wheel).

But we decided to remove the annotation from cluster object because it is unnecessary code.

nikhiljindal · 2017-04-03T23:00:57Z

@k8s-bot ok to test

lgtm except adding the annotations to cluster objects and specifically breaking kubectl create clusters.

perotinus · 2017-04-07T18:33:05Z

@madhusudancs @nikhiljindal This should be good to review now; I've removed all of the code that added the annotations to the cluster object.

perotinus · 2017-04-07T19:36:57Z

Hmm. That looks like a flaky test. Can someone rerun the tests?

madhusudancs · 2017-04-07T19:38:34Z

@k8s-bot non-cri e2e test this

…deration name and (if applicable) the cluster name.

perotinus · 2017-04-10T23:41:01Z

@madhusudancs @nikhiljindal Can someone give this another review pass?

nikhiljindal · 2017-04-10T23:43:54Z

/lgtm

k8s-github-robot · 2017-04-10T23:43:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhiljindal, perotinus

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [nikhiljindal]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

madhusudancs · 2017-04-11T00:30:20Z

@perotinus I have a few minor nits. Since this PR is already LGTM'ed and approved, please feel free to make address those comments in a separate PR.

Reviewed 2 of 12 files at r1, 7 of 10 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

federation/apis/federation/annotations.go, line 2 at r2 (raw file):

/*
Copyright 2016 The Kubernetes Authors.

federation/apis/federation/annotations.go, line 20 at r2 (raw file):

// FederationNameAnnotation is the annotation which holds the name of
// the federation that an object is associated with. It must be

This comment needs some clarification.

"... that a federation control plane component is associated with ..."

federation/apis/federation/annotations.go, line 21 at r2 (raw file):

// FederationNameAnnotation is the annotation which holds the name of
// the federation that an object is associated with. It must be
// applied to all API objects associated with that federation.

"It must be applied to all the API types that represent that federation control plane components in the host cluster" or something like that.

Comments from Reviewable

k8s-github-robot · 2017-04-11T00:54:26Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-04-11T01:42:00Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 44607, 44472, 44482) [Federation] Fix the comments on FederationNameAnnotation Follow-up for minor issues raised in #42683 ```release-note NONE ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 7, 2017

k8s-github-robot assigned nikhiljindal Mar 7, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Mar 7, 2017

perotinus force-pushed the annotations branch from 87fc908 to 4f2b6cd Compare March 8, 2017 01:35

perotinus changed the title ~~WIP for annotations for all objects.~~ [Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. Mar 8, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 8, 2017

perotinus force-pushed the annotations branch 4 times, most recently from 1644d67 to 08ef331 Compare March 8, 2017 01:55

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2017

nikhiljindal reviewed Apr 3, 2017

View reviewed changes

grodrigues3 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 5, 2017

perotinus force-pushed the annotations branch from 08ef331 to 16a6c8f Compare April 6, 2017 21:08

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2017

perotinus force-pushed the annotations branch from 16a6c8f to ebf7d2a Compare April 7, 2017 18:29

perotinus force-pushed the annotations branch from ebf7d2a to c65b695 Compare April 10, 2017 23:36

[Federation][kubefed] Annotate all Federation API objects with the fe…

85b4313

…deration name and (if applicable) the cluster name.

perotinus force-pushed the annotations branch from c65b695 to 85b4313 Compare April 10, 2017 23:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2017

k8s-github-robot merged commit 3c7616e into kubernetes:master Apr 11, 2017

perotinus mentioned this pull request Apr 13, 2017

[Federation] Fix the comments on FederationNameAnnotation #44472

Merged

perotinus deleted the annotations branch May 10, 2017 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. #42683

[Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. #42683

perotinus commented Mar 7, 2017 •

edited

k8s-ci-robot commented Mar 7, 2017

k8s-reviewable commented Mar 7, 2017

nikhiljindal Apr 3, 2017

perotinus Apr 6, 2017

madhusudancs Apr 7, 2017

nikhiljindal Apr 7, 2017

perotinus Apr 7, 2017

nikhiljindal Apr 7, 2017

perotinus Apr 7, 2017

perotinus Apr 7, 2017

madhusudancs Apr 7, 2017 •

edited

nikhiljindal commented Apr 3, 2017

perotinus commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 10, 2017

nikhiljindal commented Apr 10, 2017

k8s-github-robot commented Apr 10, 2017

madhusudancs commented Apr 11, 2017

k8s-github-robot commented Apr 11, 2017

k8s-github-robot commented Apr 11, 2017

[Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. #42683

[Federation][kubefed] Annotate all Federation API objects with the federation name and (if applicable) the cluster name. #42683

Conversation

perotinus commented Mar 7, 2017 • edited

k8s-ci-robot commented Mar 7, 2017

k8s-reviewable commented Mar 7, 2017

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

madhusudancs Apr 7, 2017 • edited

Choose a reason for hiding this comment

nikhiljindal commented Apr 3, 2017

perotinus commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 10, 2017

nikhiljindal commented Apr 10, 2017

k8s-github-robot commented Apr 10, 2017

madhusudancs commented Apr 11, 2017

k8s-github-robot commented Apr 11, 2017

k8s-github-robot commented Apr 11, 2017

perotinus commented Mar 7, 2017 •

edited

madhusudancs Apr 7, 2017 •

edited