Make VMI non-migratable when incompatible CPU #2982

petrkotas · 2020-01-13T17:14:53Z

What this PR does / why we need it:
During migration compatible CPU model have to be used.
This is hard when cpu-passthrough or host-model is used
as a choice for CPU.

This issue can be ovecomed in a short term by blocking
migration for host-model or cpu-passthrough.

Signed-off-by: Petr Kotas petr.kotas@gmail.com

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 # https://bugzilla.redhat.com/show_bug.cgi?id=1760028

Release note:

NONE

petrkotas · 2020-01-15T07:10:36Z

/retest

petrkotas · 2020-01-15T07:11:37Z

Hi @MarSik would you please have time to take a look at this PR as it is related to the part you have been working on? I understand you have you workload, so once you are OK with it. Thank you.

petrkotas · 2020-01-15T11:15:04Z

/retest

petrkotas · 2020-01-15T15:48:45Z

/retest

danielBelenky

A few minor comments with some personal suggestions... feel free to change them or not...

danielBelenky · 2020-01-15T17:10:44Z

pkg/virt-handler/vm.go

+// checkCPUForMigration is a hotfix for bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1760028
+// to disable migration when host-model od cpu-pass-through is used.
+// The check here relies on the fact the first defaulting is done in mutating webhook
+// where the default is set based od cluster-wide config, which still can be empty.


danielBelenky · 2020-01-15T17:11:46Z

pkg/virt-handler/vm.go

+// to disable migration when host-model od cpu-pass-through is used.
+// The check here relies on the fact the first defaulting is done in mutating webhook
+// where the default is set based od cluster-wide config, which still can be empty.
+// If no CPU is setup in VMI and the empty CPU gets here it is passed over to virt-launcher.


s/setup in/set on

pkg/virt-handler/vm.go

danielBelenky · 2020-01-15T17:16:24Z

staging/src/kubevirt.io/client-go/api/v1/types.go

@@ -233,6 +233,8 @@ const (
 	VirtualMachineInstanceReasonDisksNotMigratable = "DisksNotLiveMigratable"
 	// Reason means that VMI is not live migratioable because of it's network interfaces collection
 	VirtualMachineInstanceReasonInterfaceNotMigratable = "InterfaceNotLiveMigratable"
+	// Reason means that VMI is not live migratioable because of it's network interfaces collection


I think that the comment here is wrong...

danielBelenky · 2020-01-17T08:09:36Z

/lgtm

ksimon1 · 2020-01-17T09:36:05Z

/lgtm

petrkotas · 2020-01-17T14:41:55Z

/retest

petrkotas · 2020-01-17T19:18:19Z

/retest

petrkotas · 2020-01-19T09:28:58Z

Hi @danielBelenky and @ksimon1 would you have time to do recheck for this PR? I see the tests are failing for following issues, which are unrelated.

k8s-1.14.6: [rfe_id:3423][vendor:cnv-qe@redhat.com][level:component]oc/kubectl get vm/vmi tests should verify set of wide columns for [test_id:3422]virtualmachineinstance
k8s-1.16.2: [rfe_id:3423][vendor:cnv-qe@redhat.com][level:component]oc/kubectl get vm/vmi tests should verify set of wide columns for [test_id:3422]virtualmachineinstance

ksimon1

LGTM

danielBelenky · 2020-01-20T08:00:04Z

/lgtm

dhiller

Looking good already. The only thing I'm missing here is unit tests for the new method checkCPUForMigration. While you're at it, could you please add unit tests for checkNetworkInterfacesForMigration also?

pkg/virt-handler/vm.go

pkg/virt-handler/vm_test.go

enp0s3 · 2020-09-16T10:58:30Z

/assign

fabiand · 2020-09-17T10:52:57Z

High level comment:
On a homogeneous cluster OR if CPUNodeDiscovery is used, then host-model is safe to migrate.
Thus it must not block migration (speak: not featuregated).

host-passthrough is also migratable, but under much stricter constraints. But because this is required for certain workloads we should allow this, but make it harder to use (speak: featuregated)

During migration compatible CPU model have to be used. This is hard when cpu-passthrough or host-model is used as a choice for CPU. This issue can be ovecomed in a short term by blocking migration for host-model or cpu-passthrough. This commit hotfixes https://bugzilla.redhat.com/show_bug.cgi?id=1760028 Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

enp0s3 · 2020-10-04T12:24:36Z

/hold fixing merge issues

enp0s3 · 2020-10-04T12:32:37Z

/hold cancel

dankenigsberg

Please modify the PR text (and summary?) to mention that migration is STILL possible if the feature gates are enabled.
Also, please confirm that the feature gate can be requested via HCO.

dankenigsberg · 2020-10-04T12:54:25Z

pkg/virt-config/feature-gates.go

@@ -34,6 +34,11 @@ const (
 	SnapshotGate          = "Snapshot"
 	HostDiskGate          = "HostDisk"
 	VirtIOFSGate          = "ExperimentalVirtiofsSupport"
+	// 	CPUMigrationGate allows users to block migrations for virtual machines with host-model
+	CPUMigrationGate = "MigrationCPU"


this name is too opaque for me - I cannot tell what it means. How about MigratableHostModel ?

@dankenigsberg Completely agree. I haven't started the re-factoring. It was only the rebase. Now I will update the PR code itself according to the discussion

Signed-off-by: L. Pivarc <lpivarc@redhat.com>

kubevirt-bot · 2020-10-06T07:19:37Z

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

Test name	Commit	Details	Rerun command
pull-kubevirt-e2e-k8s-1.16.2	`f0743e1`	link	`/test pull-kubevirt-e2e-k8s-1.16.2`
pull-kubevirt-e2e-k8s-1.15.1-ceph	`f0743e1`	link	`/test pull-kubevirt-e2e-k8s-1.15.1-ceph`
pull-kubevirt-e2e-k8s-1.14.6	`f0743e1`	link	`/test pull-kubevirt-e2e-k8s-1.14.6`
pull-kubevirt-e2e-k8s-1.14	`d7c1f09`	link	`/test pull-kubevirt-e2e-k8s-1.14`
pull-kubevirt-e2e-k8s-1.15-ceph	`7384be5`	link	`/test pull-kubevirt-e2e-k8s-1.15-ceph`
pull-kubevirt-e2e-kind-k8s-1.17.0-ipv6	`99b76c7`	link	`/test pull-kubevirt-e2e-kind-k8s-1.17.0-ipv6`
pull-kubevirt-goveralls	`d2b65f8`	link	`/test pull-kubevirt-goveralls`

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.

enp0s3 · 2020-10-06T10:59:48Z

Due to many changes I would like to close this PR and relate to #4323

enp0s3 · 2020-10-06T11:02:07Z

/hold

enp0s3 · 2020-10-06T11:26:47Z

/close

kubevirt-bot · 2020-10-06T11:26:51Z

@enp0s3: Closed this PR.

In response to this:

/close

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 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. labels Jan 13, 2020

kubevirt-bot requested review from ILpinto and phoracek January 13, 2020 17:14

kubevirt-bot added size/L kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jan 13, 2020

petrkotas force-pushed the migration-hostmodel branch 2 times, most recently from 3660090 to 142e6e1 Compare January 14, 2020 13:00

danielBelenky suggested changes Jan 15, 2020

View reviewed changes

kubevirt-bot assigned danielBelenky Jan 15, 2020

petrkotas force-pushed the migration-hostmodel branch from 142e6e1 to dfbc9df Compare January 16, 2020 13:51

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

kubevirt-bot assigned ksimon1 Jan 17, 2020

ksimon1 removed their assignment Jan 17, 2020

petrkotas force-pushed the migration-hostmodel branch from dfbc9df to 3c86ac0 Compare January 17, 2020 10:39

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2020

ksimon1 approved these changes Jan 20, 2020

View reviewed changes

kubevirt-bot assigned ksimon1 Jan 20, 2020

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

dhiller requested changes Jan 21, 2020

View reviewed changes

pkg/virt-handler/vm.go Outdated Show resolved Hide resolved

pkg/virt-handler/vm_test.go Outdated Show resolved Hide resolved

kubevirt-bot assigned enp0s3 Sep 16, 2020

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2020

petrkotas added 5 commits October 4, 2020 14:47

Add feature-gate to check CPU for migration

a88dfc7

Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

Enable feature-gate for CPU migration

7691b7c

Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

Add configurable timeout for FedoraExpecter

408b063

Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

Disable feature gate

e22bf7f

Signed-off-by: Petr Kotas <petr.kotas@gmail.com>

enp0s3 force-pushed the migration-hostmodel branch from 99b76c7 to a46b75c Compare October 4, 2020 12:04

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2020

enp0s3 force-pushed the migration-hostmodel branch 2 times, most recently from ddce86f to 371a092 Compare October 4, 2020 12:08

enp0s3 force-pushed the migration-hostmodel branch from 371a092 to 05a52fa Compare October 4, 2020 12:32

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2020

dankenigsberg suggested changes Oct 4, 2020

View reviewed changes

enp0s3 force-pushed the migration-hostmodel branch 3 times, most recently from 0191cce to f724c39 Compare October 5, 2020 11:28

Allow host-passthrough with feature gate

d2b65f8

Signed-off-by: L. Pivarc <lpivarc@redhat.com>

enp0s3 force-pushed the migration-hostmodel branch from f724c39 to d2b65f8 Compare October 6, 2020 06:46

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2020

kubevirt-bot closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make VMI non-migratable when incompatible CPU #2982

Make VMI non-migratable when incompatible CPU #2982

petrkotas commented Jan 13, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

danielBelenky left a comment

danielBelenky Jan 15, 2020

danielBelenky Jan 15, 2020

danielBelenky Jan 15, 2020

danielBelenky commented Jan 17, 2020

ksimon1 commented Jan 17, 2020

petrkotas commented Jan 17, 2020

petrkotas commented Jan 17, 2020

petrkotas commented Jan 19, 2020

ksimon1 left a comment

danielBelenky commented Jan 20, 2020

dhiller left a comment

enp0s3 commented Sep 16, 2020

fabiand commented Sep 17, 2020

enp0s3 commented Oct 4, 2020

enp0s3 commented Oct 4, 2020

dankenigsberg left a comment •

edited

Loading

dankenigsberg Oct 4, 2020

enp0s3 Oct 4, 2020

kubevirt-bot commented Oct 6, 2020 •

edited

Loading

enp0s3 commented Oct 6, 2020

enp0s3 commented Oct 6, 2020

enp0s3 commented Oct 6, 2020

kubevirt-bot commented Oct 6, 2020

Make VMI non-migratable when incompatible CPU #2982

Make VMI non-migratable when incompatible CPU #2982

Conversation

petrkotas commented Jan 13, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

petrkotas commented Jan 15, 2020

danielBelenky left a comment

Choose a reason for hiding this comment

danielBelenky Jan 15, 2020

Choose a reason for hiding this comment

danielBelenky Jan 15, 2020

Choose a reason for hiding this comment

danielBelenky Jan 15, 2020

Choose a reason for hiding this comment

danielBelenky commented Jan 17, 2020

ksimon1 commented Jan 17, 2020

petrkotas commented Jan 17, 2020

petrkotas commented Jan 17, 2020

petrkotas commented Jan 19, 2020

ksimon1 left a comment

Choose a reason for hiding this comment

danielBelenky commented Jan 20, 2020

dhiller left a comment

Choose a reason for hiding this comment

enp0s3 commented Sep 16, 2020

fabiand commented Sep 17, 2020

enp0s3 commented Oct 4, 2020

enp0s3 commented Oct 4, 2020

dankenigsberg left a comment • edited Loading

Choose a reason for hiding this comment

dankenigsberg Oct 4, 2020

Choose a reason for hiding this comment

enp0s3 Oct 4, 2020

Choose a reason for hiding this comment

kubevirt-bot commented Oct 6, 2020 • edited Loading

enp0s3 commented Oct 6, 2020

enp0s3 commented Oct 6, 2020

enp0s3 commented Oct 6, 2020

kubevirt-bot commented Oct 6, 2020

dankenigsberg left a comment •

edited

Loading

kubevirt-bot commented Oct 6, 2020 •

edited

Loading