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

kube-controller-manager: fix description of service-cluster-ip-range option #91511

Closed
wants to merge 1 commit into from
Closed

kube-controller-manager: fix description of service-cluster-ip-range option #91511

wants to merge 1 commit into from

Conversation

ymmt2005
Copy link
Contributor

@ymmt2005 ymmt2005 commented May 27, 2020 •
edited

What type of PR is this?

/kind documentation

What this PR does / why we need it:

The description of kube-controller-manager's --service-cluster-ip-range option said that
it requires --allocate-node-cidrs=true, but this is not true.

In fact, --allocate-node-cidrs are meant to be used with --cluster-cidr.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

None

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 27, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @ymmt2005. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 27, 2020
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 May 27, 2020
@ymmt2005
Copy link
Contributor Author

/assign @lavalamp

@ymmt2005 ymmt2005 mentioned this pull request May 27, 2020
Closed
@lavalamp
Copy link
Member

/assign @cheftako

@sftim
Copy link
Contributor

sftim commented May 28, 2020

/sig docs

@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label May 28, 2020
@sftim
Copy link
Contributor

sftim commented May 28, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 28, 2020
@ymmt2005
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@cheftako
Copy link
Member

/cc @jiahuif

@k8s-ci-robot k8s-ci-robot requested a review from jiahuif May 28, 2020 20:08
@ymmt2005
Copy link
Contributor Author

ymmt2005 commented Jun 1, 2020

@jiahuif @feiskyer @eparis
Would you mind take a look?

@jiahuif
Copy link
Member

jiahuif commented Jun 1, 2020

Could you give us an example where --allocate-node-cidrs=false but --cluster-cidr still makes sense? Thank you.

@ymmt2005
Copy link
Contributor Author

ymmt2005 commented Jun 1, 2020

Could you give us an example where --allocate-node-cidrs=false but --cluster-cidr still makes sense?

I don't think --cluster-cidr makes sense w/o --allocate-node-cidrs=true.

@ymmt2005
Copy link
Contributor Author

Is there anything else I should do?

@cheftako
Copy link
Member

Could you give us an example where --allocate-node-cidrs=false but --cluster-cidr still makes sense?

I don't think --cluster-cidr makes sense w/o --allocate-node-cidrs=true.

kubernetes/cmd/kube-controller-manager/app/core.go

Line 101 in 3b1b2d4

if !ctx.ComponentConfig.KubeCloudShared.AllocateNodeCIDRs {
suggests that for several configurations if allocate-node-cidrs is set to false, then cluster-cidrs will be ignored. I think we either need to keep this warning or we need to improve our validation code.

@ymmt2005
Copy link
Contributor Author

@cheftako @jiahuif
This PR does not say anything about cluster-cidrs.
Don't be confused.

This only fixes the wrong description for service-cluster-ip-range option.
The warning is really a lie.

@cheftako
Copy link
Member

Walter Fender Jiahui Feng
This PR does not say anything about cluster-cidrs.
Don't be confused.

This only fixes the wrong description for service-cluster-ip-range option.
The warning is really a lie.

Sorry I should have written service cidr (which is the field service-cluster-ip-range populates).
My point however holds. If allocate-node-cidrs is set to false node ipam controller will be skipped.
If its skipped then essentially the service-cluster-ip-range field is ignored. (At least for KCM only cases)
So for KCM only cases, if allocate-node-cidrs is set to false, then service-cluster-ip-range will be ignored

@ymmt2005
Copy link
Contributor Author

ymmt2005 commented Jun 18, 2020 •
edited

OK, let me sort things.

  • service-cluster-ip-range for kube-apiserver is used to allocate ClusterIP to Services.
  • service-cluster-ip-range for kube-controller-manager is only used to exclude addresses in the specified range from Pod addresses allocated from cluster-cidr.

So, there is no need to give serivce-cluster-ip-range for KCM if we don't give cluster-cidr (and allocate-node-cidrs).

If I understand correctly, the proper fix would be as follows?

- CIDR Range for Services in cluster. Requires --allocate-node-cidrs to be true
+ CIDR Range for Services in cluster. Meaningful only when --allocate-node-cidrs is true

@cheftako
Copy link
Member

If I understand correctly, the proper fix would be as follows?

- CIDR Range for Services in cluster. Requires --allocate-node-cidrs to be true
+ CIDR Range for Services in cluster. Meaningful only when --allocate-node-cidrs is true

That sounds correct. By the way thank you for the help and putting up with the multiple round trips.

@ymmt2005
kube-controller-manager: fix description of service-cluster-ip-range …
a461903 
…option

--service-cluster-ip-range is used only when --allocate-node-cidrs is true
to exclude IP ranges for ClusterIP from IP addresses assigned to Pods.

This commit corrects and clarifies the option's description.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ymmt2005
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ymmt2005
Copy link
Contributor Author

@cheftako
Thank you! I force-pushed the amended commit.

@ymmt2005
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-ipv6

1 similar comment
@ymmt2005
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-ipv6

@aojea
Copy link
Member

aojea commented Jun 20, 2020

/retest

@ymmt2005
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-ipv6

@aojea
Copy link
Member

aojea commented Jun 20, 2020

@ymmt2005 there is a problem with this pull-kubernetes-e2e-kind-ipv6 job right now I'm investigation, no need to retrigger and is not caused by this PR, so feel free to dismiss its result

@ymmt2005
Copy link
Contributor Author

Can anyone take a look, please?

@cheftako
Copy link
Member

/test pull-kubernetes-e2e-kind-ipv6

@cheftako
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2020
@ymmt2005
Copy link
Contributor Author

Thank you very much Walter!

@lavalamp Could you approve this?

@nikhita
Copy link
Member

nikhita commented Jul 9, 2020

/assign @mikedanese
for approval

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 7, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 6, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eparis eparis Awaiting requested review from eparis

@feiskyer feiskyer Awaiting requested review from feiskyer

@jiahuif jiahuif Awaiting requested review from jiahuif

Assignees

@lavalamp lavalamp

@cheftako cheftako

@mikedanese mikedanese

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@ymmt2005 @k8s-ci-robot @lavalamp @sftim @cheftako @jiahuif @aojea @nikhita @fejta-bot @mikedanese