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

bugfix: nodeport quota check failure result into failing to create a clusterip service #97451

Merged
merged 1 commit into from Jan 7, 2021
Merged

bugfix: nodeport quota check failure result into failing to create a clusterip service #97451

merged 1 commit into from Jan 7, 2021

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Dec 22, 2020 •
edited

What type of PR is this?
/kind bug

What this PR does / why we need it:

apiVersion: v1
kind: ResourceQuota
metadata:
  name: default-quota
  namespace: default
spec:
  hard:
    services.nodeports: "0"

When quota is like above and there are already two nodeports service created before the quota creation.

$ k create svc clusterip cip --tcp=1234:1234
Error from server (Forbidden): services "cip" is forbidden: exceeded quota: default-quota, requested: services.nodeports=0, used: services.nodeports=2, limited: services.nodeports=0

This PR tries to fix this. As the created service is clusterip, it can be created as no quota is set for services, only for nodeport.

Which issue(s) this PR fixes:
Fixes #97437 and #73628

Special notes for your reviewer:
Cherrypicks from #97319 to fix NodePort and LB problem as well. Thanks @uablrek for that commit.

To summarize the expectations:
live = current
NP = nodeport
LB = load balancer

Service hard quota NP hard quota LB hard quota live Services count live NP count live LB count create Service (ClusterIP) create NP create LB with NP create LB with AllocateLBNodePorts=false
None 0 None 2 2 0 should success but failed expected fail expected failed should success but failed
2 None None 3 0 0 expected fail expected fail expected fail expected fail
4 2 None 4 2 0 expected fail expected fail expected fail expected fail
4 2 None 5 1 0 expected fail expected fail expected fail expected fail
4 2 None 3 2 0 expected success expected fail expected fail should success but failed
4 2 1 3 2 1 expected success expected fail expected fail expected failed

Edited after #97451 (comment) comments.

Does this PR introduce a user-facing change?:

fix counting error in service/nodeport/loadbalancer quota check

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 22, 2020
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 22, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Dec 22, 2020

/sig network
as it is related with service creations.

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Dec 22, 2020
@pacoxu pacoxu force-pushed the fix/cluster-ip-quota branch 2 times, most recently from c265664 to ca2edd3 Compare December 22, 2020 16:28
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 22, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Dec 22, 2020

/retest

@fedebongio
Copy link
Contributor

/assign @yliaog
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 22, 2020
@yliaog
Copy link
Contributor

yliaog commented Dec 22, 2020

please see discussions in the issue #73628

@pacoxu
Copy link
Member Author

pacoxu commented Dec 23, 2020 •
edited

thanks. @yliaog

However, #97319 focus on LB and NodePorts, besides, #97437 focus on nodeports quota check failure results in that clusterip service cannot be created.
What @b10s mentioned in #97437 is still a problem.
@uablrek @thockin would you look into #97437 as well ?

I think they are different indeed.

@pacoxu pacoxu changed the title bugfix: check nodeport count only if the runtime object is nodeport during quota validation bugfix: nodeport quota check failure result into failing to create a clusterip service Dec 23, 2020
@uablrek
Copy link
Contributor

uablrek commented Dec 23, 2020

@uablrek @thockin would you look into #97437 as well ?

I focused on the problem introduced by Spec.AllocateLoadBalancerNodePorts which is a different problem. I realized that was not the problem described in #73628 but I did not have time for both at the time.

#97437 is clearly a duplicate of #73628 both will be fixed by this PR.

@aojea
Copy link
Member

aojea commented Dec 23, 2020

@uablrek @thockin would you look into #97437 as well ?

I focused on the problem introduced by Spec.AllocateLoadBalancerNodePorts which is a different problem. I realized that was not the problem described in #73628 but I did not have time for both at the time.

#97437 is clearly a duplicate of #73628 both will be fixed by this PR.

However, Services LoadBalancer quota depends on NodePort quota (at least meanwhile Lars Spec.AllocateLoadBalancerNodePorts feature gate is disabled)
I think that the first line @pacoxu , Quotas (Services None, NodePort 0, LoadBalancer None) the expectaction is that LoadBalancer fails on create

@pacoxu
Copy link
Member Author

pacoxu commented Dec 24, 2020 •
edited

I think that the first line @pacoxu , Quotas (Services None, NodePort 0, LoadBalancer None) the expectaction is that LoadBalancer fails on create

@aojea I edited the table.

@uablrek I didn't realize that LB would include NodePort at the time. There is some overlap here. We can fix it in one pr or separate PRs. Either is ok.

@pacoxu
bugfix: check Spec.AllocateLoadBalancerNodePorts for nodeport and ski…
15867d9 
…p zero usage in delta evaluator

