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

[wip] Kubeadm updates for Windows and Containerd #32862

Closed

Conversation

jsturtevant
Copy link
Contributor

This PR updates to set up instructions for using Kubeadm on Windows with Containerd. This is part of the larger overhaul of Windows documentation: #31428

This requires some minor changes to the Host process examples: kubernetes-sigs/sig-windows-tools#212

/sig windows

@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Apr 11, 2022
@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 11, 2022
@netlify
Copy link

netlify bot commented Apr 11, 2022

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

Name Link
🔨 Latest commit 135ebb4
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/62545292889a3800097e7f2f

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 11, 2022
@k8s-ci-robot k8s-ci-robot requested a review from bart0sh April 11, 2022 16:09
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jimangel after the PR has been reviewed.
You can assign the PR to them by writing /assign @jimangel in a comment when ready.

The full list of commands accepted by this bot can be found 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 requested a review from marosset April 11, 2022 16:09
@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 11, 2022
@jsturtevant jsturtevant changed the title Kubeamd updates for Windows and Containerd Kubeadm updates for Windows and Containerd Apr 11, 2022
We will leverage host-process containers to run kube-proxy as a DaemonSet. This can be run from any Linux machine that has kubectl installed with its context configured to your new cluster.

``` powershell
curl -L https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/kube-proxy-flannel-hpc.yml | sed 's/KUBERNETES_VERSION/v1.23.5/g' | kubectl apply -f -
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a floating image tag for latest patch release for each minor version (in addition to a tag for each major/minor/patch)?

If we do that we can use v{{< skew currentVersion >}} instead of hardcoding the K8s version to use in the sec command here.

@sftim
Copy link
Contributor

sftim commented Apr 11, 2022

/hold

Changes for #31428 are not yet ready to merge.

@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 11, 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.

Thanks for proposing these changes.

It looks to me as if this page needs quite a lot of work. I'd like to see a comprehensive rewrite or, at a minimum, a PR that makes the page better with no regressive changes. Please do consider revising this PR in light of the inline feedback here.


#### Install kube-proxy DaemonSet

We will leverage host-process containers to run kube-proxy as a DaemonSet. This can be run from any Linux machine that has kubectl installed with its context configured to your new cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Please address the reader as you.
  • Setting up networking is not specific to containerd, so this advice seems to be in the wrong place.
  • Can you run kubectl on Windows, or does it require Linux?

If you no longer have this command, or the token has expired, you can run `kubeadm token create --print-join-command`
(on a control plane host) to generate a new token and join command.

#### Install Flannel CNI DaemonSet
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Because Flannel is not part of Kubernetes, this doesn't comply with our content guide. Please read https://kubernetes.io/blog/2020/05/third-party-dual-sourced-content/ for context.
    Make sure to include the relevant third-party shortcode as part of this page rewrite.
  • Setting up networking is not specific to containerd, so this advice seems to be in the wrong place.

@@ -172,7 +172,7 @@ installing the `containerd.io` package can be found at
{{% /tab %}}
{{% tab name="Windows (PowerShell)" %}}

Start a Powershell session, set `$Version` to the desired version (ex: `$Version="1.4.3"`),
Start a Powershell session, set `$Version` to the desired version (ex: `$Version="1.6.2"`),
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using an example that is deliberately not valid, and then helping the reader work out the right version.

Also, this page is likely to change: see #32738


### Intro
Copy link
Contributor

Choose a reason for hiding this comment

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

We never have a heading “introduction” in our documentation. Actual introductory text should appear before the first heading at <h2> level.

Please reword this heading. Alternatively, you can list prerequisites under the Prerequisites heading (line 20 ish).


### Joining a Windows worker node
The Before running any of these step the Windows node should have the following Windows Features installed: `Containers`,`Hyper-V`,`Hyper-V-PowerShell`. These can be installed using the Powershell `Install-WindowsFeature` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can list prerequisites under the Prerequisites heading (line 20 ish). Also, PowerShell has a capital S.

[Install `crictl` from the cri-tools package](https://github.com/kubernetes-sigs/cri-tools)
which is required so that kubeadm can talk to the CRI endpoint.
curl.exe -L https://dl.k8s.io/$KubernetesVersion/bin/windows/amd64/kubeadm.exe -o c:\k\kubeadm.exe
```

