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(cleanVm): add recovery method for duplicated vhd uuid containing… #6314

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions @xen-orchestra/backups/_cleanVm.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ exports.cleanVm = async function cleanVm(
const vhdsToJSons = new Set()
const vhdParents = { __proto__: null }
const vhdChildren = { __proto__: null }
const vhdById = new Map()

const { vhds, interruptedVhds, aliases } = await listVhds(handler, vmDir)

Expand All @@ -208,6 +209,18 @@ exports.cleanVm = async function cleanVm(
}
vhdChildren[parent] = path
}
const duplicate = vhdById.get(vhd.footer.uuid)
if (duplicate !== undefined) {
logWarn('uuid is duplicated', { uuid: vhd.footer.uuid })
if (duplicate.containsAllDataOf(vhd)) {
logWarn(`should delete ${path}`)
} else if (vhd.containsAllDataOf(duplicate)) {
logWarn(`should delete ${duplicate._path}`)
} else {
logWarn(`same ids but different content`)
}
}
vhdById.set(vhd.footer.uuid, vhd)
Comment on lines +212 to +223
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to release this in prod?

})
} catch (error) {
vhds.delete(path)
Expand All @@ -218,6 +231,9 @@ exports.cleanVm = async function cleanVm(
}
}
})
// the vhd are closed at the end of the disposable
// it's unsafe to use them later
vhdById.clear()

// remove interrupted merge states for missing VHDs
for (const interruptedVhd of interruptedVhds.keys()) {
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

<!--packages-start-->

- @xen-orchestra/backups minor
- xo-web minor
- vhd-lib minor
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetic order please 🙂


<!--packages-end-->
23 changes: 23 additions & 0 deletions packages/vhd-lib/Vhd/VhdAbstract.integ.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,3 +275,26 @@ it('can stream content', async () => {
}
})
})

it('can check vhd contained in on another', async () => {
const rawFile = `${tempDir}/contained`
await createRandomFile(rawFile, 4)
const containedVhdFileName = `${tempDir}/contained.vhd`
await convertFromRawToVhd(rawFile, containedVhdFileName)

const after = `${tempDir}/after`
await createRandomFile(after, 4)

fs.appendFile(rawFile, await fs.readFile(after))

const cnotainerVhdFileName = `${tempDir}/container.vhd`
await convertFromRawToVhd(rawFile, cnotainerVhdFileName)

await Disposable.use(async function* () {
const handler = yield getSyncedHandler({ url: 'file://' + tempDir })
const contained = yield openVhd(handler, 'contained.vhd')
const container = yield openVhd(handler, 'container.vhd')
expect(await contained.contains(container)).toEqual(false)
expect(await container.contains(contained)).toEqual(true)
})
})
21 changes: 21 additions & 0 deletions packages/vhd-lib/Vhd/VhdAbstract.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,4 +334,25 @@ exports.VhdAbstract = class VhdAbstract {
stream.length = footer.currentSize
return stream
}

/*
* check if all the data of a child are already contained in this vhd
*/

async containsAllDataOf(child) {
julien-f marked this conversation as resolved.
Show resolved Hide resolved
await this.readBlockAllocationTable()
await child.readBlockAllocationTable()
Comment on lines +343 to +344
Copy link
Member

Choose a reason for hiding this comment

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

I believe this method (and some others), are pitfalls, and should be implcitely called when necessary (e.g., when calling blocks).

If you agree, I'll do the change.

for await (const block of child.blocks()) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't read blocks before checking the parent contains them.

Maybe we should add another iterator (blockIds())?

const { id, data: childData } = block
// block is in child not in parent
if (!this.containsBlock(id)) {
return false
}
const { data: parentData } = await this.readBlock(id)
if (!childData.equals(parentData)) {
return false
}
}
return true
}
}