-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for Multus (multiple interfaces) CNI plugin #3166
Adds support for Multus (multiple interfaces) CNI plugin #3166
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
serviceAccountName: multus | ||
containers: | ||
- name: kube-multus | ||
image: kusanagi9999/multus:3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to use an image provided by the Multus community here.
ed96eae
to
d02c082
Compare
shell: | | ||
cd {{ multus_conf_dir }} | ||
cat $(ls -p | grep -v / | head -1) | ||
register: output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed_when: false
could be added here since no changes are actually being made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks for the input. I might be moving the generation of the Multus conf file into the Multus docker image but not sure about that yet.
5d1d854
to
2840346
Compare
path: "{{ multus_conf_dir }}/{{ multus_conf_filename }}" | ||
|
||
- name: Multus | Read CNI configuration of master plugin | ||
shell: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to use native modules when possible, instead of command/shell: https://docs.ansible.com/ansible/2.5/modules/slurp_module.html
owner: kube | ||
|
||
- name: Multus | Load Multus CNI configuration for formatting | ||
command: "cat {{ multus_conf_dir }}/{{ multus_conf_filename }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@Kusanagi9999 Any plans to continue or can I pick this up? |
@gageorsburn yes, I am just about to pick this up again. Sorry for the delay. The multus.conf needs to be generated later to work with some of the other plugins. For now, we can change the PR that the multus.conf is still generated by Kubspray/Ansible just in a later stage. If my PR to Multus (k8snetworkplumbingwg/multus-cni#160) get's accepted, the multus.conf generation part can be handled directly in the docker image. I'll push updates here shortly. |
0e3879b
to
90cdf95
Compare
I'd like to start running CI tests on this. Could some admin/collaborator add the "ci check this" to this PR? @Atoms ? |
ci check this |
is there any plans on writing CI job for this plugin? we cannot guarantee that in future it will not break if there is no CI job |
@Atoms @gageorsburn @mattymo this is ready for another round of reviews. Is there a way I can manually trigger gitlab jobs or is that option only for maintainers? |
@Kusanagi9999 only maintainers can trigger jobs :) which job you want me to trigger :) |
@Atoms that's what I figured. Could you trigger gce_centos7-multus-calico? I already tested it but it might be good for the reviewers to see that it passes. Thanks! |
test seems green, so if you confident of this now we can merge ? :) |
@Atoms awesome! Could you maybe also kick off the Contiv jobs - just to be on the safe side? |
@Atoms I am fine with merging this. You might want to kick of the contiv jobs because I changed the CNI version. |
Strange that the contiv test keeps failing with Does it pass on master? This seems unrelated to my change. Also, I don't see this happening on my local Vagrant cluster. |
544ce9d
to
1be62e9
Compare
Looks like that did not help. I am seeing the same issue on a different PR - the Cililium uplift (https://gitlab.com/kargo-ci/kubernetes-incubator__kubespray/-/jobs/113730855). It definitely seems to be a broader issue. |
this fixes this issue: #3605 |
94a4373
to
2283bd1
Compare
2283bd1
to
4b86923
Compare
Did you rebased? seems no |
Multus is a latin word for "Multi". As the name suggests, it acts as a Multi plugin in Kubernetes and provides multiple network interface support in a pod. Multus uses the concept of invoking delegates by grouping multiple plugins into delegates and invoking them in the sequential order of the CNI configuration file provided in json format.
4b86923
to
1072676
Compare
@Atoms rebased now |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Atoms 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:
Approvers can indicate their approval by writing |
Add support for the Multus CNI plugin (https://github.com/intel/multus-cni). Multus delegates CNI calls to other backend CNI plugins, i.e., it requires at least on master CNI plugin. Therefore, Multus has to be enabled via separate variable
kube_network_plugin_multus: true
,kube_network_plugin: multus
would not work.I have so far manually tested this PR with the following CNI plugins:
*The current Cilium version supported by Kubespray (v1.2.x) generates 00-cilium.conf which takes precedence over 00-multus.conf. Because of this problem the cilium authors have made a change to generate 05-cilium.conf instead (cilium/cilium#5191). I tested with v1.3.0-rc1 of Cilium which contains this fix and it worked.
Automated testing: I added a gitlab job that brings up Multus+Calico and does the standard networking test (essentially testing Calico when called through Multus). In addition, I added Ansible code that creates a macvlan network, annotates a pod with that network and checks that the second interface is created.