Timeout Unschedulable Migration Target Pods #6737

davidvossel · 2021-11-04T20:57:59Z

We now observe two timeouts

5m timeout will cancel any target pod stuck specifically in an "unschedulable" pending pods for > 5m
15m timeout will cancel any target pod stuck in pending for any reason > 15m

I created the 15m catch all to account for all the unknown reasons for why a pod will never transition successfully to a running phase, this could be issues on the node with pulling the container image, or perhaps something new we can't predict.

Pending migration target pods timeout after 5 minutes when unschedulable

davidvossel · 2021-11-04T20:58:15Z

/hold

I want to write a functional test before this is merged.

maiqueb

Just a couple of comments; the most important one - imo - is to name the variable to shed some light into the re-queing mechanism.

maiqueb · 2021-11-05T09:14:59Z

pkg/virt-controller/watch/migration.go

+		// Make sure we check this again after some time
+		c.Queue.AddAfter(migrationKey, time.Second*time.Duration(c.unschedulableTimeoutSeconds-diffSeconds))


Maybe I'm just too thick, but I don't follow this c.unschedulableTimeoutSeconds-diffSeconds .

Would you assign it to a named variable to help people like me reason this out ?

i changed this around a bit to try and help

Tks. Better now.

maiqueb · 2021-11-05T09:21:31Z

pkg/virt-controller/watch/migration.go

+		return nil
+	}
+
+	isUnschedulable := false


I don't think this variable is required, nor the control code to exit the function if the pod is not k8sv1.PodReasonUnschedulable .

You could move all the code in https://github.com/kubevirt/kubevirt/pull/6737/files#diff-1baf192bb9c4560d7d40f008b7ba5c680ebce669b8f1b7639445abd3e7269688R820-R844 to a couple of new functions (named didPodTimeOut(pod *v1.Pod) bool {...} and func (c *MigrationController) podTimedOut(vmi, pod) error {...}) then directly call them when the right condition is met - i.e. condition.Type == k8sv1.PodScheduled && condition.Status == k8sv1.ConditionFalse && condition.Reason == k8sv1.PodReasonUnschedulable.

Not a must though, I won't insist.

I refactored this a bit to hopefully make it easier to follow

maiqueb · 2021-11-05T09:24:59Z

pkg/virt-controller/watch/migration_test.go

@@ -584,6 +599,43 @@ var _ = Describe("Migration watcher", func() {
 			shouldExpectMigrationSchedulingState(migration)
 			controller.Execute()
 		})
+
+		table.DescribeTable("should handle pod stuck in unschedulable state", func(phase virtv1.VirtualMachineInstanceMigrationPhase, shouldTimeout bool, timeLapse int64) {


I'm not fond of defining the expected test state in the test's input parameters - i.e. the shouldTimeout boolean input argument - but I don't see simpler alternatives.

this is a fairly common pattern we use with tables. Pass in various input and declare what output is expected. It's a way to condense more tests into a single table.

davidvossel · 2021-11-05T13:38:35Z

/hold cancel

I added a functional test

davidvossel · 2021-11-05T13:59:41Z

/retest

enp0s3 · 2021-11-09T13:02:58Z

/assign @enp0s3

maiqueb

Thanks. I think it reads better now.

kbidarkar

Migration takes more than 5minutes if vm with CD-ROM.
We need to consider increasing the timeout value here, till this bug is fixed, https://bugzilla.redhat.com/show_bug.cgi?id=2014438

Both the Source and Target virt-launcher Pods would be in Running state during this long migration period of more than 5 minutes.

davidvossel · 2021-11-10T15:23:07Z

Migration takes more than 5minutes if vm with CD-ROM. We need to consider increasing the timeout value here, till this bug is fixed, https://bugzilla.redhat.com/show_bug.cgi?id=2014438

Both the Source and Target virt-launcher Pods would be in Running state during this long migration period of more than 5 minutes.

The timeout in this PR only get's triggered if the target pod is stuck in pending due to scheduling issues for > 5 minutes. If the target pod is running, this time out will not occur.

davidvossel · 2021-11-10T20:19:00Z

/retest

vladikr

Looks great to me.
/approve

vladikr · 2021-11-10T18:02:58Z

pkg/virt-controller/watch/migration.go

@@ -103,6 +108,8 @@ func NewMigrationController(templateService services.TemplateService,
 		migrationStartLock: &sync.Mutex{},
 		clusterConfig:      clusterConfig,
 		statusUpdater:      status.NewMigrationStatusUpdater(clientset),
+
+		unschedulableTimeoutSeconds: defaultUnschedulableTimeoutSeconds,


maybe not for this PR, but did you think about making this configurable?
In that case, we could then override this value in a migration policy. For example, in a policy that targets workloads with a requirement for specific resources.

yep, we definitely could make it configurable. I'll give this some thought. I'm doing a follow up PR to this now where i have a "catch all" timeout for a target pod stuck in pending for any unknown reason, i'll need the timeout to be configurable for that simply to be able to functionally test it.

kubevirt-commenter-bot · 2021-11-12T04:54:02Z

/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 · 2021-11-12T08:46:26Z

Looks great to me.
/approve

Also for me. 👍

/approve

kubevirt-bot · 2021-11-12T08:47:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr, vladikr

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~~ [rmohr,vladikr]

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 · 2021-11-12T16:54:48Z

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

davidvossel · 2021-11-12T21:24:46Z

@vladikr @rmohr take another look. I made a new change.

We now observe two timeouts

5m timeout will cancel any target pod stuck specifically in an "unschedulable" pending pods for > 5m
15m timeout will cancel any target pod stuck in pending for any reason > 15m

I created the 15m catch all to account for all the unknown reasons for why a pod will never transition successfully to a running phase, this could be issues on the node with pulling the container image, or perhaps something new we can't predict.

stu-gott · 2021-11-15T15:43:57Z

/retest

Signed-off-by: David Vossel <davidvossel@gmail.com>

behavior for functional tests Signed-off-by: David Vossel <davidvossel@gmail.com>

Signed-off-by: David Vossel <davidvossel@gmail.com>

kubevirt-bot · 2021-11-18T21:46:03Z

@davidvossel: The following test 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-check-tests-for-flakes	`7a99b5a`	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.

davidvossel · 2021-11-19T13:25:54Z

/retest

rmohr · 2021-11-19T13:43:50Z

/lgtm

enp0s3 · 2021-11-25T12:48:48Z

/cherry-pick release-0.44

kubevirt-bot · 2021-11-25T12:49:24Z

@enp0s3: #6737 failed to apply on top of branch "release-0.44":

Applying: Timeout unschedulable migration target pods
Applying: unit test for timing out of unschedulable migration pods
Using index info to reconstruct a base tree...
M	pkg/virt-controller/watch/migration_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/watch/migration_test.go
CONFLICT (content): Merge conflict in pkg/virt-controller/watch/migration_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 unit test for timing out of unschedulable migration pods
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.44

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 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 Nov 4, 2021

kubevirt-bot requested review from acardace and maiqueb November 4, 2021 20:58

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2021

maiqueb reviewed Nov 5, 2021

View reviewed changes

davidvossel force-pushed the timeout-migrations branch from 776058b to db16906 Compare November 5, 2021 13:37

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2021

davidvossel force-pushed the timeout-migrations branch from db16906 to b762830 Compare November 8, 2021 21:35

kubevirt-bot assigned enp0s3 Nov 9, 2021

maiqueb approved these changes Nov 10, 2021

View reviewed changes

kubevirt-bot assigned maiqueb Nov 10, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2021

kbidarkar reviewed Nov 10, 2021

View reviewed changes

vladikr reviewed Nov 11, 2021

View reviewed changes

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

davidvossel force-pushed the timeout-migrations branch from b762830 to 24a6e64 Compare November 12, 2021 21:21

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2021

davidvossel force-pushed the timeout-migrations branch from 24a6e64 to 53bea6f Compare November 12, 2021 21:30

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2021

davidvossel added 7 commits November 18, 2021 09:08

Timeout unschedulable migration target pods

e61cc9c

Signed-off-by: David Vossel <davidvossel@gmail.com>

unit test for timing out of unschedulable migration pods

56346ef

Signed-off-by: David Vossel <davidvossel@gmail.com>

func test to verify unschedulable migrations timeout

f5a936e

Signed-off-by: David Vossel <davidvossel@gmail.com>

Add catch all timeout for pending target pods

d51d7d4

Signed-off-by: David Vossel <davidvossel@gmail.com>

Add unit tests for catch all pending target pod timeout

f689bb5

Signed-off-by: David Vossel <davidvossel@gmail.com>

Add func test annotations to migration object capable influencing

5731bbc

behavior for functional tests Signed-off-by: David Vossel <davidvossel@gmail.com>

Add functional test to exercise catch all pending target pod timeout

7a99b5a

Signed-off-by: David Vossel <davidvossel@gmail.com>

davidvossel force-pushed the timeout-migrations branch from 53bea6f to 7a99b5a Compare November 18, 2021 14:15

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021

kubevirt-bot assigned rmohr Nov 19, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2021

kubevirt-bot merged commit 0ec801d into kubevirt:main Nov 19, 2021

enp0s3 mentioned this pull request Nov 28, 2021

[release-0.44] Timeout Unschedulable Migration Target Pods #6871

Closed

Timeout Unschedulable Migration Target Pods #6737

Timeout Unschedulable Migration Target Pods #6737

davidvossel commented Nov 4, 2021 •

edited

davidvossel commented Nov 4, 2021

maiqueb left a comment

maiqueb Nov 5, 2021

davidvossel Nov 8, 2021

maiqueb Nov 10, 2021

maiqueb Nov 5, 2021

davidvossel Nov 8, 2021

maiqueb Nov 5, 2021

davidvossel Nov 8, 2021

davidvossel commented Nov 5, 2021

davidvossel commented Nov 5, 2021

enp0s3 commented Nov 9, 2021

maiqueb left a comment

kbidarkar left a comment •

edited

davidvossel commented Nov 10, 2021

davidvossel commented Nov 10, 2021

vladikr left a comment

vladikr Nov 10, 2021

davidvossel Nov 11, 2021

kubevirt-commenter-bot commented Nov 12, 2021

rmohr commented Nov 12, 2021

kubevirt-bot commented Nov 12, 2021

kubevirt-commenter-bot commented Nov 12, 2021

davidvossel commented Nov 12, 2021

stu-gott commented Nov 15, 2021

kubevirt-bot commented Nov 18, 2021 •

edited

davidvossel commented Nov 19, 2021

rmohr commented Nov 19, 2021

enp0s3 commented Nov 25, 2021

kubevirt-bot commented Nov 25, 2021

		// Make sure we check this again after some time
		c.Queue.AddAfter(migrationKey, time.Second*time.Duration(c.unschedulableTimeoutSeconds-diffSeconds))

Timeout Unschedulable Migration Target Pods #6737

Timeout Unschedulable Migration Target Pods #6737

Conversation

davidvossel commented Nov 4, 2021 • edited

davidvossel commented Nov 4, 2021

maiqueb 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

davidvossel commented Nov 5, 2021

davidvossel commented Nov 5, 2021

enp0s3 commented Nov 9, 2021

maiqueb left a comment

Choose a reason for hiding this comment

kbidarkar left a comment • edited

Choose a reason for hiding this comment

davidvossel commented Nov 10, 2021

davidvossel commented Nov 10, 2021

vladikr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Nov 12, 2021

rmohr commented Nov 12, 2021

kubevirt-bot commented Nov 12, 2021

kubevirt-commenter-bot commented Nov 12, 2021

davidvossel commented Nov 12, 2021

stu-gott commented Nov 15, 2021

kubevirt-bot commented Nov 18, 2021 • edited

davidvossel commented Nov 19, 2021

rmohr commented Nov 19, 2021

enp0s3 commented Nov 25, 2021

kubevirt-bot commented Nov 25, 2021

davidvossel commented Nov 4, 2021 •

edited

kbidarkar left a comment •

edited

kubevirt-bot commented Nov 18, 2021 •

edited