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

Resource quota observes deletes faster #17133

Merged
merged 1 commit into from
Dec 6, 2015
Merged

Resource quota observes deletes faster #17133

merged 1 commit into from
Dec 6, 2015

Conversation

derekwaynecarr
Copy link
Member

User's have complained that the quota system takes too long to observe deletes.

This can cause excessive delays in new work being allowed to proceed.

Related to:
#17098

Major changes:

  1. Quota controller now watches for quota creations, and quota.spec changes
  2. Quota controller now watches for pod deletions in order to record quota being freed.
  3. Quota controller full resync period goes from 10s to 5m
  4. Quota controller can run N workers to work through queue

Net:

  1. If I add a quota, my current quota usage is calculated ASAP
  2. If I edit my quota spec, the quota status is calculated ASAP
  3. If I delete a pod, usage for cpu/mem/pod counts are fixed up ASAP
  4. If I delete any other resource (service, replication controller, etc.), object count quota for those changes may not observe the delete for up to 5 minutes.

@derekwaynecarr
Copy link
Member Author

Marked WIP while I fix unit tests.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2015
@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch 2 times, most recently from b837854 to 1a2212e Compare November 11, 2015 21:58
@derekwaynecarr derekwaynecarr changed the title WIP: Resource quota observes deletes faster Resource quota observes deletes faster Nov 11, 2015
@derekwaynecarr
Copy link
Member Author

/cc @smarterclayton @erictune @wojtek-t - if any of you have time to review, this is a big usability improvement for quota.

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit 03a7b8676a8d9e11af4e5a38016292b9e1a6080c.

@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from 1a2212e to bf7e2de Compare November 11, 2015 22:17
@derekwaynecarr
Copy link
Member Author

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit b837854010d49e981fa65d639dd0b8a904515fd9.

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit 1a2212ec9ad312ce2b06014fea856be73d6be0b8.

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit bf7e2de6260b7d87a2747bc869adbdd4fa813c2e.

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit bf7e2de6260b7d87a2747bc869adbdd4fa813c2e.

@derekwaynecarr
Copy link
Member Author

@k8s-bot tes this

@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from bf7e2de to 91f0cf4 Compare November 12, 2015 17:59
@k8s-bot
Copy link

k8s-bot commented Nov 12, 2015

GCE e2e build/test failed for commit 91f0cf4ace98ce1ee919ef56f2f1b0ff520fb9f4.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2015
@derekwaynecarr derekwaynecarr mentioned this pull request Nov 13, 2015
Closed
@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from 91f0cf4 to 8fc9939 Compare November 13, 2015 20:03
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit 8fc993902a25e45758abe3b45fb884063a63411f.

@smarterclayton
Copy link
Contributor

Exciting!

@derekwaynecarr
Copy link
Member Author

@smarterclayton - hope "Exciting!" implied good for users, and not wow that is a lot of failures.

Looks like an issue with doc munger, will update Monday.

@derekwaynecarr derekwaynecarr mentioned this pull request Nov 14, 2015
Closed
@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from 8fc9939 to ebaeb32 Compare November 16, 2015 15:03
@k8s-bot
Copy link

k8s-bot commented Nov 16, 2015

GCE e2e build/test failed for commit ebaeb327cc3da626f8e2b32449fa421ca4cb6111.

@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from ebaeb32 to 1592b31 Compare November 16, 2015 18:39
@derekwaynecarr
Copy link
Member Author

@eparis @smarterclayton - any chance you want to tag this prior to gobbling on turkey this week?

eparis
eparis reviewed Nov 24, 2015
View reviewed changes
cmd/kube-controller-manager/app/controllermanager.go
ServiceSyncPeriod: 5 * time.Minute,
NodeSyncPeriod: 10 * time.Second,
ResourceQuotaSyncPeriod: 10 * time.Second,
ResourceQuotaSyncPeriod: 10 * time.Minute,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what you were saying is 5min in the summary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Obviously the summary and the code got out of sync.

I will drop this down to 5 minutes because that feels appropriate to me.

@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from d75bd57 to dafa488 Compare November 30, 2015 16:56
@derekwaynecarr
Copy link
Member Author

@eparis - updated to 5 minutes per summary.

@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e test build/test passed for commit dafa4885113091846e3638eab2ff0dd8333cc7b1.

@derekwaynecarr
Copy link
Member Author

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e test build/test passed for commit dafa4885113091846e3638eab2ff0dd8333cc7b1.

@derekwaynecarr
Copy link
Member Author

@eparis @smarterclayton - nag to get a review :-)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2015
@eparis
Copy link
Contributor

eparis commented Dec 2, 2015

hahaha, and then to rebase bot comes along and screws you.

@derekwaynecarr derekwaynecarr force-pushed the quota_controller_improvements branch from dafa488 to 55d4f70 Compare December 3, 2015 22:35
@derekwaynecarr
Copy link
Member Author

@eparis - if you have time to review before i get bit by rebase bug again.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 55d4f70.

@eparis eparis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2015
@eparis eparis closed this Dec 4, 2015
@eparis eparis reopened this Dec 4, 2015
@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 Dec 4, 2015

GCE e2e build/test failed for commit 55d4f70.

@derekwaynecarr
Copy link
Member Author

@k8s-bot - test this

@gmarek
Copy link
Contributor

gmarek commented Dec 6, 2015

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Dec 6, 2015

GCE e2e test build/test passed for commit 55d4f70.

@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 Dec 6, 2015

GCE e2e test build/test passed for commit 55d4f70.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 6, 2015
Merge pull request #17133 from derekwaynecarr/quota_controller_improv…
db11f1b 
…ements

Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit db11f1b into kubernetes:master Dec 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eparis eparis

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@derekwaynecarr @k8s-github-robot @k8s-bot @smarterclayton @ixdy @eparis @gmarek @k8s-teamcity-mesosphere @googlebot