Add an Helm chart for Antrea #3578

antoninbas · 2022-04-01T23:06:53Z

And use the Helm templates (instead of Kustomize) to generate the
standard Antrea YAML manifests (which are checked-in and uploaded as
release assets).

Standard manifests are generated based on Helm values files located
under build/yamls/chart-values/, using a new script
(./hack/generate-standard-manifests.sh). It is much faster than the old
version.

While I believe that using Helm directly and specifying YAML values
whenever a new manifest needs to be generated would be better, the
./hack/generate-manifest.sh script is kept as-is, but it now uses Helm
instead of Kustomize.

Documentation for the Helm chart is autogenerated using helm-docs. In a
future PR, we will look into the release process for the Helm
chart. After that, Helm charts could be added for Antrea components
(Flow Aggregator, Flow visibility).

For #2641

Signed-off-by: Antonin Bas abas@vmware.com

antoninbas · 2022-04-01T23:09:03Z

All the actual changes are in the first commit. The second commit (big diff) is for auto-generated YAML changes.

antoninbas · 2022-04-01T23:24:22Z

I used dyff between to compare generated manifests and ensure that there were no unintended changes. It is even K8s aware and is pretty good to compare actual K8s manifest differences. It doesn't work well for the Antrea config though, since it is embedded YAML (ConfigMap data).

codecov-commenter · 2022-04-01T23:29:42Z

Codecov Report

Merging #3578 (ba53a18) into main (2ab80d0) will increase coverage by 1.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3578      +/-   ##
==========================================
+ Coverage   63.62%   64.81%   +1.19%     
==========================================
  Files         278      278              
  Lines       39360    39360              
==========================================
+ Hits        25041    25510     +469     
+ Misses      12381    11879     -502     
- Partials     1938     1971      +33

Flag	Coverage Δ
e2e-tests	`49.91% <ø> (?)`
kind-e2e-tests	`52.23% <ø> (+1.48%)`	⬆️
unit-tests	`43.79% <ø> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apiserver/handlers/endpoint/handler.go	`56.52% <0.00%> (-13.05%)`	⬇️
pkg/apiserver/certificate/certificate.go	`70.37% <0.00%> (-6.49%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`68.54% <0.00%> (-3.23%)`	⬇️
...g/controller/networkpolicy/store/appliedtogroup.go	`87.61% <0.00%> (-2.86%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`77.55% <0.00%> (-1.54%)`	⬇️
pkg/controller/externalippool/controller.go	`84.82% <0.00%> (-1.34%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`85.88% <0.00%> (-1.18%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`54.45% <0.00%> (-0.97%)`	⬇️
pkg/agent/cniserver/pod_configuration.go	`53.75% <0.00%> (-0.29%)`	⬇️
pkg/agent/memberlist/cluster.go	`74.44% <0.00%> (+0.31%)`	⬆️
... and 19 more

tnqn

LGTM overall

tnqn · 2022-04-07T16:24:39Z

.github/workflows/helm_docs.yml

+    - feature/*
+
+jobs:
+  check-helm-docs:


could this be a step in the existing "Verify docs and spelling" job? I found there may be a long queue waiting for runners when debugging, do you know runner will be allocated per workflow or job?

there is one runner per job, I made the change you suggested

tnqn · 2022-04-07T16:40:13Z

build/charts/antrea/conf/antrea-controller.conf

+#   ca.crt: <CA certificate>
+#   tls.crt: <TLS certificate>
+#   tls.key: <TLS private key>
+selfSignedCert: {{ .Values.controller.selfSigned }}


is it intended to remove "Cert"?

no not intended, not sure why I removed it in the first place

tnqn · 2022-04-07T16:55:00Z

build/charts/antrea/templates/agent/daemonset.yaml

+            - "--log_file_max_size={{ .Values.agent.antreaOVS.logFileMaxSize }}"
+            - "--log_file_max_num={{ .Values.agent.antreaOVS.logFileMaxNum }}"
+            {{- if .Values.logVerbosity }}
+            - "--v={{ .Values.logVerbosity }}"


Does start_ovs support "--v"? this may lead to failure once set?

luolanzone

LGTM overall, just two nits.

luolanzone · 2022-04-11T02:30:50Z

build/yamls/patches/release/.gitignore

@@ -1 +0,0 @@
-# placeholder


not release with this PR change, but I saw there are a few other .gitignore files have the same #placeholder, do we need them? eg: build/yamls/flow-visibility/patches/release/.gitignore.

luolanzone · 2022-04-11T02:33:00Z

build/yamls/chart-values/antrea-aks.yml

@@ -0,0 +1 @@
+trafficEncapMode: "networkPolicyOnly"


Could you add a comment in file build/yamls/chart-values/antrea.yml, it leads a little confusion when I see it's empty. I am unable to leave a comment to an empty file, so leave it here.

luolanzone · 2022-04-11T02:44:18Z

btw, the step Checking whether autogenerated Helm chart documentation is up-to-date in go.yml didn't run because of doc verify failure, looks like you need to rerun make helm-docs to regenerate README.md, there is a variable change from controller.selfSigned to controller.selfSignedCert

jianjuns · 2022-04-11T21:10:05Z

build/charts/antrea/Chart.yaml

+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
+keywords:


Should we add "OVS"?

I'll look into adding keywords in a follow-up PR. I still have to figure versioning & release upload for the Helm chart.

hack/generate-manifest.sh

hack/generate-standard-manifests.sh

hack/verify-helm.sh

tnqn

LGTM

antoninbas · 2022-04-12T18:18:43Z

/test-all
/test-ipv6-all

antoninbas · 2022-04-13T18:54:30Z

/test-e2e
/test-ipv6-conformance
/test-ipv6-e2e
/test-ipv6-only-all

And use the Helm templates (instead of Kustomize) to generate the standard Antrea YAML manifests (which are checked-in and uploaded as release assets). Standard manifests are generated based on Helm values files located under build/yamls/chart-values/, using a new script (./hack/generate-standard-manifests.sh). It is much faster than the old version. While I believe that using Helm directly and specifying YAML values whenever a new manifest needs to be generated would be better, the ./hack/generate-manifest.sh script is kept as-is, but it now uses Helm instead of Kustomize. Documentation for the Helm chart is autogenerated using helm-docs. In a future PR, we will look into the release process for the Helm chart. After that, Helm charts could be added for Antrea components (Flow Aggregator, Flow visibility). For antrea-io#2641 Signed-off-by: Antonin Bas <abas@vmware.com>

Signed-off-by: Antonin Bas <abas@vmware.com>

antoninbas · 2022-04-15T00:45:43Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

antoninbas · 2022-04-15T19:35:23Z

/test-ipv6-e2e
/test-ipv6-only-e2e
/test-ipv6-conformance

antoninbas · 2022-04-18T17:26:09Z

/test-integration
/test-ipv6-e2e

antoninbas · 2022-04-18T23:37:25Z

/test-ipv6-e2e

antoninbas · 2022-04-19T01:05:55Z

Merging this now as the jenkins-ipv6-ds-e2e job is broken

hangyan · 2022-04-19T03:23:34Z

Sorry i didn't notice this PR before. Thanks. A few comments:

In helm3, a new crds dir is introduced to make sure crds are always installed first. it should be the same level of directory as templates
we can also add the LICENSE file to the charts
Maybe we can add some links to our docs in the NOTES.txt ? or following actions, like list antrea pods command?
I think github have native support for hosts a charts repo ( for publishing antrea chart), maybe there is a github action for that too. Are we planning to support this?
@antoninbas

hangyan · 2022-04-19T03:24:23Z

https://github.com/helm/chart-releaser-action

antoninbas · 2022-04-19T22:09:19Z

@hangyan thanks for the comments, this is the first PR to add Helm support, and I haven't looked at the release publishing process yet.

Your first point about CRDs is interesting. I have intentionally not placed CRDs in the crds directory for now, as it comes with some limitations (no automatic upgrade). I have been unsure about the correct course of action, and I'll open an issue to discuss this.

hangyan · 2022-04-20T02:38:18Z

@antoninbas That make sense, i also have some doubts about this choice, just rise it up to confirm that we have considered all the limitations of different approaches..

antoninbas added action/release-note Indicates a PR that should be included in release notes. area/build-release Issues or PRs related to building and releasing labels Apr 1, 2022

antoninbas force-pushed the add-helm-chart branch 3 times, most recently from c84ecf0 to ecd5e94 Compare April 1, 2022 23:19

antoninbas force-pushed the add-helm-chart branch 3 times, most recently from 6eff071 to f04c15a Compare April 5, 2022 19:02

tnqn reviewed Apr 7, 2022

View reviewed changes

antoninbas force-pushed the add-helm-chart branch from f04c15a to 2c70f86 Compare April 7, 2022 19:32

antoninbas mentioned this pull request Apr 8, 2022

Support ClickHouse deployment with Persistent Volume #3608

Merged

antoninbas force-pushed the add-helm-chart branch from 3611fef to 90f738c Compare April 8, 2022 22:23

antoninbas requested review from tnqn, jianjuns and luolanzone April 8, 2022 22:23

luolanzone reviewed Apr 11, 2022

View reviewed changes

antoninbas force-pushed the add-helm-chart branch from 90f738c to ffa6e22 Compare April 11, 2022 18:13

jianjuns reviewed Apr 11, 2022

View reviewed changes

antoninbas force-pushed the add-helm-chart branch from ffa6e22 to 274cb5b Compare April 11, 2022 22:12

tnqn previously approved these changes Apr 12, 2022

View reviewed changes

antoninbas added 4 commits April 14, 2022 17:43

Updated standard manifests

e749866

Signed-off-by: Antonin Bas <abas@vmware.com>

Ignore auto-generated Helm chart doc for markdownlint

e35b8f2

Signed-off-by: Antonin Bas <abas@vmware.com>

Fix kubeAPIServerOverride substitution in test-e2e-kind.sh

3781104

Signed-off-by: Antonin Bas <abas@vmware.com>

Address review comments

ba53a18

Signed-off-by: Antonin Bas <abas@vmware.com>

antoninbas dismissed tnqn’s stale review via ba53a18 April 15, 2022 00:45

antoninbas force-pushed the add-helm-chart branch from 274cb5b to ba53a18 Compare April 15, 2022 00:45

jianjuns approved these changes Apr 15, 2022

View reviewed changes

antoninbas added this to the Antrea v1.7 release milestone Apr 15, 2022

antoninbas merged commit 4a3b76c into antrea-io:main Apr 19, 2022

antoninbas deleted the add-helm-chart branch April 19, 2022 01:06

jianjuns mentioned this pull request Apr 19, 2022

[ExternalNode] Add ExternalNode CRD #3639

Merged

abdallahyas mentioned this pull request Apr 29, 2022

Manifests building with hw-offload is broken due to wrong helm templates #3705

Closed

yanjunz97 mentioned this pull request May 10, 2022

Add Helm support for Theia with ClickHouse PV antrea-io/theia#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an Helm chart for Antrea #3578

Add an Helm chart for Antrea #3578

antoninbas commented Apr 1, 2022

antoninbas commented Apr 1, 2022

antoninbas commented Apr 1, 2022

codecov-commenter commented Apr 1, 2022 •

edited

Loading

tnqn left a comment

tnqn Apr 7, 2022

antoninbas Apr 8, 2022

tnqn Apr 7, 2022

antoninbas Apr 8, 2022

tnqn Apr 7, 2022

antoninbas Apr 8, 2022

luolanzone left a comment

luolanzone Apr 11, 2022

luolanzone Apr 11, 2022

luolanzone commented Apr 11, 2022

jianjuns Apr 11, 2022

antoninbas Apr 11, 2022

tnqn left a comment

antoninbas commented Apr 12, 2022

antoninbas commented Apr 13, 2022

antoninbas commented Apr 15, 2022

antoninbas commented Apr 15, 2022

antoninbas commented Apr 18, 2022

antoninbas commented Apr 18, 2022

antoninbas commented Apr 19, 2022

hangyan commented Apr 19, 2022 •

edited

Loading

hangyan commented Apr 19, 2022

antoninbas commented Apr 19, 2022

hangyan commented Apr 20, 2022 •

edited

Loading

Add an Helm chart for Antrea #3578

Add an Helm chart for Antrea #3578

Conversation

antoninbas commented Apr 1, 2022

antoninbas commented Apr 1, 2022

antoninbas commented Apr 1, 2022

codecov-commenter commented Apr 1, 2022 • edited Loading

Codecov Report

tnqn 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

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

luolanzone commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Apr 12, 2022

antoninbas commented Apr 13, 2022

antoninbas commented Apr 15, 2022

antoninbas commented Apr 15, 2022

antoninbas commented Apr 18, 2022

antoninbas commented Apr 18, 2022

antoninbas commented Apr 19, 2022

hangyan commented Apr 19, 2022 • edited Loading

hangyan commented Apr 19, 2022

antoninbas commented Apr 19, 2022

hangyan commented Apr 20, 2022 • edited Loading

codecov-commenter commented Apr 1, 2022 •

edited

Loading

hangyan commented Apr 19, 2022 •

edited

Loading

hangyan commented Apr 20, 2022 •

edited

Loading