Use helm to generate Antrea Windows manifests #6360

shikharish · 2024-05-22T21:17:09Z

Fixes #5564

shikharish · 2024-05-22T21:18:21Z

cc: @antoninbas @luolanzone

build/charts/antrea-windows/templates/agent.yaml

build/charts/antrea-windows/templates/configmap.yaml

build/charts/antrea-windows/templates/daemonset.yaml

build/yamls/antrea-windows-with-ovs.yml

luolanzone · 2024-05-23T03:57:28Z

build/yamls/windows/patches/release/.gitignore

Do we still need this file or the directory? @antoninbas

build/charts/antrea-windows/.helmignore

antoninbas

Are you missing the values.yaml file?

You also need to rebase this PR because of conflicts caused by another change that was merged yesterday.

build/charts/antrea-windows/Chart.yaml

antoninbas · 2024-05-24T17:38:48Z

build/charts/antrea-windows/Chart.yaml

+appVersion: latest
+kubeVersion: ">= 1.16.0-0"
+icon: https://raw.githubusercontent.com/antrea-io/antrea/main/docs/assets/logo/antrea_logo.svg
+description: Kubernetes networking based on Open vSwitch


Suggested change

description: Kubernetes networking based on Open vSwitch

description: Kubernetes networking based on Open vSwitch for Windows Nodes

antoninbas · 2024-05-24T17:42:19Z

build/charts/antrea-windows/templates/agent.yaml

why is the name agent.yaml?
Maybe we could have configmaps/antrea-agent-windows.yaml and configmaps/antrea-windows-config.yaml instead?
Or alternatively configmap-antrea-agent-windows.yaml and configmap-antrea-windows-config.yaml.

antoninbas · 2024-05-24T17:43:22Z

build/charts/antrea-windows/conf/Install-WindowsCNI-Containerd.ps1

In a PR that was merged yesterday we have stripped the -Containerd suffix for all these files, so you will need to look out for this

antoninbas · 2024-05-24T17:44:09Z

build/charts/antrea-windows/templates/agent.yaml

+ {{- tpl ((.Files.Glob "conf/*.ps1").AsConfig) . | nindent 2 | replace " \n" "\n" }}
+ {{- if .Values.includeOVS }}
+ {{- tpl ((.Files.Glob "conf/ovs/*.ps1").AsConfig) . | nindent 2 | replace " \n" "\n" }}
+ {{- end }}


I feel like it may be more readable to list all files individually in this case (unlike for the main Linux chart). Do you think it is possible?

Same comment for the other ConfigMap

luolanzone

A few nits

build/charts/antrea-windows/Chart.yaml

hack/generate-manifest-windows.sh

build/charts/antrea-windows/values.yaml

Fixes antrea-io#5564 Signed-off-by: Shikhar Soni <[email protected]>

antoninbas

If you run cd build/charts && make locally, it should generate a README.md file for the chart (build/charts/antrea-windows/README.md) and you need to check it in. Otherwise CI should fail (I believe this is why the Go / Verify docs and spelling job is failing at the moment).

Another thing: did you confirm that ./hack/release/prepare-assets.sh runs correctly after this change? It should produce the correct Windows manifests, with the correct image tags (versioned tags instead of latest).

hack/generate-manifest-windows.sh

antoninbas · 2024-05-30T22:51:18Z

build/charts/antrea/Chart.yaml

@@ -5,7 +5,7 @@ displayName: Antrea
 home: https://antrea.io/
 version: 0.0.0
 appVersion: latest
-kubeVersion: ">= 1.16.0-0"
+kubeVersion: ">= 1.19.0-0"


It's pretty minor but this change (and the corresponding one in build/charts/flow-aggregator/Chart.yaml) should be in a sepatate PR (that we could even backport to release-2.0).

Alright reverting this.

You need to run make again under build/charts after this change.
Also, could you open a new PR to update the kubeVersion requirement for existing charts?

ping for this

build/charts/antrea-windows/values.yaml

Signed-off-by: Shikhar Soni <[email protected]>

antoninbas

@wenyingd would you mind taking a quick look?