#### Run `kubeadm` to join the node
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a third-level heading?

@@ -226,6 +252,35 @@ curl.exe -LO https://raw.githubusercontent.com/kubernetes-sigs/sig-windows-tools
.\PrepareNode.ps1 -KubernetesVersion {{< param "fullversion" >}}
```

#### Add Windows Flannel and kube-proxy DaemonSets
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Because Flannel is not part of Kubernetes, this doesn't comply with our content guide. Please read https://kubernetes.io/blog/2020/05/third-party-dual-sourced-content/ for context.
    Make sure to include the relevant third-party shortcode as part of this page rewrite.
  • Setting up networking is not specific to containerd, so this advice seems to be in the wrong place.
  • Overall, it might be better to link to the Flannel docs about how to set that up on a Windows node.
    • If those docs don't exist, make them (in the Flannel docs) and then hyperlink to the page you made.

Now you can add Windows-compatible versions of Flannel and kube-proxy. In order
to ensure that you get a compatible version of kube-proxy, you'll need to substitute
the tag of the image. The following example shows usage for Kubernetes {{< param "fullversion" >}},
but you should adjust the version for your own deployment.
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
but you should adjust the version for your own deployment.
however, if you are running a different version of Kubernetes, you should consult the
documentation for that release instead.

Comment on lines +263 to +264
curl -L https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/kube-proxy.yml | sed 's/VERSION/{{< param "fullversion" >}}/g' | kubectl apply -f -
kubectl apply -f https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/flannel-overlay.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid recommending that readers fetch things using curl and then apply those manifests to production clusters.
If there's no reasonable alternative, explain that readers should verify the manifest before they apply it.

If you're using a different interface rather than Ethernet (i.e. "Ethernet0 2") on the Windows nodes, you have to modify the line:

```powershell
wins cli process run --path /k/flannel/setup.exe --args "--mode=overlay --interface=Ethernet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid recommending that readers create a directory k directly under C:\; this does not represent good practice.

@jsturtevant
Copy link
Contributor Author

It looks to me as if this page needs quite a lot of work. I'd like to see a comprehensive rewrite or, at a minimum, a PR that makes the page better with no regressive changes

I think this makes sense to me to comply with the third party dual sourced content. The current version of this doc does heavy use Flannel and many of the things you've called out as we should remove/avoid.

I've always felt this doc was heavily focused on the wrong things but wasn't sure how to fix it. If I understand correctly, we should probably close this and address the items in the current doc. In reality, kubeadm join works just as expected with Windows, given you have the node set up properly.

This doc was focused on setting up the node more than kubeadm aspects and should probably be cleaned up to highlight kubeadm not flannel and containerd set up. We should have other pages that go into more details how to configure a Windows node which is currently missing (or dispersed randomly throughout the large doc https://kubernetes.io/docs/setup/production-environment/windows/intro-windows-in-kubernetes/)

@marosset
Copy link
Contributor

I've always felt this doc was heavily focused on the wrong things but wasn't sure how to fix it. If I understand correctly, we should probably close this and address the items in the current doc. In reality, kubeadm join works just as expected with Windows, given you have the node set up properly.

big +1 to this

@jsturtevant jsturtevant changed the title Kubeadm updates for Windows and Containerd [wip] Kubeadm updates for Windows and Containerd Apr 11, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2022
@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

@jsturtevant: 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

nate-double-u commented Apr 26, 2022

Hi @jsturtevant, I'm not sure if this PR should target dev-1.24

/cc @marosset

@k8s-ci-robot k8s-ci-robot requested a review from marosset April 26, 2022 20:56
@marosset
Copy link
Contributor

/close
I spoke with @jsturtevant and we are going to take another approach at updates to this page based on PR feedback.

@k8s-ci-robot
Copy link
Contributor

@marosset: Closed this PR.

In response to this:

/close
I spoke with @jsturtevant and we are going to take another approach at updates to this page based on PR feedback.

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.

@jsturtevant
Copy link
Contributor Author

We discussed at sig-windows doc pairing session and we are going take similiar approach the rest of the documentation is taking with regards to https://kubernetes.io/blog/2020/05/third-party-dual-sourced-content/ for context.

There is no major differences on joining a Windows node beyond adding the named pipe flag so we will augment the existing kubeadm documentation with the differences for Windows as we have been doing with the other work in #31428

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants