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

Windows documentation overhaul #33582

Merged
merged 6 commits into from
May 27, 2022

Conversation

marosset
Copy link
Contributor

@marosset marosset commented May 9, 2022

This PR moves all content out of /docs/setup/production-environment/windows to new locations.
Where possible we merged Windows information into the existing docs.

This is a big PR but most of the contents have already been reviewed

I was initially hoping to get all of these changes in with the 1.24 release branch but in order to ease pressure on the release team we all decided to merge these changes after the 1.24 release.

Part of #31428

/label refactor
/sig windows
/assign @jsturtevant @sftim
/cc @tengqm @aravindhp @nate-double-u

@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 May 9, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels May 9, 2022
@marosset
Copy link
Contributor Author

marosset commented May 9, 2022

/cc @brasmith-ms

@k8s-ci-robot
Copy link
Contributor

@marosset: GitHub didn't allow me to request PR reviews from the following users: brasmith-ms.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @brasmith-ms

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.

@netlify
Copy link

netlify bot commented May 9, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 70206d9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/628fa56422749f0008335181
😎 Deploy Preview https://deploy-preview-33582--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.

@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 11, 2022
@marosset marosset force-pushed the windows-docs-merged-updates branch from ac81afe to babc202 Compare May 12, 2022 20:11
@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 12, 2022
qualified has failed
On Windows, you can only have 1 DNS suffix, which is the DNS suffix associated with that
pod's namespace (mydns.svc.cluster.local for example). Windows can resolve FQDNs
and services or names resolvable with just that suffix. For example, a pod spawned
Copy link
Contributor

Choose a reason for hiding this comment

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

"Windows can resolve FQDNs and services or names resolvable with just that suffix."

This sentence is difficult to parse.

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 tried to make this less confusing. Let me know if it makes more sense.

content/en/docs/concepts/windows/intro.md Outdated Show resolved Hide resolved
content/en/docs/concepts/windows/intro.md Outdated Show resolved Hide resolved
content/en/docs/concepts/windows/intro.md Outdated Show resolved Hide resolved
* A kubelet running on a Windows node does not have memory
restrictions. `--kubelet-reserve` and `--system-reserve` do not set limits on
kubelet or processes running on the host. This means kubelet or a process on the host
could cause memory resource starvation outside the node-allocatable and scheduler.
Copy link
Contributor

Choose a reason for hiding this comment

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

“This means kubelet or a process on the host could cause memory resource starvation outside the node-allocatable and scheduler.”

This sentence is difficult to parse.

content/en/docs/concepts/windows/intro.md Outdated Show resolved Hide resolved
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.

lgtm for sig-windows, The initial plan is move these files with minimal changes to the correct content locations to make edits to them easier and make the Windows Introduction page easier to manage and comprehend.

Comment on lines +244 to +246
Kubernetes on Windows does not support single-stack "IPv6-only" networking. However,
dual-stack IPv4/IPv6 networking for pods and nodes with single-family services
is supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

@daschott could you help parse the intent of the paragraph? This was a straight copy of https://kubernetes.io/docs/setup/production-environment/windows/intro-windows-in-kubernetes/#ipv6-networking.

I think the plan was to mostly move things as is in this first iteration but this one is pretty confusing. Another thing missing here is what version of Windows is required. WS2019 does not support dual-stack. It is available in 20H2 and WS2022.

@marosset
Copy link
Contributor Author

@tengqm I have incorporated all of the suggested edits.
Since this PR is primarily moving pages around in an attempt to break up the giant intro-windows-in-kubernetes page (which is hard to edit and consume) would you be OK if the rest of the suggestions get addressed in a seperate PR?

aravindhp and others added 4 commits May 17, 2022 13:54
- Move FlexVolume plugins section to docs/concepts/storage/volumes.md
- Move CSI plugins section to en/docs/concepts/storage/volumes.md
- Move in-tree plugins section to en/docs/concepts/storage/volumes.md
/docs/setup/production-environment/windows to various locations
- Moving windows DNS limitations to/docs/concepts/services-networking/dns-pod-service.md
- Moving windows session sticky time disclaimer to /docs/concepts/services-networking/service.md
- Moving windows dual stack support info to /docs/concepts/services-networking/dual-stack.md
- Moving generic Windows content to
/docs/concepts/services-networking/windows-networking.md

Signed-off-by: Mark Rossetti <[email protected]>
@marosset marosset force-pushed the windows-docs-merged-updates branch from e711b4e to 2d955bb Compare May 17, 2022 20:54
@tengqm
Copy link
Contributor

tengqm commented May 17, 2022

@tengqm I have incorporated all of the suggested edits. Since this PR is primarily moving pages around in an attempt to break up the giant intro-windows-in-kubernetes page (which is hard to edit and consume) would you be OK if the rest of the suggestions get addressed in a seperate PR?

Sure. I'm okay with whichever approach to improve these pages, provided that the ball is not dropped.

@marosset
Copy link
Contributor Author

@tengqm I have incorporated all of the suggested edits. Since this PR is primarily moving pages around in an attempt to break up the giant intro-windows-in-kubernetes page (which is hard to edit and consume) would you be OK if the rest of the suggestions get addressed in a seperate PR?

Sure. I'm okay with whichever approach to improve these pages, provided that the ball is not dropped.

Understood.
I will follow up with a PR to update the intro-windows page.
I will try and find someone with more networking knowledge to update that page and if not I'll try to update that too.

@marosset
Copy link
Contributor Author

@sftim @tengqm - do you see any issues with the structural changes in this PR?
I can open up new PRs for content changes after this merges.

@tengqm
Copy link
Contributor

tengqm commented May 26, 2022

I'm okay to move this forward.
/approve
/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 May 26, 2022
@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 May 26, 2022
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 cancel

There's one place where this needs a fix.

Comment on lines 186 to 193
=======
The scheduler does not use the value of `.spec.os.name` when assigning Pods to nodes.
You should use normal Kubernetes mechanisms for
[assigning pods to nodes](/docs/concepts/scheduling-eviction/assign-pod-node/)
to ensure that the control plane for your cluster places pods onto nodes that are running the
appropriate operating system.
{{< /note >}}
>>>>>>> Moving windows containers user guide out of /setup/production-environment/:content/en/docs/concepts/windows/user-guide.md
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a broken merge that needs tidying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, I'll scan the rest of the PR again now too.

@marosset marosset force-pushed the windows-docs-merged-updates branch from 2d955bb to 70206d9 Compare May 26, 2022 16:05
@jsturtevant
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 May 27, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 90cd4e8c0493f2ae84dd933577bd48df20f7ca75

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. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants