Add VM LiveMigratable Condition to KubevirtMachine #291

orenc1 · 2024-07-14T07:28:34Z

What this PR does / why we need it:
Virtual Machines serving as guest cluster nodes can be live migrated, this is beneficial in case of infra cluster nodes maintainance or mulfunction,
requiring the Virtual Machines to be migrated to another node.
This PR introduces the "VMLiveMigratable" condition to KubevirtMachine, and it takes that value as-is from the underlying VirtualMachine instance.
Then, consumers of cluster-api-provider-kubevirt could make use of it and buble that information up to the end user.

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

Special notes for your reviewer:

Release notes:

add VMLiveMigratable Condition to KubevirtMachine

orenc1 · 2024-07-14T07:31:53Z

/ok-to-test

k8s-ci-robot · 2024-07-14T07:32:07Z

@orenc1: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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-sigs/prow repository.

nunnatsa · 2024-07-14T14:15:55Z

/ok-to-test

coveralls · 2024-07-14T14:18:20Z

Pull Request Test Coverage Report for Build 9975585266

Details

25 of 42 (59.52%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 59.245%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/kubevirtmachine_controller.go	9	13	69.23%
pkg/kubevirt/machine.go	0	13	0.0%

Totals
Change from base Build 9421398414:	-0.04%
Covered Lines:	1570
Relevant Lines:	2650

💛 - Coveralls

nunnatsa

Added a few comments

nunnatsa · 2024-07-14T14:19:08Z

controllers/kubevirtmachine_controller.go

+		conditions.MarkTrue(ctx.KubevirtMachine, infrav1.VMLiveMigratableCondition)
+	} else {
+		conditions.MarkFalse(ctx.KubevirtMachine, infrav1.VMLiveMigratableCondition, infrav1.VMNotLiveMigratableReason, clusterv1.ConditionSeverityInfo,
+			fmt.Sprintf("%s is not a live migratable machine", ctx.KubevirtMachine.Name))


Can we bubble up the actual reason from the vmi?

yes, good point.
do we want only the reason, or also the message text from the VM condition?

I would say, message is more important here. please add the underline message

done. the message is now being shown if the condition status is false.

nunnatsa · 2024-07-14T14:22:16Z

pkg/infracluster/infracluster_test.go

 	"sigs.k8s.io/controller-runtime/pkg/client"
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	. "sigs.k8s.io/cluster-api-provider-kubevirt/pkg/infracluster"


Please avoid dot import

this was before my change.
probably it moved by running make generate

nunnatsa · 2024-07-15T12:33:14Z

pkg/kubevirt/machine.go

+	for _, cond := range m.vmiInstance.Status.Conditions {
+		if cond.Type == kubevirtv1.VirtualMachineInstanceIsMigratable &&
+			cond.Status == corev1.ConditionTrue {
+			return true
+		}
+	}


use conditions.IsTrue()

it is not possible to use conditions.IsTrue(...) on VirtualMachineInstance because it does not implement the Getter method GetConditions() clusterv1.Conditions

oh, custom conditions. grrr!

nunnatsa · 2024-07-15T12:34:24Z

pkg/kubevirt/machine.go

+func (m *Machine) IsLiveMigratable() bool {
+	return m.hasLiveMigratableCondition()
+}


Why do we need this wrap over hasLiveMigratableCondition? we can just move the logic here.

i did it in a similar fashion as done with the IsReady() interface.

ok, the logic has been moved to IsLiveMigratable() and it is now returning two more values (message and error). m.hasLiveMigratableCondition() has been removed.

davidvossel · 2024-07-15T17:02:24Z

pkg/kubevirt/machine.go

+	for _, cond := range m.vmiInstance.Status.Conditions {
+		if cond.Type == kubevirtv1.VirtualMachineInstanceIsMigratable {
+			if cond.Status == corev1.ConditionTrue {
+				return true, "", nil
+			} else {
+				return false, cond.Message, nil
+			}
+		}
+	}


in addition to the message, would it also make sense to pass the original "Reason" back from the VMI condition and copy it to the KubeVirtMachine reason?

done. the reason from the VMI condition is now being copied to the KubevirtMachine condition as-is.

Virtual Machines serving as guest cluster nodes can be live migrated, this is beneficial in case of infra cluster nodes maintainance or mulfunction, requiring the Virtual Machines to be migrated to another node. This PR introduces the "VMLiveMigratable" condition to KubevirtMachine, and it takes that value as-is from the underlying VirtualMachine instance. Then, consumers of cluster-api-provider-kubevirt could make use of it and buble that information up to the end user. Signed-off-by: Oren Cohen <[email protected]>

davidvossel

/approve

nunnatsa

/lgtm

k8s-ci-robot · 2024-07-21T05:40:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, nunnatsa, orenc1

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,nunnatsa]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 14, 2024

