Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR #7086

acardace · 2022-01-14T16:35:41Z

What this PR does / why we need it:

If set this will apply to all VMIs which don't have an EvictionStrategy setting set in their VMI's spec.

Also add 'EvictionStrategyNone' as a VMI Eviction Strategy, This option can be used by the single VMI to opt-out from being live-migrated on eviction if the cluster is set to do so.

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:

Release note:

Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR

acardace · 2022-01-17T19:46:27Z

/retest

iholder101

Great job!! comments below!

I would also add documentation to here.

pkg/util/migrations/migrations.go

pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter.go

tests/migration_test.go

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter_test.go

tests/migration_test.go

acardace · 2022-01-19T13:49:46Z

Great job!! comments below!

I would also add documentation to here.

I added some docs here 03cdd41#diff-34a591e3aa47260cfad791064c20f34f98fdca4a63532fbf7184e68682f4fcb8R2049.

Thanks for the review!

acardace · 2022-01-20T09:16:24Z

/retest

acardace · 2022-01-20T16:33:57Z

/retest

iholder101 · 2022-01-20T17:21:00Z

Thank you @acardace!
/lgtm

acardace · 2022-01-26T12:48:01Z

/retest

acardace · 2022-01-28T14:19:35Z

/retest

acardace · 2022-02-03T10:29:25Z

/retest

acardace · 2022-02-04T13:54:46Z

/test pull-kubevirt-unit-test

…t CR If set this will apply to all VMIs which don't have an EvictionStrategy setting set in their VMI's spec. Signed-off-by: Antonio Cardace <acardace@redhat.com>

This patch takes into account the cluster-wide setting if the VMI specific EvictionStrategy has not been set. Signed-off-by: Antonio Cardace <acardace@redhat.com>

Removing vmi.VirtualMachineInstance.IsEvictable() as it's now unused in our codebase. Signed-off-by: Antonio Cardace <acardace@redhat.com>

This option can be used by the single VMI to opt-out from being live-migrated on eviction if the cluster is set to do so. Signed-off-by: Antonio Cardace <acardace@redhat.com>

Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace · 2022-02-11T14:49:56Z

/retest

davidvossel

/approve

Thanks!

kubevirt-bot · 2022-02-11T22:32:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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:

~~OWNERS~~ [davidvossel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fossedihelm

Looks good overall. Just a nits that can be addressed later. Thanks

pkg/virt-controller/watch/drain/evacuation/evacuation_test.go

…g unit tests Signed-off-by: Antonio Cardace <acardace@redhat.com>

fossedihelm · 2022-02-15T11:05:21Z

/lgtm Thanks

kubevirt-bot · 2022-02-15T18:14:54Z

@acardace: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.20-cgroupsv2	`fcd1797`	link	true	`/test pull-kubevirt-e2e-k8s-1.20-cgroupsv2`
pull-kubevirt-check-tests-for-flakes	`3730f7a`	link	false	`/test pull-kubevirt-check-tests-for-flakes`

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 · 2022-02-15T21:47:58Z

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

rmohr · 2022-02-16T08:58:28Z

Sorry for not checking the code in detail which probably answers this, but how will this behave if the VMI is non-migratable? Will there be warnings triggered, or do you take care of differentiating between explicitly set eviction strategies and defaulting and inhibit the warning events? Or do you expect users to always set None as strategy explicitly to avoid warnings of any sort?

acardace · 2022-02-16T09:17:57Z

Sorry for not checking the code in detail which probably answers this, but how will this behave if the VMI is non-migratable? Will there be warnings triggered, or do you take care of differentiating between explicitly set eviction strategies and defaulting and inhibit the warning events? Or do you expect users to always set None as strategy explicitly to avoid warnings of any sort?

@rmohr no worries, basically something like the "current" behavior is kept, if you specify you want to live migrate on eviction for all VMIs in the cluster you will get warnings if some VMIs are not.

https://github.com/acardace/kubevirt/blob/3730f7aeb15c16f5821f4006020034d436978341/pkg/virt-handler/vm.go#L910

rmohr · 2022-02-16T10:10:04Z

@rmohr no worries, basically something like the "current" behavior is kept, if you specify you want to live migrate on eviction for all VMIs in the cluster you will get warnings if some VMIs are not.

https://github.com/acardace/kubevirt/blob/3730f7aeb15c16f5821f4006020034d436978341/pkg/virt-handler/vm.go#L910

What I want to get at: I did not have to worry about opting out of live-migrations before if I e.g. chose a local volume, a local resouce, ... Now I will get warnings although I never opted in?

acardace · 2022-02-17T10:24:28Z

@rmohr no worries, basically something like the "current" behavior is kept, if you specify you want to live migrate on eviction for all VMIs in the cluster you will get warnings if some VMIs are not.
https://github.com/acardace/kubevirt/blob/3730f7aeb15c16f5821f4006020034d436978341/pkg/virt-handler/vm.go#L910

What I want to get at: I did not have to worry about opting out of live-migrations before if I e.g. chose a local volume, a local resouce, ... Now I will get warnings although I never opted in?

I see, in practice yes if the cluster admin set the global conf to live migrate everything on eviction, unless you explicitly set EvictionStrategyNone.

I don't know if it's better to still see the alert or just hide it, @rmohr would you like to silence it?

rmohr · 2022-09-30T08:39:07Z

I don't know if it's better to still see the alert or just hide it, @rmohr would you like to silence it?

Sorry for not following up. I think that in the case where we only set a default recommendation we could come up with an alternative cluster-strategy: Something like LiveMigrateIfPossible which would not trigger a warning if the VMI is not live-migratable. If users don't specify it, they would not care about it, and we would not have to trigger a warning and would just shut them down on cluster evictions. What do you think?

Especially with local storage in play that is anyway the right action. The VMI is bound to that node still and will come up when the node comes up again.

acardace · 2022-10-03T07:55:51Z

I don't know if it's better to still see the alert or just hide it, @rmohr would you like to silence it?

Sorry for not following up. I think that in the case where we only set a default recommendation we could come up with an alternative cluster-strategy: Something like LiveMigrateIfPossible which would not trigger a warning if the VMI is not live-migratable. If users don't specify it, they would not care about it, and we would not have to trigger a warning and would just shut them down on cluster evictions. What do you think?

Especially with local storage in play that is anyway the right action. The VMI is bound to that node still and will come up when the node comes up again.

Yes, that's a neat idea, I like it!

I'll create an issue to track it and get to it to as soon as possible.

fabiand · 2022-12-20T12:58:23Z

@acardace @stu-gott was the user-guide ever updated?

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Jan 14, 2022

kubevirt-bot requested review from EdDev and ILpinto January 14, 2022 16:35

iholder101 reviewed Jan 18, 2022

View reviewed changes

acardace force-pushed the cluster-eviction-strategy branch from cb927a3 to fcd1797 Compare January 19, 2022 13:47

kubevirt-bot added the area/monitoring label Jan 19, 2022

acardace force-pushed the cluster-eviction-strategy branch 2 times, most recently from 168a00d to 760ef2f Compare January 19, 2022 14:53

kubevirt-bot added size/XL and removed size/L labels Jan 19, 2022

acardace force-pushed the cluster-eviction-strategy branch from 760ef2f to 6fcca39 Compare January 20, 2022 14:34

kubevirt-bot assigned iholder101 Jan 20, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

acardace force-pushed the cluster-eviction-strategy branch from 6fcca39 to b38e67a Compare February 4, 2022 14:48

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Feb 4, 2022

acardace added 3 commits February 10, 2022 11:29

core: add 'EvictionStrategy' as a cluster-wide setting in the KubeVir…

49124f4

…t CR If set this will apply to all VMIs which don't have an EvictionStrategy setting set in their VMI's spec. Signed-off-by: Antonio Cardace <acardace@redhat.com>

core: honor the 'EvictionStrategy' field in the KubeVirt CR

0d2eaba

This patch takes into account the cluster-wide setting if the VMI specific EvictionStrategy has not been set. Signed-off-by: Antonio Cardace <acardace@redhat.com>

core: remove unused API

707a27c

Removing vmi.VirtualMachineInstance.IsEvictable() as it's now unused in our codebase. Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace added 3 commits February 10, 2022 11:29

core: add 'EvictionStrategyNone' as a VMI Eviction Strategy

10171cb

This option can be used by the single VMI to opt-out from being live-migrated on eviction if the cluster is set to do so. Signed-off-by: Antonio Cardace <acardace@redhat.com>

migration: add 'EvictionStrategy' cluster-wide settings func tests

43a3791

Signed-off-by: Antonio Cardace <acardace@redhat.com>

webhooks: remove unit test duplicated code

809f053

Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace force-pushed the cluster-eviction-strategy branch from b38e67a to 0f2cc54 Compare February 10, 2022 10:30

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2022

davidvossel reviewed Feb 11, 2022

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2022

fossedihelm approved these changes Feb 15, 2022

View reviewed changes

pkg/virt-controller/watch/drain/evacuation/evacuation_test.go Outdated Show resolved Hide resolved

kubevirt-bot assigned fossedihelm Feb 15, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2022

virt-controller, webhooks: add 'EvictionStrategy' cluster-wide settin…

3730f7a

…g unit tests Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace force-pushed the cluster-eviction-strategy branch from 0f2cc54 to 3730f7a Compare February 15, 2022 11:02

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2022

fossedihelm approved these changes Feb 15, 2022

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2022

kubevirt-bot merged commit 66cedbf into kubevirt:main Feb 16, 2022

acardace mentioned this pull request Oct 3, 2022

Add LiveMigrateIfPossible eviction strategy #8555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR #7086

Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR #7086

acardace commented Jan 14, 2022

acardace commented Jan 17, 2022

iholder101 left a comment

acardace commented Jan 19, 2022

acardace commented Jan 20, 2022

acardace commented Jan 20, 2022

iholder101 commented Jan 20, 2022

acardace commented Jan 26, 2022

acardace commented Jan 28, 2022

acardace commented Feb 3, 2022

acardace commented Feb 4, 2022

acardace commented Feb 11, 2022

davidvossel left a comment

kubevirt-bot commented Feb 11, 2022

fossedihelm left a comment

fossedihelm commented Feb 15, 2022

kubevirt-bot commented Feb 15, 2022 •

edited

kubevirt-commenter-bot commented Feb 15, 2022

rmohr commented Feb 16, 2022

acardace commented Feb 16, 2022 •

edited

rmohr commented Feb 16, 2022

acardace commented Feb 17, 2022

rmohr commented Sep 30, 2022 •

edited

acardace commented Oct 3, 2022

fabiand commented Dec 20, 2022

Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR #7086

Add 'EvictionStrategy' as a cluster-wide setting in the KubeVirt CR #7086

Conversation

acardace commented Jan 14, 2022

acardace commented Jan 17, 2022

iholder101 left a comment

Choose a reason for hiding this comment

acardace commented Jan 19, 2022

acardace commented Jan 20, 2022

acardace commented Jan 20, 2022

iholder101 commented Jan 20, 2022

acardace commented Jan 26, 2022

acardace commented Jan 28, 2022

acardace commented Feb 3, 2022

acardace commented Feb 4, 2022

acardace commented Feb 11, 2022

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Feb 11, 2022

fossedihelm left a comment

Choose a reason for hiding this comment

fossedihelm commented Feb 15, 2022

kubevirt-bot commented Feb 15, 2022 • edited

kubevirt-commenter-bot commented Feb 15, 2022

rmohr commented Feb 16, 2022

acardace commented Feb 16, 2022 • edited

rmohr commented Feb 16, 2022

acardace commented Feb 17, 2022

rmohr commented Sep 30, 2022 • edited

acardace commented Oct 3, 2022

fabiand commented Dec 20, 2022

kubevirt-bot commented Feb 15, 2022 •

edited

acardace commented Feb 16, 2022 •

edited

rmohr commented Sep 30, 2022 •

edited