Signed-off-by: pacoxu <paco.xu@daocloud.io>

When Spec.AllocateLoadBalancerNodePorts is "false" NodePort shall
not be included when computing quota for type:LoadBalancer.

Co-authored-by: uablrek
@pacoxu
Copy link
Member Author

pacoxu commented Jan 7, 2021

/test pull-kubernetes-e2e-kind-ipv6

@liggitt
Copy link
Member

liggitt commented Jan 7, 2021

/hold cancel
/lgtm
/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 7, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit f9c7c59 into kubernetes:master Jan 7, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Jan 7, 2021
@uablrek uablrek mentioned this pull request Jan 7, 2021
Closed
@pacoxu
Copy link
Member Author

pacoxu commented Jan 7, 2021

We probably want to backport this fix (at least the zero-value portion of it). Let's get this in as-is, then follow-up with including the health check node port in quota in a separate PR

need I cherry pick it to 1.20 or other old version？ @liggitt

@liggitt
Copy link
Member

liggitt commented Jan 7, 2021 •
edited

I would recommend picking the zero portion of it to 1.17-1.20

pacoxu added a commit to pacoxu/kubernetes that referenced this pull request Jan 8, 2021
@pacoxu
cherry-pick part of kubernetes#97451: fix nodeport quota check failur…
2d0ffc1 
…e during creating clusterip

Signed-off-by: pacoxu <paco.xu@daocloud.io>
@pacoxu pacoxu mentioned this pull request Jan 8, 2021
Merged
pacoxu added a commit to pacoxu/kubernetes that referenced this pull request Jan 8, 2021
@pacoxu
cherry-pick part of kubernetes#97451: fix nodeport quota check failur…
7d11fca 
…e during creating clusterip

Signed-off-by: pacoxu <paco.xu@daocloud.io>
pacoxu added a commit to pacoxu/kubernetes that referenced this pull request Jan 8, 2021
@pacoxu
cherry-pick part of kubernetes#97451: fix nodeport quota check failur…
d72ed48 
…e during creating clusterip

Signed-off-by: pacoxu <paco.xu@daocloud.io>
This was referenced Jan 8, 2021
Merged
Merged
pacoxu added a commit to pacoxu/kubernetes that referenced this pull request Jan 8, 2021
@pacoxu
cherry-pick part of kubernetes#97451: fix nodeport quota check failur…
2c222ce 
…e during creating clusterip

Signed-off-by: pacoxu <paco.xu@daocloud.io>
@pacoxu pacoxu mentioned this pull request Jan 8, 2021
Merged
k8s-ci-robot added a commit that referenced this pull request Jan 8, 2021
@k8s-ci-robot
Merge pull request #97828 from pacoxu/automated-cherry-pick-of-#97826…
85c99cf 
…-upstream-release-1.19

cherry-pick part of #97451: fix counting error in service/nodeport/loadbalancer quota check
k8s-ci-robot added a commit that referenced this pull request Jan 8, 2021
@k8s-ci-robot
Merge pull request #97829 from pacoxu/automated-cherry-pick-of-#97826…
86f0248 
…-upstream-release-1.18

cherry-pick part of #97451: fix counting error in service/nodeport/loadbalancer quota check
k8s-ci-robot added a commit that referenced this pull request Jan 8, 2021
@k8s-ci-robot
Merge pull request #97830 from pacoxu/automated-cherry-pick-of-#97826…
6eaeb5f 
…-upstream-release-1.17

cherry-pick part of #97451: fix nodeport quota check failure during c…
k8s-ci-robot added a commit that referenced this pull request Jan 8, 2021
@k8s-ci-robot
Merge pull request #97826 from pacoxu/cherry-pick/nodeport-quota
bde7218 
cherry-pick part of #97451: fix counting error in service/nodeport/loadbalancer quota check
@github-actions github-actions bot mentioned this pull request Jan 12, 2021
Open
@pacoxu pacoxu mentioned this pull request Mar 12, 2021
Closed
@liggitt liggitt mentioned this pull request May 1, 2021
Closed
@pacoxu pacoxu deleted the fix/cluster-ip-quota branch June 23, 2021 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@thockin thockin thockin left review comments

@yliaog yliaog yliaog left review comments

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@vishh vishh Awaiting requested review from vishh

@aojea aojea Awaiting requested review from aojea

Assignees

@liggitt liggitt

@thockin thockin

@aojea aojea

@yliaog yliaog

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

Creation of cluterip fails due to quota on nodeports
8 participants
@pacoxu @fedebongio @yliaog @uablrek @aojea @liggitt @k8s-ci-robot @thockin