[multus] added support for mixed type of container engine #9224

mr-yaky · 2022-08-26T12:13:08Z

@cristicalin I have reworked this PR completely according to your request and recommendations on the previous closed one. This is tested on our cluster, I have added only {{ image_arch }} because our tests were with 2.18.1 version of kubespray. So, I have adopted it also for the version from head.

What type of PR is this?
/kind feature

What this PR does / why we need it:
Kubespray should support mixed type of container engine, eg: docker+crio etc. This is very useful for large clusters during migrations to different container engines and when we need more time to test and adopt everything.

Which issue(s) this PR fixes:
The current Multus support has following issues:

Doesn't support more than one container engine type inside one cluster
Doesn't contain different Multus daemonsets yamls for each container engine type.
Uploads yaml manifests to all k8s node instead of saving them only on a k8s master (it applies only from master)
Should contain the latest fix for crio /run mount to prevent the issue: crio: mount /run rslave k8snetworkplumbingwg/multus-cni#802

Special notes for your reviewer:

This PR contains also additional logic which is needed to manage mixed type of container engine by using nodeSelector and node labels. For example:

% kubectl -n kube-system get ds -l app=multus
NAME                       DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   NODE SELECTOR                                                    AGE
kube-multus-crio-amd64     4         4         4       4            4           kubernetes.io/arch=amd64,kubespray.io/container_manager=crio     18h
kube-multus-docker-amd64   28        28        28      28           28          kubernetes.io/arch=amd64,kubespray.io/container_manager=docker   18h

kubespray.io/container_manager="" label with container engine type must be set manually by using group_vars and additional groups in your inventory, for example:

...
[kube_node_docker]
node01

[kube_node_crio]
node02

[kube_node:children]
kube_node_docker
kube_node_crio
...

% cat group_vars/kube_node_crio/all.yaml
container_manager: crio
node_labels:
  kubespray.io/container_manager: crio

multus_cni_version: "0.3.1"

% cat group_vars/kube_node_docker/all.yaml
container_manager: docker
node_labels:
  kubespray.io/container_manager: docker

If Multus was installed and used previously on a cluster only with a one container engine then nothing will be changed.
Docs with clear instructions can be added also by me.

Does this PR introduce a user-facing change?:

[multus] added support for mixed type of container engine

k8s-ci-robot · 2022-08-26T12:13:16Z

Hi @mr-yaky. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cristicalin · 2022-08-30T05:41:44Z

Hi @mr-yaky , please rebase this PR on latest state of the master branch to be able to pass the CI tests.

mr-yaky · 2022-08-30T16:14:48Z

Hi @mr-yaky , please rebase this PR on latest state of the master branch to be able to pass the CI tests.

done

oomichi

Thanks for adding a new support for multus.
I kicked the existing multus CI job, let's see the result after that.
Just one question to understand this clearly.

/ok-to-test

oomichi · 2022-08-31T04:46:45Z

roles/network_plugin/multus/templates/multus-daemonset.yml.j2

@@ -52,6 +59,7 @@ spec:
 {% if container_manager == 'crio' %}
        - name: run
          mountPath: {{ multus_cni_run_dir }}
+          mountPropagation: HostToContainer


I could not get the reason why here needs to specify mountPropagation: HostToContainer here.
Can I get the point?

oomichi

/cc @oomichi

oomichi · 2022-09-08T04:53:58Z

roles/network_plugin/multus/tasks/main.yml

+  set_fact:
+    container_manager_types: "{{ ansible_play_hosts_all|map('extract', hostvars, ['container_manager'])|list|unique }}"
+  run_once: true
+  when: inventory_hostname == groups['kube_control_plane'][0]

 - name: Multus | Copy manifest templates


This task was failed like https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/2957405780#L3428

TASK [network_plugin/multus : Multus | Copy manifest templates] **************** task path: /builds/kargo-ci/kubernetes-sigs-kubespray/roles/network_plugin/multus/tasks/main.yml:21 Wednesday 31 August 2022 05:09:30 +0000 (0:00:00.114) 0:11:25.330 ****** Wednesday 31 August 2022 05:09:30 +0000 (0:00:00.114) 0:11:25.330 ****** fatal: [instance-2 -> {{ groups['kube_control_plane'][0] }}]: FAILED! => {"msg": "The conditional check 'item.engine in container_manager_types' failed. The error was: error while evaluating conditional (item.engine in container_manager_types): 'container_manager_types' is undefined\n\nThe error appears to be in '/builds/kargo-ci/kubernetes-sigs-kubespray/roles/network_plugin/multus/tasks/main.yml': line 21, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: Multus | Copy manifest templates\n ^ here\n"}

The root problem seems 'container_manager_types' is undefined
At the above line 17, container_manager_types is set only if inventory_hostname == groups['kube_control_plane'][0]
So we might need to add the same condition to the line 35.

mr-yaky · 2022-10-24T16:19:53Z

@oomichi I think you can try to run your test again. I have checked with all 3 main playbooks cluster/upgrade-cluster/scale and it works for our cluster. So, please try one more type.
Thank you in advance.

oomichi · 2022-11-08T04:09:17Z

@mr-yaky
Sorry for my late response.
We have fixed some CI issue recently.
So could you rebase this pull request based on the latest master branch?

mr-yaky · 2022-11-14T16:31:56Z

@oomichi Done. PR has rebased.

oomichi · 2022-11-14T19:45:03Z

Thanks for updating.
packet_centos7-multus-calico is good now.

/lgtm

mr-yaky · 2022-11-21T12:14:00Z

@oomichi Is it possible to get this on the next upstream release to don't apply our own patches each time ?

mr-yaky · 2022-11-21T12:16:22Z

BTW this PR tested with 2.19.x and 2.20.x released and it works good for us.

floryut · 2022-12-30T16:04:57Z

Also missed this one, sorry guys especially @mr-yaky

mr-yaky · 2022-12-30T16:08:14Z

@floryut if we can merge it to upstream we will be very thankful. We are still waiting on this.
Thanks in advance.

cristicalin · 2023-01-17T07:26:53Z

Thanks for this work @mr-yaky !

/approve

k8s-ci-robot · 2023-01-17T07:26:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, mr-yaky

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~~ [cristicalin]

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

…-sigs#9224) * [multus] added support for mixed type of container engine * [multus] fixed for using with cluster/upgrade-cluster/scale playbooks

[multus] added support for mixed type of container engine

58f62d8

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2022

k8s-ci-robot requested review from EppO and holmsten August 26, 2022 12:13

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 26, 2022

Merge branch 'kubernetes-sigs:master' into master

1efd5c6

oomichi reviewed Aug 31, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 31, 2022

oomichi reviewed Sep 8, 2022

View reviewed changes

k8s-ci-robot requested a review from oomichi September 8, 2022 04:54

mr-yaky added 3 commits October 24, 2022 18:12

[multus] fixed for using with cluster/upgrade-cluster/scale playbooks

5fd1e9d

Merge branch 'master' of github.com:serverbee/kubespray

0e4fa33

Merge branch 'kubernetes-sigs:master' into master

4741cb1

Merge branch 'kubernetes-sigs:master' into master

d82e510

k8s-ci-robot assigned oomichi Nov 14, 2022

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2023

k8s-ci-robot merged commit d919c58 into kubernetes-sigs:master Jan 17, 2023

floryut mentioned this pull request Jan 17, 2023

Release Proposal v2.21 #9638

Closed

DANic-git mentioned this pull request Feb 13, 2023

Failed TASK [network_plugin/multus : Multus | Check container engine type] when upgrade-cluster #9790

Closed

blackluck mentioned this pull request Jul 20, 2023

multus - mixed type of container engine support commit fails #10305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multus] added support for mixed type of container engine #9224

[multus] added support for mixed type of container engine #9224

mr-yaky commented Aug 26, 2022 •

edited by floryut

k8s-ci-robot commented Aug 26, 2022

cristicalin commented Aug 30, 2022

mr-yaky commented Aug 30, 2022

oomichi left a comment

oomichi Aug 31, 2022

oomichi left a comment

oomichi Sep 8, 2022

mr-yaky commented Oct 24, 2022

oomichi commented Nov 8, 2022

mr-yaky commented Nov 14, 2022

oomichi commented Nov 14, 2022

mr-yaky commented Nov 21, 2022

mr-yaky commented Nov 21, 2022

floryut commented Dec 30, 2022

mr-yaky commented Dec 30, 2022 •

edited

cristicalin commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

[multus] added support for mixed type of container engine #9224

[multus] added support for mixed type of container engine #9224

Conversation

mr-yaky commented Aug 26, 2022 • edited by floryut

k8s-ci-robot commented Aug 26, 2022

cristicalin commented Aug 30, 2022

mr-yaky commented Aug 30, 2022

oomichi left a comment

Choose a reason for hiding this comment

oomichi Aug 31, 2022

Choose a reason for hiding this comment

oomichi left a comment

Choose a reason for hiding this comment

oomichi Sep 8, 2022

Choose a reason for hiding this comment

mr-yaky commented Oct 24, 2022

oomichi commented Nov 8, 2022

mr-yaky commented Nov 14, 2022

oomichi commented Nov 14, 2022

mr-yaky commented Nov 21, 2022

mr-yaky commented Nov 21, 2022

floryut commented Dec 30, 2022

mr-yaky commented Dec 30, 2022 • edited

cristicalin commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

mr-yaky commented Aug 26, 2022 •

edited by floryut

mr-yaky commented Dec 30, 2022 •

edited