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.

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

Added rate limiting to pod deleting #6355

Merged
merged 1 commit into from Apr 3, 2015
Merged

Conversation

piosz
Copy link
Member

@piosz piosz commented Apr 2, 2015

Fixes #6228

@piosz
Copy link
Member Author

piosz commented Apr 2, 2015

RFC @davidopp @gmarek @ddysher

@@ -177,7 +177,7 @@ func (s *CMServer) Run(_ []string) error {
}

nodeController := nodeControllerPkg.NewNodeController(cloud, s.MinionRegexp, s.MachineList, nodeResources,
kubeClient, kubeletClient, s.RegisterRetryCount, s.PodEvictionTimeout)
kubeClient, kubeletClient, s.RegisterRetryCount, s.PodEvictionTimeout, util.NewTokenBucketRateLimiter(0.01, 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make them flags here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@brendandburns
Copy link
Contributor

LGTM, modulo the flags comment. If you get to it tonight, that's great. Otherwise, I'll merge later today and we can handle the flags as a follow on PR.

@brendandburns brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2015
@brendandburns brendandburns self-assigned this Apr 2, 2015
@piosz
Copy link
Member Author

piosz commented Apr 2, 2015

I'll do in 3 hours.

@davidopp
Copy link
Member

davidopp commented Apr 2, 2015

Can you please wait to merge until I've taken a look? I'll do it today.

@piosz
Copy link
Member Author

piosz commented Apr 2, 2015

Could you please wait for David?

@piosz piosz changed the title WIP: Added rate limiting to pod deleting Added rate limiting to pod deleting Apr 2, 2015
@brendandburns
Copy link
Contributor

yes, will wait.

--brendan

@@ -104,6 +106,8 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet) {
fs.DurationVar(&s.ResourceQuotaSyncPeriod, "resource_quota_sync_period", s.ResourceQuotaSyncPeriod, "The period for syncing quota usage status in the system")
fs.DurationVar(&s.NamespaceSyncPeriod, "namespace_sync_period", s.NamespaceSyncPeriod, "The period for syncing namespace life-cycle updates")
fs.DurationVar(&s.PodEvictionTimeout, "pod_eviction_timeout", s.PodEvictionTimeout, "The grace peroid for deleting pods on failed nodes.")
fs.Float32Var(&s.DeletingPodsQps, "deleting_pods_qps", 0.1, "Number of nodes per second on which pods are deleted in case of node failure.")
fs.IntVar(&s.DeletingPodsBurst, "deleting_pods_burst", 10, "Number of nodes on which pods are bursty deleted in case of node failure.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe define what "bursty deleted" means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added note pointing to RateLimiter.

@davidopp
Copy link
Member

davidopp commented Apr 3, 2015

LGTM

@piosz
Copy link
Member Author

piosz commented Apr 3, 2015

Rebased.

@gmarek
Copy link
Contributor

gmarek commented Apr 3, 2015

Travis failure is a known race for which fix required this PR.

Thanks, merging.

gmarek added a commit that referenced this pull request Apr 3, 2015
Added rate limiting to pod deleting in NodeController.
@gmarek gmarek merged commit cec84f5 into kubernetes:master Apr 3, 2015
@piosz piosz deleted the rate_limiting branch April 29, 2015 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
5 participants