Filter out LiveMigrate workloadUpdateMethod on SNO #1823

tiraboschi · 2022-03-17T22:16:57Z

Explicitly filter out LiveMigrate
workloadUpdateMethod on SNO clusters.
On SNO cluster Live Migration FG is globally
disabled because we have a single worker node
but still we allow the user (and it's
the default) to pass
workloadUpdateMethod=["LiveMigrate"]
and this causes an infinite loop in
the upgrade handler in virt-controller
that currently ignores the LiveMigration FG.
Let's explicitly filter out LiveMigrate
workloadUpdateMethod to prevent the issue.

Bug-url: https://bugzilla.redhat.com/show_bug.cgi?id=2065308

Signed-off-by: Simone Tiraboschi stirabos@redhat.com

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Release note:

Filter out LiveMigrate workloadUpdateMethod on SNO

tiraboschi · 2022-03-17T22:19:57Z

/cherry-pick release-1.6

kubevirt-bot · 2022-03-17T22:19:58Z

@tiraboschi: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.6

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.

coveralls · 2022-03-17T22:24:15Z

Pull Request Test Coverage Report for Build 2014768203

15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 84.532%

Totals
Change from base Build 1991213068:	0.05%
Covered Lines:	3749
Relevant Lines:	4435

💛 - Coveralls

tiraboschi · 2022-03-18T07:03:28Z

/retest

hco-bot · 2022-03-18T08:14:47Z

hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

kubevirt-bot · 2022-03-18T08:14:52Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-sno-azure

In response to this:

hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

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.

hco-bot · 2022-03-18T08:35:28Z

hco-e2e-image-index-azure, hco-e2e-image-index-gcp lanes succeeded.
/override ci/prow/hco-e2e-image-index-aws
hco-e2e-upgrade-prev-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-azure
hco-e2e-upgrade-prev-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-azure

kubevirt-bot · 2022-03-18T08:35:35Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-aws, ci/prow/hco-e2e-upgrade-prev-index-azure, ci/prow/hco-e2e-upgrade-prev-index-sno-azure

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-gcp lanes succeeded.
/override ci/prow/hco-e2e-image-index-aws
hco-e2e-upgrade-prev-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-azure
hco-e2e-upgrade-prev-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-azure

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.

tiraboschi · 2022-03-18T09:56:03Z

/retest

hco-bot · 2022-03-18T12:51:58Z

hco-e2e-upgrade-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-azure

kubevirt-bot · 2022-03-18T12:52:02Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-azure

In response to this:

hco-e2e-upgrade-index-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-azure

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.

nunnatsa

Added several inline comments.

nunnatsa · 2022-03-21T07:02:51Z

controllers/operands/kubevirt.go

@@ -280,9 +280,11 @@ func hcWorkloadUpdateStrategyToKv(hcObject *hcov1beta1.HyperConvergedWorkloadUpd
 			*kvObject.BatchEvictionSize = *hcObject.BatchEvictionSize
 		}

-		if size := len(hcObject.WorkloadUpdateMethods); size > 0 {
+		filterWorkloadUpdateMethods := filterWorkloadupdatemethods(hcObject.WorkloadUpdateMethods)


Please rename to prevent name collision with the function, and improve readability.

nunnatsa · 2022-03-21T07:04:01Z

controllers/operands/kubevirt.go

+		if size := len(filterWorkloadUpdateMethods); size > 0 {
 			kvObject.WorkloadUpdateMethods = make([]kubevirtcorev1.WorkloadUpdateMethod, size)
-			for i, updateMethod := range hcObject.WorkloadUpdateMethods {
+			for i, updateMethod := range filterWorkloadUpdateMethods {
 				kvObject.WorkloadUpdateMethods[i] = kubevirtcorev1.WorkloadUpdateMethod(updateMethod)
 			}
 		}


check if the copy builtin function can be used here.

no because we also need a typecast between string and kubevirtcorev1.WorkloadUpdateMethod

nunnatsa · 2022-03-21T07:04:35Z

controllers/operands/kubevirt.go

@@ -291,6 +293,20 @@ func hcWorkloadUpdateStrategyToKv(hcObject *hcov1beta1.HyperConvergedWorkloadUpd
 	return kvObject
 }

+func filterWorkloadupdatemethods(workloadUpdateMethods []string) []string {
+	var filterWorkloadUpdateMethods []string


please rename. Same name as the function - I find it confusing

nunnatsa · 2022-03-21T07:11:16Z

controllers/operands/kubevirt.go

+	for i := range workloadUpdateMethods {
+		if workloadUpdateMethods[i] == string(kubevirtcorev1.WorkloadUpdateMethodLiveMigrate) {
+			if hcoutil.GetClusterInfo().IsInfrastructureHighlyAvailable() {
+				filterWorkloadUpdateMethods = append(filterWorkloadUpdateMethods, workloadUpdateMethods[i])
+			}
+		} else {
+			filterWorkloadUpdateMethods = append(filterWorkloadUpdateMethods, workloadUpdateMethods[i])
+		}
+	}
+	return filterWorkloadUpdateMethods
+}


What do you think about this?

Suggested change

for i := range workloadUpdateMethods {

if workloadUpdateMethods[i] == string(kubevirtcorev1.WorkloadUpdateMethodLiveMigrate) {

if hcoutil.GetClusterInfo().IsInfrastructureHighlyAvailable() {

filterWorkloadUpdateMethods = append(filterWorkloadUpdateMethods, workloadUpdateMethods[i])

}

} else {

filterWorkloadUpdateMethods = append(filterWorkloadUpdateMethods, workloadUpdateMethods[i])

}

}

return filterWorkloadUpdateMethods

}

if hcoutil.GetClusterInfo().IsInfrastructureHighlyAvailable() {

copy(workloadUpdateMethods, workloadUpdateMethods)

} else {

for _, method := range workloadUpdateMethods {

if method != string(kubevirtcorev1.WorkloadUpdateMethodLiveMigrate) {

filterWorkloadUpdateMethods = append(filterWorkloadUpdateMethods, method)

}

}

}

return filterWorkloadUpdateMethods

}

nice, and we can even do better avoiding the copy at all if not on SNO

Explicitly filter out LiveMigrate workloadUpdateMethod on SNO clusters. On SNO cluster Live Migration FG is globally disabled because we have a single worker node but still we allow the user (and it's the default) to pass workloadUpdateMethod=["LiveMigrate"] and this causes an infinite loop in the upgrade handler in virt-controller that currently ignores the LiveMigration FG. Let's explicitly filter out LiveMigrate workloadUpdateMethod to prevent the issue. Bug-url: https://bugzilla.redhat.com/show_bug.cgi?id=2065308 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

sonarcloud · 2022-03-21T07:49:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

openshift-ci · 2022-03-21T10:50:37Z

@tiraboschi: 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
ci/prow/hco-e2e-upgrade-prev-index-sno-aws	`95f1f2b`	link	false	`/test hco-e2e-upgrade-prev-index-sno-aws`
ci/prow/hco-e2e-upgrade-index-sno-aws	`95f1f2b`	link	false	`/test hco-e2e-upgrade-index-sno-aws`

Full PR test history. Your PR dashboard.

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.

hco-bot · 2022-03-21T10:54:24Z

hco-e2e-upgrade-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-sno-aws
hco-e2e-upgrade-prev-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-aws

kubevirt-bot · 2022-03-21T10:54:27Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-sno-aws, ci/prow/hco-e2e-upgrade-prev-index-sno-aws

In response to this:

hco-e2e-upgrade-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-sno-aws
hco-e2e-upgrade-prev-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-index-sno-aws

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.

nunnatsa

/approve

kubevirt-bot · 2022-03-21T14:23:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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~~ [nunnatsa]

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

kubevirt-bot · 2022-03-21T14:26:07Z

@tiraboschi: new pull request created: #1825

In response to this:

/cherry-pick release-1.6

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.

tiraboschi requested review from machadovilaca, orenc1 and nunnatsa March 17, 2022 22:16

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. labels Mar 17, 2022

kubevirt-bot requested a review from sradco March 17, 2022 22:17

kubevirt-bot added the size/L label Mar 17, 2022

nunnatsa reviewed Mar 21, 2022

View reviewed changes

tiraboschi force-pushed the bz_2065308 branch from c619461 to 95f1f2b Compare March 21, 2022 07:47

tiraboschi requested a review from nunnatsa March 21, 2022 10:58

nunnatsa approved these changes Mar 21, 2022

View reviewed changes

kubevirt-bot assigned nunnatsa Mar 21, 2022

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

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

kubevirt-bot merged commit f7d4d37 into kubevirt:main Mar 21, 2022

kubevirt-bot mentioned this pull request Mar 21, 2022

[release-1.6] Filter out LiveMigrate workloadUpdateMethod on SNO #1825

Merged

tiraboschi deleted the bz_2065308 branch April 1, 2022 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out LiveMigrate workloadUpdateMethod on SNO #1823

Filter out LiveMigrate workloadUpdateMethod on SNO #1823

tiraboschi commented Mar 17, 2022

tiraboschi commented Mar 17, 2022

kubevirt-bot commented Mar 17, 2022

coveralls commented Mar 17, 2022 •

edited

Loading

tiraboschi commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

tiraboschi commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

nunnatsa left a comment

nunnatsa Mar 21, 2022

tiraboschi Mar 21, 2022

nunnatsa Mar 21, 2022

tiraboschi Mar 21, 2022

nunnatsa Mar 21, 2022

tiraboschi Mar 21, 2022

nunnatsa Mar 21, 2022

tiraboschi Mar 21, 2022

sonarcloud bot commented Mar 21, 2022

openshift-ci bot commented Mar 21, 2022

hco-bot commented Mar 21, 2022

kubevirt-bot commented Mar 21, 2022

nunnatsa left a comment

kubevirt-bot commented Mar 21, 2022

kubevirt-bot commented Mar 21, 2022

Filter out LiveMigrate workloadUpdateMethod on SNO #1823

Filter out LiveMigrate workloadUpdateMethod on SNO #1823

Conversation

tiraboschi commented Mar 17, 2022

tiraboschi commented Mar 17, 2022

kubevirt-bot commented Mar 17, 2022

coveralls commented Mar 17, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2014768203

💛 - Coveralls

tiraboschi commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

tiraboschi commented Mar 18, 2022

hco-bot commented Mar 18, 2022

kubevirt-bot commented Mar 18, 2022

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

sonarcloud bot commented Mar 21, 2022

openshift-ci bot commented Mar 21, 2022

hco-bot commented Mar 21, 2022

kubevirt-bot commented Mar 21, 2022

nunnatsa left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Mar 21, 2022

kubevirt-bot commented Mar 21, 2022

coveralls commented Mar 17, 2022 •

edited

Loading