[Federation] Convert the ReplicaSet controller to a sync controller. #46527

perotinus · 2017-05-26T19:25:14Z

See #45563 for previous discussion: that PR was split into two, with this one containing the actual conversion work and that one containing the implementation of the scheduling methods in the sync controller.

Release note:

NONE

k8s-reviewable · 2017-05-30T15:35:17Z

This change is

perotinus · 2017-05-30T21:43:31Z

@k8s-bot test this

k8s-ci-robot · 2017-05-31T00:06:52Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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-ci-robot · 2017-05-31T00:06:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

perotinus · 2017-05-31T01:25:07Z

The CLA should already be signed.

perotinus · 2017-05-31T01:28:09Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

perotinus · 2017-05-31T01:29:29Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

marun

Hmm, neglected to publish this previously...

Would be good to see #45563 merge before reviewing in-depth. My quibbles about testing aside, though, I think this is ok so long as the integration and e2e tests pass.

marun · 2017-05-30T15:52:22Z

federation/pkg/federation-controller/sync/replicasetcontroller_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package replicaset
+package sync


Is this coverage still required?

I'm not quite sure. I think it is testing a bit more than the current integration tests–namely, that the scheduling causes the replicas to be distributed to clusters such that the numbers add up–but it's certainly a test that would be better removed in the future and replaced with unit tests for scheduling and integration tests for propagation.

I'd hope the numbers adding up could be validated separately from propagation, but sure.

marun · 2017-05-30T16:01:42Z

federation/pkg/federatedtypes/replicaset_test.go

