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

docs, network-binding-plugin: Document migration support #11870

Merged
merged 1 commit into from
May 16, 2024

Conversation

EdDev
Copy link
Member

@EdDev EdDev commented May 8, 2024

What this PR does

Document the migrations support details in the network binding plugin developer-guide.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels May 8, 2024
section on how to define it.
- The backend `libvirt` and `qemu` needs to support migration for the specific
device that is used in the domain.
- The `hostdevice` devices in the domain do not support migration out-of-the-box.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this limitation is related to devices that use vfio driver, isn't it right?

nit:

Suggested change
- The `hostdevice` devices in the domain do not support migration out-of-the-box.
- The `hostdevice` devices in the domain doesn't support migration out-of-the-box.

Copy link
Member Author

Choose a reason for hiding this comment

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

devices are plural so I think do is the correct form.

Comment on lines 487 to 489
Kubevirt. The special handling is currently implemented for core SR-IOV
binding by unplugging the device at the source before migration is started
and plugging it back at the destination after the migration completed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would present the core SR-IOV binding as an example, something along these lines:

Suggested change
Kubevirt. The special handling is currently implemented for core SR-IOV
binding by unplugging the device at the source before migration is started
and plugging it back at the destination after the migration completed.
Kubevirt.
For example, Kubevirt core [SR-IOV binding](https://kubevirt.io/user-guide/virtual_machines/interfaces_and_networks/#sriov) implements migration support
by unplugging the device at the source before migration is started
and plugging it back at the destination after the migration completed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be incorrect.
We do not have support for hostdevice devices through the plugin infrastructure.
The SR-IOV binding is not an example of a plugin binding.

I used it to express what is needed to support something like this and explicitly mentioned we do not yet have support for it in the plugins.

Copy link
Contributor

@ormergi ormergi May 13, 2024

Choose a reason for hiding this comment

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

The current state is not clear enough, one might think the unplug/plug can be used for any binding plugin

I suggested to refer to how kubevirt core sr-iov binding does the "special handling" for migration(the uplug/plug thing), not to say its an example for a plugin or plugin support host devices.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current state is not clear enough, one might think the unplug/plug can be used for any binding plugin

I do not understand how hotplug got involved here.
The text here is about migration and in the specific section about the limitations hostdevices have.

It explicitly says hostdevices are not supported at the moment.

I suggested to refer to how kubevirt core sr-iov binding does the "special handling" for migration(the uplug/plug thing), not to say its an example for a plugin or plugin support host devices.

It explains how hostdevices are treated by the SR-IOV binding to express the challenge.

I see no point in ref to that binding because it says nothing on how it works internally. That is a user-guide document, it does not enter into such details.

Maybe you should point me to the sentence that is problematic in your opinion here and how one can interpret it wrongly.

Copy link
Contributor

@ormergi ormergi May 13, 2024

Choose a reason for hiding this comment

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

I do not understand how hotplug got involved here. The text here is about migration and in the specific section about the limitations hostdevices have.

The unplug/plug core SR-IOV binding does on migration, this PR also mentions.

Maybe you should point me to the sentence that is problematic in your opinion here and how one can interpret it wrongly.

Its about the phrasing, to me the "The special handling is currently implemented.." part sound as it can be used for binding plugin.

Anyhow, I am not insisting on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rephrased, please take another look.

@ormergi
Copy link
Contributor

ormergi commented May 13, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024
@EdDev EdDev force-pushed the docs-net-binding-plugin-migration branch from df03d57 to 0d48a65 Compare May 15, 2024 06:24
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2024
@ormergi
Copy link
Contributor

ormergi commented May 15, 2024

Thanks, it looks better

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 15, 2024
Signed-off-by: Edward Haas <edwardh@redhat.com>
@EdDev EdDev force-pushed the docs-net-binding-plugin-migration branch from 0d48a65 to 9591437 Compare May 16, 2024 03:35
@kubevirt-bot kubevirt-bot added sig/network and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2024
@EdDev
Copy link
Member Author

EdDev commented May 16, 2024

change: Rebase

@ormergi
Copy link
Contributor

ormergi commented May 16, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2024
@EdDev
Copy link
Member Author

EdDev commented May 16, 2024

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EdDev

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@kubevirt-bot kubevirt-bot merged commit b072e42 into kubevirt:main May 16, 2024
37 checks passed
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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/network size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants