Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update volume migration doc #840

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update volume migration doc #840

wants to merge 2 commits into from

Conversation

alicefr
Copy link
Member

@alicefr alicefr commented Oct 9, 2024

What this PR does / why we need it:
Update the volume migration with the information about the volume migration cancellation and recovery. Additionally, it also fix the title of the document

Release note:

NONE

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Oct 9, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alicefr. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@alicefr
Copy link
Member Author

alicefr commented Oct 9, 2024

/hold
This can be merged after this PR kubevirt/kubevirt#13030 is merged

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2024
awels
awels reviewed Oct 10, 2024
View reviewed changes
docs/storage/volume_migration.md Outdated Show resolved Hide resolved
docs/storage/volume_migration.md Outdated Show resolved Hide resolved
@mhenriks
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2024
aglitke
aglitke reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@aglitke aglitke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this information! I just have a couple of grammar nits but otherwise it looks good.

docs/storage/volume_migration.md Outdated
The users could, for various reasons, wish to stop and cancel the ongoing volume migration.
Migration cancellations are also handled declaratively, thus users must restore the previous set of volumes, which will be read as a cancellation for the volume update and migration.

A possible way to cancel the migration consists apply the old version of the VM declaration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible way to cancel the migration is to apply the old version of the VM declaration.

docs/storage/volume_migration.md Outdated

If for any reasons the VMI disappears, then the volume migration isn't retried anymore. This might happen if the users inadvertently shutdown the VM or the VMI is accidentally deleted.

However, in these situations, the VM spec is in an inconsistent state because the volume set contains the destination volumes but the copy wasn't successful, and users could fail to boot correctly the VM. For this reason the VM is marked with the condition `ManualRecoveryRequired` and KubeVirt will refuses to start a VM which has this condition.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will refuse

@duyanyan
Copy link
Contributor

/lgtm

@kubevirt-bot
Copy link
Contributor

@duyanyan: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@kubevirt-bot
Copy link
Contributor

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

@kubevirt-bot kubevirt-bot added the needs-approver-review Indicates that a PR requires a review from an approver. label Oct 21, 2024
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
@alicefr
Copy link
Member Author

alicefr commented Oct 22, 2024

/unhold
Last PR has been merged

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2024
@alicefr
Copy link
Member Author

alicefr commented Oct 22, 2024

@awels @duyanyan do you mind re-lgtm this?

@duyanyan
Copy link
Contributor

/lgtm

@kubevirt-bot
Copy link
Contributor

@duyanyan: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@awels
Copy link
Member

awels commented Oct 22, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
aburdenthehand
aburdenthehand reviewed Nov 8, 2024
View reviewed changes
Copy link
Member

@aburdenthehand aburdenthehand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff. I have a couple of little changes.
Two additional things:

  • major: it looks as though Document update volume strategy and volume migration #805 did not add an volume_migration.md to the user-guide/docs/storage/.pages file, so it is not visible in the TOC. We may as well add it in this PR.
  • minor: You can change the 'the user' to 'you can' etc to make the sentences active. We have both in the docs so it's not a big deal but active is better and more commonly used throughout the user guide.
    (An exception would be if the doc is targeted to an admin, who is performing tasks for their users.)

docs/storage/volume_migration.md

In order to recover the VM spec, the users need to revert the volume set in the VM spec as it is the case for the volume migration cancellation.

The volume migration information is stored in the VM status as well, and the users can see the full list of the migrated volumes which contain the source and destination names as well as the corresponding volume name .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The volume migration information is stored in the VM status as well, and the users can see the full list of the migrated volumes which contain the source and destination names as well as the corresponding volume name .
The volume migration information is stored in the VM status as well, and the users can see the full list of the migrated volumes which contain the source and destination names as well as the corresponding volume name.

docs/storage/volume_migration.md

### Manual recovery required

If for any reasons the VMI disappears, then the volume migration isn't retried anymore. This might happen if the users inadvertently shutdown the VM or the VMI is accidentally deleted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If for any reasons the VMI disappears, then the volume migration isn't retried anymore. This might happen if the users inadvertently shutdown the VM or the VMI is accidentally deleted.
If for any reasons the VMI disappears, then the volume migration is not retried anymore. This might happen if the users inadvertently shutdown the VM or the VMI is accidentally deleted.

As a general rule we should avoid contractions in documentation.

docs/storage/volume_migration.md

If for any reasons the VMI disappears, then the volume migration isn't retried anymore. This might happen if the users inadvertently shutdown the VM or the VMI is accidentally deleted.

However, in these situations, the VM spec is in an inconsistent state because the volume set contains the destination volumes but the copy wasn't successful, and users could fail to boot correctly the VM. For this reason the VM is marked with the condition `ManualRecoveryRequired` and KubeVirt will refuse to start a VM which has this condition.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
However, in these situations, the VM spec is in an inconsistent state because the volume set contains the destination volumes but the copy wasn't successful, and users could fail to boot correctly the VM. For this reason the VM is marked with the condition `ManualRecoveryRequired` and KubeVirt will refuse to start a VM which has this condition.
However, in these situations, the VM spec is in an inconsistent state because the volume set contains the destination volumes but the copy was not successful, and users could fail to boot correctly the VM. For this reason the VM is marked with the condition `ManualRecoveryRequired` and KubeVirt will refuse to start a VM which has this condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aburdenthehand aburdenthehand aburdenthehand left review comments

@awels awels awels left review comments

@aglitke aglitke aglitke left review comments

@akalenyu akalenyu Awaiting requested review from akalenyu

Assignees

@awels awels

@mhenriks mhenriks

Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. needs-approver-review Indicates that a PR requires a review from an approver. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alicefr @kubevirt-bot @mhenriks @duyanyan @awels @aglitke @aburdenthehand