[release-0.58] Use exponential backoff for failing migrations

[kubevirt-bot]

This is an automated cherry-pick of #8530

/assign acardace

Use exponential backoff for failing migrations

[enp0s3]

/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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 [enp0s3]

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

[kubevirt-bot]

@kubevirt-bot: 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-goveralls	cc00896	link	false	/test pull-kubevirt-goveralls-0.58
pull-kubevirt-e2e-k8s-1.22-sig-performance	cc00896	link	false	/test pull-kubevirt-e2e-k8s-1.22-sig-performance-0.58

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.

[rmohr]

I am in general not too concerned about this change, but it looks like this would in general add a slightly weird cross-object relationship which is hard to understand from outside and very uncommon from a general usage perspective. The eviction problem is however real and definitely deserves a solution.

I discussed a few options with @xpivarc and I think, in order to backport this it would make sense to limit this for now the migrations which are solely created by the eviction webhook (e.g. adding a label and filtering by it) and leaving the general migration behaviour unchanged.

On main we could then decide if we want to keep it this way, or try moving the whole backoff logic to the webhook.

What do you think?

[enp0s3]

/hold
looking

[acardace]

I am in general not too concerned about this change, but it looks like this would in general add a slightly weird cross-object relationship which is hard to understand from outside and very uncommon from a general usage perspective. The eviction problem is however real and definitely deserves a solution.

I see your point, it does make sense.

I discussed a few options with @xpivarc and I think, in order to backport this it would make sense to limit this for now the migrations which are solely created by the eviction webhook (e.g. adding a label and filtering by it) and leaving the general migration behaviour unchanged.

Yes, I think this could work.

On main we could then decide if we want to keep it this way, or try moving the whole backoff logic to the webhook.

What do you think?

So I'd first introduce this change here in release-0.58 and then implement it in main? It would be a first.

[rmohr]

I am in general not too concerned about this change, but it looks like this would in general add a slightly weird cross-object relationship which is hard to understand from outside and very uncommon from a general usage perspective. The eviction problem is however real and definitely deserves a solution.

I see your point, it does make sense.

I discussed a few options with @xpivarc and I think, in order to backport this it would make sense to limit this for now the migrations which are solely created by the eviction webhook (e.g. adding a label and filtering by it) and leaving the general migration behaviour unchanged.

Yes, I think this could work.

On main we could then decide if we want to keep it this way, or try moving the whole backoff logic to the webhook.
What do you think?

So I'd first introduce this change here in release-0.58 and then implement it in main? It would be a first.

ah no, lets' follow the usual flow: main and back-port.

[acardace]

@xpivarc @rmohr
I created the new PR #8808, can you guys take a look?

[acardace]

/retest-required

[acardace]

@enp0s3 @rmohr can we now unhold this?

[enp0s3]

/unhold

@acardace I Don't know what is the desired strategy in this case - to have a manual backport that contains the 2 PRs, or to have 2 automated backports. IMO we don't take a high risk by separating it into 2 separate automated backports.

[enp0s3]

/retest-required

[rmohr]

/unhold

@acardace I Don't know what is the desired strategy in this case - to have a manual backport that contains the 2 PRs, or to have 2 automated backports. IMO we don't take a high risk by separating it into 2 separate automated backports.

+1 two should be fine.