antoninbas · 2024-05-31T22:23:02Z

build/charts/antrea-windows/values.yaml

+ repository: "antrea/antrea-windows"
+ tag: "latest"
+
+# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent pod


Suggested change

# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent pod

# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent Pod

(remember to regenerate the README)

Signed-off-by: Shikhar Soni <[email protected]>

luolanzone

LGTM @wenyingd @XinShuYang please help to double check, thanks.

rajnkamr · 2024-06-03T08:57:32Z

build/charts/antrea-windows/values.yaml

+ tag: "latest"
+
+# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent Pod
+includeOVS: false


By default, when the includeOVS flag is set to false in the Helm chart values, it means that Windows Open vSwitch (OVS) processes inside the antrea-ovs container won't start automatically when deploying with Helm. However, OVS processes are essential for Antrea's operation. While it's acceptable to have includeOVS set to false by default for flexibility, it's crucial to explicitly mention in the documentation that when deploying with Helm, users must set includeOVS to true. The current pull request (PR) does not contain documentation updates regarding this setting.

I am fine with defaulting includeOVS to true in this PR, but it doesn't matter much: this PR introduces Helm templating to generate the Antrea Windows manifests, but at the moment we are not exposing this Helm chart to users. For the same reason, there is no need to update documentation as part of this PR.

We may start publishing the Helm chart in a subsequent release.

Defaulting includeOVS to true makes sense,
If the Helm chart is not currently exposed to users in current release then it would not be necessary to add documentation in this PR. Anyway We will revisit the documentation and user-facing aspects when we start considering publishing the Helm chart in a subsequent release.

@shikharish Could you default includeOVS to true and make the necessary changes for manifest generation? I think that going forward most users will let Antrea run OVS daemons inside a container, and when we do expose the chart to users, a default value of true will therefore make more sense.
Please use a separate commit for that change, for easier reviewing.

@antoninbas So generate-manifest-windows.sh by default should create the manifest with OVS enabled?

I think you can keep the same behavior for generate-manifest-windows.sh as now. But if the default value is changed for the chart, then the implementation of the script needs to change a little bit. In particular, if --include-ovs is not provided, you will need to explicitly set the chart value to false. I think the best thing to do in the script would be to always explicitly set the value based on the value of $INCLUDE_OVS.

Signed-off-by: Shikhar Soni <[email protected]>

wenyingd

The code change looks good to me.

One question is I saw that this file build/charts/antrea-windows/conf/antrea-agent.conf is actually renamed from this file https://github.com/antrea-io/antrea/blob/main/build/yamls/windows/base/conf/antrea-agent.conf , so it is not strictly using helm key-value format as what Linux was doing. Do we have plan to use another patch to modify it or we will keep the current actions to separately maintain the Windows configurations? @antoninbas

XinShuYang · 2024-06-05T03:19:56Z

build/charts/antrea-windows/README.md

+
+## Requirements
+
+Kubernetes: `>= 1.19.0-0`


@shikharish @antoninbas Could you explain why 1.19 is set as the minimum required version? Is it because the versions of Helm and Kubernetes are related?

@XinShuYang please check this issue #5879 for the discussion. We changed the minimum supported K8s to v1.19 in Antrea since 2.0.

@XinShuYang please check this issue #5879 for the discussion. We changed the minimum supported K8s to v1.19 in Antrea since 2.0.

Since we have removed windows docker support, the windows container manifest can only be deployed when windows host process feature is enabled. But as I know this feature was first introduced in kubernetes 1.22 (https://kubernetes.io/blog/2021/08/16/windows-hostprocess-containers/) Maybe we should update the minimum supported version for antrea windows? Do you have any suggestion? @antoninbas @luolanzone @wenyingd

That's a good point. @shikharish please update the version requirement to 1.22 for this chart (and this chart only), and re-generate the Readme

hack/generate-manifest-windows.sh

antoninbas · 2024-06-05T17:16:34Z

One question is I saw that this file build/charts/antrea-windows/conf/antrea-agent.conf is actually renamed from this file https://github.com/antrea-io/antrea/blob/main/build/yamls/windows/base/conf/antrea-agent.conf , so it is not strictly using helm key-value format as what Linux was doing. Do we have plan to use another patch to modify it or we will keep the current actions to separately maintain the Windows configurations? @antoninbas

@wenyingd the file is not using templating at the moment. This is because the chart is not exposed to users yet and we only use it for helm templating. Before exposing the chart to users, we will add the appropriate configuration values to values.yaml, and we will use these values to render the configuration. Then it will more closely match what we do for Linux. For now, the only Helm values we have are the ones necessary to render the 2 different supported Windows manifests. Hopefully I understood your question correctly.

wenyingd · 2024-06-06T00:33:22Z

@wenyingd the file is not using templating at the moment. This is because the chart is not exposed to users yet and we only use it for helm templating. Before exposing the chart to users, we will add the appropriate configuration values to values.yaml, and we will use these values to render the configuration. Then it will more closely match what we do for Linux. For now, the only Helm values we have are the ones necessary to render the 2 different supported Windows manifests. Hopefully I understood your question correctly.

Got it, thanks for the explanation.

XinShuYang · 2024-06-06T08:22:11Z

build/charts/antrea-windows/templates/daemonset.yaml

+ - $env:CONTAINER_SANDBOX_MOUNT_POINT/var/lib/antrea-windows/Install-OVSDriver.ps1
+ command:
+ - powershell
+ image: antrea/antrea-windows:latest


I think this value should not be hardcoded, could you update it to align with the others? This bug was introduced by a previous PR, but it would be better if we could fix it in this PR. The GitHub CI didn't catch this bug because when running generate-manifest-windows.sh, it works well in the dev mode but only fails in the release mode. cc @antoninbas

Thanks for catching this @XinShuYang
@shikharish please replace with {{ .Values.image.repository }}:{{ .Values.image.tag }}

Signed-off-by: Shikhar Soni <[email protected]>

antoninbas · 2024-06-07T17:36:36Z

/test-windows-all

antoninbas · 2024-06-10T17:15:10Z

/test-windows-containerd-networkpolicy
/test-windows-containerd-e2e

wenyingd · 2024-06-11T09:13:54Z

build/charts/antrea-windows/Chart.yaml

+home: https://antrea.io/
+version: 0.0.0
+appVersion: latest
+kubeVersion: ">= 1.19.0-0"


wenyingd · 2024-06-11T09:14:02Z

build/charts/antrea-windows/README.md

+
+## Requirements
+
+Kubernetes: `>= 1.19.0-0`


XinShuYang · 2024-06-11T15:54:10Z

/test-windows-containerd-networkpolicy
/test-windows-containerd-e2e

Signed-off-by: Shikhar Soni <[email protected]>

antoninbas · 2024-06-11T16:52:59Z

/test-windows-all

antoninbas

LGTM, thanks for working on this!

After merging antrea-io#6360, this directory no longer exists. Signed-off-by: Antonin Bas <[email protected]>

After merging #6360, this directory no longer exists. Signed-off-by: Antonin Bas <[email protected]>

antoninbas requested review from antoninbas and luolanzone May 22, 2024 21:23

shikharish force-pushed the helm-windows branch from f9ac4ef to 1f6d34d Compare May 22, 2024 22:17

luolanzone reviewed May 23, 2024

View reviewed changes

shikharish requested a review from luolanzone May 23, 2024 16:23

luolanzone added the area/OS/windows Issues or PRs related to the Windows operating system. label May 24, 2024

antoninbas reviewed May 24, 2024

View reviewed changes

shikharish force-pushed the helm-windows branch from 087f1a0 to f956c95 Compare May 25, 2024 12:55

shikharish requested a review from antoninbas May 26, 2024 07:07

luolanzone reviewed May 28, 2024

View reviewed changes

build/charts/antrea-windows/Chart.yaml Outdated Show resolved Hide resolved

hack/generate-manifest-windows.sh Show resolved Hide resolved

hack/generate-manifest-windows.sh Outdated Show resolved Hide resolved

build/charts/antrea-windows/values.yaml Outdated Show resolved Hide resolved

luolanzone requested review from wenyingd and XinShuYang May 28, 2024 08:34

Use helm to generate Antrea Windows manifests

f1d17c0

Fixes antrea-io#5564 Signed-off-by: Shikhar Soni <[email protected]>

shikharish force-pushed the helm-windows branch from f956c95 to f1d17c0 Compare May 29, 2024 18:06

shikharish requested a review from luolanzone May 29, 2024 18:07

antoninbas reviewed May 30, 2024

View reviewed changes

minor changes

25ce3ca

Signed-off-by: Shikhar Soni <[email protected]>

antoninbas mentioned this pull request May 31, 2024

[Windows] Optimize the containerized OVS installation #6383

Open

minor changes

def72f9

Signed-off-by: Shikhar Soni <[email protected]>

shikharish requested a review from antoninbas May 31, 2024 22:08

antoninbas reviewed May 31, 2024

View reviewed changes

minor changes

bcf52ed

Signed-off-by: Shikhar Soni <[email protected]>

luolanzone previously approved these changes Jun 3, 2024

View reviewed changes

rajnkamr reviewed Jun 3, 2024

View reviewed changes

set includeOVS to true by default

6512f16

Signed-off-by: Shikhar Soni <[email protected]>

shikharish dismissed luolanzone’s stale review via 6512f16 June 4, 2024 20:27

shikharish requested a review from antoninbas June 4, 2024 20:28

antoninbas previously approved these changes Jun 4, 2024

View reviewed changes

wenyingd previously approved these changes Jun 5, 2024

View reviewed changes

XinShuYang reviewed Jun 5, 2024

View reviewed changes

hack/generate-manifest-windows.sh Show resolved Hide resolved

luolanzone added this to the Antrea v2.1 release milestone Jun 5, 2024

XinShuYang reviewed Jun 6, 2024

View reviewed changes

minor fix

3fde559

Signed-off-by: Shikhar Soni <[email protected]>

shikharish dismissed stale reviews from wenyingd and antoninbas via 3fde559 June 7, 2024 17:34

shikharish requested a review from antoninbas June 7, 2024 17:35

antoninbas previously approved these changes Jun 7, 2024

View reviewed changes

wenyingd reviewed Jun 11, 2024

View reviewed changes

update kube version

52be112

Signed-off-by: Shikhar Soni <[email protected]>

shikharish dismissed antoninbas’s stale review via 52be112 June 11, 2024 16:08

shikharish requested review from wenyingd and antoninbas June 11, 2024 16:08

antoninbas approved these changes Jun 11, 2024

View reviewed changes

antoninbas merged commit 6fce4e8 into antrea-io:main Jun 11, 2024
52 of 60 checks passed

antoninbas added a commit to antoninbas/antrea that referenced this pull request Jun 12, 2024

Fix invalid links to build/yamls/windows

27b05a8

After merging antrea-io#6360, this directory no longer exists. Signed-off-by: Antonin Bas <[email protected]>

antoninbas mentioned this pull request Jun 12, 2024

Fix invalid links to build/yamls/windows #6435

Merged

antoninbas added a commit that referenced this pull request Jun 13, 2024

Fix invalid links to build/yamls/windows (#6435)

615907d

After merging #6360, this directory no longer exists. Signed-off-by: Antonin Bas <[email protected]>

	description: Kubernetes networking based on Open vSwitch
	description: Kubernetes networking based on Open vSwitch for Windows Nodes

	# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent pod
	# -- Enable running Windows OVS processes inside antrea-ovs container in antrea-agent Pod

Use helm to generate Antrea Windows manifests #6360

Use helm to generate Antrea Windows manifests #6360

Conversation

shikharish commented May 22, 2024

shikharish commented May 22, 2024

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Jun 5, 2024

wenyingd commented Jun 6, 2024

XinShuYang Jun 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Jun 7, 2024

antoninbas commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinShuYang commented Jun 11, 2024

antoninbas commented Jun 11, 2024

antoninbas left a comment

Choose a reason for hiding this comment

luolanzone Jun 5, 2024 •

edited

XinShuYang Jun 6, 2024 •

edited