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

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
249bb24
feat(xo-web/disks): allow user to delete snapshots before migrating VDI
MelissaFrncJrg Aug 7, 2024
a349bc6
changelog entry
MelissaFrncJrg Aug 7, 2024
e14e30d
Merge branch 'master' into removeSnapshotBeforeMigratingVdi
MelissaFrncJrg Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
> Users must be able to say: “Nice enhancement, I'm eager to test it”

- [REST API] Add backup repository information in the `/rest/v0/dashboard` endpoint (PR [#7882](https://github.com/vatesfr/xen-orchestra/pull/7882))
- [VM/Disks] Allow user to delete snapshots before migrating VDI [#7275](https://github.com/vatesfr/xen-orchestra/issues/7275) (PR [#7903](https://github.com/vatesfr/xen-orchestra/pull/7903))

### Bug fixes

Expand All @@ -36,8 +37,9 @@

<!--packages-start-->


- @xen-orchestra/proxy patch
- xo-server minor
- xo-web patch
- xo-web minor

<!--packages-end-->
5 changes: 3 additions & 2 deletions packages/xo-server/src/api/vdi.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

removeSnapshotsBeforeMigrating → destroySnapshots.

const xapi = this.getXapi(vdi)

if (this.apiContext.permission !== 'admin') {
Expand All @@ -118,13 +118,14 @@ export async function migrate({ vdi, sr, resourceSet }) {
}
}

await xapi.moveVdi(vdi._xapiRef, sr._xapiRef)
await xapi.moveVdi(vdi._xapiRef, sr._xapiRef, removeSnapshotsBeforeMigrating)

return true
}

migrate.params = {
id: { type: 'string' },
removeSnapshotsBeforeMigrating: { type: 'boolean', default: false },
resourceSet: { type: 'string', optional: true },
sr_id: { type: 'string' },
}
Expand Down
12 changes: 11 additions & 1 deletion packages/xo-server/src/xapi/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,14 +1035,24 @@ export default class Xapi extends XapiBase {
return this.callAsync('VDI.clone', vdi.$ref)
}

async moveVdi(vdiId, srId) {
async moveVdi(vdiId, srId, removeSnapshotsBeforeMigrating) {
const vdi = this.getObject(vdiId)
const sr = this.getObject(srId)

if (vdi.SR === sr.$ref) {
return vdi
}

if (removeSnapshotsBeforeMigrating) {
for (const vdiRef of vdi.snapshots) {
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"

}
}
}

log.debug(`Moving VDI ${vdi.name_label} from ${vdi.$SR.name_label} to ${sr.name_label}`)
try {
return this.barrier(
Expand Down
1 change: 1 addition & 0 deletions packages/xo-web/src/common/intl/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,6 +1328,7 @@ const messages = {
vdiMigrateSelectSr: 'Destination SR:',
vdiMigrateNoSr: 'No SR',
vdiMigrateNoSrMessage: 'A target SR is required to migrate a VDI',
vdiMigrateWithoutSnapshots: 'Delete snapshots before migrate?',
vdiForget: 'Forget',
noControlDomainVdis: 'No VDIs attached to control domain',
vbdBootableStatus: 'Boot flag',
Expand Down
3 changes: 2 additions & 1 deletion packages/xo-web/src/common/xo/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2480,9 +2480,10 @@ export const deleteOrphanedVdis = vdis =>
),
}).then(() => Promise.all(map(resolveIds(vdis), id => _call('vdi.delete', { id }))), noop)

export const migrateVdi = (vdi, sr, resourceSet) =>
export const migrateVdi = (vdi, sr, resourceSet, removeSnapshotsBeforeMigrating) =>
_call('vdi.migrate', {
id: resolveId(vdi),
removeSnapshotsBeforeMigrating,
resourceSet,
sr_id: resolveId(sr),
})
Expand Down
18 changes: 17 additions & 1 deletion packages/xo-web/src/common/xo/migrate-vdi-modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import { Container, Col } from 'grid'
import { createCompare, createCompareContainers } from 'utils'
import { createSelector } from 'selectors'
import { SelectResourceSetsSr, SelectSr as SelectAnySr } from 'select-objects'
import { Toggle } from 'form'

import { isSrIso, isSrShared, isSrWritable } from '../'

const compareSrs = createCompare([isSrShared])

export default class MigrateVdiModalBody extends Component {
static propTypes = {
nSnapshots: PropTypes.number,
pool: PropTypes.string.isRequired,
resourceSet: PropTypes.object,
warningBeforeMigrate: PropTypes.func.isRequired,
Expand All @@ -23,6 +25,10 @@ export default class MigrateVdiModalBody extends Component {
return this.state
}

state = {
removeSnapshotsBeforeMigrating: false,
}

_getCompareContainers = createSelector(() => this.props.pool, createCompareContainers)

_getWarningBeforeMigrate = createSelector(
Expand All @@ -38,7 +44,7 @@ export default class MigrateVdiModalBody extends Component {
)

render() {
const { resourceSet } = this.props
const { nSnapshots, resourceSet } = this.props
const warningBeforeMigrate = this._getWarningBeforeMigrate()
const SelectSr = resourceSet !== undefined ? SelectResourceSetsSr : SelectAnySr
return (
Expand All @@ -57,6 +63,16 @@ export default class MigrateVdiModalBody extends Component {
/>
</Col>
</SingleLineRow>
<SingleLineRow>
<Col size={6}>{_('vdiMigrateWithoutSnapshots')}</Col>
<Col size={6}>
<Toggle
disabled={nSnapshots === 0}
value={this.state.removeSnapshotsBeforeMigrating}
onChange={this.toggleState('removeSnapshotsBeforeMigrating')}
/>
</Col>
</SingleLineRow>
{warningBeforeMigrate !== null && (
<SingleLineRow>
<Col>{warningBeforeMigrate}</Col>
Expand Down
14 changes: 12 additions & 2 deletions packages/xo-web/src/xo-app/vm/tab-disks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,17 +594,26 @@ export default class TabDisks extends Component {
})

_migrateVdis = vdis => {
const vdiSnapshots = []
Object.keys(vdis).forEach(vdi => {
const nSnapshots = vdis[vdi].snapshots
if (nSnapshots) {
vdiSnapshots.push(...nSnapshots)
}
})

const { resolvedResourceSet, vm } = this.props
return confirm({
title: _('vdiMigrate'),
body: (
<MigrateVdiModalBody
nSnapshots={vdiSnapshots.length}
pool={vm.$pool}
resourceSet={resolvedResourceSet}
warningBeforeMigrate={this._getGenerateWarningBeforeMigrate()}
/>
),
}).then(({ sr }) => {
}).then(({ sr, removeSnapshotsBeforeMigrating }) => {
if (sr === undefined) {
return error(_('vdiMigrateNoSr'), _('vdiMigrateNoSrMessage'))
}
Expand All @@ -614,7 +623,8 @@ export default class TabDisks extends Component {
migrateVdi(
vdi,
sr,
getDefined(() => resolvedResourceSet.id)
getDefined(() => resolvedResourceSet.id),
removeSnapshotsBeforeMigrating
)
)
)
Expand Down
Loading