[e2e]: fix CPU hotplug flakes #10835

enp0s3 · 2023-12-05T11:28:31Z

Signed-off-by: Igor Bezukh ibezukh@redhat.com

What this PR does / why we need it:
CPU hotplug involves live-migration and since GetRunningPodByLabel was used in the test, sometimes
it returned the wrong running pod, thus resulted in a flake. The PR aims to fix GetRunningPodByLabel and
do some code cleanup on the way.

Fixes #
#10690

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

NONE

enp0s3 · 2023-12-05T11:58:14Z

/test pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations

enp0s3 · 2023-12-05T12:39:32Z

/test pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations

enp0s3 · 2023-12-05T16:03:49Z

/test all

fossedihelm · 2023-12-11T08:30:47Z

/cc

fossedihelm · 2023-12-11T08:55:55Z

I think this is a great opportunity to fix this TODO

kubevirt/tests/utils.go

Lines 470 to 475 in 7e3aa9f

    
           for _, pod := range pods.Items { 
        
           	// TODO: This needs to be reworked. 
        
           	// During migration there can be more than one pod 
        
           	readyPod = &pod 
        
           	break 
        
           }

I don't think its trivial to rework the GetRunningPodByLabel function to return the correct running
pod in case it was called during a live-migration, because there can be multiple migrations in the test
in the same namespace, and we can hit a race when we get 2 running pods, and all the listed migrations
are at succeeded phase.

IMHO there could not be multiple migrations in the test in the same namespace, because tests runs in parallel in different namespace. So I think this is a non-problem.
It should not be enough something like this?

        for _, pod := range pods.Items {
		// During migration there can be more than one pod
		// In this case we want the target pod
		if readyPod != nil || readyPod.CreationTimestamp.Before(&pod.CreationTimestamp) {
			continue
		}

		readyPod = &pod
	}

Thanks!
In any case, I agree with you that we can be more precise in the test. WDYT?

fossedihelm · 2023-12-11T09:47:08Z

/kind flake

xpivarc · 2023-12-12T13:00:15Z

@fossedihelm
I think what Federico suggests, fixing the function, makes total sense. In this fix, we rely on implementation details and we will need to change the tests once again in the future.

enp0s3 · 2024-01-24T09:41:27Z

@xpivarc @fossedihelm Can you PTAL?

fossedihelm · 2024-01-24T14:14:50Z

tests/libpod/query.go

+		if readyPod != nil && readyPod.CreationTimestamp.Before(&pod.CreationTimestamp) {
+			continue
+		}
+
+		readyPod = &pod


My bad for the previous wrong suggestion.
Now that I am reading better, shouldn't this be reversed?

Suggested change

if readyPod != nil && readyPod.CreationTimestamp.Before(&pod.CreationTimestamp) {

continue

}

readyPod = &pod

if readyPod != nil && pod.CreationTimestamp.Before(&readyPod.CreationTimestamp) {

continue

}

readyPod = &pod

We want to skip this pod if the "current" readyPod was created before it.
Or am I totally confused?
Thanks

@xpivarc @fossedihelm Thanks folks!

fossedihelm

Thanks for the patience!

xpivarc · 2024-01-24T15:27:31Z

tests/libpod/query.go

+			continue
+		}
+
+		readyPod = &pod


shouldn't you do

pod := pod readyPod = &pod

see https://go.dev/play/p/5EJYJjoIh3K

Interesting! Nice one!
but maybe is it better to index from range?

I think it does not matter much but I think pod := pod was mentioned in "effective Go".

@fossedihelm @xpivarc Hey. I have added a function that sorts by creation ts, please share your thoughts about the updated approach

My motivation was not re-inventing the wheel when its about a sorting, and eliminate redundant boilerplate with pointers

this is done as part of the effort to reduce the utils.go boilerplate. Signed-off-by: Igor Bezukh <ibezukh@redhat.com>

the improvement gives the function a sense of library function. Signed-off-by: Igor Bezukh <ibezukh@redhat.com>

fossedihelm · 2024-01-29T08:27:25Z

tests/libpod/sort.go

+import v1 "k8s.io/api/core/v1"
+
+// PodsByCreationTimestamp sorts a list of Pods by creation timestamp, using their names as a tie breaker.
+type PodsByCreationTimestamp []v1.Pod


Looks good to me! But honestly I am wondering if it is worth it to make it more general that can be used elsewhere, with smth like:

import ( k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // KMetaObject sorts a list of k8s meta objects by creation timestamp. type KMetaObject []k8smetav1.ObjectMeta func (o KMetaObject) Len() int { return len(o) } func (o KMetaObject) Swap(i, j int) { o[i], o[j] = o[j], o[i] } func (o KMetaObject) Less(i, j int) bool { if o[i].CreationTimestamp.Equal(&o[j].CreationTimestamp) { return o[i].Name < o[j].Name } return o[i].CreationTimestamp.Before(&o[j].CreationTimestamp) }

I see that in the migration controller, we do basically the same thing, but for migrations

kubevirt/pkg/virt-controller/watch/migration.go

Lines 1629 to 1643 in a6f4f91

type vmimCollection []*virtv1.VirtualMachineInstanceMigration

func (c vmimCollection) Len() int {

return len(c)

}

func (c vmimCollection) Less(i, j int) bool {

t1 := &c[i].CreationTimestamp

t2 := &c[j].CreationTimestamp

return t1.Before(t2)

}

func (c vmimCollection) Swap(i, j int) {

c[i], c[j] = c[j], c[i]

}

I don't know if it is worth it. WDYT?

@fossedihelm Thanks for the feedback. The problem here is that the casting won't work: sort.Reverse(PodsByCreationTimestamp(pods.Items)

But we can solve it with Go generics. However IMO we would need some place like test-infra to put generic stuff there, and the libs would implement their things on top of that, it would require a separate PR though.

That's fine! Thanks

Note: It would be good to backport this as well.

xpivarc

The 3rd commit is now redundant, the sort is a little more than we need but fine by me.

tests/libpod/query.go

it wasn't compatible with VM migration case where there could be 2 running virt-laucnher pods. With the fix the function will return the target pod when its ready. also added library function that sorts pods by creation ts Signed-off-by: Igor Bezukh <ibezukh@redhat.com>

fossedihelm

Thanks

enp0s3 · 2024-01-29T11:28:52Z

/retest-required

xpivarc

/approve

kubevirt-bot · 2024-01-29T14:51:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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:

~~tests/OWNERS~~ [xpivarc]

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

kubevirt-commenter-bot · 2024-01-29T14:51:22Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-kind-1.27-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-bot · 2024-01-30T00:50:30Z

@enp0s3: 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.29-sig-monitoring	`b69d77e`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring`
pull-kubevirt-check-tests-for-flakes	`0991bae`	link	false	`/test pull-kubevirt-check-tests-for-flakes`
pull-kubevirt-e2e-k8s-1.29-sig-storage	`0991bae`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.29-sig-storage`

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 · 2024-01-30T04:58:09Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot requested review from kbidarkar and maiqueb December 5, 2023 11:28

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch from f7bf712 to 41e039a Compare December 5, 2023 11:33

enp0s3 marked this pull request as draft December 5, 2023 11:48

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2023

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch from 41e039a to e23f91a Compare December 5, 2023 12:38

enp0s3 marked this pull request as ready for review December 11, 2023 08:23

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023

kubevirt-bot requested a review from fossedihelm December 11, 2023 08:30

kubevirt-bot added the kind/flake Categorizes issue or PR as related to a flaky test. label Dec 11, 2023

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch from e23f91a to 3bb20ea Compare January 24, 2024 09:07

kubevirt-bot added size/L and removed size/S labels Jan 24, 2024

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch 2 times, most recently from 74cfdcf to e220b23 Compare January 24, 2024 09:38

enp0s3 marked this pull request as ready for review January 24, 2024 13:54

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2024

kubevirt-bot requested a review from fossedihelm January 24, 2024 13:54

fossedihelm reviewed Jan 24, 2024

View reviewed changes

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch 2 times, most recently from 909ac7c to b69d77e Compare January 24, 2024 14:46

fossedihelm approved these changes Jan 24, 2024

View reviewed changes

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

xpivarc reviewed Jan 24, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2024

enp0s3 added 2 commits January 28, 2024 10:12

e2e: move pod related query functions to libpod

0a807c8

this is done as part of the effort to reduce the utils.go boilerplate. Signed-off-by: Igor Bezukh <ibezukh@redhat.com>

e2e: pass virtClient as argument to GetRunningPodByLabel

920e6fd

the improvement gives the function a sense of library function. Signed-off-by: Igor Bezukh <ibezukh@redhat.com>

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch from b69d77e to 0ed213d Compare January 28, 2024 08:26

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 28, 2024

fossedihelm reviewed Jan 29, 2024

View reviewed changes

xpivarc reviewed Jan 29, 2024

View reviewed changes

tests/libpod/query.go Outdated Show resolved Hide resolved

enp0s3 force-pushed the cpu-hotplug-flakes-fix branch from a36a15e to 0991bae Compare January 29, 2024 08:56

fossedihelm approved these changes Jan 29, 2024

View reviewed changes

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

xpivarc reviewed Jan 29, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2024

kubevirt-bot merged commit 72bb713 into kubevirt:main Jan 30, 2024
34 of 36 checks passed

acardace mentioned this pull request Jan 30, 2024

CPU Hotplug failures #10690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[e2e]: fix CPU hotplug flakes #10835

[e2e]: fix CPU hotplug flakes #10835

enp0s3 commented Dec 5, 2023 •

edited

enp0s3 commented Dec 5, 2023

enp0s3 commented Dec 5, 2023

enp0s3 commented Dec 5, 2023

fossedihelm commented Dec 11, 2023

fossedihelm commented Dec 11, 2023

fossedihelm commented Dec 11, 2023

xpivarc commented Dec 12, 2023

enp0s3 commented Jan 24, 2024

fossedihelm Jan 24, 2024

xpivarc Jan 24, 2024

enp0s3 Jan 24, 2024

fossedihelm left a comment

xpivarc Jan 24, 2024

fossedihelm Jan 24, 2024

xpivarc Jan 25, 2024

enp0s3 Jan 28, 2024

enp0s3 Jan 28, 2024

fossedihelm Jan 29, 2024

enp0s3 Jan 29, 2024

fossedihelm Jan 29, 2024

xpivarc Jan 29, 2024

xpivarc left a comment

fossedihelm left a comment

enp0s3 commented Jan 29, 2024

xpivarc left a comment

kubevirt-bot commented Jan 29, 2024

kubevirt-commenter-bot commented Jan 29, 2024

kubevirt-bot commented Jan 30, 2024 •

edited

kubevirt-commenter-bot commented Jan 30, 2024

	type vmimCollection []*virtv1.VirtualMachineInstanceMigration

	func (c vmimCollection) Len() int {
	return len(c)
	}

	func (c vmimCollection) Less(i, j int) bool {
	t1 := &c[i].CreationTimestamp
	t2 := &c[j].CreationTimestamp
	return t1.Before(t2)
	}

	func (c vmimCollection) Swap(i, j int) {
	c[i], c[j] = c[j], c[i]
	}

[e2e]: fix CPU hotplug flakes #10835

[e2e]: fix CPU hotplug flakes #10835

Conversation

enp0s3 commented Dec 5, 2023 • edited

enp0s3 commented Dec 5, 2023

enp0s3 commented Dec 5, 2023

enp0s3 commented Dec 5, 2023

fossedihelm commented Dec 11, 2023

fossedihelm commented Dec 11, 2023

fossedihelm commented Dec 11, 2023

xpivarc commented Dec 12, 2023

enp0s3 commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossedihelm 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

xpivarc left a comment

Choose a reason for hiding this comment

fossedihelm left a comment

Choose a reason for hiding this comment

enp0s3 commented Jan 29, 2024

xpivarc left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jan 29, 2024

kubevirt-commenter-bot commented Jan 29, 2024

kubevirt-bot commented Jan 30, 2024 • edited

kubevirt-commenter-bot commented Jan 30, 2024

enp0s3 commented Dec 5, 2023 •

edited

kubevirt-bot commented Jan 30, 2024 •

edited