+		cluster1UnschedulableReplicas *int64
+		cluster2UnschedulableReplicas *int64
+	}{
+		{rs1Replicas: 2, rs2Replicas: 2, rs1ReadyReplicas: 2, rs2ReadyReplicas: 2, podsGetter: uncalledPodsGetter, cluster1Replicas: &two, cluster2Replicas: &two},


Consider adding test names to document your intent. Also, consider defining only the values that vary to make it clear what is being tested.

marun · 2017-05-30T16:02:31Z

federation/pkg/federatedtypes/replicaset_test.go

+	k8s2        = "k8s-2"
+)
+
+func TestClusterReplicaState(t *testing.T) {


This test seems to be doing a lot of work. Is there a way to break the code under test down so it can be tested in isolation?

Hmm. I thought about it for a little while, but I'm concerned that I couldn't think of a way to do so without making this more convoluted. I'll think about this a bit more, perhaps in the context of the deployments PR that follows this one up.

marun · 2017-05-30T16:04:10Z

federation/pkg/federatedtypes/replicaset.go

+
+func (a *ReplicaSetAdapter) schedule(frs *extensionsv1.ReplicaSet, clusterNames []string,
+	current map[string]int64, estimatedCapacity map[string]int64) map[string]int64 {
+	// TODO: integrate real scheduler


Is this a TODO from the previous code or was there something lost in translation?

Yep, this is a TODO from the previous code.

marun · 2017-05-30T16:04:53Z

federation/pkg/federatedtypes/replicaset.go

+	replicas := int64(*frs.Spec.Replicas)
+	scheduleResult, overflow := plnr.Plan(replicas, clusterNames, current, estimatedCapacity,
+		frs.Namespace+"/"+frs.Name)
+	// make sure the return contains clusters need to zero the replicas


Consider rewording for clarity.

perotinus · 2017-05-31T22:47:20Z

@k8s-bot pull-kubernetes-unit test this

perotinus · 2017-05-31T22:47:25Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

perotinus · 2017-06-01T00:05:40Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

perotinus · 2017-06-01T00:05:45Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

perotinus · 2017-06-01T00:05:49Z

@k8s-bot pull-kubernetes-unit test this

perotinus · 2017-06-01T06:15:37Z

@k8s-bot pull-kubernetes-unit test this

perotinus · 2017-06-03T00:32:26Z

@k8s-bot pull-kubernetes-unit test this

perotinus · 2017-06-08T20:32:27Z

@marun Can you LGTM this? (after the tests pass, assuming they do)

perotinus · 2017-06-08T20:48:49Z

Related issue: #39986

perotinus · 2017-06-08T20:49:11Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

perotinus · 2017-06-09T01:14:06Z

Federation E2E tests passed: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/46527/pull-kubernetes-federation-e2e-gce/9670/

marun · 2017-06-14T16:49:57Z

/lgtm

marun · 2017-06-21T23:08:40Z

/retest

marun · 2017-06-23T02:01:30Z

/lgtm

k8s-github-robot · 2017-06-23T02:01:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csbell, marun, perotinus

Associated issue: 45563

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

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

~~federation/OWNERS~~ [csbell]
~~test/e2e_federation/OWNERS~~ [csbell,marun]

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

k8s-github-robot · 2017-06-23T09:59:28Z

Automatic merge from submit-queue (batch tested with PRs 47403, 46646, 46906, 46527, 46792)

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

k8s-github-robot assigned ghost and madhusudancs May 26, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 26, 2017

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

perotinus mentioned this pull request May 30, 2017

[Federation] Add a SchedulingAdapter that can extend the FederatedTypeAdapter and that provides hooks for scheduling objects into clusters. #45563

Merged

perotinus force-pushed the rssyncconversiontwo branch from a3210d1 to bab423e Compare May 30, 2017 21:43

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

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 31, 2017

marun mentioned this pull request May 31, 2017

Review each commit separately doesn't always correspond to PR commits Reviewable/Reviewable#479

Closed

marun reviewed May 31, 2017

View reviewed changes

perotinus force-pushed the rssyncconversiontwo branch from bab423e to b7504c7 Compare May 31, 2017 20:07

perotinus force-pushed the rssyncconversiontwo branch from b7504c7 to ef7a9fe Compare May 31, 2017 23:00

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

marun added the sig/federation label Jun 8, 2017

perotinus force-pushed the rssyncconversiontwo branch from 32aa318 to 060c692 Compare June 8, 2017 20:28

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

csbell added the approved-for-milestone label Jun 9, 2017

csbell modified the milestones: v1.7, next-candidate Jun 12, 2017

k8s-ci-robot assigned marun Jun 14, 2017

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

marun mentioned this pull request Jun 22, 2017

[Federation] Update namespace support to use the sync controller #47890

Merged

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

[Federation] Convert the ReplicaSet controller to a sync controller.

16943f6

perotinus force-pushed the rssyncconversiontwo branch from 060c692 to 16943f6 Compare June 23, 2017 00:48

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 23, 2017

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

k8s-github-robot merged commit d7c569e into kubernetes:master Jun 23, 2017

perotinus deleted the rssyncconversiontwo branch June 23, 2017 18:46

[Federation] Convert the ReplicaSet controller to a sync controller. #46527

[Federation] Convert the ReplicaSet controller to a sync controller. #46527

Conversation

perotinus commented May 26, 2017 • edited

k8s-reviewable commented May 30, 2017

perotinus commented May 30, 2017

k8s-ci-robot commented May 31, 2017

k8s-ci-robot commented May 31, 2017

perotinus commented May 31, 2017

perotinus commented May 31, 2017

perotinus commented May 31, 2017

marun left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perotinus commented May 31, 2017

perotinus commented May 31, 2017

perotinus commented Jun 1, 2017

perotinus commented Jun 1, 2017

perotinus commented Jun 1, 2017

perotinus commented Jun 1, 2017

perotinus commented Jun 3, 2017

perotinus commented Jun 8, 2017 • edited

perotinus commented Jun 8, 2017

perotinus commented Jun 8, 2017

perotinus commented Jun 9, 2017

marun commented Jun 14, 2017

marun commented Jun 21, 2017

marun commented Jun 23, 2017

k8s-github-robot commented Jun 23, 2017

k8s-github-robot commented Jun 23, 2017

perotinus commented May 26, 2017 •

edited

perotinus commented Jun 8, 2017 •

edited