feat(cleanVm): add recovery method for duplicated vhd uuid containing… #6314
Conversation
fbeauchamp
force-pushed
the
check_vhd_uniq_id
branch
from
6512d23 to
4a1c481
Compare
fbeauchamp
force-pushed
the
check_vhd_uniq_id
branch
from
4a1c481 to
64a6b62
Compare
packages/vhd-lib/Vhd/VhdAbstract.js
Outdated
| * check if all the data of a child are already contained in this vhd | ||
| */ | ||
|
|
||
| async contains(child) { |
There was a problem hiding this comment.
More explicit name, which make it more obvious that this is not a light operation:
| async contains(child) { | |
| async containsAllBlockOf(child) { |
| await this.readBlockAllocationTable() | ||
| await child.readBlockAllocationTable() |
There was a problem hiding this comment.
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.
| async contains(child) { | ||
| await this.readBlockAllocationTable() | ||
| await child.readBlockAllocationTable() | ||
| for await (const block of child.blocks()) { |
There was a problem hiding this comment.
Don't read blocks before checking the parent contains them.
Maybe we should add another iterator (blockIds())?
@xen-orchestra/backups/_cleanVm.js
Outdated
| const duplicate = vhdIds.get(vhd.footer.uuid) | ||
| if (duplicate) { | ||
| logWarn('uuid is duplicated', { uuid: vhd.footer.uuid }) | ||
| // uuid is already present |
There was a problem hiding this comment.
Is this comment really necessary?
It's obvious by the if (duplicate) or am I missing something?
@xen-orchestra/backups/_cleanVm.js
Outdated
| @@ -208,6 +209,19 @@ exports.cleanVm = async function cleanVm( | |||
| } | |||
| vhdChildren[parent] = path | |||
| } | |||
| const duplicate = vhdIds.get(vhd.footer.uuid) | |||
| if (duplicate) { | |||
There was a problem hiding this comment.
As per current XO coding style: never rely on implicit boolean conversation, ie explicit comparison.
@xen-orchestra/backups/_cleanVm.js
Outdated
| @@ -189,6 +189,7 @@ exports.cleanVm = async function cleanVm( | |||
| const vhdsToJSons = new Set() | |||
| const vhdParents = { __proto__: null } | |||
| const vhdChildren = { __proto__: null } | |||
| const vhdIds = new Map() | |||
There was a problem hiding this comment.
| const vhdIds = new Map() | |
| const vhdById = new Map() |
There was a problem hiding this comment.
Otherwise I think it's a set when reading the code 🙂
| 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) |
There was a problem hiding this comment.
Do we want to release this in prod?
| - xo-web minor | ||
| - vhd-lib minor |
|
supeseed by #6317 |
… the same data
Check list
Fixes #007orSee xoa-support#42)CHANGELOG.unreleased.md:${name} v${new version})cron/parse.spec.js)xo-serverAPI changes, the corresponding test has been added to/updated onxo-server-testProcess
WiP:(Work in Progress) if not ready to be mergedFrom the Four Agreements: