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

Moving windows networking information to a new page #31852

Closed

Conversation

marosset
Copy link
Contributor

Signed-off-by: Mark Rossetti marosset@microsoft.com

Moving Windows networking information to a new page.

Part of #31428

/label refactor
/sig windows

@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Feb 22, 2022
@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. refactor Indicates a PR with large refactoring changes e.g. removes files or moves content labels Feb 22, 2022
@netlify
Copy link

netlify bot commented Feb 22, 2022

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

Name Link
🔨 Latest commit 641aa2c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/624c9e4bd3b5f30009fad8a6

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 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. labels Feb 22, 2022
@marosset marosset mentioned this pull request Feb 22, 2022
31 tasks
@marosset
Copy link
Contributor Author

@brasmith-ms @daschott @jayunit100
We should review/update this page for accuracy too. I mostly just copy/pasted what was present in the intro-to-windows page.

@marosset
Copy link
Contributor Author

Hmm, perhaps this page should go under /concepts/services-networking/
What do people think?

@jihoon-seo
Copy link
Member

@sftim
Copy link
Contributor

sftim commented Feb 23, 2022

Would it be reasonable to put “Networking on Windows Nodes” within https://kubernetes.io/docs/reference/node/ ?

@sftim
Copy link
Contributor

sftim commented Feb 23, 2022

If this merges, I can update #30817 to link to the information about Windows specifics.

@marosset
Copy link
Contributor Author

Would it be reasonable to put “Networking on Windows Nodes” within https://kubernetes.io/docs/reference/node/ ?

Let me move some sections out of this document and into the existing pages (like DNS limitations to https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/) and then we can see what information is left.

If I was a user I think I expect some of the information in this page to be near other pages in https://kubernetes.io/docs/concepts/services-networking/ . I also think we can probably remove "node" from the page heading to make it better fit in here.
I'm also OK moving it to /reference/node/ if folks agree that is a good place.

What do you all think? @immuzz @aravindhp @jsturtevant @jayunit100 @perithompson @brasmith-ms

@aravindhp
Copy link
Contributor

Let me move some sections out of this document and into the existing pages (like DNS limitations to https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/) and then we can see what information is left.

If I was a user I think I expect some of the information in this page to be near other pages in https://kubernetes.io/docs/concepts/services-networking/ . I also think we can probably remove "node" from the page heading to make it better fit in here.

My vote is for the above.

I'm also OK moving it to /reference/node/ if folks agree that is a good place.

I not sure if users would end up on this page looking for Windows networking information.

@marosset
Copy link
Contributor Author

Let me move some sections out of this document and into the existing pages (like DNS limitations to https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/) and then we can see what information is left.
If I was a user I think I expect some of the information in this page to be near other pages in https://kubernetes.io/docs/concepts/services-networking/ . I also think we can probably remove "node" from the page heading to make it better fit in here.

My vote is for the above.

I'm also OK moving it to /reference/node/ if folks agree that is a good place.

I not sure if users would end up on this page looking for Windows networking information.

Updated - PTAL

@marosset
Copy link
Contributor Author

@bridgetkromhout FYI too!

Copy link
Contributor

@aravindhp aravindhp 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 doing this, @marosset

LGTM

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, this layout looks OK.

I've suggested a few tweaks; the key thing to address is heading levels. Other feedback is nits.

@@ -361,6 +361,10 @@ You can also set the maximum session sticky time by setting
`service.spec.sessionAffinityConfig.clientIP.timeoutSeconds` appropriately.
(the default value is 10800, which works out to be 3 hours).

{{< note >}}
Setting the maximum session sticky time for services that run on Windows is not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Setting the maximum session sticky time for services that run on Windows is not supported.
Setting the maximum session sticky time for Services that run on Windows is not supported.

What does it mean for a Service to “run on” Windows. For example, if I run two Deployments and select both deployments with a single Service, what happens?
I'm guessing it's undefined behavior, but maybe I'm wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question and I was struggling with the wording here.
Services are not tied to any OS.
Does something like:

Setting the maximum session sticky time for services connected to pods running on Windows nodes is not supported?

Unfortunately, I'm not sure exactly what does not work in this scenario and was copying this from other parts of the docs :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the capitalization. Leaving this comment open for the additional discussions.

content/en/docs/concepts/services-networking/dual-stack.md Outdated Show resolved Hide resolved
@marosset marosset added this to In Review (v1.24) in SIG-Windows Feb 24, 2022
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

This is a good start to make changes easier to manage for the Windows docs. The Windows networking do need some more love but this is a good start to making it easier to do so in the future.

@marosset marosset moved this from In Review (v1.24) to Docs updates (#31428) in SIG-Windows Mar 10, 2022
@tengqm
Copy link
Contributor

tengqm commented Apr 5, 2022

This PR has been dangling for over a month. I'd like to move this forward instead of blocking it for trivial issues.
/approve

@tengqm
Copy link
Contributor

tengqm commented Apr 5, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 5, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2022
@marosset
Copy link
Contributor Author

marosset commented Apr 5, 2022

I can address most of the easy comments today after some community meetings.

@marosset
Copy link
Contributor Author

marosset commented Apr 5, 2022

@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 Apr 5, 2022
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
…ns-pod-service.md

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
…ces-networking/service.md

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
…wokring/dual-stack.md

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
…windows-netwoorking.md

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
@marosset marosset force-pushed the move-windows-networking-1.24 branch from dc481a0 to 641aa2c Compare April 5, 2022 19:53
@tengqm
Copy link
Contributor

tengqm commented Apr 6, 2022

@marosset Could you please elaborate what do you mean by "We want all the PRs..."? Do you intend to merge all these refactor PRs into a single one, or break the huge task into several pieces? For the sake of easier reviewing, I'd like to suggest the second approach. However, the overall plan and pace would be up to you. Hopefully, these changes are not introducing a lot of dangling links between them.

@sftim
Copy link
Contributor

sftim commented Apr 6, 2022

What I suggested (this was in the SIG Docs meeting) was that once there are a bunch of PRs that are ready to merge into the main branch, SIG Docs can help get those merged.

In my view, there great part of the effort is lining up PRs that are good to merge, and actually merging them is more restricted (needs an approver's time) but less effort.

We also agreed in that meeting to continue working towards doing that merge after the v1.24 release.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022
@k8s-ci-robot
Copy link
Contributor

@marosset: PR needs rebase.

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.

@nate-double-u
Copy link
Contributor

Hi @marosset, I don't think this PR should target dev-1.24

@marosset
Copy link
Contributor Author

/close
I will squash the commits and create a single PR with all of the related changes targeting main.

SIG-Windows automation moved this from Docs updates (#31428) to Done (v1.25) Apr 27, 2022
@k8s-ci-robot
Copy link
Contributor

@marosset: Closed this PR.

In response to this:

/close
I will squash the commits and create a single PR with all of the related changes targeting main.

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.

@nate-double-u
Copy link
Contributor

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the 1.24 milestone Apr 27, 2022
@sftim
Copy link
Contributor

sftim commented Apr 28, 2022

(sorry about the mixed messaging here @marosset)
You'll likely have an easier time of it if you hold of making that single PR until after the v1.24 release.

The way I suggest you do that is by waiting 'til then, making a new branch, and then merging in all of your existing work. If you then want to rebase against main, or against the merge point for the v1.24 release, that's tidier but optional.

@marosset
Copy link
Contributor Author

(sorry about the mixed messaging here @marosset) You'll likely have an easier time of it if you hold of making that single PR until after the v1.24 release.

The way I suggest you do that is by waiting 'til then, making a new branch, and then merging in all of your existing work. If you then want to rebase against main, or against the merge point for the v1.24 release, that's tidier but optional.

No worries.
Yea, I was thinking about waiting for 1.24 release - thanks for the advice!

yoonian added a commit to yoonian/kubernetes-website that referenced this pull request Aug 1, 2022
#dns-limitations was relocated and renamed. kubernetes#31852
@yoonian yoonian mentioned this pull request Aug 1, 2022
ameukam pushed a commit to ameukam/website that referenced this pull request Aug 5, 2022
#dns-limitations was relocated and renamed. kubernetes#31852
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. refactor Indicates a PR with large refactoring changes e.g. removes files or moves content sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
SIG-Windows
  
Done (v1.25)
Development

Successfully merging this pull request may close these issues.

None yet

8 participants