Skip to content
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

Merged

Conversation

Kusanagi9999
Copy link
Contributor

@Kusanagi9999 Kusanagi9999 commented Aug 22, 2018

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:

  • calico
  • canal
  • cilium*
  • contiv (I had to change the CNI version for this to work, separate commit)
  • flanneld
  • weave
  • kube-router

*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.

@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2018
serviceAccountName: multus
containers:
- name: kube-multus
image: kusanagi9999/multus:3
Copy link
Contributor Author

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 22, 2018
@Kusanagi9999 Kusanagi9999 force-pushed the multus-cni-support branch 5 times, most recently from ed96eae to d02c082 Compare August 23, 2018 03:26
shell: |
cd {{ multus_conf_dir }}
cat $(ls -p | grep -v / | head -1)
register: output

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.

Copy link
Contributor Author

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.

@Kusanagi9999 Kusanagi9999 force-pushed the multus-cni-support branch 2 times, most recently from 5d1d854 to 2840346 Compare September 2, 2018 03:52
path: "{{ multus_conf_dir }}/{{ multus_conf_filename }}"

- name: Multus | Read CNI configuration of master plugin
shell: |
Copy link
Contributor

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 }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@gageorsburn
Copy link

@Kusanagi9999 Any plans to continue or can I pick this up?

@Kusanagi9999
Copy link
Contributor Author

@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.

@Kusanagi9999 Kusanagi9999 force-pushed the multus-cni-support branch 3 times, most recently from 0e3879b to 90cdf95 Compare October 5, 2018 19:51
@Kusanagi9999 Kusanagi9999 changed the title [WIP] Adds support for Multus (multiple interfaces) CNI plugin Adds support for Multus (multiple interfaces) CNI plugin Oct 8, 2018
@Kusanagi9999
Copy link
Contributor Author

I'd like to start running CI tests on this. Could some admin/collaborator add the "ci check this" to this PR? @Atoms ?

@Atoms
Copy link
Member

Atoms commented Oct 9, 2018

ci check this

@Atoms
Copy link
Member

Atoms commented Oct 9, 2018

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

@Kusanagi9999
Copy link
Contributor Author

@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?

@Atoms
Copy link
Member

Atoms commented Oct 25, 2018

@Kusanagi9999 only maintainers can trigger jobs :) which job you want me to trigger :)

@Kusanagi9999
Copy link
Contributor Author

Kusanagi9999 commented Oct 25, 2018

@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!

@Atoms
Copy link
Member

Atoms commented Oct 25, 2018

test seems green, so if you confident of this now we can merge ? :)

@Kusanagi9999
Copy link
Contributor Author

@Atoms awesome! Could you maybe also kick off the Contiv jobs - just to be on the safe side?

@Kusanagi9999 Kusanagi9999 mentioned this pull request Oct 25, 2018
@Kusanagi9999
Copy link
Contributor Author

@Atoms I am fine with merging this. You might want to kick of the contiv jobs because I changed the CNI version.

@Kusanagi9999
Copy link
Contributor Author

Kusanagi9999 commented Oct 26, 2018

Strange that the contiv test keeps failing with Friday 26 October 2018 20:29:02 +0000 (0:00:00.073) 0:06:07.093 ******** fatal: [k8s-34225179-113293346-3]: FAILED! => {"changed": false, "cmd": ["openssl", "x509", "-in", "/etc/ssl/etcd/ssl/node-k8s-34225179-113293346-3.pem", "-noout", "-serial"], "delta": "0:00:00.004548", "end": "2018-10-26 20:29:03.203278", "msg": "non-zero return code", "rc": 1, "start": "2018-10-26 20:29:03.198730", "stderr": "Error opening Certificate /etc/ssl/etcd/ssl/node-k8s-34225179-113293346-3.pem\n140718972581528:error:02001002:system library:fopen:No such file or directory:bss_file.c:398:fopen('/etc/ssl/etcd/ssl/node-k8s-34225179-113293346-3.pem','r')\n140718972581528:error:20074002:BIO routines:FILE_CTRL:system lib:bss_file.c:400:\nunable to load certificate", "stderr_lines": ["Error opening Certificate /etc/ssl/etcd/ssl/node-k8s-34225179-113293346-3.pem", "140718972581528:error:02001002:system library:fopen:No such file or directory:bss_file.c:398:fopen('/etc/ssl/etcd/ssl/node-k8s-34225179-113293346-3.pem','r')", "140718972581528:error:20074002:BIO routines:FILE_CTRL:system lib:bss_file.c:400:", "unable to load certificate"], "stdout": "", "stdout_lines": []}

Does it pass on master? This seems unrelated to my change. Also, I don't see this happening on my local Vagrant cluster.

@Kusanagi9999
Copy link
Contributor Author

@Atoms I rebased after this got merged #3486 today - not sure if the fix helps with the Contiv job.

@Kusanagi9999
Copy link
Contributor Author

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.

@Kusanagi9999
Copy link
Contributor Author

Kusanagi9999 commented Oct 30, 2018

These issues seem related #3570 #2892

@Atoms
Copy link
Member

Atoms commented Oct 31, 2018

this fixes this issue: #3605

@Kusanagi9999 Kusanagi9999 force-pushed the multus-cni-support branch 2 times, most recently from 94a4373 to 2283bd1 Compare November 1, 2018 16:24
@Kusanagi9999 Kusanagi9999 reopened this Nov 2, 2018
@Kusanagi9999
Copy link
Contributor Author

@Atoms now that #3605 merged can we give this another shot?

@Atoms
Copy link
Member

Atoms commented Nov 3, 2018

Did you rebased? seems no
/needs-rebase

Kusanagi9999 and others added 2 commits November 3, 2018 07:37
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.
@Kusanagi9999
Copy link
Contributor Author

@Atoms rebased now

@Atoms
Copy link
Member

Atoms commented Nov 4, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2018
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2018
@k8s-ci-robot k8s-ci-robot merged commit bc9e14a into kubernetes-sigs:master Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants