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

Refresh kubeadm docs for Windows #33324

Merged
merged 2 commits into from
Jun 20, 2022
Merged

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Apr 29, 2022

Updates Windows kubeadm documentation:

  • aligns with the documentation policy as specified in https://kubernetes.io/blog/2020/05/third-party-dual-sourced-content/ removing flannel specific details.
  • Make note for users in the standard join documentation. Running kubeadm join from windows doesn't require additional work beyond specifying the correct cri-socket parameter.
  • updates the upgrade documentation

This is a follow up from the feedback recieved in #32862 (comment)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 29, 2022
@jsturtevant
Copy link
Contributor Author

/sig windows
/cc @marosset

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Apr 29, 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 Apr 29, 2022
@@ -333,6 +333,10 @@ The nodes are where your workloads (containers and Pods, etc) run. To add new no
kubeadm join --token <token> <control-plane-host>:<control-plane-port> --discovery-token-ca-cert-hash sha256:<hash>
```

{{< note >}}
On Windows you may need to specify the `cri-socket` parameter. For example, with Containerd as the runtime add `--cri-socket "npipe:////./pipe/containerd-containerd"` to the `kubeadm join` command
Copy link
Contributor

Choose a reason for hiding this comment

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

When do users not need to specify the cri-socket parameter for Windows?
If we say may need to I think we should give guidance on when they would or wouldn't need to specify the extra arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I believe it is required for any remote runtimes. If using docker shim then it is not required. Since these docs are for 1.24+ should I be more direct and be more direct that it is requird something like:

Suggested change
On Windows you may need to specify the `cri-socket` parameter. For example, with Containerd as the runtime add `--cri-socket "npipe:////./pipe/containerd-containerd"` to the `kubeadm join` command
On Windows when using a remote runtime the `cri-socket` parameter is required. For example when using Containerd as the runtime add `--cri-socket "npipe:////./pipe/containerd-containerd"` to the `kubeadm join` command

Copy link
Contributor

Choose a reason for hiding this comment

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

I think from Tuesday (the expected v1.24 release date), setting a CRI socket path becomes mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this required then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should say:

On Windows you should specify the cri-socket parameter in the kubeadm join command. The default path is "npipe:////./pipe/containerd-containerd" for containerd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@netlify
Copy link

netlify bot commented Apr 29, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 3693d1c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/62a760cc8cbbb90009e7a610
😎 Deploy Preview https://deploy-preview-33324--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
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.

/hold

Until v1.24 is released, I'd like to avoid provoking merge conflicts for the v1.24 docs branch. This PR touches a doc that was updated in light of the dockershim removal.

OK to unhold after that release.

@@ -333,6 +333,10 @@ The nodes are where your workloads (containers and Pods, etc) run. To add new no
kubeadm join --token <token> <control-plane-host>:<control-plane-port> --discovery-token-ca-cert-hash sha256:<hash>
```

{{< note >}}
On Windows you may need to specify the `cri-socket` parameter. For example, with Containerd as the runtime add `--cri-socket "npipe:////./pipe/containerd-containerd"` to the `kubeadm join` command
Copy link
Contributor

Choose a reason for hiding this comment

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

I think from Tuesday (the expected v1.24 release date), setting a CRI socket path becomes mandatory.

@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 29, 2022
@jsturtevant jsturtevant force-pushed the windows-kubeadm branch 2 times, most recently from 289e095 to 1e032ac Compare May 2, 2022 16:25
@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 4, 2022
@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 10, 2022
@marosset marosset added this to Docs updates (#31428) in SIG-Windows May 20, 2022
@marosset
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 26, 2022
@marosset
Copy link
Contributor

/assign @onlydole
/hold cancel

@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 May 26, 2022
@jsturtevant
Copy link
Contributor Author

/assign @sftim

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.

If someone visits https://kubernetes.io/docs/tasks/administer-cluster/kubeadm/adding-windows-nodes/ then this PR will serve them a 404 response.

Can we redirect them to a different page?

Also, some specific (inline) feedback.

@k8s-ci-robot k8s-ci-robot requested a review from sftim June 6, 2022 12:40
@jsturtevant
Copy link
Contributor Author

I've updated the docs based on the feedback. Thanks!

@marosset
Copy link
Contributor

marosset commented Jun 8, 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 Jun 8, 2022
@sftim
Copy link
Contributor

sftim commented Jun 12, 2022

@jsturtevant could you add the redirect that I mentioned in #33324 (review) ?

The file to change is static/_redirects.

/lgtm cancel
(until that redirect is in place)

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2022
@jsturtevant
Copy link
Contributor Author

@sftim sorry about missing that, updated. There were a few other redirects on that file, I left them in place but let me know if they should be consolidated in someway.

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 Jun 20, 2022
@tengqm
Copy link
Contributor

tengqm commented Jun 20, 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 Jun 20, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5a4e6acdf3111856706ac92d9527060d7e5e1394

@k8s-ci-robot k8s-ci-robot merged commit 117fa22 into kubernetes:main Jun 20, 2022
SIG-Windows automation moved this from Docs updates (#31428) to Done (v1.25) Jun 20, 2022
@marosset marosset deleted the windows-kubeadm branch July 5, 2022 18:45
@marosset marosset restored the windows-kubeadm branch July 5, 2022 18:45
@marosset marosset deleted the windows-kubeadm branch July 5, 2022 18:45
@marosset marosset restored the windows-kubeadm branch July 5, 2022 18:45
@marosset marosset deleted the windows-kubeadm branch July 5, 2022 18:45
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/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
SIG-Windows
  
Done (v1.25)
Development

Successfully merging this pull request may close these issues.

None yet

6 participants