From 05bdd0842cf002f1fd4ebdd1e5be8c8bed0826fb Mon Sep 17 00:00:00 2001 From: Florent Beauchamp Date: Tue, 7 Dec 2021 13:24:53 +0100 Subject: [PATCH 1/5] fix(vhd-lib): VhdDirectory#readHeaderAndFooter should throw a assertion error if header or footer are missing --- packages/vhd-lib/Vhd/VhdDirectory.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/vhd-lib/Vhd/VhdDirectory.js b/packages/vhd-lib/Vhd/VhdDirectory.js index 76fbaf60cbe..786bd26b94d 100644 --- a/packages/vhd-lib/Vhd/VhdDirectory.js +++ b/packages/vhd-lib/Vhd/VhdDirectory.js @@ -173,10 +173,17 @@ exports.VhdDirectory = class VhdDirectory extends VhdAbstract { } async readHeaderAndFooter() { - // we need to know if thre is compression before reading headers - await this.#readChunkFilters() - const { buffer: bufHeader } = await this._readChunk('header') - const { buffer: bufFooter } = await this._readChunk('footer') + let bufHeader, bufFooter + try { + bufHeader = (await this._readChunk('header')).buffer + bufFooter = (await this._readChunk('footer')).buffer + } catch (error) { + if (error.code === 'ENOENT') { + assert(false, 'Header And Footer should exists') + } else { + throw error + } + } const footer = unpackFooter(bufFooter) const header = unpackHeader(bufHeader, footer) From f120070599c8b315d349286d7f6d4b4d2b9fd9d3 Mon Sep 17 00:00:00 2001 From: Florent Beauchamp Date: Tue, 7 Dec 2021 13:28:22 +0100 Subject: [PATCH 2/5] chore(backup): cleanVm tests cleanup + pout vhd data in a ./data subfolder --- @xen-orchestra/backups/_cleanVm.integ.spec.js | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/@xen-orchestra/backups/_cleanVm.integ.spec.js b/@xen-orchestra/backups/_cleanVm.integ.spec.js index c821e892c9d..5ed5d2b8f15 100644 --- a/@xen-orchestra/backups/_cleanVm.integ.spec.js +++ b/@xen-orchestra/backups/_cleanVm.integ.spec.js @@ -9,6 +9,7 @@ const crypto = require('crypto') const { RemoteAdapter } = require('./RemoteAdapter') const { VHDFOOTER, VHDHEADER } = require('./tests.fixtures.js') const { VhdFile, Constants, VhdDirectory, VhdAbstract } = require('vhd-lib') +const { dirname, basename } = require('path') let tempDir, adapter, handler, jobId, vdiId, basePath @@ -35,7 +36,11 @@ const uniqueId = () => crypto.randomBytes(16).toString('hex') async function generateVhd(path, opts = {}) { let vhd - const dataPath = opts.useAlias ? path + '.data' : path + let dataPath = path + if (opts.useAlias) { + await handler.mkdir(dirname(path) + '/data/') + dataPath = dirname(path) + '/data/' + basename(path) + } if (opts.mode === 'directory') { await handler.mkdir(dataPath) vhd = new VhdDirectory(handler, dataPath) @@ -162,7 +167,7 @@ test('it remove backup meta data referencing a missing vhd in delta backup', asy `${basePath}/deleted.vhd`, // in metadata but not in vhds `${basePath}/orphan.vhd`, `${basePath}/child.vhd`, - // abandonned.json is not here + // abandonned.vhd is not here anymore ], }), { flags: 'w' } @@ -235,12 +240,8 @@ test('it finish unterminated merge ', async () => { `metadata.json`, JSON.stringify({ mode: 'delta', - size: undefined, - vhds: [ - `${basePath}/orphan.vhd`, // grand child should not be merged - `${basePath}/child.vhd`, - // orphan is not here, he should be merged in child - ], + size: 209920, + vhds: [`${basePath}/orphan.vhd`, `${basePath}/child.vhd`], }) ) @@ -266,7 +267,6 @@ test('it finish unterminated merge ', async () => { }) ) - // a unfinished merging await adapter.cleanVm('/', { remove: true, merge: true }) // merging is already tested in vhd-lib, don't retest it here (and theses vhd are as empty as my stomach at 12h12) @@ -279,12 +279,17 @@ test('it finish unterminated merge ', async () => { // each of the vhd can be a file, a directory, an alias to a file or an alias to a directory // the message an resulting files should be identical to the output with vhd files which is tested independantly -describe('tests mulitple combination ', () => { +describe('tests multiple combination ', () => { for (const useAlias of [true, false]) { for (const vhdMode of ['file', 'directory']) { test(`alias : ${useAlias}, mode: ${vhdMode}`, async () => { // a broken VHD - const brokenVhdDataPath = basePath + useAlias ? 'broken.data' : 'broken.vhd' + if (useAlias) { + await handler.mkdir(basePath + '/data') + } + + const brokenVhdDataPath = basePath + (useAlias ? '/data/broken.vhd' : '/broken.vhd') + if (vhdMode === 'directory') { await handler.mkdir(brokenVhdDataPath) } else { @@ -305,6 +310,7 @@ describe('tests mulitple combination ', () => { parentUid: crypto.randomBytes(16), }, }) + // an ancestor of a vhd present in metadata const ancestor = await generateVhd(`${basePath}/ancestor.vhd`, { useAlias, @@ -367,6 +373,7 @@ describe('tests mulitple combination ', () => { ], }) ) + await adapter.cleanVm('/', { remove: true, merge: true }) const metadata = JSON.parse(await handler.readFile(`metadata.json`)) @@ -379,14 +386,16 @@ describe('tests mulitple combination ', () => { const survivors = await handler.list(basePath) // console.log(survivors) if (useAlias) { + const dataSurvivors = await handler.list(basePath + '/data') // the goal of the alias : do not move a full folder - expect(survivors).toContain('ancestor.vhd.data') - expect(survivors).toContain('grandchild.vhd.data') - expect(survivors).toContain('cleanAncestor.vhd.data') + expect(dataSurvivors).toContain('ancestor.vhd') + expect(dataSurvivors).toContain('grandchild.vhd') + expect(dataSurvivors).toContain('cleanAncestor.vhd') expect(survivors).toContain('clean.vhd.alias.vhd') expect(survivors).toContain('child.vhd.alias.vhd') expect(survivors).toContain('grandchild.vhd.alias.vhd') - expect(survivors.length).toEqual(6) + expect(survivors.length).toEqual(4) // the 3 ok + data + expect(dataSurvivors.length).toEqual(3) // the 3 ok + data } else { expect(survivors).toContain('clean.vhd') expect(survivors).toContain('child.vhd') From 340512e493860852ed350cbd4b429be4724f0973 Mon Sep 17 00:00:00 2001 From: Florent Beauchamp Date: Tue, 7 Dec 2021 13:30:49 +0100 Subject: [PATCH 3/5] feat(backup): add sanity check of aliases in cleanVm --- @xen-orchestra/backups/_cleanVm.integ.spec.js | 21 +++++++ @xen-orchestra/backups/_cleanVm.js | 62 +++++++++++++++++-- CHANGELOG.unreleased.md | 1 + packages/vhd-lib/Vhd/VhdDirectory.js | 3 + 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/@xen-orchestra/backups/_cleanVm.integ.spec.js b/@xen-orchestra/backups/_cleanVm.integ.spec.js index 5ed5d2b8f15..0d3a1b1989a 100644 --- a/@xen-orchestra/backups/_cleanVm.integ.spec.js +++ b/@xen-orchestra/backups/_cleanVm.integ.spec.js @@ -9,6 +9,7 @@ const crypto = require('crypto') const { RemoteAdapter } = require('./RemoteAdapter') const { VHDFOOTER, VHDHEADER } = require('./tests.fixtures.js') const { VhdFile, Constants, VhdDirectory, VhdAbstract } = require('vhd-lib') +const { checkAliases } = require('./_cleanVm') const { dirname, basename } = require('path') let tempDir, adapter, handler, jobId, vdiId, basePath @@ -406,3 +407,23 @@ describe('tests multiple combination ', () => { } } }) + +test('check Aliases should work alone', async () => { + await handler.mkdir('vhds') + await handler.mkdir('vhds/data') + await generateVhd(`vhds/data/ok.vhd`) + await VhdAbstract.createAlias(handler, 'vhds/ok.alias.vhd', 'vhds/data/ok.vhd') + + await VhdAbstract.createAlias(handler, 'vhds/missingData.alias.vhd', 'vhds/data/nonexistent.vhd') + + await generateVhd(`vhds/data/missingalias.vhd`) + + await checkAliases(['vhds/missingData.alias.vhd', 'vhds/ok.alias.vhd'], 'vhds/data', { remove: true, handler }) + + // only ok have suvived + const alias = (await handler.list('vhds')).filter(f => f.endsWith('.vhd')) + expect(alias.length).toEqual(1) + + const data = await handler.list('vhds/data') + expect(data.length).toEqual(1) +}) diff --git a/@xen-orchestra/backups/_cleanVm.js b/@xen-orchestra/backups/_cleanVm.js index bd1c4858ef0..a1c93d62cb4 100644 --- a/@xen-orchestra/backups/_cleanVm.js +++ b/@xen-orchestra/backups/_cleanVm.js @@ -1,7 +1,7 @@ const assert = require('assert') const sum = require('lodash/sum') const { asyncMap } = require('@xen-orchestra/async-map') -const { Constants, mergeVhd, openVhd, VhdAbstract, VhdFile } = require('vhd-lib') +const { Constants, isVhdAlias, mergeVhd, openVhd, VhdAbstract, VhdFile, resolveAlias } = require('vhd-lib') const { dirname, resolve } = require('path') const { DISK_TYPES } = Constants const { isMetadataFile, isVhdFile, isXvaFile, isXvaSumFile } = require('./_backupType.js') @@ -82,7 +82,6 @@ async function mergeVhdChain(chain, { handler, onLog, remove, merge }) { ) clearInterval(handle) - await Promise.all([ VhdAbstract.rename(handler, parent, child), asyncMap(children.slice(0, -1), child => { @@ -103,6 +102,7 @@ const noop = Function.prototype const INTERRUPTED_VHDS_REG = /^\.(.+)\.merge.json$/ const listVhds = async (handler, vmDir) => { const vhds = new Set() + const aliases = {} const interruptedVhds = new Set() await asyncMap( @@ -119,7 +119,7 @@ const listVhds = async (handler, vmDir) => { const list = await handler.list(vdiDir, { filter: file => isVhdFile(file) || INTERRUPTED_VHDS_REG.test(file), }) - + aliases[vdiDir] = list.filter(vhd => isVhdAlias(vhd)) list.forEach(file => { const res = INTERRUPTED_VHDS_REG.exec(file) if (res === null) { @@ -132,9 +132,55 @@ const listVhds = async (handler, vmDir) => { ) ) - return { vhds, interruptedVhds } + return { vhds, interruptedVhds, aliases } } +async function checkAliases(aliasPaths, targetDataRepository, { handler, onLog = noop, remove = false }) { + const aliasFound = [] + for (const path of aliasPaths) { + const target = await resolveAlias(handler, path) + + if (!isVhdFile(target)) { + onLog(`Alias ${path} references a non vhd target: ${target}`) + if (remove) { + await handler.unlink(target) + await handler.unlink(path) + } + continue + } + + try { + const { dispose } = await openVhd(handler, target) + dispose() + } catch (e) { + onLog(`target ${target} of alias ${path} is missing or broken`) + if (remove) { + try { + await VhdAbstract.unlink(handler, path) + } catch(e){} + } + continue + } + + aliasFound.push(resolve('/', target)) + } + + const entries = await handler.list(targetDataRepository, { + ignoreMissing: true, + prependDir: true, + }) + + entries.forEach(async entry => { + if (!aliasFound.includes(entry)) { + onLog(`the Vhd ${entry} is not referenced by a an alias`) + if (remove) { + await VhdAbstract.unlink(handler, entry) + } + } + }) +} +exports.checkAliases = checkAliases + const defaultMergeLimiter = limitConcurrency(1) exports.cleanVm = async function cleanVm( @@ -177,8 +223,12 @@ exports.cleanVm = async function cleanVm( } } }) - - // @todo : add check for data folder of alias not referenced in a valid alias + // check if alias are correct + // check if all vhd in data subfolder have a corresponding alias + await asyncMap(Object.keys(vhdsList.aliases), async dir => { + const aliases = vhdsList.aliases[dir] + await checkAliases(aliases, `${dir}/data`, { handler, onLog, remove }) + }) // remove VHDs with missing ancestors { diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 1f17d67f84b..f8170afd403 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -9,6 +9,7 @@ - Limit number of concurrent VM migrations per pool to `3` [#6065](https://github.com/vatesfr/xen-orchestra/issues/6065) (PR [#6076](https://github.com/vatesfr/xen-orchestra/pull/6076)) Can be changed in `xo-server`'s configuration file: `xapiOptions.vmMigrationConcurrency` +- [Backup] Add sanity check of alias after backup. PR [6043](https://github.com/vatesfr/xen-orchestra/pull/6043) ### Bug fixes diff --git a/packages/vhd-lib/Vhd/VhdDirectory.js b/packages/vhd-lib/Vhd/VhdDirectory.js index 786bd26b94d..29d0ae9a2e0 100644 --- a/packages/vhd-lib/Vhd/VhdDirectory.js +++ b/packages/vhd-lib/Vhd/VhdDirectory.js @@ -178,6 +178,9 @@ exports.VhdDirectory = class VhdDirectory extends VhdAbstract { bufHeader = (await this._readChunk('header')).buffer bufFooter = (await this._readChunk('footer')).buffer } catch (error) { + // to keep as much as possible of the vhdFile way of handlong broken vhd + // let's emit an assertion error (the vhd is broken, you should try to delete it) + // instead of a file not found error (the vhd is missing) if (error.code === 'ENOENT') { assert(false, 'Header And Footer should exists') } else { From 4ce1041596e8f2c135af0f85c39a9eda38ef2dab Mon Sep 17 00:00:00 2001 From: Florent Beauchamp Date: Wed, 12 Jan 2022 09:47:42 +0100 Subject: [PATCH 4/5] code cleanup following review --- @xen-orchestra/backups/_cleanVm.js | 16 ++++++++++++---- packages/vhd-lib/Vhd/VhdDirectory.js | 4 +--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/@xen-orchestra/backups/_cleanVm.js b/@xen-orchestra/backups/_cleanVm.js index a1c93d62cb4..4055a9f30d2 100644 --- a/@xen-orchestra/backups/_cleanVm.js +++ b/@xen-orchestra/backups/_cleanVm.js @@ -151,13 +151,21 @@ async function checkAliases(aliasPaths, targetDataRepository, { handler, onLog = try { const { dispose } = await openVhd(handler, target) - dispose() - } catch (e) { - onLog(`target ${target} of alias ${path} is missing or broken`) + try { + await dispose() + } catch (e) { + // error during dispose should not trigger a deletion + } + } catch (error) { + onLog(`target ${target} of alias ${path} is missing or broken`, { error }) if (remove) { try { await VhdAbstract.unlink(handler, path) - } catch(e){} + } catch (e) { + if (e.code !== 'ENOENT') { + onLog(`Error while deleting target ${target} of alias ${path}`, { error: e }) + } + } } continue } diff --git a/packages/vhd-lib/Vhd/VhdDirectory.js b/packages/vhd-lib/Vhd/VhdDirectory.js index 29d0ae9a2e0..93451787d07 100644 --- a/packages/vhd-lib/Vhd/VhdDirectory.js +++ b/packages/vhd-lib/Vhd/VhdDirectory.js @@ -178,9 +178,7 @@ exports.VhdDirectory = class VhdDirectory extends VhdAbstract { bufHeader = (await this._readChunk('header')).buffer bufFooter = (await this._readChunk('footer')).buffer } catch (error) { - // to keep as much as possible of the vhdFile way of handlong broken vhd - // let's emit an assertion error (the vhd is broken, you should try to delete it) - // instead of a file not found error (the vhd is missing) + // emit an AssertionError if the VHD is broken to stay as close as possible to the VhdFile API if (error.code === 'ENOENT') { assert(false, 'Header And Footer should exists') } else { From 0caf6cf96c1ea25fab5cd6e2180e927c558be096 Mon Sep 17 00:00:00 2001 From: Julien Fontanet Date: Thu, 13 Jan 2022 11:22:41 +0100 Subject: [PATCH 5/5] fix merge --- @xen-orchestra/backups/_cleanVm.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/@xen-orchestra/backups/_cleanVm.js b/@xen-orchestra/backups/_cleanVm.js index 3d1e2a44a52..bb9a528f681 100644 --- a/@xen-orchestra/backups/_cleanVm.js +++ b/@xen-orchestra/backups/_cleanVm.js @@ -252,8 +252,7 @@ exports.cleanVm = async function cleanVm( // check if alias are correct // check if all vhd in data subfolder have a corresponding alias await asyncMap(Object.keys(aliases), async dir => { - const aliases = aliases[dir] - await checkAliases(aliases, `${dir}/data`, { handler, onLog, remove }) + await checkAliases(aliases[dir], `${dir}/data`, { handler, onLog, remove }) }) // remove VHDs with missing ancestors