Graduation of VolumeUpdateStrategy and VolumeMigration #13288
Conversation
kubevirt-bot
added
the
release-note
kubevirt-bot
added
the
dco-signoff: yes
dosubot
bot
added
the
kind/api-change
|
/cc @awels @mhenriks @aglitke |
kubevirt-bot
requested review from
aglitke,
awels and
mhenriks
and removed request for
victortoso and
iholder101
|
/hold |
kubevirt-bot
added
the
do-not-merge/hold
|
As discussed offline, we don't consider #13112 since there is a mitigation. If the user notice that they missed data from the source it means that the writes were performed on the destination and the transfer has been successful. Hence, the user can switch and replace the source volume with the destination one. |
|
/lgtm |
|
My understanding after offline discussion is that while there is a risk of split brain/corruption we have a way for users to recover. I would still want this to explicitly noted down for users and an issue that would track fixing this in a proper way. As well we should do some testing, either manual in which case we share the steps so reviewers can verify. A automatic test is preferred. |
Isn't the #13112 enough? |
|
Pull requests that are marked with After that period the bot marks them with the label /label needs-approver-review |
kubevirt-bot
added
the
needs-approver-review
|
@xpivarc I would really like to see this merged. While it's true there is a known issue (#13112) this will be fixed and without needing to break the current API. There is no chance that we will delete this feature. In addition, volume migration is optional functionality that is only used when explicitly requested by a VM owner. Given all of this, I don't see why the community cannot commit to long-term maintenance of this feature by merging this PR. |
I don't believe that our existing API will need to change. It seems to serve the use case well. On the other hand, some bugs are still affecting this feature. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
Required labels detected, running phase 2 presubmits: |
|
/retest |
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
|
/test pull-kubevirt-e2e-k8s-1.31-sig-performance |
kubevirt-bot
added
the
needs-rebase
kubevirt-bot
removed
lgtm
|
Rebased |
The VolumeUpdateStrategy and VolumeMigration feature gates were introduced in v1.3.0 by kubevirt#11533. Both feature gates have been present for 2 releases and since then multiple issues have been reported and fixed. With the graduation of the two feature gates, we want to indicate to KubeVirt users that the feature will remain and that the API may be considered stable for use. Signed-off-by: Alice Frosi <[email protected]>
tests/storage/migration.go
Outdated
| @@ -78,8 +78,7 @@ var _ = SIGDescribe("Volumes update with migration", decorators.RequiresTwoSched | |||
| WorkloadUpdateMethods: []virtv1.WorkloadUpdateMethod{virtv1.WorkloadUpdateMethodLiveMigrate}, | |||
| } | |||
| rolloutStrategy := pointer.P(virtv1.VMRolloutStrategyLiveUpdate) | |||
| config.PatchWorkloadUpdateMethodAndRolloutStrategy(originalKv.Name, virtClient, updateStrategy, rolloutStrategy, | |||
| []string{virtconfig.VolumesUpdateStrategy, virtconfig.VolumeMigration}) | |||
| config.PatchWorkloadUpdateMethodAndRolloutStrategy(originalKv.Name, virtClient, updateStrategy, rolloutStrategy, []string{}) | |||
There was a problem hiding this comment.
Can't we make the tests parallel now?
There was a problem hiding this comment.
We are still patching kubevirt CR for the rolloutStrategy. But yes, I will investigate this in a separate PR
| @@ -78,8 +78,7 @@ var _ = SIGDescribe("Volumes update with migration", decorators.RequiresTwoSched | |||
| WorkloadUpdateMethods: []virtv1.WorkloadUpdateMethod{virtv1.WorkloadUpdateMethodLiveMigrate}, | |||
| } | |||
| rolloutStrategy := pointer.P(virtv1.VMRolloutStrategyLiveUpdate) | |||
| config.PatchWorkloadUpdateMethodAndRolloutStrategy(originalKv.Name, virtClient, updateStrategy, rolloutStrategy, | |||
| []string{virtconfig.VolumesUpdateStrategy, virtconfig.VolumeMigration}) | |||
| config.PatchWorkloadUpdateMethodAndRolloutStrategy(originalKv.Name, virtClient, updateStrategy, rolloutStrategy, []string{}) | |||
|
|
|||
There was a problem hiding this comment.
different line so this is a separate discussion;
[]string{} won't nil out the feature gates, right?
There was a problem hiding this comment.
I have, now, inlined the function PatchWorkloadUpdateMethodAndRolloutStrategy since it was only used by the tests in the volume migration
Remove the unused feature gates because of their graduation. Signed-off-by: Alice Frosi <[email protected]>
Removed feature gates because of the graduation. Inline the function PatchWorkloadUpdateMethodAndRolloutStrategy since it is only used by the volume migration tests. Signed-off-by: Alice Frosi <[email protected]>
|
Required labels detected, running phase 2 presubmits: |
vladikr
removed
the
needs-approver-review
|
/test pull-kubevirt-code-lint |
|
@alicefr: The following tests failed, say
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. I understand the commands that are listed here. |
|
/retest-required |
What this PR does
Before this PR:
Volume Migration was behind a feature gate
After this PR:
The VolumeUpdateStrategy and VolumeMigration features are consider stable and general available.
The
VolumeUpdateStrategyandVolumeMigrationfeature gates were introduced in v1.3.0 by #11533.Both feature gates have been present for 2 releases and since then, multiple issues have been reported and fixed.
With the graduation of the two feature gates, we want to indicate to KubeVirt users that the feature will remain and that the API may be considered stable for use.
Release note