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

KEP-3453 to GA #41031

Merged
merged 1 commit into from
Jul 17, 2023
Merged

KEP-3453 to GA #41031

merged 1 commit into from
Jul 17, 2023

Conversation

danwinship
Copy link
Contributor

(The fact that it looks like we missed adding a Beta feature-gates line for 1.27 is an artifact of the current state of the dev-1.28 branch. The line exists in main.)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 8, 2023
@k8s-ci-robot k8s-ci-robot requested a review from bradtopol May 8, 2023 19:47
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label May 8, 2023
@k8s-ci-robot k8s-ci-robot requested a review from shannonxtreme May 8, 2023 19:47
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label May 8, 2023
@tengqm
Copy link
Contributor

tengqm commented May 9, 2023

/approve

Not sure if we want to merge this because the 'dev-1.28' is not well sync'ed with 'main'.
/hold

Feel free to lift the hold if the potential conflict is not a big issue.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 9, 2023
@@ -133,7 +133,7 @@ iptables:

##### Performance optimization for `iptables` mode {#minimize-iptables-restore}

{{< feature-state for_k8s_version="v1.27" state="beta" >}}
{{< feature-state for_k8s_version="v1.28" state="stable" >}}

In Kubernetes {{< skew currentVersion >}} the kube-proxy defaults to a minimal approach
Copy link
Contributor

Choose a reason for hiding this comment

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

This might well need a rewrite: I'm assuming that the feature gate is going to be GA and locked to true, so “defaults to” isn't correct any more.

@@ -133,7 +133,7 @@ iptables:

##### Performance optimization for `iptables` mode {#minimize-iptables-restore}
Copy link
Contributor

Choose a reason for hiding this comment

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

The sense could change here; for example:

##### Legacy `minSyncPeriod` configuration in `iptables` mode {#minimize-iptables-restore}

Also, consider moving this into or after the minSyncPeriod section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this heading isn't needed at all?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 9, 2023
@danwinship
Copy link
Contributor Author

updated... I still don't have a good sense of how we refer to different versions in the docs. (I feel like we should say "kube-proxy 1.28 and later do X" but it seems like the preferred style is to always talk about currentVersion instead?)

Current subsection order is

#### Optimizing iptables mode performance
##### `minSyncPeriod`
##### Updating legacy `minSyncPeriod` configuration {#minimize-iptables-restore}
##### `syncPeriod`

And I feel like maybe the "Updating" section breaks the flow? But it doesn't seem like it fits after syncPeriod either. And putting syncPeriod first also feels wrong since the minSyncPeriod section is more important.

Maybe it could be "minSyncPeriod, syncPeriod, Updating legacy config", ane the part in the syncPeriod section about "in the past, it was sometimes useful to set [syncPeriod] to a very large value (eg, 1h). This is no longer recommended" could be moved to the "legacy config" section?

@sftim
Copy link
Contributor

sftim commented May 9, 2023

I still don't have a good sense of how we refer to different versions in the docs. (I feel like we should say "kube-proxy 1.28 and later do X" but it seems like the preferred style is to always talk about currentVersion instead?)

If we can, we write wording that will still feel relevant even 5 minor releases later. So we avoid phrases like “new implementation” or “has graduated to GA”. The best way to achieve that timelessness depends on what we want to say.

Watch out for one thing though. Any time we say “foo 1.28 and later”, consider if any future minor release might make this previously true statement false. Maybe, in that scenario, you can reword to make that problem case less likely?

@sftim
Copy link
Contributor

sftim commented May 9, 2023

Do we even need an “Optimizing iptables mode performance” section in the v1.28 docs? We can instead suggest that if people are running an older version, to read the docs for that version as there are some important hints.

@tengqm
Copy link
Contributor

tengqm commented May 9, 2023

updated... I still don't have a good sense of how we refer to different versions in the docs. (I feel like we should say "kube-proxy 1.28 and later do X" but it seems like the preferred style is to always talk about currentVersion instead?)

I'm more inclined to explicitly call out the specific version when we mention something added/removed/changed. Users can get a good sense of when the change was introduced. Switching this to the skew shortcode may cause some confusion. A change introduced in 1.24 may smell something new. As a project that has 3 releases each year, Kubernetes users really need to know whether a feature has been there for "a long time".

Later on, say two years from now, our attitude towards this change will be different. The "something" has become so stable and well understood that no one cares when it was introduced. We will remove the mention "kube-proxy 1.24 and later on". On the contrary, if we use the skew shortcode, we won't be confident whether that shortcode can be removed. It will read as "kube-proxy 1.33 blah blah", "kubernetes 1.34 blah blah...". It will become very difficult to know how old that change is.

The skew shortcode does have an important use case, which is for referencing the source code, the release, the generated API reference etc. But attempting to use it everywhere doesn't sound to me like the right thing to do.

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.

How about this? It's not ideal but maybe it's better.

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
@danwinship
Copy link
Contributor Author

updated

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2023
@danwinship
Copy link
Contributor Author

/hold cancel
sorry, this has been ready to go for a while but I didn't notice it was marked /hold

@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 Jun 26, 2023
@sftim
Copy link
Contributor

sftim commented Jun 26, 2023

/hold
pending merge of #41720 (or its replacement)

@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 Jun 26, 2023
@danwinship
Copy link
Contributor Author

/hold cancel
rebased (no changes)

@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 Jul 3, 2023
@Rishit-dagli
Copy link
Member

nit: changing the status of this KEP to "Docs PR Ready for review" in the board

@kbhawkey
Copy link
Contributor

@danwinship
Is this PR ready to merge?

@danwinship
Copy link
Contributor Author

yes

@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey, 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

@kbhawkey
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 0b797469d61e2ec2ba1e3cc4d6c02a5066173fa3

@k8s-ci-robot k8s-ci-robot merged commit b96f687 into kubernetes:dev-1.28 Jul 17, 2023
@k8s-ci-robot k8s-ci-robot added this to the 1.28 milestone Jul 17, 2023
@danwinship danwinship deleted the kep-3453-stable branch July 17, 2023 13:08
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. 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