k8s-ci-robot requested review from qinqon and timothysc July 14, 2024 07:28

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 14, 2024

orenc1 force-pushed the add_vm_live_migratable_condition branch from 0ef59c2 to 1bd1cba Compare July 14, 2024 09:19

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 14, 2024

orenc1 changed the title ~~[WIP] Add vm live migratable condition~~ Add VM LiveMigratable Condition to KubevirtMachine Jul 14, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2024

orenc1 force-pushed the add_vm_live_migratable_condition branch from 1bd1cba to dcf7114 Compare July 14, 2024 09:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 14, 2024

orenc1 force-pushed the add_vm_live_migratable_condition branch from dcf7114 to e7d006a Compare July 14, 2024 14:04

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 14, 2024

orenc1 mentioned this pull request Jul 14, 2024

CNV-32074: Add Kubevirt live-migratable condition for HC and NP openshift/hypershift#4361

Merged

4 tasks

nunnatsa reviewed Jul 15, 2024

View reviewed changes

orenc1 force-pushed the add_vm_live_migratable_condition branch 3 times, most recently from c398c0c to feaa795 Compare July 15, 2024 15:12

davidvossel reviewed Jul 15, 2024

View reviewed changes

orenc1 force-pushed the add_vm_live_migratable_condition branch 2 times, most recently from 7cf12df to 0e6cc11 Compare July 17, 2024 13:15

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 17, 2024

orenc1 force-pushed the add_vm_live_migratable_condition branch from 0e6cc11 to c279635 Compare July 17, 2024 13:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2024

orenc1 force-pushed the add_vm_live_migratable_condition branch from c279635 to 0ce6061 Compare July 17, 2024 13:55

davidvossel reviewed Jul 17, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2024

nunnatsa approved these changes Jul 21, 2024

View reviewed changes

k8s-ci-robot assigned nunnatsa Jul 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2024

k8s-ci-robot merged commit 8c78f30 into kubernetes-sigs:main Jul 21, 2024
13 of 15 checks passed

orenc1 deleted the add_vm_live_migratable_condition branch July 22, 2024 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VM LiveMigratable Condition to KubevirtMachine #291

Add VM LiveMigratable Condition to KubevirtMachine #291

orenc1 commented Jul 14, 2024 •

edited

Loading

orenc1 commented Jul 14, 2024

k8s-ci-robot commented Jul 14, 2024

nunnatsa commented Jul 14, 2024

coveralls commented Jul 14, 2024 •

edited

Loading

nunnatsa left a comment

nunnatsa Jul 14, 2024

orenc1 Jul 15, 2024

nunnatsa Jul 15, 2024

orenc1 Jul 15, 2024

nunnatsa Jul 14, 2024

orenc1 Jul 15, 2024

nunnatsa Jul 15, 2024

orenc1 Jul 15, 2024

nunnatsa Jul 15, 2024

nunnatsa Jul 15, 2024

orenc1 Jul 15, 2024

orenc1 Jul 15, 2024

davidvossel Jul 15, 2024

orenc1 Jul 16, 2024

davidvossel left a comment

nunnatsa left a comment

k8s-ci-robot commented Jul 21, 2024

Add VM LiveMigratable Condition to KubevirtMachine #291

Add VM LiveMigratable Condition to KubevirtMachine #291

Conversation

orenc1 commented Jul 14, 2024 • edited Loading

orenc1 commented Jul 14, 2024

k8s-ci-robot commented Jul 14, 2024

nunnatsa commented Jul 14, 2024

coveralls commented Jul 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9975585266

Details

💛 - Coveralls

nunnatsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvossel left a comment

Choose a reason for hiding this comment

nunnatsa left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 21, 2024

orenc1 commented Jul 14, 2024 •

edited

Loading

coveralls commented Jul 14, 2024 •

edited

Loading