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.

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

feat(xo-web/disks): allow user to delete snapshots before migrating VDI #7903

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MelissaFrncJrg
Copy link
Contributor

Description

Fixes #7275

image

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

@MelissaFrncJrg MelissaFrncJrg marked this pull request as ready for review August 7, 2024 14:02
@@ -107,7 +107,7 @@ set.resolve = {

// -------------------------------------------------------------------

export async function migrate({ vdi, sr, resourceSet }) {
export async function migrate({ vdi, sr, removeSnapshotsBeforeMigrating, resourceSet }) {
Copy link
Member

Choose a reason for hiding this comment

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

removeSnapshotsBeforeMigratingdestroySnapshots.

const vdi = this.getObject(vdiRef)
for (const vbdRef of vdi.VBDs) {
const vbd = this.getObject(vbdRef)
await this.VM_destroy(vbd.VM)
Copy link
Member

Choose a reason for hiding this comment

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

Do not destroy VMs when migrating a VDI!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I should have commented the code because indeed it's not very clear like this.
So when testing with @MathieuRA we realised that vdi.snapshots returns the VDI snapshots instead of the actual VM snapshots so we have to go through their VBDs to actually destroy the snapshots

Copy link
Member

Choose a reason for hiding this comment

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

The issue is about migrating VDI, not VM, and therefore destroying all its snapshot.

We are not talking about destroying VM snapshots here, that's a possibility but that's not the original request.

Copy link
Member

Choose a reason for hiding this comment

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

Either:

  1. we destroy only VDI snapshots (detaching it from any existing VMs)
  2. we destroy the VDI snapshots and all VMs which contains them

In either case, this should be obvious in the API and the UI.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any sense to keep some VM snapshots alone without their VDIs. Since we decided to migrate the VM original VDI, we have a binary choice:

  1. leaving XAPI moving the entire chain to the new SR
  2. removing all the VM snapshots (with their VDIs) before migrating to avoid a far longer migration process

Most of the time, there's one disk per VM, but maybe 2 or 3 VM.snapshot. Since delta won't work as soon as we move the VM VDI to a new SR, most of the time, removing all VM.snapshots make sense (and when it's not, the user can make a choice)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a second toggle to allow the user to also destroy the VDIs snapshots?
I pinged @AtaxyaNetwork on mattermost about this and i'm waiting for some clarifications on what she meant by "remove snapshots"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask user if they want to migrate snap or not when VDI migrate
3 participants