From 22f7e3823c6f160126f2ea4db8ce6029a7cfc968 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 10:43:39 +0200 Subject: [PATCH 01/16] S3C supports having different queueProcessor probeserver configuration per site. Joi configuration supports having multiple types for a single config parameter, this will allow supporting passing both an array or a single object containing the probeserver config. Issue: BB-536 --- extensions/replication/ReplicationConfigValidator.js | 5 ++++- lib/config/configItems.joi.js | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index 310d5a2e9..5c60bb503 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -71,7 +71,10 @@ const joiSchema = joi.object({ concurrency: joi.number().greater(0).default(10), mpuPartsConcurrency: joi.number().greater(0).default(10), minMPUSizeMB: joi.number().greater(0).default(20), - probeServer: probeServerJoi.default(), + probeServer: probeServerJoi.alternatives().try( + probeServerJoi, + probeServerPerSite + ).default(), circuitBreaker: joi.object().optional(), }).required(), replicationStatusProcessor: { diff --git a/lib/config/configItems.joi.js b/lib/config/configItems.joi.js index d165ebcf7..e270eec86 100644 --- a/lib/config/configItems.joi.js +++ b/lib/config/configItems.joi.js @@ -112,6 +112,14 @@ const probeServerJoi = joi.object({ port: joi.number().required(), }); +const probeServerPerSite = joi.array().items( + joi.object({ + bindAddress: joi.string().default('localhost'), + port: joi.number().required(), + site: joi.string().required() + }) +) + const mongoJoi = joi.object({ replicaSetHosts: joi.string().default('localhost:27017'), logName: joi.string().default('s3-recordlog'), From a3559a9f0be442385734227cd67c90f149f18944 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 11:46:54 +0200 Subject: [PATCH 02/16] Fix lint and export issue Issue: BB-536 --- extensions/replication/ReplicationConfigValidator.js | 2 +- lib/config/configItems.joi.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index 5c60bb503..d311251e5 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -1,7 +1,7 @@ const fs = require('fs'); const joi = require('joi'); const { hostPortJoi, transportJoi, bootstrapListJoi, adminCredsJoi, - retryParamsJoi, probeServerJoi } = + retryParamsJoi, probeServerJoi, probeServerPerSite } = require('../../lib/config/configItems.joi'); const qpRetryJoi = joi.object({ diff --git a/lib/config/configItems.joi.js b/lib/config/configItems.joi.js index e270eec86..fa05fbdb1 100644 --- a/lib/config/configItems.joi.js +++ b/lib/config/configItems.joi.js @@ -118,7 +118,7 @@ const probeServerPerSite = joi.array().items( port: joi.number().required(), site: joi.string().required() }) -) +); const mongoJoi = joi.object({ replicaSetHosts: joi.string().default('localhost:27017'), @@ -169,6 +169,7 @@ module.exports = { retryParamsJoi, certFilePathsJoi, probeServerJoi, + probeServerPerSite, stsConfigJoi, mongoJoi, qpKafkaJoi, From e213f82ba90eb693baa51d862805f368f4885a8f Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 12:27:25 +0200 Subject: [PATCH 03/16] Refactor ReplicationConfigValidator.js to use a more generic Joi schema for probeServer --- extensions/replication/ReplicationConfigValidator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index d311251e5..8c1d55340 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -71,7 +71,7 @@ const joiSchema = joi.object({ concurrency: joi.number().greater(0).default(10), mpuPartsConcurrency: joi.number().greater(0).default(10), minMPUSizeMB: joi.number().greater(0).default(20), - probeServer: probeServerJoi.alternatives().try( + probeServer: joi.alternatives().try( probeServerJoi, probeServerPerSite ).default(), From c03ab91fa325654dfa4b6d07526750ad062895d9 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 14:07:50 +0200 Subject: [PATCH 04/16] Add getProbeConfig function to get the config for the specified site, and if no site specified, just use the global config Issue: BB-536 --- .../replication/ReplicationConfigValidator.js | 2 +- extensions/replication/queueProcessor/task.js | 21 ++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index 8c1d55340..cffb427a1 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -74,7 +74,7 @@ const joiSchema = joi.object({ probeServer: joi.alternatives().try( probeServerJoi, probeServerPerSite - ).default(), + ).default({bindAddress: 'localhost', port: ""}), circuitBreaker: joi.object().optional(), }).required(), replicationStatusProcessor: { diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index aa5b1de5b..7d09492bd 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -239,8 +239,27 @@ function initAndStart(zkClient) { } }); + /** + * Get probe config will pull the configuration for the probe server based on + * the provided site key, and if the probe server config is not an array, it will + * return the global probe server config. + * + * @param {Object} queueProcessorConfig - Configuration of the queue processor that + * holds the probe server configs for all sites + * @param {string} site - Name of the site we are processing + * @returns {ProbeServerConfig} Config for site or global config + */ + function getProbeConfig(queueProcessorConfig, site) { + if (Array.isArray(queueProcessorConfig.probeServer) && site) { + return queueProcessorConfig.probeServer.filter(probe => probe.site === site)[0]; + } + return queueProcessorConfig.probeServer; + } + startProbeServer( - repConfig.queueProcessor.probeServer, + // get the probe server config for the first site in the bootstrap list, + // as if the probeConfig is an array there is only one element in bootstrap list + getProbeConfig(repConfig.queueProcessor, bootstrapList[0].site), (err, probeServer) => { if (err) { log.fatal('error creating probe server', { From 716091e679c4e6e6ed9ddfc37f23eebb2ea84ad2 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 14:16:19 +0200 Subject: [PATCH 05/16] Fix some lint issues Issue: BB-536 --- extensions/replication/ReplicationConfigValidator.js | 2 +- extensions/replication/queueProcessor/task.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index cffb427a1..0c7577a7a 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -74,7 +74,7 @@ const joiSchema = joi.object({ probeServer: joi.alternatives().try( probeServerJoi, probeServerPerSite - ).default({bindAddress: 'localhost', port: ""}), + ).default({ bindAddress: 'localhost', port: '' }), circuitBreaker: joi.object().optional(), }).required(), replicationStatusProcessor: { diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index 7d09492bd..2944971de 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -243,7 +243,7 @@ function initAndStart(zkClient) { * Get probe config will pull the configuration for the probe server based on * the provided site key, and if the probe server config is not an array, it will * return the global probe server config. - * + * * @param {Object} queueProcessorConfig - Configuration of the queue processor that * holds the probe server configs for all sites * @param {string} site - Name of the site we are processing From 45a1ff7fedafba8638f1a18178410c36717a54a4 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 16:36:04 +0200 Subject: [PATCH 06/16] Check that site names is not empty before use an element Issue: BB-536 --- extensions/replication/queueProcessor/task.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index 2944971de..c308c758b 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -243,23 +243,24 @@ function initAndStart(zkClient) { * Get probe config will pull the configuration for the probe server based on * the provided site key, and if the probe server config is not an array, it will * return the global probe server config. + * + * Get the probe server config for the first site in the site names list, + * as if the probeConfig is an array there is only one element in site names * * @param {Object} queueProcessorConfig - Configuration of the queue processor that * holds the probe server configs for all sites - * @param {string} site - Name of the site we are processing + * @param {Array} siteNames - List of site names * @returns {ProbeServerConfig} Config for site or global config */ - function getProbeConfig(queueProcessorConfig, site) { - if (Array.isArray(queueProcessorConfig.probeServer) && site) { - return queueProcessorConfig.probeServer.filter(probe => probe.site === site)[0]; + function getProbeConfig(queueProcessorConfig, siteNames) { + if (Array.isArray(queueProcessorConfig.probeServer) && siteNames[0]) { + return queueProcessorConfig.probeServer.filter(probe => probe.site === siteNames[0])[0]; } return queueProcessorConfig.probeServer; } startProbeServer( - // get the probe server config for the first site in the bootstrap list, - // as if the probeConfig is an array there is only one element in bootstrap list - getProbeConfig(repConfig.queueProcessor, bootstrapList[0].site), + getProbeConfig(repConfig.queueProcessor, siteNames), (err, probeServer) => { if (err) { log.fatal('error creating probe server', { From b7b9606c2244d921aaf086e3426182a3e8f68a55 Mon Sep 17 00:00:00 2001 From: KillianG Date: Wed, 9 Oct 2024 16:40:34 +0200 Subject: [PATCH 07/16] Fix trailing space Issue: BB-536 --- extensions/replication/queueProcessor/task.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index c308c758b..db869b475 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -243,7 +243,7 @@ function initAndStart(zkClient) { * Get probe config will pull the configuration for the probe server based on * the provided site key, and if the probe server config is not an array, it will * return the global probe server config. - * + * * Get the probe server config for the first site in the site names list, * as if the probeConfig is an array there is only one element in site names * From 60102225689b03ac8f95fb26ecd44320a1c78e75 Mon Sep 17 00:00:00 2001 From: KillianG Date: Thu, 10 Oct 2024 17:19:03 +0200 Subject: [PATCH 08/16] Add some verification, put the getProbeConfig out to be able to test it and make joi config have array minimal size of 1 Issue: BB-536 --- extensions/replication/queueProcessor/task.js | 42 ++++++++++--------- lib/config/configItems.joi.js | 2 +- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index db869b475..138634ae5 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -155,6 +155,28 @@ function setupZkSiteNode(qp, zkClient, site, done) { }); } +/** + * Get probe config will pull the configuration for the probe server based on + * the provided site key, and if the probe server config is not an array, it will + * return the global probe server config. + * + * Get the probe server config for the first site in the site names list, + * as if the probeConfig is an array there is only one element in site names + * + * @param {Object} queueProcessorConfig - Configuration of the queue processor that + * holds the probe server configs for all sites + * @param {Array} siteNames - List of site names + * @returns {ProbeServerConfig} Config for site or global config + */ +function getProbeConfig(queueProcessorConfig, siteNames) { + if (Array.isArray(queueProcessorConfig.probeServer) && + queueProcessorConfig.probeServer.length > 0 && + siteNames.length > 0) { + return queueProcessorConfig.probeServer.filter(probe => probe.site === siteNames[0])[0]; + } + return queueProcessorConfig.probeServer; +} + function initAndStart(zkClient) { initManagement({ serviceName: 'replication', @@ -239,26 +261,6 @@ function initAndStart(zkClient) { } }); - /** - * Get probe config will pull the configuration for the probe server based on - * the provided site key, and if the probe server config is not an array, it will - * return the global probe server config. - * - * Get the probe server config for the first site in the site names list, - * as if the probeConfig is an array there is only one element in site names - * - * @param {Object} queueProcessorConfig - Configuration of the queue processor that - * holds the probe server configs for all sites - * @param {Array} siteNames - List of site names - * @returns {ProbeServerConfig} Config for site or global config - */ - function getProbeConfig(queueProcessorConfig, siteNames) { - if (Array.isArray(queueProcessorConfig.probeServer) && siteNames[0]) { - return queueProcessorConfig.probeServer.filter(probe => probe.site === siteNames[0])[0]; - } - return queueProcessorConfig.probeServer; - } - startProbeServer( getProbeConfig(repConfig.queueProcessor, siteNames), (err, probeServer) => { diff --git a/lib/config/configItems.joi.js b/lib/config/configItems.joi.js index fa05fbdb1..757cbed21 100644 --- a/lib/config/configItems.joi.js +++ b/lib/config/configItems.joi.js @@ -112,7 +112,7 @@ const probeServerJoi = joi.object({ port: joi.number().required(), }); -const probeServerPerSite = joi.array().items( +const probeServerPerSite = joi.array().min(1).items( joi.object({ bindAddress: joi.string().default('localhost'), port: joi.number().required(), From 5dc377bf686d4ef3e8e0e6b9c5dfb9686dc19dea Mon Sep 17 00:00:00 2001 From: KillianG Date: Thu, 10 Oct 2024 17:47:55 +0200 Subject: [PATCH 09/16] rework getProbeConfig function to take all possibilites into account Issue: BB-536 --- extensions/replication/queueProcessor/task.js | 24 +-------- lib/util/probe.js | 31 ++++++++++++ tests/unit/lib/util/probe.spec.js | 49 ++++++++++++++++++- 3 files changed, 80 insertions(+), 24 deletions(-) diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index 138634ae5..590c45509 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -22,7 +22,7 @@ const internalHttpsConfig = config.internalHttps; const mConfig = config.metrics; const { connectionString, autoCreateNamespace } = zkConfig; const RESUME_NODE = 'scheduledResume'; -const { startProbeServer } = require('../../../lib/util/probe'); +const { startProbeServer, getProbeConfig } = require('../../../lib/util/probe'); const { DEFAULT_LIVE_ROUTE, DEFAULT_METRICS_ROUTE, DEFAULT_READY_ROUTE } = require('arsenal').network.probe.ProbeServer; const { sendSuccess } = require('arsenal').network.probe.Utils; @@ -155,28 +155,6 @@ function setupZkSiteNode(qp, zkClient, site, done) { }); } -/** - * Get probe config will pull the configuration for the probe server based on - * the provided site key, and if the probe server config is not an array, it will - * return the global probe server config. - * - * Get the probe server config for the first site in the site names list, - * as if the probeConfig is an array there is only one element in site names - * - * @param {Object} queueProcessorConfig - Configuration of the queue processor that - * holds the probe server configs for all sites - * @param {Array} siteNames - List of site names - * @returns {ProbeServerConfig} Config for site or global config - */ -function getProbeConfig(queueProcessorConfig, siteNames) { - if (Array.isArray(queueProcessorConfig.probeServer) && - queueProcessorConfig.probeServer.length > 0 && - siteNames.length > 0) { - return queueProcessorConfig.probeServer.filter(probe => probe.site === siteNames[0])[0]; - } - return queueProcessorConfig.probeServer; -} - function initAndStart(zkClient) { initManagement({ serviceName: 'replication', diff --git a/lib/util/probe.js b/lib/util/probe.js index e51392d08..6d45f7de7 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -60,8 +60,39 @@ function observeKafkaStats(msg) { kafkaMetrics.observe(JSON.parse(msg.message)); } +/** + * Get probe config will pull the configuration for the probe server based on + * the provided site name. If siteNames is empty, it returns the global probe server config + * only if it's a single object. + * + * @param {Object} queueProcessorConfig - Configuration of the queue processor that + * holds the probe server configs for all sites + * @param {Array} siteNames - List of site names (should contain at most one element) + * @returns {Object|undefined} Config for site or global config, undefined if no match found or invalid config + */ +function getProbeConfig(queueProcessorConfig, siteNames) { + if (siteNames.length === 0) { + if (!Array.isArray(queueProcessorConfig.probeServer)) { + return queueProcessorConfig.probeServer; + } + return undefined; + } + + if (Array.isArray(queueProcessorConfig.probeServer)) { + if (siteNames.length !== 1) { + return undefined; + } + + const siteConfig = queueProcessorConfig.probeServer.find(config => config.site === siteNames[0]); + return siteConfig || undefined; + } + + return undefined; +} + module.exports = { startProbeServer, startProbeServerPromise, observeKafkaStats, + getProbeConfig, }; diff --git a/tests/unit/lib/util/probe.spec.js b/tests/unit/lib/util/probe.spec.js index a2fa05015..e91b58369 100644 --- a/tests/unit/lib/util/probe.spec.js +++ b/tests/unit/lib/util/probe.spec.js @@ -1,5 +1,5 @@ const assert = require('assert'); -const { startProbeServer } = +const { startProbeServer, getProbeConfig } = require('../../../../lib/util/probe'); describe('Probe server', () => { @@ -25,3 +25,50 @@ describe('Probe server', () => { }); }); }); + +describe.only('getProbeConfig', function() { + it('returns the probeServer config when siteNames is empty and probeServer is a single object', function() { + const queueProcessorConfig = { + probeServer: { bindAddress: '127.0.0.1', port: '8080' } + }; + const siteNames = []; + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.deepStrictEqual(result, { bindAddress: '127.0.0.1', port: '8080' }); + }); + + it('returns undefined when siteNames is empty and probeServer is not a single object', function() { + const queueProcessorConfig = { + probeServer: [{ site: 'site1', bindAddress: '127.0.0.1', port: '8080' }] + }; + const siteNames = []; + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.strictEqual(result, undefined); + }); + + it('returns the correct site config when probeServer is an array and siteNames has one matching element', function() { + const queueProcessorConfig = { + probeServer: [ + { site: 'site1', bindAddress: '127.0.0.1', port: '8080' }, + { site: 'site2', bindAddress: '127.0.0.2', port: '8081' } + ] + }; + const siteNames = ['site2']; + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.deepStrictEqual(result, { site: 'site2', bindAddress: '127.0.0.2', port: '8081' }); + }); + + it('returns undefined when probeServer is an array and siteNames has no matching element', function() { + const queueProcessorConfig = { + probeServer: [ + { site: 'site1', bindAddress: '127.0.0.1', port: '8080' } + ] + }; + const siteNames = ['site2']; + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.strictEqual(result, undefined); + }); + }); From b6b7636c894ebc3a17872357a5cffd0e511f300a Mon Sep 17 00:00:00 2001 From: KillianG Date: Thu, 10 Oct 2024 17:57:36 +0200 Subject: [PATCH 10/16] Fix some lint issue and remove only Issue: BB-536 --- tests/unit/lib/util/probe.spec.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/unit/lib/util/probe.spec.js b/tests/unit/lib/util/probe.spec.js index e91b58369..161880bb3 100644 --- a/tests/unit/lib/util/probe.spec.js +++ b/tests/unit/lib/util/probe.spec.js @@ -26,28 +26,28 @@ describe('Probe server', () => { }); }); -describe.only('getProbeConfig', function() { - it('returns the probeServer config when siteNames is empty and probeServer is a single object', function() { +describe('getProbeConfig', () => { + it('returns the probeServer config when siteNames is empty and probeServer is a single object', () => { const queueProcessorConfig = { probeServer: { bindAddress: '127.0.0.1', port: '8080' } }; const siteNames = []; - + const result = getProbeConfig(queueProcessorConfig, siteNames); assert.deepStrictEqual(result, { bindAddress: '127.0.0.1', port: '8080' }); }); - - it('returns undefined when siteNames is empty and probeServer is not a single object', function() { + + it('returns undefined when siteNames is empty and probeServer is not a single object', () => { const queueProcessorConfig = { probeServer: [{ site: 'site1', bindAddress: '127.0.0.1', port: '8080' }] }; const siteNames = []; - + const result = getProbeConfig(queueProcessorConfig, siteNames); assert.strictEqual(result, undefined); }); - - it('returns the correct site config when probeServer is an array and siteNames has one matching element', function() { + + it('returns the correct site config when probeServer is an array and siteNames has one matching element', () => { const queueProcessorConfig = { probeServer: [ { site: 'site1', bindAddress: '127.0.0.1', port: '8080' }, @@ -55,19 +55,19 @@ describe.only('getProbeConfig', function() { ] }; const siteNames = ['site2']; - + const result = getProbeConfig(queueProcessorConfig, siteNames); assert.deepStrictEqual(result, { site: 'site2', bindAddress: '127.0.0.2', port: '8081' }); }); - - it('returns undefined when probeServer is an array and siteNames has no matching element', function() { + + it('returns undefined when probeServer is an array and siteNames has no matching element', () => { const queueProcessorConfig = { probeServer: [ { site: 'site1', bindAddress: '127.0.0.1', port: '8080' } ] }; const siteNames = ['site2']; - + const result = getProbeConfig(queueProcessorConfig, siteNames); assert.strictEqual(result, undefined); }); From 9c87ed37699c27e40309d6c7c233868da19fc04d Mon Sep 17 00:00:00 2001 From: KillianG Date: Thu, 10 Oct 2024 18:11:02 +0200 Subject: [PATCH 11/16] Add missing unit test Issue: BB-536 --- tests/unit/lib/util/probe.spec.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/unit/lib/util/probe.spec.js b/tests/unit/lib/util/probe.spec.js index 161880bb3..f04069543 100644 --- a/tests/unit/lib/util/probe.spec.js +++ b/tests/unit/lib/util/probe.spec.js @@ -71,4 +71,27 @@ describe('getProbeConfig', () => { const result = getProbeConfig(queueProcessorConfig, siteNames); assert.strictEqual(result, undefined); }); + + it('returns undefined when siteNames contains more than one element', () => { + const queueProcessorConfig = { + probeServer: [ + { site: 'site1', bindAddress: '127.0.0.1', port: '8080' }, + { site: 'site2', bindAddress: '127.0.0.2', port: '8081' } + ] + }; + const siteNames = ['site1', 'site2']; // More than one element in siteNames + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.strictEqual(result, undefined); + }); + + it('returns undefined when probeServer is not an array and siteNames is not empty', () => { + const queueProcessorConfig = { + probeServer: { bindAddress: '127.0.0.1', port: '8080' } // probeServer is a single object + }; + const siteNames = ['site1']; // siteNames is not empty + + const result = getProbeConfig(queueProcessorConfig, siteNames); + assert.strictEqual(result, undefined); + }); }); From fb61b6b59812180fb153294245b538f13e9ae9a0 Mon Sep 17 00:00:00 2001 From: KillianG Date: Tue, 15 Oct 2024 13:50:15 +0200 Subject: [PATCH 12/16] Change default port and add error log for getProbeConfig Issue: BB-536 --- .../replication/ReplicationConfigValidator.js | 2 +- lib/util/probe.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index 0c7577a7a..71ca68714 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -74,7 +74,7 @@ const joiSchema = joi.object({ probeServer: joi.alternatives().try( probeServerJoi, probeServerPerSite - ).default({ bindAddress: 'localhost', port: '' }), + ).default({ bindAddress: 'localhost', port: 4042 }), circuitBreaker: joi.object().optional(), }).required(), replicationStatusProcessor: { diff --git a/lib/util/probe.js b/lib/util/probe.js index 6d45f7de7..7085c1f68 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -2,6 +2,10 @@ const util = require('util'); const { ProbeServer } = require('arsenal').network.probe.ProbeServer; const { ZenkoMetrics } = require('arsenal').metrics; const RdkafkaStats = require('node-rdkafka-prometheus'); +const werelogs = require('werelogs'); + +const Logger = werelogs.Logger; + /** * Configure probe servers @@ -75,11 +79,20 @@ function getProbeConfig(queueProcessorConfig, siteNames) { if (!Array.isArray(queueProcessorConfig.probeServer)) { return queueProcessorConfig.probeServer; } + + Logger.error('Configuration set for specific sites, but no site provided to the process', { + siteNames, + queueProcessorConfig, + }); return undefined; } if (Array.isArray(queueProcessorConfig.probeServer)) { if (siteNames.length !== 1) { + Logger.error('Process configured for more than one site', { + siteNames, + queueProcessorConfig, + }); return undefined; } From be2be13fc1a2ca80c65c1e8076d562b84f15dfe6 Mon Sep 17 00:00:00 2001 From: KillianG Date: Tue, 15 Oct 2024 14:15:27 +0200 Subject: [PATCH 13/16] Better logger Issue: BB-536 --- lib/util/probe.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/util/probe.js b/lib/util/probe.js index 7085c1f68..2d57a836d 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -75,12 +75,14 @@ function observeKafkaStats(msg) { * @returns {Object|undefined} Config for site or global config, undefined if no match found or invalid config */ function getProbeConfig(queueProcessorConfig, siteNames) { + const logger = new Logger('Backbeat:Probe:getProbeConfig'); + if (siteNames.length === 0) { if (!Array.isArray(queueProcessorConfig.probeServer)) { return queueProcessorConfig.probeServer; } - Logger.error('Configuration set for specific sites, but no site provided to the process', { + logger.error('Configuration set for specific sites, but no site provided to the process', { siteNames, queueProcessorConfig, }); @@ -89,7 +91,7 @@ function getProbeConfig(queueProcessorConfig, siteNames) { if (Array.isArray(queueProcessorConfig.probeServer)) { if (siteNames.length !== 1) { - Logger.error('Process configured for more than one site', { + logger.error('Process configured for more than one site', { siteNames, queueProcessorConfig, }); From 30a3a01b31f4d6ff828e5a10fb0ab2794b63f785 Mon Sep 17 00:00:00 2001 From: KillianG Date: Mon, 21 Oct 2024 11:16:36 +0200 Subject: [PATCH 14/16] User log from param and return probeconfig Issue: BB-536 --- extensions/replication/queueProcessor/task.js | 2 +- lib/util/probe.js | 10 +++------- tests/unit/lib/util/probe.spec.js | 18 ++++++++++-------- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/extensions/replication/queueProcessor/task.js b/extensions/replication/queueProcessor/task.js index 590c45509..6eb351907 100644 --- a/extensions/replication/queueProcessor/task.js +++ b/extensions/replication/queueProcessor/task.js @@ -240,7 +240,7 @@ function initAndStart(zkClient) { }); startProbeServer( - getProbeConfig(repConfig.queueProcessor, siteNames), + getProbeConfig(repConfig.queueProcessor, siteNames, log), (err, probeServer) => { if (err) { log.fatal('error creating probe server', { diff --git a/lib/util/probe.js b/lib/util/probe.js index 2d57a836d..728c596b9 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -2,10 +2,6 @@ const util = require('util'); const { ProbeServer } = require('arsenal').network.probe.ProbeServer; const { ZenkoMetrics } = require('arsenal').metrics; const RdkafkaStats = require('node-rdkafka-prometheus'); -const werelogs = require('werelogs'); - -const Logger = werelogs.Logger; - /** * Configure probe servers @@ -72,10 +68,10 @@ function observeKafkaStats(msg) { * @param {Object} queueProcessorConfig - Configuration of the queue processor that * holds the probe server configs for all sites * @param {Array} siteNames - List of site names (should contain at most one element) + * @param {Logger} logger - Logger instance * @returns {Object|undefined} Config for site or global config, undefined if no match found or invalid config */ -function getProbeConfig(queueProcessorConfig, siteNames) { - const logger = new Logger('Backbeat:Probe:getProbeConfig'); +function getProbeConfig(queueProcessorConfig, siteNames, logger) { if (siteNames.length === 0) { if (!Array.isArray(queueProcessorConfig.probeServer)) { @@ -102,7 +98,7 @@ function getProbeConfig(queueProcessorConfig, siteNames) { return siteConfig || undefined; } - return undefined; + return queueProcessorConfig.probeServer; } module.exports = { diff --git a/tests/unit/lib/util/probe.spec.js b/tests/unit/lib/util/probe.spec.js index f04069543..d3afdcd56 100644 --- a/tests/unit/lib/util/probe.spec.js +++ b/tests/unit/lib/util/probe.spec.js @@ -1,6 +1,7 @@ const assert = require('assert'); const { startProbeServer, getProbeConfig } = require('../../../../lib/util/probe'); +const Logger = require('werelogs').Logger; describe('Probe server', () => { it('is not created with no config', done => { @@ -27,13 +28,14 @@ describe('Probe server', () => { }); describe('getProbeConfig', () => { + const log = new Logger('getProbeConfig'); it('returns the probeServer config when siteNames is empty and probeServer is a single object', () => { const queueProcessorConfig = { probeServer: { bindAddress: '127.0.0.1', port: '8080' } }; const siteNames = []; - const result = getProbeConfig(queueProcessorConfig, siteNames); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); assert.deepStrictEqual(result, { bindAddress: '127.0.0.1', port: '8080' }); }); @@ -43,7 +45,7 @@ describe('getProbeConfig', () => { }; const siteNames = []; - const result = getProbeConfig(queueProcessorConfig, siteNames); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); assert.strictEqual(result, undefined); }); @@ -56,7 +58,7 @@ describe('getProbeConfig', () => { }; const siteNames = ['site2']; - const result = getProbeConfig(queueProcessorConfig, siteNames); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); assert.deepStrictEqual(result, { site: 'site2', bindAddress: '127.0.0.2', port: '8081' }); }); @@ -68,7 +70,7 @@ describe('getProbeConfig', () => { }; const siteNames = ['site2']; - const result = getProbeConfig(queueProcessorConfig, siteNames); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); assert.strictEqual(result, undefined); }); @@ -81,17 +83,17 @@ describe('getProbeConfig', () => { }; const siteNames = ['site1', 'site2']; // More than one element in siteNames - const result = getProbeConfig(queueProcessorConfig, siteNames); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); assert.strictEqual(result, undefined); }); - it('returns undefined when probeServer is not an array and siteNames is not empty', () => { + it('returns probeserver when probeServer is not an array and siteNames is not empty', () => { const queueProcessorConfig = { probeServer: { bindAddress: '127.0.0.1', port: '8080' } // probeServer is a single object }; const siteNames = ['site1']; // siteNames is not empty - const result = getProbeConfig(queueProcessorConfig, siteNames); - assert.strictEqual(result, undefined); + const result = getProbeConfig(queueProcessorConfig, siteNames, log); + assert.deepStrictEqual(result, queueProcessorConfig.probeServer); }); }); From 7fc67bdd74a42a2d8ae172d92eca40157bfd4a33 Mon Sep 17 00:00:00 2001 From: KillianG Date: Mon, 21 Oct 2024 14:40:45 +0200 Subject: [PATCH 15/16] Log warning when config is not found for specified site Issue: BB-536 --- extensions/replication/ReplicationConfigValidator.js | 2 +- lib/util/probe.js | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/extensions/replication/ReplicationConfigValidator.js b/extensions/replication/ReplicationConfigValidator.js index 71ca68714..bdfbb5dea 100644 --- a/extensions/replication/ReplicationConfigValidator.js +++ b/extensions/replication/ReplicationConfigValidator.js @@ -73,7 +73,7 @@ const joiSchema = joi.object({ minMPUSizeMB: joi.number().greater(0).default(20), probeServer: joi.alternatives().try( probeServerJoi, - probeServerPerSite + probeServerPerSite, ).default({ bindAddress: 'localhost', port: 4042 }), circuitBreaker: joi.object().optional(), }).required(), diff --git a/lib/util/probe.js b/lib/util/probe.js index 728c596b9..3764c8da9 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -72,7 +72,6 @@ function observeKafkaStats(msg) { * @returns {Object|undefined} Config for site or global config, undefined if no match found or invalid config */ function getProbeConfig(queueProcessorConfig, siteNames, logger) { - if (siteNames.length === 0) { if (!Array.isArray(queueProcessorConfig.probeServer)) { return queueProcessorConfig.probeServer; @@ -95,7 +94,14 @@ function getProbeConfig(queueProcessorConfig, siteNames, logger) { } const siteConfig = queueProcessorConfig.probeServer.find(config => config.site === siteNames[0]); - return siteConfig || undefined; + if (siteConfig === undefined) { + logger.warn('Probe server configuration for site not found', { + siteName: siteNames[0], + queueProcessorConfig, + }); + } + + return siteConfig; } return queueProcessorConfig.probeServer; From 6cd316e2d2ec959ab1a8494285570b037b166137 Mon Sep 17 00:00:00 2001 From: KillianG Date: Tue, 22 Oct 2024 10:39:53 +0200 Subject: [PATCH 16/16] Simplify getProbeConfig Issue: BB-536 --- lib/util/probe.js | 48 +++++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 31 deletions(-) diff --git a/lib/util/probe.js b/lib/util/probe.js index 3764c8da9..026e3111f 100644 --- a/lib/util/probe.js +++ b/lib/util/probe.js @@ -72,41 +72,27 @@ function observeKafkaStats(msg) { * @returns {Object|undefined} Config for site or global config, undefined if no match found or invalid config */ function getProbeConfig(queueProcessorConfig, siteNames, logger) { - if (siteNames.length === 0) { - if (!Array.isArray(queueProcessorConfig.probeServer)) { - return queueProcessorConfig.probeServer; + if (Array.isArray(queueProcessorConfig.probeServer)) { + if (siteNames.length !== 1) { + logger.error('Process configured for more than one site or no site provided', { + siteNames, + queueProcessorConfig, + }); + return undefined; + } + const siteConfig = queueProcessorConfig.probeServer.find(config => config.site === siteNames[0]); + if (siteConfig === undefined) { + logger.warn('Probe server configuration for site not found', { + siteName: siteNames[0], + queueProcessorConfig, + }); + } + return siteConfig; } - logger.error('Configuration set for specific sites, but no site provided to the process', { - siteNames, - queueProcessorConfig, - }); - return undefined; + return queueProcessorConfig.probeServer; } - if (Array.isArray(queueProcessorConfig.probeServer)) { - if (siteNames.length !== 1) { - logger.error('Process configured for more than one site', { - siteNames, - queueProcessorConfig, - }); - return undefined; - } - - const siteConfig = queueProcessorConfig.probeServer.find(config => config.site === siteNames[0]); - if (siteConfig === undefined) { - logger.warn('Probe server configuration for site not found', { - siteName: siteNames[0], - queueProcessorConfig, - }); - } - - return siteConfig; - } - - return queueProcessorConfig.probeServer; -} - module.exports = { startProbeServer, startProbeServerPromise,