Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Federation replicaset controller #29741

Merged
merged 3 commits into from
Aug 22, 2016
Merged

Federation replicaset controller #29741

merged 3 commits into from
Aug 22, 2016

Conversation

jianhuiz
Copy link
Contributor

@jianhuiz jianhuiz commented Jul 28, 2016 •
edited
Loading

TBD: integrate with the scheduler, events

@quinton-hoole @deepak-vij @kshafiee

This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments
@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jul 28, 2016
@ghost ghost added area/cluster-federation priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jul 28, 2016
@ghost ghost assigned mwielgus and unassigned nikhiljindal Jul 28, 2016
@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch from de1e359 to 32b436a Compare July 29, 2016 00:21
@mfanjie mfanjie mentioned this pull request Jul 29, 2016
Closed
@mwielgus
Copy link
Contributor

Can we split this PR into 2 PRs:

  • uncontroversial, easy to merge api/client related PR
  • controller related stuff?

mwielgus
mwielgus reviewed Jul 29, 2016
View reviewed changes
federation/pkg/federation-controller/replicaset/replicasetcontroller.go
replicaSetController *framework.Controller
replicaSetStore cache.StoreToReplicaSetLister

replicasetQueue workqueue.DelayingInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the best structure to use. DelayingQueue pushes all updates, for example if 1000 pods changes their status we get 1000 notifications while in fact we would like to get only one after 1 min.

Copy link
Contributor Author

@jianhuiz jianhuiz Jul 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know, I'll adapt it to your deliverer once I sorted the rest code.
And the planner, too.

@jianhuiz
Copy link
Contributor Author

@mwielgus It is two PRs, I just (temporarily) merged #29241 for this to compile

@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch from 32b436a to c4ebe9b Compare August 2, 2016 18:58
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 2, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 11, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@ghost
Copy link

ghost commented Aug 11, 2016

ok to test

@ghost ghost added this to the v1.4 milestone Aug 11, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch from c4ebe9b to 66215c6 Compare August 16, 2016 08:41
@ghost
Copy link

ghost commented Aug 16, 2016

Hi @jianhuiz @mwielgus what is the status with this PR?

@jianhuiz
Copy link
Contributor Author

the controller code is reviewable, I'm working on the tests. e2e will be in separate PR.

@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch from 66215c6 to 7b9a9b8 Compare August 17, 2016 09:20
@k8s-github-robot
Copy link

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge label.

@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only labels Aug 20, 2016
@mwielgus mwielgus mentioned this pull request Aug 20, 2016
Closed
ghost
ghost reviewed Aug 20, 2016
View reviewed changes
federation/pkg/federation-controller/replicaset/replicasetcontroller.go
return nil
}

func (frsc *ReplicaSetController) reconcileNamespacesOnClusterChange() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jianhuiz @mwielgus Surely this should be reconcileReplicaSetsOnClusterChange?

@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch 2 times, most recently from 53617e6 to 50d1f0a Compare August 22, 2016 05:51
mwielgus
mwielgus reviewed Aug 22, 2016
View reviewed changes
federation/pkg/federation-controller/replicaset/replicasetcontroller.go
)
}
clusterLifecycle := fedutil.ClusterLifecycleHandlerFuncs{
ClusterAvailable: func(cluster *fedv1.Cluster) { /* no rebalancing for now */ },
Copy link
Contributor

@mwielgus mwielgus Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not? Its up to the RS configuration. We must trigger on cluster addition.

@mwielgus
Copy link
Contributor

Two comments, after fixing them LGTM.

@jianhuiz jianhuiz force-pushed the federation-replicaset-controller branch from 50d1f0a to 257bda7 Compare August 22, 2016 16:46
@jianhuiz
Copy link
Contributor Author

comments fixed

@mwielgus
Copy link
Contributor

LGTM

@mwielgus mwielgus added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 22, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 257bda7.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 257bda7.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b899059 into kubernetes:master Aug 22, 2016
This was referenced Aug 22, 2016
Closed
Closed
This was referenced Aug 30, 2016
Merged
Closed
k8s-github-robot pushed a commit that referenced this pull request Sep 6, 2016
Merge pull request #31677 from jianhuiz/federation-replicaset-control…
59a25fb 
…ler-delay-fix

Automatic merge from submit-queue

fix deliverer dealy constant usage

some delay constants were not correctly used

#29741
@quinton-hoole @deepak-vij @kshafiee @mwielgus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mwielgus mwielgus

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@jianhuiz @k8s-bot @mwielgus @k8s-github-robot @nikhiljindal @googlebot