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

Update iptables perf / KEP-3453 discussion for 1.27 #39188

Merged
merged 1 commit into from Mar 22, 2023

Conversation

danwinship
Copy link
Contributor

This is an update for a feature going to beta, so I assume it belongs on dev-1.27, but that branch doesn't seem to be up-to-date? In particular, it doesn't have #38435, which this rewrites.

(I moved the feature-specific section because it seemed to make more sense to talk about that first now, since it's enabled by default.)

/cc @sftim @aojea

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 31, 2023
@netlify
Copy link

netlify bot commented Jan 31, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 14263a3
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/63e3deaa6d1d150008913e87
😎 Deploy Preview https://deploy-preview-39188--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.

@sftim
Copy link
Contributor

sftim commented Jan 31, 2023

/hold

This PR needs to target dev-1.27 (it's OK to wait until dev-1.27 incorporates everything that's in main today).

OK to unhold once we fix the base branch.

@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 Jan 31, 2023
@sftim
Copy link
Contributor

sftim commented Jan 31, 2023

/sig network

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jan 31, 2023
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

For beta, please also revise the text that talks about Endpoints - refer to EndpointSlices if you can. I'm assuming that kube-proxy actually watches EndpointSlices these days (if not, we ought to document that detail somewhere else, and can file an issue to track that).

content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
Comment on lines 137 to 149
If you were previously overriding `minSyncPeriod`, you should try
removing that override and letting kube-proxy use the default value
(`1s`) or at least a smaller value than you were using before.

If you notice kube-proxy's
`sync_proxy_rules_iptables_restore_failures_total` or
`sync_proxy_rules_iptables_partial_restore_failures_total` metrics
increasing after upgrading to 1.27, that likely indicates you are
encountering bugs in the new code, and you should file a bug report.
(You can disable the new optimizations by disabling the
`MinimizeIPTablesRestore` [feature
gate](/docs/reference/command-line-tools-reference/feature-gates/) for
kube-proxy with `--feature-gates=MinimizeIPTablesRestore=false,…`.)
Copy link
Contributor

Choose a reason for hiding this comment

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

We avoid documenting transitions between versions within the reference docs (exception: https://kubernetes.io/docs/reference/using-api/deprecation-guide/ and friends). As docs and features move towards GA, this approach becomes more important.

As a result, this information probably belongs in the post-release blog article and in the v1.27 release notes.
The PR that adds the blog article can also update this page to link there, and we can merge that on release day provided that folks plan ahead and get everything ready for review in plenty of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it's already in the release note for kubernetes/kubernetes#115138

Copy link
Contributor

Choose a reason for hiding this comment

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

The next step is to cut out or reword this section - otherwise, you'll need to commit SIG Network to maintaining this and updating it for the v1.28 release, when this wording will be stale.

Comment on lines 133 to 135
In Kubernetes 1.27, new performance improvements to the iptables proxy
mode are enabled by default, which should make it much less necessary to
override the default syncing timeouts.
Copy link
Contributor

@sftim sftim Jan 31, 2023

Choose a reason for hiding this comment

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

Try to describe the current state of Kubernetes, post-release. Eg:

By default, the `kube-proxy` component of Kubernetes {{< skew currentVersion >}}
aggregates its updates to `iptables` rules.
Previous versions of Kubernetes (before v1.27) did not default to this behavior, and
FIXME - what does this mean for a cluster operator?

@danwinship
Copy link
Contributor Author

For beta, please also revise the text that talks about Endpoints - refer to EndpointSlices if you can.

Changed one "Endpoints" to "EndpointSlices" and another to "endpoints"; we still use lowercase-e "endpoints" in many places to refer to endpoints as a general idea

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.

Once this targets the right branch, I think it'll be good to go in. The tweaks I'm suggesting here aren't blockers.

content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2023
@danwinship danwinship changed the base branch from main to dev-1.27 March 9, 2023 14:31
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023
@danwinship
Copy link
Contributor Author

/hold cancel
this was marked /hold because it was originally targeting main, but that's fixed now

@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 Mar 21, 2023
@aojea
Copy link
Member

aojea commented Mar 21, 2023

/lgtm

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

LGTM label has been added.

Git tree hash: a98f36df84ff9151f057b7a4057615214788712a

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.

/lgtm

with further tweaks available

content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
content/en/docs/reference/networking/virtual-ips.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2023
@tengqm
Copy link
Contributor

tengqm commented Mar 22, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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 Mar 22, 2023
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@tengqm
Copy link
Contributor

tengqm commented Mar 22, 2023

/lgtm
again

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

LGTM label has been added.

Git tree hash: 0aecedf4a7beb71a26ebdc72f92202d5ffbf9570

@k8s-ci-robot k8s-ci-robot merged commit f48a415 into kubernetes:dev-1.27 Mar 22, 2023
@k8s-ci-robot k8s-ci-robot added this to the 1.27 milestone Mar 22, 2023
@danwinship danwinship deleted the kep-3453-beta branch March 22, 2023 18:16
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. 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. sig/network Categorizes an issue or PR as relevant to SIG Network. 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.

None yet

5 participants