Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins #3454

jainpulkit22 · 2022-03-16T05:59:58Z

This PR adds a new feature by introducing skip_cni_binaries variable, to allow users to skip
installing some of the CNI plugin binaries which they think are not
required for certain reasons.

Signed-off-by: Pulkit Jain [email protected]

codecov-commenter · 2022-03-16T06:25:59Z

Codecov Report

Merging #3454 (68ccf8a) into main (ed0ad19) will decrease coverage by 10.31%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #3454       +/-   ##
===========================================
- Coverage   65.28%   54.97%   -10.32%     
===========================================
  Files         268      374      +106     
  Lines       26900    41447    +14547     
===========================================
+ Hits        17563    22784     +5221     
- Misses       7432    16263     +8831     
- Partials     1905     2400      +495

Flag	Coverage Δ
integration-tests	`35.82% <ø> (?)`
kind-e2e-tests	`55.76% <ø> (+0.04%)`	⬆️
unit-tests	`42.44% <ø> (ø)`

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

Impacted Files	Coverage Δ
pkg/agent/apiserver/apiserver.go	`69.56% <0.00%> (-10.15%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`93.54% <0.00%> (-6.46%)`	⬇️
pkg/ovs/ovsctl/appctl.go	`42.39% <0.00%> (-2.18%)`	⬇️
pkg/agent/openflow/pipeline.go	`79.80% <0.00%> (-0.07%)`	⬇️
...ea/pkg/agent/ipassigner/local_ip_detector_linux.go	`0.00% <0.00%> (ø)`
...g/agent/cniserver/interface_configuration_linux.go	`24.87% <0.00%> (ø)`
antrea/pkg/apis/controlplane/v1beta2/register.go	`91.66% <0.00%> (ø)`
.../agent/flowexporter/connections/conntrack_linux.go	`2.41% <0.00%> (ø)`
antrea/pkg/agent/config/node_config.go	`50.00% <0.00%> (ø)`
antrea/pkg/agent/openflow/network_policy.go	`49.86% <0.00%> (ø)`
... and 106 more

tnqn · 2022-03-16T13:19:51Z

build/yamls/base/agent.yml

+          env:
+            - name: SKIP_CNI_BINARIES
+              value: ""
+


please remove this line

tnqn · 2022-03-18T08:13:48Z

build/images/scripts/install_cni

+for args in $flags; do
+  binaries+=($args)
+done
+IFS=$oldDelimeter


The above can be written in one line:

IFS=',' read -r -a binaries <<< "$SKIP_CNI_BINARIES"

I think by specifying IFS before the command, it only affects that line. Maybe you can test to verify it.

tnqn · 2022-03-18T08:34:37Z

build/images/scripts/install_cni

 # Install the loopback plugin.
 # It is required by kubelet on Linux when using docker as the container runtime.
 # We replace the binary files even they are already present on the Node to make
 # sure expected versions are used.
-install -m 755 /opt/cni/bin/loopback /host/opt/cni/bin/loopback
+if [[ ! " ${binaries[*]}" =~ "loopback" ]]; then


Although it works for current case, it's not very precise. For example, if there is one cni called loopback2, it would be matched by mistake. This would be more precise:

if [[ ! " ${binaries[*]} " =~ " loopback " ]]; then

luolanzone · 2022-03-18T09:09:00Z

build/yamls/antrea-aks.yml

@@ -3296,6 +3296,9 @@ spec:
      initContainers:
      - command:
        - install_cni_chaining
+        env:
+        - name: SKIP_CNI_BINARIES


Could you provide a sample about how to set this variable? I didn't see you mention it anywhere.
I remember kustomize will remove all comments from yaml @tnqn any suggestion about where to put the sample information?

can I add the comment in the antrea.yml file

It should be used by distros and end users won't care about it. So perhaps adding comment to the base yaml is enough, I think it's fine to not have it in the generated yaml.

BTW, could you update the PR title to be more accurate?

yes sure would do that

tnqn · 2022-03-18T10:26:46Z

build/images/scripts/install_cni

+for args in $flags; do
+  binaries+=($args)
+done
+IFS=$oldDelimeter


I think by specifying IFS before the command, it only affects that line. Maybe you can test to verify it.

tnqn · 2022-03-18T10:28:49Z

build/yamls/base/agent.yml

@@ -48,6 +48,11 @@ spec:
              add:
                # SYS_MODULE is required to load the OVS kernel module.
                - SYS_MODULE
+          env:
+            # SKIP_CNI_BINARIES takes in values as a comma separated list of
+            # binaries that need to be skipped.


Suggested change

# binaries that need to be skipped.

# binaries that need to be skipped for installation, e.g. "portmap, bandwdith".

tnqn · 2022-03-18T10:34:19Z

This title may be more proper: Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins

tnqn

LGTM, thanks

tnqn · 2022-03-18T11:19:43Z

/test-all

tnqn · 2022-03-18T12:46:37Z

build/yamls/base/agent.yml

@@ -48,6 +48,11 @@ spec:
              add:
                # SYS_MODULE is required to load the OVS kernel module.
                - SYS_MODULE
+          env:
+            # SKIP_CNI_BINARIES takes in values as a comma separated list of
+            # binaries that need to be skipped for installation, e.g. "portmap, bandwdith".


Suggested change

# binaries that need to be skipped for installation, e.g. "portmap, bandwdith".

# binaries that need to be skipped for installation, e.g. "portmap, bandwidth".

This caused spelling check failure

my bad, I din't gave a relook to it. I will make the change

…d cni bins This commit adds a skip_cni_binaries variable, to allow users to skip installing some of the CNI plugin binaries which they think are not required for certain reasons. Signed-off-by: Pulkit Jain <[email protected]>

tnqn

LGTM

tnqn · 2022-03-18T14:08:11Z

/test-all

build/images/scripts/install_cni

antoninbas · 2022-03-18T17:41:34Z

build/images/scripts/install_cni

@@ -2,25 +2,38 @@



I wonder if in the future we should consider replacing this bash script with a small Go binary for maintainability and unit testing?

jainpulkit22 force-pushed the add-cni-plugin-install-policy branch 2 times, most recently from 68ccf8a to 3a0ef43 Compare March 16, 2022 13:05

jainpulkit22 changed the title ~~Added feature CNI_PLUGIN_INSTALL_POLICY~~ Add CNI_PLUGIN_INSTALL_POLICY for custom /opt/cni/bin plugin bundles Mar 16, 2022

jainpulkit22 marked this pull request as ready for review March 16, 2022 13:09

tnqn reviewed Mar 18, 2022

View reviewed changes

luolanzone reviewed Mar 18, 2022

View reviewed changes

jainpulkit22 changed the title ~~Add CNI_PLUGIN_INSTALL_POLICY for custom /opt/cni/bin plugin bundles~~ Add SKIP_CNI_BINARIES feature for custom /opt/cni/bin plugin bundles Mar 18, 2022

jainpulkit22 force-pushed the add-cni-plugin-install-policy branch from 3a0ef43 to fe0a93e Compare March 18, 2022 10:24

tnqn reviewed Mar 18, 2022

View reviewed changes

jainpulkit22 changed the title ~~Add SKIP_CNI_BINARIES feature for custom /opt/cni/bin plugin bundles~~ Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins Mar 18, 2022

jainpulkit22 force-pushed the add-cni-plugin-install-policy branch from fe0a93e to f38ddda Compare March 18, 2022 10:57

tnqn previously approved these changes Mar 18, 2022

View reviewed changes

tnqn reviewed Mar 18, 2022

View reviewed changes

jainpulkit22 dismissed tnqn’s stale review via cfac4d6 March 18, 2022 12:57

jainpulkit22 force-pushed the add-cni-plugin-install-policy branch from f38ddda to cfac4d6 Compare March 18, 2022 12:57

tnqn approved these changes Mar 18, 2022

View reviewed changes

antoninbas approved these changes Mar 18, 2022

View reviewed changes

tnqn added this to the Antrea v1.6 release milestone Mar 21, 2022

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 21, 2022

tnqn merged commit e274ab1 into antrea-io:main Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins #3454

Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins #3454

jainpulkit22 commented Mar 16, 2022 •

edited

Loading

codecov-commenter commented Mar 16, 2022 •

edited

Loading

tnqn Mar 16, 2022

tnqn Mar 18, 2022

tnqn Mar 18, 2022

tnqn Mar 18, 2022

luolanzone Mar 18, 2022 •

edited

Loading

jainpulkit22 Mar 18, 2022

tnqn Mar 18, 2022

tnqn Mar 18, 2022

jainpulkit22 Mar 18, 2022

tnqn Mar 18, 2022

tnqn Mar 18, 2022

tnqn commented Mar 18, 2022

tnqn left a comment

tnqn commented Mar 18, 2022

tnqn Mar 18, 2022

jainpulkit22 Mar 18, 2022

tnqn left a comment

tnqn commented Mar 18, 2022

antoninbas Mar 18, 2022

	# binaries that need to be skipped.
	# binaries that need to be skipped for installation, e.g. "portmap, bandwdith".

Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins #3454

Add SKIP_CNI_BINARIES env var to support skipping installing specified cni bins #3454

Conversation

jainpulkit22 commented Mar 16, 2022 • edited Loading

codecov-commenter commented Mar 16, 2022 • edited Loading

Codecov Report

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 Mar 18, 2022 • edited Loading

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

tnqn commented Mar 18, 2022

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 18, 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

tnqn commented Mar 18, 2022

Choose a reason for hiding this comment

jainpulkit22 commented Mar 16, 2022 •

edited

Loading

codecov-commenter commented Mar 16, 2022 •

edited

Loading

luolanzone Mar 18, 2022 •

edited

Loading