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

Quota was not counting services with multiple nodeports properly #29457

Merged

Conversation

derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Jul 22, 2016

If a service of type node port declares multiple ports, quota on "services.nodeports" will charge for each port in the service.

Fixes #29456

/cc @kubernetes/rh-cluster-infra @sdminonne

@derekwaynecarr derekwaynecarr added this to the v1.4 milestone Jul 22, 2016
@derekwaynecarr derekwaynecarr added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 22, 2016
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 22, 2016
@derekwaynecarr derekwaynecarr added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 22, 2016
@ncdc ncdc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2016
@derekwaynecarr
Copy link
Member Author

looks like i missed a unit test, will update.

@derekwaynecarr derekwaynecarr force-pushed the service-node-port-quota-fix branch from e7b2827 to 305411b Compare July 22, 2016 17:57
@derekwaynecarr
Copy link
Member Author

Fixed other unit test.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 22, 2016
@ncdc ncdc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2016
@k8s-github-robot
Copy link

@derekwaynecarr
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2016
@ncdc
Copy link
Member

ncdc commented Jul 25, 2016

@k8s-bot e2e test this issue: #28411

@ncdc ncdc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2016
@ncdc
Copy link
Member

ncdc commented Jul 27, 2016

@k8s-bot unit test this issue: #23533

@k8s-bot
Copy link

k8s-bot commented Jul 28, 2016

GCE e2e build/test passed for commit 305411b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e008087 into kubernetes:master Jul 28, 2016
@girishkalele
Copy link

girishkalele commented Sep 7, 2016

@ncdc @derekwaynecarr @bprashanth I believe services of type LoadBalancer also allocate nodePorts.

The ServiceUsageFunc (https://github.com/kubernetes/kubernetes/blob/master/pkg/quota/evaluator/core/services.go#L72) only accounts for nodePorts when type=NodePort.

spec:
  clusterIP: 10.0.92.243
  loadBalancerSourceRanges:
  - 10.0.0.0/8
  - 133.3.3.0/24
  - 104.0.0.0/8
  ports:
  - name: distributor
    nodePort: 32175
    port: 10001
    protocol: UDP
    targetPort: 10001
  selector:
    app: distributor
  sessionAffinity: None
  type: LoadBalancer
status:
  loadBalancer: {}

@derekwaynecarr
Copy link
Member Author

@girishkalele - can you open an issue with a sample LoadBalancer service and I will provide a fix?

@girishkalele
Copy link

Thanks - filed #32221

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants