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.

Sign up for GitHub

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

Jump to bottom

add macvtap BindingMechanism #2935

Merged
merged 15 commits into from
Oct 22, 2020
Merged

add macvtap BindingMechanism #2935

merged 15 commits into from
Oct 22, 2020

Conversation

maiqueb
Copy link
Contributor

@maiqueb maiqueb commented Dec 13, 2019 •
edited
Loading

Signed-off-by: Miguel Duarte Barroso mdbarroso@redhat.com

What this PR does / why we need it:
This PR adds a new binding mechanism for macvtap interfaces.

Using the macvtap plugin the VM can be directly connected to a host interface, with no intermediary devices such as veth pairs and in-pod bridge. This provides a more reliable connection, with better performance.

The macvtap devices are created by the macvtap device-plugin, and configured by the macvtap-cni, both of which make part of the macvtap-cni repository.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
The CNI plugin is responsible for creating & configuring the macvtap interface.

Release note:

Add the macvtap BindMechanism.
This new BindMechanism adapts the VMs domain xml to use unprivileged
(already configured) macvtap interfaces.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Dec 13, 2019
@kubevirt-bot kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 13, 2019
@kubevirt-bot
Copy link
Contributor

Hi @maiqueb. Thanks for your PR.

I'm waiting for a kubevirt 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.

@kubevirt-bot kubevirt-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 13, 2019
@phoracek phoracek self-requested a review December 13, 2019 15:37
@phoracek
Copy link
Member

phoracek commented Jan 3, 2020

/retest

@phoracek
Copy link
Member

phoracek commented Jan 3, 2020

/ok-to-test

@kubevirt-bot kubevirt-bot 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 Jan 3, 2020
dankenigsberg
dankenigsberg reviewed Jan 10, 2020
View reviewed changes
Copy link
Member

@dankenigsberg dankenigsberg left a comment

Choose a reason for hiding this comment

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

notably missing are e2e tests and documentation (yes, I know it is only a draft right now)

pkg/virt-launcher/virtwrap/api/schema.go Outdated Show resolved Hide resolved
pkg/virt-launcher/virtwrap/api/schema.go Outdated Show resolved Hide resolved
@vladikr
Copy link
Member

vladikr commented Jan 15, 2020 •
edited
Loading

I didn't look deeply into the code yet, however, I'm wondering... if this work is relying on a device plugin, perhaps we should handle this assignment the same way we handle SRIOV devices instead of using a Binding mechanism?

@maiqueb
Copy link
Contributor Author

maiqueb commented Jan 16, 2020

I didn't look deeply into the code yet, however, I'm wondering... if this work is relying on a device plugin, perhaps we should handle this assignment the same way we handle SRIOV devices instead of using a Binding mechanism?

One reason I can think of right now is that one of the parameters we need to set on the VM's domxml is the MAC address of the interface; that's easily extracted from the VIF object, which is one of the attributes of the MacvtapPodInterface we're adding - e.g. that fits nicely w/ the BindMechanism flow.

Not sure that's easily retrieved from the converter - where I think all the sr-iov stuff is done.

@vladikr
Copy link
Member

vladikr commented Jan 16, 2020

One reason I can think of right now is that one of the parameters we need to set on the VM's domxml is the MAC address of the interface; that's easily extracted from the VIF object, which is one of the attributes of the MacvtapPodInterface we're adding - e.g. that fits nicely w/ the BindMechanism flow.

Not sure that's easily retrieved from the converter - where I think all the sr-iov stuff is done.

Can't the device plug-in pass the mac address to the pod, via an env. variable?
Same way as the vgpu device plugin passes the mdev ids.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2020
@jcaamano
Copy link

One reason I can think of right now is that one of the parameters we need to set on the VM's domxml is the MAC address of the interface; that's easily extracted from the VIF object, which is one of the attributes of the MacvtapPodInterface we're adding - e.g. that fits nicely w/ the BindMechanism flow.
Not sure that's easily retrieved from the converter - where I think all the sr-iov stuff is done.

Can't the device plug-in pass the mac address to the pod, via an env. variable?
Same way as the vgpu device plugin passes the mdev ids.

AFAIK the binding mechanism is there to learn the mac address of the macvtap interface in case none has been explicitly configured (via the VMI) so that it can then be passed to libvirt later on.

The mac address seen by the device plugin might not be final if one has been configured in the VMI. The configured mac address is passed to the CNI via multus annotation and then set by the CNI overriding the auto-generated one in the device plugin.

The discovered mac address in the binding mechanism is the final one, after the CNI has configured the interface.

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2020
@maiqueb maiqueb force-pushed the add_macvtap_binding_mech branch 2 times, most recently from 970d503 to 13dee8a Compare March 5, 2020 14:00
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020
@maiqueb
Copy link
Contributor Author

maiqueb commented Oct 22, 2020

/retest

@RamLavi
Copy link
Contributor

RamLavi commented Oct 22, 2020

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020
@maiqueb
Copy link
Contributor Author

maiqueb commented Oct 22, 2020

/retest

@maiqueb
macvtap, multus: add macvtap BindingMechanism
ff7494b 
Add a new macvtap based bind mechanism, which allows the VM to be
plugged directly into the host's network.

This new bind mechanism requires multus CNI.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
unit tests, macvtap, multus: introduce macvtap
d8da555 
Added unit tests for:
  - converter    (api pkg)
  - podinterface (network pkg)

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
automation, macvtap: restrict macvtap func tests to multus lanes
fa0bb73 
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
tests: remove all net-attach-defs on test cleanup
9aede71 
Since the `net-attach-def` objects would only exist in the CNAO
lane, we cannot panic on error when removing them fails; we list
them, then delete them one by one.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, macvtap, multus: add test with a custom MAC address
4e53de1 
Assure that a vm with a custom MAC address on a macvtap interface
can be started.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, multus: refactor configInterface to allow sudo
4f7ec88 
VMs based on Cirros images require sudo to configure the IP
address.

A follow up patch will reuse this function for cirros based VMs.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
macvtap: feature gate macvtap feature
6d57e12 
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, macvtap, multus: add connectivity test between VMs
05aa248 
Provide a function to create a cirros based VM with a single macvtap
interface with static IP addresses.

Then, one of the VMs (clientVM) pings the other (serverVM).

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
macvtap, admitter: macvtap requires multus network
3891565 
Add tests to check the admitter, thus ensuring the feature works
only when the feature gate is enabled, and when a multus network
is used.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
examples, macvtap, multus: add example for macvtap VMI
efa0cc5 
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
tests: update the StartVmOnNode method to return the started VMI
084a5ea 
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, macvtap, multus: schedule the VMs in the same node
b7c92cb 
Since the 2 VMs are placed in an isolated subnet, whose routes
are not configured, and as a result it doesn't have inter-node
communication.

As such, we must ensure both VMs are co-located in the same k8s
node.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, macvtap, multus: use libvmi Cirros VMI factory
97c106f 
Add to the factory means of generating:
  - a multus network
  - a macvtap interface

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, migration: move some asserter subset to common helpers
9f3cc05 
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb
functests, macvtap, migration: successful macvtap VMI migration
9a8edb0 
The migration of VMIs with macvtap interfaces is possible *if* the
mac address of the VMI is defined.

This leaves a gap in the feature set, where migrating a VMI with
a randomly generated mac address fails.

A follow-up PR will address this gap, by making sure the newly
instantiated VMI honors the mac address it had before migration.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020
@RamLavi
Copy link
Contributor

RamLavi commented Oct 22, 2020

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020
@maiqueb
Copy link
Contributor Author

maiqueb commented Oct 22, 2020

/retest

1 similar comment
@maiqueb
Copy link
Contributor Author

maiqueb commented Oct 22, 2020

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Oct 22, 2020 •
edited
Loading

@maiqueb: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubevirt-e2e-k8s-1.14.6 16b32c0 link /test pull-kubevirt-e2e-k8s-1.14.6
pull-kubevirt-e2e-k8s-1.14 8197326 link /test pull-kubevirt-e2e-k8s-1.14
pull-kubevirt-e2e-k8s-1.15-ceph 941745f link /test pull-kubevirt-e2e-k8s-1.15-ceph
pull-kubevirt-e2e-kind-1.17-sriov 7132026 link /test pull-kubevirt-e2e-kind-1.17-sriov

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.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit b7e8d36 into kubevirt:master Oct 22, 2020
@maiqueb maiqueb deleted the add_macvtap_binding_mech branch November 20, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dankenigsberg dankenigsberg dankenigsberg left review comments

@rmohr rmohr rmohr requested changes

@AlonaKaplan AlonaKaplan AlonaKaplan requested changes

@RamLavi RamLavi RamLavi requested changes

@vladikr vladikr Awaiting requested review from vladikr

@phoracek phoracek Awaiting requested review from phoracek

@qinqon qinqon Awaiting requested review from qinqon

Assignees

@qinqon qinqon

@phoracek phoracek

@rmohr rmohr

@AlonaKaplan AlonaKaplan

@RamLavi RamLavi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/build-change Categorizes PRs as related to changing build files of virt-* components lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.