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

Migrate virtual instances using virtctl (KubeVirt) #1094

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgrzybek
Copy link

@mgrzybek mgrzybek commented Mar 3, 2025

This PR adds support for migrating virtual machines managed by KubeVirt.

  1. An Evacuator interface is used to publish a generic type.
  2. A Kubevirt evacuator implements this interface.

Other implementations could be added in the future, according to what users need. This could draw a way to solve #737 for example.

Concerning KubeVirt SDK usage, two options were tested:

  • Using the native client (https://github.com/kubevirt/client-go/tree/main) broked some Kubernetes library dependancies. Versions conflicts occured when using at the same time the Kubernetes SDK and the KubeVirt one.
  • Using virctl provided a low dependancy method to interact with KubeVirtks objects. Moreover, the required data could be read directly from Pods' labels.

@evrardjp
Copy link
Collaborator

evrardjp commented Mar 10, 2025

I like the principle of having different evacuators. Some people want to evacuate differently than others.
This is fairly close to what I do in another branch, where I refactor the whole main loop.
I would love if we could work together in order to avoid the merge conflicts.

The more recent ideas we have we kured is to reduce the access it requires (trim down the role's scope). This PR seems to be heading to another direction for the needs of kubevirt. I am not yet sure how we can solve that (did you read the node maintenance kep?). Would you be okay to wait a bit so we discuss the approach in a next meeting?

Next to that, this PR lacks testing (there is neither unit tests nor functional tests included in it).
As the core team aren't kubeVirt experts (afaik), the lack of testing is (IMO) problematic (as I am expecting no one will spend time to increase test coverage).
No tests means no guarantee to ensure quality on the long run.
Yet merging this PR would mean claiming to support kubevirt. I am therefore afraid of the impact on the project in the long run.

Hence the most important question for the review: Are you willing to step up and maintain that bit of code (ensure its quality before we cut every release for example), or is this closer to a "drive-by" contribution to fix your issue?

@mgrzybek
Copy link
Author

mgrzybek commented Mar 13, 2025

I think a generic evacuator running shell scripts should be the way to go. The end users would be able to use their own scripts. Moreover this wouldn't claim any official support from any other tool.

About KubeVirt, evictonStrategy exists to deal with draining nodes. However this means that some configuration should be made by the users or the k8s admins. I wrote this patch because my users and clusters don't set this value properly. This is a workaround.

I won't maintain this patch on a long-term basis. I can on the other hand write a patch to handle external binaries / shell scripts.

@mgrzybek
Copy link
Author

Moreover,

Another Evacuator implementation, in addition to a script / binary executor, could be a HTTP caller to trigger some external process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants