fix(backups,vhd-lib): merge with VhdSynthetic (#6317)

@xen-orchestra/backups/_cleanVm.js

@@ -2,6 +2,7 @@
 
 const assert = require('assert')
 const sum = require('lodash/sum')
+const UUID = require('uuid')
 const { asyncMap } = require('@xen-orchestra/async-map')
 const { Constants, mergeVhd, openVhd, VhdAbstract, VhdFile } = require('vhd-lib')
 const { isVhdAlias, resolveVhdAlias } = require('vhd-lib/aliases')
@@ -50,7 +51,7 @@ const computeVhdsSize = (handler, vhdPaths) =>
 async function mergeVhdChain(chain, { handler, logInfo, remove, merge }) {
   assert(chain.length >= 2)
   const chainCopy = [...chain]
-  const parent = chainCopy.pop()
+  const parent = chainCopy.shift()
   const children = chainCopy
 
   if (merge) {
@@ -187,6 +188,7 @@ exports.cleanVm = async function cleanVm(
   const handler = this._handler
 
   const vhdsToJSons = new Set()
+  const vhdById = new Map()
   const vhdParents = { __proto__: null }
   const vhdChildren = { __proto__: null }
 
@@ -208,6 +210,27 @@ exports.cleanVm = async function cleanVm(
           }
           vhdChildren[parent] = path
         }
+        // Detect VHDs with the same UUIDs
+        //
+        // Due to a bug introduced in a1bcd35e2
+        const duplicate = vhdById.get(UUID.stringify(vhd.footer.uuid))
+        let vhdKept = vhd
+        if (duplicate !== undefined) {
+          logWarn('uuid is duplicated', { uuid: UUID.stringify(vhd.footer.uuid) })
+          if (duplicate.containsAllDataOf(vhd)) {
+            logWarn(`should delete ${path}`)
+            vhdKept = duplicate
+            vhds.delete(path)
+          } else if (vhd.containsAllDataOf(duplicate)) {
+            logWarn(`should delete ${duplicate._path}`)
+            vhds.delete(duplicate._path)
+          } else {
+            logWarn(`same ids but different content`)
+          }
+        } else {
+          logInfo('not duplicate', UUID.stringify(vhd.footer.uuid), path)
+        }
+        vhdById.set(UUID.stringify(vhdKept.footer.uuid), vhdKept)
       })
     } catch (error) {
       vhds.delete(path)
@@ -362,7 +385,7 @@ exports.cleanVm = async function cleanVm(
   const unusedVhdsDeletion = []
   const toMerge = []
   {
-    // VHD chains (as list from child to ancestor) to merge indexed by last
+    // VHD chains (as list from oldest to most recent) to merge indexed by most recent
     // ancestor
     const vhdChainsToMerge = { __proto__: null }
 
@@ -386,7 +409,7 @@ exports.cleanVm = async function cleanVm(
       if (child !== undefined) {
         const chain = getUsedChildChainOrDelete(child)
         if (chain !== undefined) {
-          chain.push(vhd)
+          chain.unshift(vhd)
           return chain
         }
       }

CHANGELOG.unreleased.md

@@ -14,6 +14,7 @@
 > Users must be able to say: “I had this issue, happy to know it's fixed”
 
 - [Tasks] Fix tasks not displayed when running CR backup job [Forum#6038](https://xcp-ng.org/forum/topic/6038/not-seeing-tasks-any-more-as-admin) (PR [#6315](https://github.com/vatesfr/xen-orchestra/pull/6315))
+- [Backup] Fix failing merge multiple VHDs at once (PR [#6317](https://github.com/vatesfr/xen-orchestra/pull/6317))
 
 ### Packages to release
 
@@ -32,6 +33,8 @@
 <!--packages-start-->
 
 - @vates/async-each major
+- @xen-orchestra/backups minor
+- vhd-lib patch
 - xo-web minor
 
 <!--packages-end-->

packages/vhd-lib/Vhd/VhdAbstract.js

@@ -343,4 +343,21 @@ exports.VhdAbstract = class VhdAbstract {
     stream.length = footer.currentSize
     return stream
   }
+
+  async containsAllDataOf(child) {
+    await this.readBlockAllocationTable()
+    await child.readBlockAllocationTable()
+    for await (const block of child.blocks()) {
+      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
+  }
 }

packages/vhd-lib/Vhd/VhdSynthetic.integ.spec.js

@@ -55,7 +55,7 @@ test('It can read block and parent locator from a synthetic vhd', async () => {
 
     await bigVhd.readHeaderAndFooter()
 
-    const syntheticVhd = yield VhdSynthetic.open(handler, [smallVhdFileName, bigVhdFileName])
+    const syntheticVhd = yield VhdSynthetic.open(handler, [bigVhdFileName, smallVhdFileName])
     await syntheticVhd.readBlockAllocationTable()
 
     expect(syntheticVhd.header.diskType).toEqual(bigVhd.header.diskType)

packages/vhd-lib/Vhd/VhdSynthetic.js

@@ -15,14 +15,13 @@ const VhdSynthetic = class VhdSynthetic extends VhdAbstract {
   #vhds = []
 
   get header() {
-    // this the VHD we want to synthetize
-    const vhd = this.#vhds[0]
+    // this the most recent vhd
+    const vhd = this.#vhds[this.#vhds.length - 1]
 
     // this is the root VHD
-    const rootVhd = this.#vhds[this.#vhds.length - 1]
+    const rootVhd = this.#vhds[0]
 
     // data of our synthetic VHD
-    // TODO: set parentLocatorEntry-s in header
     return {
       ...vhd.header,
       parentLocatorEntry: cloneDeep(rootVhd.header.parentLocatorEntry),
@@ -34,10 +33,13 @@ const VhdSynthetic = class VhdSynthetic extends VhdAbstract {
   }
 
   get footer() {
-    // this is the root VHD
-    const rootVhd = this.#vhds[this.#vhds.length - 1]
+    // this the most recent vhd
+    const vhd = this.#vhds[this.#vhds.length - 1]
+
+    // this is the oldest VHD
+    const rootVhd = this.#vhds[0]
     return {
-      ...this.#vhds[0].footer,
+      ...vhd.footer,
       dataOffset: FOOTER_SIZE,
       diskType: rootVhd.footer.diskType,
     }
@@ -77,17 +79,21 @@ const VhdSynthetic = class VhdSynthetic extends VhdAbstract {
     await asyncMap(vhds, vhd => vhd.readHeaderAndFooter())
 
     for (let i = 0, n = vhds.length - 1; i < n; ++i) {
-      const child = vhds[i]
-      const parent = vhds[i + 1]
+      const parent = vhds[i]
+      const child = vhds[i + 1]
       assert.strictEqual(child.footer.diskType, DISK_TYPES.DIFFERENCING)
       assert.strictEqual(UUID.stringify(child.header.parentUuid), UUID.stringify(parent.footer.uuid))
     }
   }
 
   #getVhdWithBlock(blockId) {
-    const index = this.#vhds.findIndex(vhd => vhd.containsBlock(blockId))
-    assert(index !== -1, `no such block ${blockId}`)
-    return this.#vhds[index]
+    for (let i = this.#vhds.length - 1; i >= 0; i--) {
+      const vhd = this.#vhds[i]
+      if (vhd.containsBlock(blockId)) {
+        return vhd
+      }
+    }
+    assert(false, `no such block ${blockId}`)
   }
 
   async readBlock(blockId, onlyBitmap = false) {
@@ -120,7 +126,7 @@ VhdSynthetic.fromVhdChain = Disposable.factory(async function* fromVhdChain(hand
   const vhds = []
   do {
     vhd = yield openVhd(handler, vhdPath)
-    vhds.push(vhd)
+    vhds.unshift(vhd) // from oldest to most recent
     vhdPath = resolveRelativeFromFile(vhdPath, vhd.header.parentUnicodeName)
   } while (vhd.footer.diskType !== DISK_TYPES.DYNAMIC)
 

packages/vhd-lib/merge.integ.spec.js

@@ -217,8 +217,7 @@ test('it cleans vhd mergedfiles', async () => {
   await handler.writeFile('child2', 'child2Data')
   await handler.writeFile('child3', 'child3Data')
 
-  // childPath  is from the grand children to the children
-  await cleanupVhds(handler, 'parent', ['child3', 'child2', 'child1'], { remove: true })
+  await cleanupVhds(handler, 'parent', ['child1', 'child2', 'child3'], { remove: true })
 
   // only child3 should stay, with the data of parent
   const [child3, ...other] = await handler.list('.')

packages/vhd-lib/merge.js

@@ -59,7 +59,7 @@ function cleanupVhds(handler, parent, children, { logInfo = noop, remove = false
   if (!Array.isArray(children)) {
     children = [children]
   }
-  const mergeTargetChild = children.shift()
+  const mergeTargetChild = children.pop()
 
   return Promise.all([
     VhdAbstract.rename(handler, parent, mergeTargetChild),