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

Adding nodeports services to quota #22154

Merged
merged 1 commit into from
Apr 13, 2016
Merged

Adding nodeports services to quota #22154

merged 1 commit into from
Apr 13, 2016
+177 −6

Conversation

sdminonne
Copy link
Contributor

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) 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 let us know the company's name.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 28, 2016

GCE e2e build/test passed for commit 14425ba6877fe92d9aa7204f35c7edb0a50b6bed.

@sdminonne
Copy link
Contributor Author

Need to double check my company for CLA (sorry for this don't know what is happening).

@thockin
Copy link
Member

thockin commented Feb 29, 2016

Does this cover pod HostPorts or just Service NodePorts? I think it is the latter - can you explain why the difference in policy?

@sdminonne
Copy link
Contributor Author

@thockin at the moment only NodePorts. I'm OK to cover HostsPorts either in this PR or in a follow-up, if you're OK with the approach.

@sdminonne
Copy link
Contributor Author

@thockin the same for LoadBalancer

@thockin
Copy link
Member

thockin commented Feb 29, 2016

Well, NodePorts and HostPorts are different. a HostPort is 1 node * 1
port, whereas a NodePort is num_nodes * num_ports_in_service. How should
that be counted?

On Sun, Feb 28, 2016 at 10:22 PM, Dario Minonne notifications@github.com
wrote:

@thockin https://github.com/thockin at the moment only NodePorts. I'm
OK to cover HostsPorts either in this PR or in a follow-up, if you're OK
with the approach.

—
Reply to this email directly or view it on GitHub
#22154 (comment)
.

@sdminonne
Copy link
Contributor Author

Code submitted in this PR simply counts the NodePort type services. Not the Ports.
If a quota specifies 2 NodePorts for a certain namespace the user cannot create more than 2 NodePort services. Don't know if this answer to your question. At the moment, a NodePort multi-port services count 1 in the quota system (not sure this is the right semantic).

HostPort should be similar: Just counting the container with the hostPorts spec.

I'm happy to modify this if you disagree or you've better options, but my understanding is that clearly ports need to enter in the quota mechanism somehow.

derekwaynecarr
derekwaynecarr reviewed Feb 29, 2016
View reviewed changes
pkg/api/types.go
@@ -2189,6 +2189,8 @@ const (
ResourceSecrets ResourceName = "secrets"
// ResourcePersistentVolumeClaims, number
ResourcePersistentVolumeClaims ResourceName = "persistentvolumeclaims"
// ResourceNodePorts, number
ResourceNodePorts ResourceName = "nodeports"
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making this services.nodeports instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK. with service.nodeports

@derekwaynecarr
Copy link
Member

I think we should make it clear that this is related to quota nodeports associated with a service, so I am biased to that being included in the resource name.

@sdminonne sdminonne force-pushed the service_nodeports_quotas branch from 14425ba to 0860452 Compare February 29, 2016 22:46
@k8s-bot
Copy link

k8s-bot commented Feb 29, 2016

GCE e2e build/test passed for commit 08604521e02d32d544b95a2df6c48f8ae6960930.

@sdminonne
Copy link
Contributor Author

@derekwaynecarr @thockin nodeports changed to services.nodeports (squashed but only modif)
PTAL

CLA issue is a pure technical problem. We're trying to solve it (not a lot of visibility on it)

@bgrant0607 bgrant0607 assigned thockin and unassigned bgrant0607 Mar 1, 2016
@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2016
@sdminonne sdminonne force-pushed the service_nodeports_quotas branch from 0860452 to 1cf877d Compare March 2, 2016 15:26
@sdminonne
Copy link
Contributor Author

@thockin waiting from cla, anything I need to do to improve this?

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

k8s-bot commented Mar 2, 2016

GCE e2e build/test failed for commit 1cf877d4751e94fe4a71036e0221a91c9210da21.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2016
@k8s-github-robot
Copy link

@sdminonne PR needs rebase

@sdminonne sdminonne force-pushed the service_nodeports_quotas branch from 1cf877d to 41ebce0 Compare March 7, 2016 09:24
@sdminonne
Copy link
Contributor Author

Rebased

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 5, 2016

GCE e2e build/test passed for commit 8b2701805c933e62a61cefe20a572e3f766b53c6.

@sdminonne sdminonne force-pushed the service_nodeports_quotas branch from 8b27018 to 27b038d Compare April 7, 2016 08:34
@k8s-bot
Copy link

k8s-bot commented Apr 7, 2016

GCE e2e build/test passed for commit 27b038d9adc6e69a8487b37e6db4300637d2556f.

derekwaynecarr
derekwaynecarr reviewed Apr 11, 2016
View reviewed changes
pkg/quota/evaluator/core/services.go
}

// QuotaService returns true if the service is eligible to track against a quota
func QuotaService(service *api.Service) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename this since a service is always eligible to track against a quota if the quota is just on 'services' and not 'services.nodeports'. Can you rename this QuotaServiceType returns true if the service type is eligible to track against a quota?

@derekwaynecarr
Copy link
Member

One nit, then this is LGTM

@sdminonne sdminonne force-pushed the service_nodeports_quotas branch from 27b038d to 15b7577 Compare April 12, 2016 09:09
@sdminonne
Copy link
Contributor Author

@derekwaynecarr right. Thanks

@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit 15b7577.

@sdminonne
Copy link
Contributor Author

@derekwaynecarr done (and squashed)

@derekwaynecarr derekwaynecarr added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-merge labels Apr 12, 2016
@derekwaynecarr
Copy link
Member

LGTM

@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 Apr 12, 2016

GCE e2e build/test passed for commit 15b7577.

@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 Apr 13, 2016

GCE e2e build/test passed for commit 15b7577.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8eb19c7 into kubernetes:master Apr 13, 2016
@sdminonne sdminonne deleted the service_nodeports_quotas branch April 14, 2016 07:34
@sdminonne sdminonne mentioned this pull request Apr 14, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Apr 23, 2016
Merge pull request #24247 from sdminonne/service_loadbalancer_quotas
de9c1a2 
Automatic merge from submit-queue

Adding loadBalancer services to quota system

@derekwaynecarr follow up to #22154.
@sdminonne sdminonne mentioned this pull request May 26, 2016
Closed
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Mar 13, 2019
@k8s-publishing-bot
Merge pull request kubernetes#22154 from sjenning/pick-70647-3.11
0f51d76 
UPSTREAM: 70647: Always run untag when removing docker image

Origin-commit: 8d848f9d6ecbae401811e1b1856bfb5b5073ec66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@derekwaynecarr derekwaynecarr

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

ResourceQuota should check NodePort services usage and limits
8 participants
@sdminonne @googlebot @k8s-github-robot @k8s-bot @thockin @derekwaynecarr @k8s-teamcity-mesosphere @bgrant0607