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

graduate ServiceIPStaticSubrange to GA #37432

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

aojea
Copy link
Member

@aojea aojea commented Oct 21, 2022

The ServiceIPStaticSubrange feature gate, that allows to change the IP allocation algorithm used for Service.ClusterIPs, is graduating to GA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2022
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language 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. labels Oct 21, 2022
@aojea
Copy link
Member Author

aojea commented Oct 21, 2022

/assign @sftim @thockin

@netlify
Copy link

netlify bot commented Oct 21, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 56cd998
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/63529f7de2fa400008b39be7
😎 Deploy Preview https://deploy-preview-37432--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2022
@sftim
Copy link
Contributor

sftim commented Oct 21, 2022

Please target this change to the dev-1.26 branch @aojea

/hold
OK to unhold once the target branch is right

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2022
@aojea aojea changed the base branch from main to dev-1.26 October 22, 2022 03:20
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 22, 2022
@aojea aojea force-pushed the static_service_ga branch from 56cd998 to 7fa9682 Compare October 22, 2022 07:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2022
@k8s-ci-robot k8s-ci-robot requested review from sftim and thockin October 22, 2022 07:43
@netlify
Copy link

netlify bot commented Oct 22, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit e8199cf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/638624a5e3dc680008b729cc

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 22, 2022
@aojea aojea force-pushed the static_service_ga branch from 7fa9682 to 6336cb9 Compare October 22, 2022 07:45
@aojea
Copy link
Member Author

aojea commented Oct 22, 2022

@sftim updated the PR against 1.26

@jihoon-seo
Copy link
Member

/unhold
since:

OK to unhold once the target branch is right

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2022
@aojea
Copy link
Member Author

aojea commented Oct 26, 2022

@thockin @sftim the lgtm and approve tags were dropped after the rebase, can you please take a look again

@sftim
Copy link
Contributor

sftim commented Nov 13, 2022

What I don't get from these docs is: can you have more than 256 Services in your cluster? It's not obvious how the whole available range eventually gets used.

@aojea
Copy link
Member Author

aojea commented Nov 14, 2022

What I don't get from these docs is: can you have more than 256 Services in your cluster? It's not obvious how the whole available range eventually gets used.

ups, no, the number of Services is not modified here, this makes a virtual division to not allocate dynamic addresses from this lower range, the blog explains it in more detail https://kubernetes.io/blog/2022/05/23/service-ip-dynamic-and-static-allocation/

@sftim
Copy link
Contributor

sftim commented Nov 14, 2022

the blog explains it in more detail https://kubernetes.io/blog/2022/05/23/service-ip-dynamic-and-static-allocation/

I'd like to be able to work out the way it works just from the docs - and I want that for readers in general too. For GA, we prefer to have docs that readers can understand without also following links to the KEP, post-release writeup, etc.

@sftim
Copy link
Contributor

sftim commented Nov 14, 2022

It's OK to copy from that blog article, as much as we'd like to. It's also reasonable to change that blog article to be evergreen: written so that the text doesn't turn stale, and marked as such. Only some minor tweaks would be needed there, and then we can link to it to provide the background detail (we can actually shorten the concept page if we do that).

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 17, 2022
@aojea
Copy link
Member Author

aojea commented Nov 18, 2022

ack, let me work on this

@aojea aojea force-pushed the static_service_ga branch from b620995 to 6f7421b Compare November 18, 2022 08:40
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/blog Issues or PRs related to the Kubernetes Blog subproject and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 18, 2022
@aojea
Copy link
Member Author

aojea commented Nov 18, 2022

It's OK to copy from that blog article, as much as we'd like to. It's also reasonable to change that blog article to be evergreen: written so that the text doesn't turn stale, and marked as such. Only some minor tweaks would be needed there, and then we can link to it to provide the background detail (we can actually shorten the concept page if we do that).

@sftim I've tried to follow your suggestions but I need you to review because I'm not sure if that is what you meant

@aojea
Copy link
Member Author

aojea commented Nov 26, 2022

ping @sftim

Comment on lines 89 to 95
#### Service IP CIDR block: 10.96.0.0/24

Range Size: 2<sup>8</sup> - 2 = 254
Band Offset: `min(max(16, 256/16), 256)` = `min(16, 256)` = 16
Static band start: 10.96.0.1
Static band end: 10.96.0.16
Range end: 10.96.0.254
Copy link
Contributor

Choose a reason for hiding this comment

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

Two problems here:

  • We usually don't have H4 for headings, and in this context, we are skipping H3?
  • The text lines above would be converted into a single text graph, difficult to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

Em... interesting that the lines above are not merged by Hugo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied from the blog article, but open to suggestions since I'm not very deep in docs/text format

Copy link
Contributor

Choose a reason for hiding this comment

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

Em... interesting that the lines above are not merged by Hugo.

The inline HTML might prevent that. Not sure.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks

Here's some feedback

reviewers:
- sftim
- thockin
title: Service ClusterIP allocation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a ClusterIP, or just a virtual cluster IP? I'm not sure on the need for writing this text monospace. Bear in mind that the Service that gets one of these virtual IP addresses might not have the type field set to "ClusterIP". For example, it could be a Service of type: LoadBalancer.

Copy link
Member Author

@aojea aojea Nov 29, 2022

Choose a reason for hiding this comment

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

this is ClusterIP specific, the LoadBalancer IP is assigned by an external controller and not explained here ... all Services types have a ClusterIP except, headless that use ClusterIP: None or those with type ExternalName

Copy link
Contributor

@sftim sftim Nov 29, 2022

Choose a reason for hiding this comment

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

all Services type have a ClusterIP

Right, that's the bit where I'm worried we're confusing readers. In Kubernetes, most Services have a thing, but should we call the thing a ClusterIP? To me ClusterIP is a particular enum value. We wouldn't, for example, write:
“the packets are rewritten as they pass through the LoadBalancer”, even though LoadBalancer is the type of Service involved there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point, let me summon @danwinship and @thockin they are good expressing these concepts

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@sftim
Copy link
Contributor

sftim commented Nov 29, 2022

If the rest of the page is technically accurate, let's ship. Would be nice to see if we can tidy further before the release.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim, thockin

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2022
@sftim
Copy link
Contributor

sftim commented Nov 29, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ca5219a653d552f4fb5d3d2bce1cf3c150d9deaf

@k8s-ci-robot k8s-ci-robot merged commit 0f3666f into kubernetes:dev-1.26 Nov 29, 2022
@k8s-ci-robot k8s-ci-robot added this to the 1.26 milestone Nov 29, 2022
@mickeyboxell
Copy link
Contributor

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/blog Issues or PRs related to the Kubernetes Blog subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants