diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 85de8fafab..fb2a6423e1 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -22,6 +22,8 @@ - [Web-core] Fix "console offline" illustration sparks color (PR [#10309](https://github.com/vatesfr/xen-orchestra/pull/10309)) - [Web-core] Fix 404 illustration color (PR [#10325](https://github.com/vatesfr/xen-orchestra/pull/10325)) - [Backup-archive] No longer create a `cache.json.gz` file on immutable/S3 remote during cleanup, which could not be deleted afterwards and stayed billed forever (PR [#10243](https://github.com/vatesfr/xen-orchestra/pull/10243)) +- [Servers] Fix endless connection attempts to a pool which is already connected through another server entry (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355)) +- [Servers] fix a mishandling in the grace period before marking a pool disconnected, this will keep the ui in sync AND not redownload all the xapi object for a transient issue (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355)) ### Packages to release @@ -43,5 +45,6 @@ - @xen-orchestra/backups patch - @xen-orchestra/web minor - @xen-orchestra/web-core minor +- xo-server patch diff --git a/packages/xo-server/src/models/server.test.mjs b/packages/xo-server/src/models/server.test.mjs new file mode 100644 index 0000000000..14f081a41e --- /dev/null +++ b/packages/xo-server/src/models/server.test.mjs @@ -0,0 +1,54 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' + +import { serializeError } from '../utils.mjs' +import { Servers } from './server.mjs' + +// Reproduces what a record goes through on its way to the database and back: +// `Collection#_add` copies it with JSON *before* the model serializes it, then +// redis stores and returns JSON +const throughDatabase = record => { + const copy = JSON.parse(JSON.stringify(record)) + Servers.prototype._serialize(copy) + + const stored = JSON.parse(JSON.stringify(copy)) + Servers.prototype._unserialize(stored) + return stored +} + +const makeServer = error => ({ id: 'server-1', enabled: true, error, host: '192.0.2.1' }) + +describe('Servers', function () { + describe('error', function () { + it('does not survive the copy made by the collection when it is a raw Error', function () { + // hence `serializeError` in `_connectXenServer`: `message`, `name` and + // `stack` are not enumerable, JSON copies them away + const { error } = throughDatabase(makeServer(new Error('this pool is already connected'))) + + assert.equal(error?.message, undefined) + }) + + it('keeps its message and code once serialized by the caller', function () { + const cause = new Error('SESSION_AUTHENTICATION_FAILED(, )') + cause.code = 'SESSION_AUTHENTICATION_FAILED' + + const { error } = throughDatabase(makeServer(serializeError(cause))) + + assert.equal(error.message, 'SESSION_AUTHENTICATION_FAILED(, )') + assert.equal(error.code, 'SESSION_AUTHENTICATION_FAILED') + assert.equal(error.name, 'Error') + }) + + it('compares equal to a new occurrence of the same error, so a failing server is not rewritten', function () { + // `_connectXenServer` skips the database write – and therefore any + // reconnection attempt triggered by it – when the error did not change + const previousError = throughDatabase( + makeServer(serializeError(new Error('this pool is already connected'))) + ).error + const currentError = serializeError(new Error('this pool is already connected')) + + assert.equal(previousError.code, currentError.code) + assert.equal(previousError.message, currentError.message) + }) + }) +}) diff --git a/packages/xo-server/src/xo-mixins/_xen-servers.test.mjs b/packages/xo-server/src/xo-mixins/_xen-servers.test.mjs new file mode 100644 index 0000000000..a3a15270e4 --- /dev/null +++ b/packages/xo-server/src/xo-mixins/_xen-servers.test.mjs @@ -0,0 +1,161 @@ +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { configure } from '@xen-orchestra/log/configure' + +import XenServers from './xen-servers.mjs' + +configure({ level: 'FATAL', transport: () => {} }) + +// Minimal XoApp mock: the constructor only registers hooks and watches a config +// duration, none of which are fired by these tests +const createMockApp = () => ({ + config: { watchDuration: () => {} }, + hooks: { on: () => {} }, +}) + +const SERVER = { id: 'server-1', enabled: true, host: '192.0.2.1', password: 'secret', username: 'root' } + +// `poolId` defaults to the pool the connection was opened with, `$id` is the +// one it currently reports (they differ after a pool UUID change) +const createXapi = ({ $id = 'pool-1' } = {}) => ({ + disconnect: async () => {}, + getObjectByRef: () => ({ uuid: 'host-1' }), + pool: { $id, master: 'OpaqueRef:master', uuid: $id }, +}) + +// `_servers` is normally created on the `core started` hook, and +// `_autoReconnectXenServer` would start a real reconnection loop +const createXenServers = (...servers) => { + const xenServers = new XenServers(createMockApp(), { safeMode: true }) + + const stored = { __proto__: null } + const serversToInitialize = servers.length === 0 ? [SERVER] : servers + + for (const server of serversToInitialize) { + stored[server.id] = { ...server } + } + const updates = [] + const reconnected = [] + + xenServers._servers = { + first: async id => (stored[id] === undefined ? undefined : { ...stored[id] }), + update: async model => { + updates.push({ ...model }) + Object.assign(stored[model.id], model) + }, + } + xenServers._autoReconnectXenServer = id => reconnected.push(id) + + return { reconnected, updates, xenServers } +} + +describe('updateXenServer', function () { + it('does not arm the auto-reconnect loop on the internal error bookkeeping write', async function () { + const { reconnected, updates, xenServers } = createXenServers() + + // `_connectXenServer` writes the error of every failed attempt: re-arming + // the loop here restarts one which just stopped on a permanent error + await xenServers.updateXenServer('server-1', { error: new Error('this pool is already connected') }) + + assert.equal(updates.length, 1, 'the error must still be persisted') + assert.deepEqual(reconnected, []) + }) + + it('does not arm the auto-reconnect loop when the error is cleared', async function () { + const { reconnected, xenServers } = createXenServers({ ...SERVER, error: { message: 'boom' } }) + + await xenServers.updateXenServer('server-1', { error: null }) + + assert.deepEqual(reconnected, []) + }) + + it('does not arm the auto-reconnect loop on an unrelated property', async function () { + const { reconnected, updates, xenServers } = createXenServers() + + await xenServers.updateXenServer('server-1', { label: 'new label' }) + + assert.equal(updates.length, 1) + assert.deepEqual(reconnected, []) + }) + + it('arms the auto-reconnect loop when the server is enabled', async function () { + const { reconnected, xenServers } = createXenServers({ ...SERVER, enabled: false }) + + await xenServers.updateXenServer('server-1', { enabled: true }) + + assert.deepEqual(reconnected, ['server-1']) + }) + + it('arms the auto-reconnect loop when the credentials or the address change', async function () { + for (const properties of [{ host: '192.0.2.2' }, { password: 'new secret' }, { username: 'admin' }]) { + const { reconnected, xenServers } = createXenServers() + + await xenServers.updateXenServer('server-1', properties) + + assert.deepEqual(reconnected, ['server-1'], `expected a reconnection for ${JSON.stringify(properties)}`) + } + }) + + it('does not arm the auto-reconnect loop while the server is already connecting', async function () { + const { reconnected, xenServers } = createXenServers({ ...SERVER, enabled: false }) + xenServers._connectingXenServers.add('server-1') + + await xenServers.updateXenServer('server-1', { enabled: true }) + + assert.deepEqual(reconnected, []) + }) +}) + +describe('disconnectXenServer', function () { + const connect = (xenServers, { poolId = 'pool-1', serverId = 'server-1' } = {}) => { + xenServers._xapis[serverId] = createXapi({ $id: poolId }) + xenServers._serverIdsByPool[poolId] = serverId + } + + it('forgets the pool of the server', async function () { + const { xenServers } = createXenServers() + connect(xenServers) + + await xenServers.disconnectXenServer('server-1') + + assert.deepEqual({ ...xenServers._serverIdsByPool }, {}) + assert.equal(xenServers._xapis['server-1'], undefined) + }) + + it('forgets the pool of the server even after a pool UUID change', async function () { + const { xenServers } = createXenServers() + connect(xenServers, { poolId: 'pool-1' }) + // `_onXenAdd` re-registers the server under the new identifier + delete xenServers._serverIdsByPool['pool-1'] + xenServers._serverIdsByPool['pool-2'] = 'server-1' + + await xenServers.disconnectXenServer('server-1') + + assert.deepEqual({ ...xenServers._serverIdsByPool }, {}) + }) + + it('does not drop the connection of the server holding the pool', async function () { + // a second entry registered on an already connected pool stays `enabled` + // but disconnected, with a `PoolAlreadyConnected` error: disconnecting or + // deleting it must not release the pool of the server which owns it + const { xenServers } = createXenServers(SERVER, { ...SERVER, id: 'server-2' }) + connect(xenServers) + const xapi = xenServers._xapis['server-1'] + + await xenServers.disconnectXenServer('server-2') + + assert.deepEqual({ ...xenServers._serverIdsByPool }, { 'pool-1': 'server-1' }) + assert.equal(xenServers._xapis['server-1'], xapi) + assert.equal(xenServers._getXenServerStatus('server-1'), 'connected') + }) + + it('leaves the pools of the other servers alone', async function () { + const { xenServers } = createXenServers() + connect(xenServers) + connect(xenServers, { poolId: 'pool-2', serverId: 'server-2' }) + + await xenServers.disconnectXenServer('server-1') + + assert.deepEqual({ ...xenServers._serverIdsByPool }, { 'pool-2': 'server-2' }) + }) +}) diff --git a/packages/xo-server/src/xo-mixins/xen-servers.mjs b/packages/xo-server/src/xo-mixins/xen-servers.mjs index 34a2a3c75d..9256a2928c 100644 --- a/packages/xo-server/src/xo-mixins/xen-servers.mjs +++ b/packages/xo-server/src/xo-mixins/xen-servers.mjs @@ -1,5 +1,4 @@ import assert from 'assert' -import findKey from 'lodash/findKey.js' import pick from 'lodash/pick.js' import { asyncEach } from '@vates/async-each' import { BaseError } from 'make-error' @@ -25,7 +24,7 @@ import { getRpuTracesConfig, openRpuTrace } from '../_rpuObservability.mjs' import xapiObjectToXo from '../xapi-object-to-xo.mjs' import XapiStats from '../xapi-stats.mjs' import { autoReconnect } from '../_xenServerAutoReconnect.mjs' -import { camelToSnakeCase, forEach, isEmpty, popProperty } from '../utils.mjs' +import { camelToSnakeCase, forEach, isEmpty, popProperty, serializeError } from '../utils.mjs' import { Servers } from '../models/server.mjs' // =================================================================== @@ -235,7 +234,19 @@ export default class XenServers { // an enabled server must not stay disconnected without retries, e.g. after // the user fixed its credentials or address - if (server.enabled && !this._connectingXenServers.has(id) && this._getXenServerStatus(id) === 'disconnected') { + // + // only an update which can actually change the outcome of a connection may + // (re-)arm the loop: `_connectXenServer` writes `{ error }` on every failed + // attempt, and that bookkeeping write must not restart a loop which just + // stopped on a permanent error (e.g. PoolAlreadyConnected), which would + // retry forever, at full speed since each loop restarts its own backoff + const canFixConnection = properties.enabled === true || connectionIdentityChanged + if ( + canFixConnection && + server.enabled && + !this._connectingXenServers.has(id) && + this._getXenServerStatus(id) === 'disconnected' + ) { this._autoReconnectXenServer(id) } } @@ -275,8 +286,7 @@ export default class XenServers { forEach(newXapiObjects, function handleObject(xapiObject, xapiId) { // handle pool UUID change if (xapiObject.$type === 'pool' && serverIdsByPool[xapiObject.$id] === undefined) { - const obsoletePoolId = findKey(serverIdsByPool, serverId => serverId === conId) - delete serverIdsByPool[obsoletePoolId] + self._forgetXenServerPool(conId) serverIdsByPool[xapiObject.$id] = conId } @@ -609,28 +619,31 @@ export default class XenServers { this.updateXenServer(id, { error: null })::ignoreErrors() - xapi.once('eventFetchingError', function eventFetchingErrorListener() { + const onEventFetchingError = () => { const timeout = setTimeout(() => { xapi.xo.uninstall() // switch server status from connected to connecting - delete serverIdsByPool[poolId] + this._forgetXenServerPool(server.id) }, this._xapiMarkDisconnectedDelay) xapi.once('eventFetchingSuccess', () => { - xapi.once('eventFetchingError', eventFetchingErrorListener) + xapi.once('eventFetchingError', onEventFetchingError) if (serverIdsByPool[poolId] === undefined) { + // the pool may now be known under another identifier, `install()` + // replays the pool object which registers it under the right one serverIdsByPool[poolId] = server.id xapi.xo.install() } else { clearTimeout(timeout) } }) - }) + } + xapi.once('eventFetchingError', onEventFetchingError) xapi.once('disconnected', () => { xapi.xo.uninstall() delete this._xapis[server.id] - delete this._serverIdsByPool[poolId] + this._forgetXenServerPool(server.id) this._app.emit('server:disconnected', { server, xapi }) // deliberate disconnections set `enabled` to false beforehand, in @@ -642,10 +655,12 @@ export default class XenServers { delete this._xapis[server.id] xapi.disconnect()::ignoreErrors() + const serializedError = serializeError(error) + // avoid a database write per auto-reconnect attempt when the error did not change const previousError = server.error - if (previousError?.code !== error?.code || previousError?.message !== error?.message) { - this.updateXenServer(id, { error })::ignoreErrors() + if (previousError?.code !== serializedError.code || previousError?.message !== serializedError.message) { + this.updateXenServer(id, { error: serializedError })::ignoreErrors() } // permanent errors: retrying is pointless, do not start the loop @@ -657,6 +672,21 @@ export default class XenServers { } } + // Removes every pool this server is registered as the connection of. + // + // The pool is looked up by server instead of by identifier because a pool + // UUID can change during the life of a connection (see `_onXenAdd`): a + // mapping left behind would make every subsequent connection to that pool + // fail with `PoolAlreadyConnected`, including the ones of this very server. + _forgetXenServerPool(serverId) { + const serverIdsByPool = this._serverIdsByPool + for (const poolId of Object.keys(serverIdsByPool)) { + if (serverIdsByPool[poolId] === serverId) { + delete serverIdsByPool[poolId] + } + } + } + async disconnectXenServer(id) { // throw no such object if the server does not exist const server = await this.getXenServer(id) @@ -678,11 +708,7 @@ export default class XenServers { const xapi = this._xapis[id] delete this._xapis[id] - const serverIdsByPool = this._serverIdsByPool - const poolId = findKey(serverIdsByPool, _ => _ === xapi) - if (poolId !== undefined) { - delete serverIdsByPool[id] - } + this._forgetXenServerPool(id) return xapi?.disconnect() } @@ -719,7 +745,7 @@ export default class XenServers { lastEventFetchedTimestamp !== undefined && Date.now() > lastEventFetchedTimestamp + this._xapiMarkDisconnectedDelay ) { - server.error = xapis[server.id].watchEventsError + server.error = serializeError(xapis[server.id].watchEventsError) } server.status = this._getXenServerStatus(server.id) if (server.status === 'connected') {