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

Update Windows documentation #5789

Merged
merged 4 commits into from
Jan 17, 2024
Merged

Conversation

antoninbas
Copy link
Contributor

@antoninbas antoninbas commented Dec 12, 2023

The user-facing Windows document is updated as follows:

  • containerd will be the only supported container runtime on Windows Nodes, Docker support (that includes cri-dockerd, Docker Desktop) is officially deprecated.
  • we split the "Installation as Pod" section into 2 new sections (one for containerd, one for Docker).
  • installation methods are re-ordered, starting with the recommended / most common one: Installation as a Pod for containerd, Installation as Windows services, Installation as a Pod for Docker (deprecated).

The main idea is to make the installation instructions clearer and easier to follow for a "normal" user, deploying the latest Antrea version on a recent K8s cluster. In this situation, the recommended installation method is to use antrea-windows-containerd.yml or antrea-windows-containerd-with-ovs.yml, which leverage HostProcess Windows Pods.

There is still a lot of room for future improvements. I suggest that for Antrea v2.0 we drop all references to Docker and kube-proxy from this document.

The "Antrea Management on Windows" section is also dropped from the Windows design document, as it was stale information.

For #5624 #5630

@antoninbas antoninbas force-pushed the update-windows-doc branch 3 times, most recently from fb4be0c to 25ec2ec Compare December 15, 2023 17:55
@luolanzone luolanzone added this to the Antrea v1.15 release milestone Jan 3, 2024
@antoninbas antoninbas force-pushed the update-windows-doc branch 4 times, most recently from 2a5346b to 5773dad Compare January 9, 2024 21:39
@antoninbas antoninbas marked this pull request as ready for review January 9, 2024 21:57
@antoninbas antoninbas added the kind/documentation Categorizes issue or PR as related to a documentation. label Jan 9, 2024
@antoninbas antoninbas added the area/OS/windows Issues or PRs related to the Windows operating system. label Jan 9, 2024
@antoninbas
Copy link
Contributor Author

Tested with #5858 and #5859

Comment on lines -394 to -412
Then, set the Node IP used by kubelet.
Open file `/var/lib/kubelet/kubeadm-flags.env`:

```text
KUBELET_KUBEADM_ARGS="--network-plugin=cni --pod-infra-container-image=mcr.microsoft.com/oss/kubernetes/pause:1.3.0"
```

Append `--node-ip=$NODE_IP` at the end of params. Replace `$NODE_IP` with
the address for kubelet. It should look like:

```text
KUBELET_KUBEADM_ARGS="--network-plugin=cni --pod-infra-container-image=mcr.microsoft.com/oss/kubernetes/pause:1.3.0 --node-ip=$NODE_IP"
```

Restart kubelet service for changes to take effect.

```powershell
restart-service kubelet
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This no longer seems to be required: the --node-ip command-line flag is already included in the generated c:\k\StartKubelet.ps1.

PS C:\Users\Administrator> cat c:\k\StartKubelet.ps1
$FileContent = Get-Content -Path "/var/lib/kubelet/kubeadm-flags.env"
$global:KubeletArgs = $FileContent.Trim("KUBELET_KUBEADM_ARGS=`"")

$global:KubeletArgs += " --cert-dir=$env:SYSTEMDRIVE\var\lib\kubelet\pki --config=/var/lib/kubelet/config.yaml --bootstrap-kubeconfig=/etc/kubernetes/bootstrap-kubelet.conf --kubeconfig=/etc/kubernetes/kubelet.conf --hostname-override=$(hostname) --pod-infra-container-image=`"mcr.microsoft.com/oss/kubernetes/pause:1.4.1`" --enable-debugging-handlers --cgroups-per-qos=false --enforce-node-allocatable=`"`" --resolv-conf=`"`" --node-ip=$env:NODE_IP"
$cmd = "C:\k\kubelet.exe $global:KubeletArgs"
Invoke-Expression $cmd

@jianjuns
Copy link
Contributor

jianjuns commented Jan 9, 2024

There is a typo in the commit message: onyl -> only.


### OVS Management

OVS is running as 2 Windows Services: one for ovsdb-server and one for ovs-vswitchd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this line, we can add a liner that "ovs userspace processes i.e ovsdb-server and ovs-vswitchd run inside container wrt 1.13 and from 1.14 windows will support pristine host wrt windows ovs driver installation" , Design doc needs to be updated wrt ovs containerisation and pristine host support.

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 added back a paragraph.
"pristine host" is not a very helpful term IMO, we should avoid it and be more accurate

The doc definitely needs more updating, there is stale information, in particular when it comes to kube-proxy.

docs/windows.md Outdated
- [Add Windows kube-proxy DaemonSet (only for Kubernetes versions prior to 1.26)](#add-windows-kube-proxy-daemonset-only-for-kubernetes-versions-prior-to-126)
- [Common steps](#common-steps)
- [For containerd](#for-containerd)
- [For docker](#for-docker)
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
- [For docker](#for-docker)
- [For Docker](#for-docker)

docs/windows.md Show resolved Hide resolved
@@ -254,7 +199,7 @@ Bcdedit.exe -set TESTSIGNING ON
Restart-Computer
```

Then, install the OVS using the script.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not introduced by this PR, but I saw both "Worker Node" and "worker Nodes" in the titles, maybe update them as well. Thanks.

docs/windows.md Outdated
#### Add Windows antrea-agent DaemonSet

For example, these commands will download the antrea-agent manifest, set
kubeAPIServerOverride, and deploy the antrea-agent DaemonSet when using the
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
kubeAPIServerOverride, and deploy the antrea-agent DaemonSet when using the
`kubeAPIServerOverride`, and deploy the antrea-agent DaemonSet when using the

docs/windows.md Outdated
Comment on lines 421 to 422
specify that you are using the docker container runtime. The script will then
take are of installing wins. For example:
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
specify that you are using the docker container runtime. The script will then
take are of installing wins. For example:
specify that you are using the Docker container runtime. The script will then
take care of installing wins. For example:

docs/windows.md Outdated
(`Prepare-AntreaAgent.ps1` needs to run at every startup).

After that, you will need to deploy a Windows-compatible version of
kube-proxy. You can download `kube-proxy.yaml` from the Kubernetes github
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
kube-proxy. You can download `kube-proxy.yaml` from the Kubernetes github
kube-proxy. You can download `kube-proxy.yml` from the Kubernetes github

docs/windows.md Outdated
```

For containerd runtime, also add the following to the kube-proxy-windows DaemonSet spec.
#### For docker
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
#### For docker
#### For Docker

@antoninbas antoninbas force-pushed the update-windows-doc branch 2 times, most recently from 657f3ca to 987045e Compare January 12, 2024 19:46
Antrea on Windows, this package can be used for testing. We also provide a helper
script `Install-OVS.ps1` to install the OVS driver and register userspace binaries
as services.
| Containerized OVS daemons? | Test-signed OVS driver? | Run this command |
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 clarified this section with a table

Copy link
Contributor

Choose a reason for hiding this comment

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

If Test-signed OVS driver is no, does it mean user actually uses a signed driver? And it means user is supposed to involve the OVS packages including the driver in the image by themselves if Containerized OVS daemons is yes in the meanwhile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Test-signed OVS driver is no, does it mean user actually uses a signed driver?

Yes

And it means user is supposed to involve the OVS packages including the driver in the image by themselves if Containerized OVS daemons is yes in the meanwhile?

I think this is similar to what we discussed with @XinShuYang in https://github.com/antrea-io/antrea/pull/5789/files#r1452075390. I don't think users should be expected to build a custom image. This is why we probably want to document this case more. We should probably suggest that users load their (signed) driver manually on each Node.

Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM overall, a few nits.
@wenyingd @XinShuYang please double check the Windows installation steps changes. Thanks.

docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
| Containerized OVS daemons? | Test-signed OVS driver? | Run this command |
| -------------------------- | ----------------------- | ---------------- |
| Yes | Yes | `.\Install-OVS.ps1 -InstallUserspace $false` |
| Yes | No | N/A |
Copy link
Contributor

Choose a reason for hiding this comment

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

In the current Install-OVS script, even if we skip 'InstallUserspace', the OVS driver can still be installed on the host with GUI support. I recall that for non-signed driver installation, a popup window appears when installing the driver, but it should still be installable with the GUI. I'm not sure if it's better to add this information instead of marking it as N/A in this table?

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'm confused by your comment. This is for the case where users don't want to use the test-signed drivers provided by Antrea. Are you saying that they should run:

.\Install-OVS.ps1 -InstallUserspace $false -ImportCertificate $false -Local -LocalFile <PathToOVSPackage>

For a signed driver, the DaemonSet should ideally take care of loading the driver: https://github.com/antrea-io/antrea/blob/main/build/yamls/antrea-windows-containerd-with-ovs.yml#L48-L55
I assume that in this case the driver needs to be placed in a specific location, which unfortunately we do not document AFAIK. Maybe in a future PR?

But my understanding is that for this case (Containerized OVS daemons? Yes - Test-signed OVS driver? No), the intent was that users don't need to run Install-OVS.ps1.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a signed driver, the DaemonSet should ideally take care of loading the driver: https://github.com/antrea-io/antrea/blob/main/build/yamls/antrea-windows-containerd-with-ovs.yml#L48-L55 I assume that in this case the driver needs to be placed in a specific location, which unfortunately we do not document AFAIK. Maybe in a future PR?

Got your point, I am okay to document it in a future PR. Also, since the popup windows issue has been fixed, maybe in this future PR we can also check if it's possible to install test-signed driver during ovs container deployment. If it work we can skip running Install-OVS for test-signed OVS driver as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this future PR we can also check if it's possible to install test-signed driver during ovs container deployment. If it work we can skip running Install-OVS for test-signed OVS driver as well.

that's a good point, I'll open an issue to track this

The user-facing Windows document is updated as follows:
* containerd will be the only supported container runtime on Windows
  Nodes, Docker support (that includes cri-dockerd, Docker Desktop) is
  officially deprecated.
* we split the "Installation as Pod" section into 2 new sections (one
  for containerd, one for Docker).
* installation methods are re-ordered, starting with the recommended /
  most common one: Installation as a Pod for containerd, Installation as
  Windows services, Installation as a Pod for Docker (deprecated).

The main idea is to make the installation instructions clearer and
easier to follow for a "normal" user, deploying the latest Antrea
version on a recent K8s cluster. In this situation, the recommended
installation method is to use antrea-windows-containerd.yml or
antrea-windows-containerd-with-ovs.yml, which leverage HostProcess
Windows Pods.

There is still a lot of room for future improvements. I suggest that for
Antrea v2.0 we drop all references to Docker and kube-proxy from this
document.

The "Antrea Management on Windows" section is also dropped from the
Windows design document, as it was stale information.

For antrea-io#5624 antrea-io#5630

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Copy link
Contributor

@XinShuYang XinShuYang left a comment

Choose a reason for hiding this comment

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

LGTM

Antrea on Windows, this package can be used for testing. We also provide a helper
script `Install-OVS.ps1` to install the OVS driver and register userspace binaries
as services.
| Containerized OVS daemons? | Test-signed OVS driver? | Run this command |
Copy link
Contributor

Choose a reason for hiding this comment

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

If Test-signed OVS driver is no, does it mean user actually uses a signed driver? And it means user is supposed to involve the OVS packages including the driver in the image by themselves if Containerized OVS daemons is yes in the meanwhile?

supported. For clusters running a version higher than 1.26, you should skip
the preparation of the kube-proxy network adapter by executing the command
`.\Prepare-AntreaAgent.ps1 -InstallKubeProxy $false`.
The script `Prepare-AntreaAgent.ps1` performs the following tasks:
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: do we have a plan to remove the support for Windows userspace kube-proxy from this document since it is deprecated?

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 think we should remove it completely from the documentation post v1.15, for the v2.0 release.

New-KubeProxyServiceInterface

New-DirectoryIfNotExist "${AntreaHome}/logs"
New-DirectoryIfNotExist "${KubeProxyLogPath}"
Copy link
Contributor

Choose a reason for hiding this comment

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

line 376 and line 379 are supposed to be under the block if ($InstallKubeProxy) in line 381 ?

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 copied this script as it was before. I didn't make any change even though that may not be obvious from the Github diff.
I think you're right but on the other hand, it should not be an issue to keep things as they are (even if we create the interface for nothing)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No issues were introduced to create the new interface and directories, just not necessary. Maybe we can remove them when kube-proxy is removed from the documentation.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas
Copy link
Contributor Author

/skip-all

@antoninbas antoninbas merged commit 913884b into antrea-io:main Jan 17, 2024
49 of 52 checks passed
@antoninbas antoninbas deleted the update-windows-doc branch January 17, 2024 20:05
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/OS/windows Issues or PRs related to the Windows operating system. kind/documentation Categorizes issue or PR as related to a documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants