Add cni bin when installing calico #9367

ErikJiang · 2022-10-08T12:20:51Z

Signed-off-by: bo.jiang bo.jiang@daocloud.io

What type of PR is this?
/kind feature

What this PR does / why we need it:
When installing calico, install cni-plugins along with it.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[Calico] Add cni bin when installing

yankay · 2022-10-08T12:27:43Z

There is another PR at the #9368 .

I think both PRs are good to solve the issue.

cristicalin · 2022-10-09T21:10:49Z

Please see the proposal I made in #9368 (comment) , it would make more sense to just enable the CNI plugins by default seeing as some distributions have a similar approach.

yankay · 2022-10-12T01:45:22Z

roles/network_plugin/calico/defaults/main.yml

@@ -163,3 +163,6 @@ calico_apiserver_enabled: false
 # Calico feature detect override, set "ChecksumOffloadBroken=true" to
 # solve the https://github.com/projectcalico/calico/issues/3145
 calico_feature_detect_override: ""
+
+# CNI plugins support
+enable_cni: true


HI @ErikJiang , Should we define it to the 'https://github.com/kubernetes-sigs/kubespray/blob/master/roles/kubespray-defaults/defaults/main.yaml#L175' as a global varable?

cyclinder · 2022-10-12T10:59:19Z

CI Flaky: https://github.com/kubernetes-sigs/kubespray/pull/9367/checks?check_run_id=8813987019

It looks like this CI Job will not download CNI-plugins, We should put the task of download cni-plugins into this job. Maybe add the following line to https://github.com/kubernetes-sigs/kubespray/blob/master/tests/files/packet_ubuntu20-calico-aio.yml#L11

# CNI plugins support
enable_cni: true

yankay · 2022-10-18T02:13:44Z

/lgtm

Thanks @ErikJiang

cristicalin · 2022-10-18T16:36:43Z

inventory/sample/group_vars/k8s_cluster/k8s-cluster.yml

@@ -69,6 +69,9 @@ credentials_dir: "{{ inventory_dir }}/credentials"
 # Can also be set to 'cloud', which lets the cloud provider setup appropriate routing
 kube_network_plugin: calico

+# CNI plugins support
+enable_cni: true


Why still mess with this? Just remove any option to disable this and always deploy the standard CNIs, this makes for a more predictable environment.

@cristicalin 😀 Do you mean that the enable_cni switch is unnecessary, and the cni-plugins should be permanently installed?

oomichi · 2022-10-19T05:27:50Z

roles/network_plugin/calico/meta/main.yml

+  - role: network_plugin/cni
+    when:
+      - enable_cni
+      - skip_cni is not defined


I am not sure why we need to have 2 options skip_cni and enable_cni.
Doesn’t enable_cni: false mean skipping cni installation?

@oomichi When executing calico's check or reset task, the dependencies in meta will be executed first, which is not expected, so I skip by defining the skip_cni variable, and enable_cni may be redundant, I plan to delete it.

Need to upte here to
- skip_cni is not defined or not skip_cni
for covering skip_cni: false case.

@oomichi 😊 Thanks, I have updated.

oomichi · 2022-10-21T04:18:49Z

Thanks for updating!

/lgtm

yankay · 2022-10-21T05:09:39Z

/lgtm

Thanks @ErikJiang

cristicalin · 2022-10-24T15:40:14Z

roles/network_plugin/calico/meta/main.yml

@@ -0,0 +1,5 @@
+---


I think this part should be replaced by just removing these lines: https://github.com/kubernetes-sigs/kubespray/blob/master/roles/network_plugin/meta/main.yml#L29-L31

😀 It does achieve the same effect and is more concise, thank you very much, @cristicalin

cristicalin · 2022-10-24T15:41:11Z

roles/kubernetes/preinstall/tasks/main.yml

@@ -129,6 +129,8 @@
  include_role:
    name: network_plugin/calico
    tasks_from: check
+  vars:


If you do this, you are essentially disabling the CNI each time since it will override any value passed from ansible group vars. Why do you need to do this?

cristicalin · 2022-10-24T15:43:38Z

roles/reset/tasks/main.yml

@@ -381,6 +381,8 @@
  include_role:
    name: "network_plugin/{{ kube_network_plugin }}"
    tasks_from: reset
+  vars:


Calling the reset task should not concern any dependencies of the called roles. If CNI plugins need to be reset then the call should be made for network_plugin/cni and network_plugin/{{ kube_network_plugin }}

yankay

Shoud we change the order to the first, because calico would overwide some CNI.

yankay · 2022-10-26T02:04:38Z

/lgtm

ErikJiang · 2022-10-28T10:53:52Z

😀 Updated and PTAL. @oomichi @cristicalin

Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

oomichi · 2022-11-08T23:52:42Z

Thanks for updating.
Looks clean and simple for me.

/approve

k8s-ci-robot · 2022-11-08T23:52:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ErikJiang, oomichi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [oomichi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cyclinder · 2022-11-09T01:35:21Z

Thanks 👍
/lgtm

yankay · 2022-11-09T01:43:36Z

Thanks @ErikJiang

/lgtm

Signed-off-by: bo.jiang <bo.jiang@daocloud.io> Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 8, 2022

k8s-ci-robot requested review from bozzo and cristicalin October 8, 2022 12:21

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 10, 2022

yankay reviewed Oct 12, 2022

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2022

k8s-ci-robot assigned yankay Oct 18, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2022

cristicalin reviewed Oct 18, 2022

View reviewed changes

oomichi reviewed Oct 19, 2022

View reviewed changes

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 19, 2022

k8s-ci-robot assigned oomichi Oct 21, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2022

cristicalin reviewed Oct 24, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2022

yankay reviewed Oct 26, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2022

Add cni bin when installing calico

9662dc8

Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2022

k8s-ci-robot merged commit 8a654b6 into kubernetes-sigs:master Nov 9, 2022

floryut added kind/network Network section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Nov 9, 2022

salifou pushed a commit to salifou/kubespray that referenced this pull request Nov 13, 2022

Add cni bin when installing calico (kubernetes-sigs#9367)

3bb9d2e

Signed-off-by: bo.jiang <bo.jiang@daocloud.io> Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

This was referenced Dec 12, 2022

Remove CNI BIN dependency for cilium #9563

Merged

Stop network_plugin/cni dependency when include_role comes from reset role #9550

Closed

floryut mentioned this pull request Jan 4, 2023

Release Proposal v2.21 #9638

Closed

enneitex pushed a commit to enneitex/kubespray that referenced this pull request Jan 25, 2023

Add cni bin when installing calico (kubernetes-sigs#9367)

7abc46b

Signed-off-by: bo.jiang <bo.jiang@daocloud.io> Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

HoKim98 pushed a commit to ulagbulag/kubespray that referenced this pull request Mar 8, 2023

Add cni bin when installing calico (kubernetes-sigs#9367)

d564ab1

Signed-off-by: bo.jiang <bo.jiang@daocloud.io> Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

nolimitkun pushed a commit to nolimitkun/kubespray that referenced this pull request Mar 19, 2023

Add cni bin when installing calico (kubernetes-sigs#9367)

5ac39bc

Signed-off-by: bo.jiang <bo.jiang@daocloud.io> Signed-off-by: bo.jiang <bo.jiang@daocloud.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cni bin when installing calico #9367

Add cni bin when installing calico #9367

ErikJiang commented Oct 8, 2022 •

edited by floryut

yankay commented Oct 8, 2022

cristicalin commented Oct 9, 2022

yankay Oct 12, 2022

ErikJiang Oct 13, 2022

cyclinder commented Oct 12, 2022 •

edited

yankay commented Oct 18, 2022 •

edited

cristicalin Oct 18, 2022

ErikJiang Oct 19, 2022

cristicalin Oct 19, 2022

oomichi Oct 19, 2022

ErikJiang Oct 19, 2022

oomichi Oct 20, 2022

ErikJiang Oct 20, 2022

oomichi commented Oct 21, 2022

yankay commented Oct 21, 2022 •

edited

cristicalin Oct 24, 2022

ErikJiang Oct 25, 2022

cristicalin Oct 24, 2022

cristicalin Oct 24, 2022

yankay left a comment

yankay commented Oct 26, 2022

ErikJiang commented Oct 28, 2022

oomichi commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

cyclinder commented Nov 9, 2022

yankay commented Nov 9, 2022

Add cni bin when installing calico #9367

Add cni bin when installing calico #9367

Conversation

ErikJiang commented Oct 8, 2022 • edited by floryut

yankay commented Oct 8, 2022

cristicalin commented Oct 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyclinder commented Oct 12, 2022 • edited

yankay commented Oct 18, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomichi commented Oct 21, 2022

yankay commented Oct 21, 2022 • 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

yankay left a comment

Choose a reason for hiding this comment

yankay commented Oct 26, 2022

ErikJiang commented Oct 28, 2022

oomichi commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

cyclinder commented Nov 9, 2022

yankay commented Nov 9, 2022

ErikJiang commented Oct 8, 2022 •

edited by floryut

cyclinder commented Oct 12, 2022 •

edited

yankay commented Oct 18, 2022 •

edited

yankay commented Oct 21, 2022 •

edited