Skip to content

Commit cecde98

Browse files
author
Kerkesni
committed
Pass instanceId to the mongodb metadata backend
instanceId will be used when generating versionIds. This will ensure uniqueness of the versionID across all Cloudserver pods in the cluster. The instanceId is a combination of the short hash contained in the name of the pods, which is unique across all pods within the same deployment, and a prefix for differenciating the different types of deployments (internal vs external). see. https://scality.atlassian.net/wiki/spaces/OS/pages/3225583692/VersionID+Collisions+in+Zenko Issue: CLDSRV-665
1 parent b6a78b9 commit cecde98

File tree

3 files changed

+123
-19
lines changed

3 files changed

+123
-19
lines changed

‎lib/Config.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -968,6 +968,38 @@ class Config extends EventEmitter {
968968
this.replicationGroupId = 'RG001';
969969
}
970970

971+
const instanceId = process.env.CLOUDSERVER_INSTANCE_ID || config.instanceId;
972+
if (instanceId) {
973+
assert(typeof instanceId === 'string',
974+
'bad config: instanceId must be a string');
975+
// versionID generation code will truncate instanceId to 6 characters
976+
// so we enforce this limit here to make the behavior predictable
977+
assert(instanceId.length <= 6,
978+
'bad config: instanceId must be at most 6 characters long');
979+
this.instanceId = instanceId;
980+
} else if (process.env.HOSTNAME) {
981+
// If running in Kubernetes, the pod name is set in the HOSTNAME env var
982+
// and is of the form: cloudserver-7869f99955-nr984
983+
// where:
984+
// - cloudserver is the name of the pod
985+
// - 7869f99955 is a long hash that guarantees uniqueness within the namespace
986+
// - nr984 is a short hash that is unique only within the current ReplicaSet/Deployment
987+
// We use the last part of the pod name as the instanceId, prefixed with 'i' for internal
988+
// or 'e' for external cloudserver.
989+
// This allows having a unique identifier that will be used when generating versionIds
990+
const instanceType = process.env.HOSTNAME.includes('internal') ? 'i' : 'e';
991+
const parts = process.env.HOSTNAME.split('-');
992+
if (parts.length >= 2) {
993+
const shortId = parts[parts.length - 1];
994+
this.instanceId = instanceType + shortId;
995+
} else {
996+
// fallback: generate a random string if format is unexpected
997+
this.instanceId = uuidv4().replace(/-/g, '').slice(0, 6);
998+
}
999+
} else {
1000+
this.instanceId = uuidv4().replace(/-/g, '').slice(0, 6);
1001+
}
1002+
9711003
this.replicationEndpoints = [];
9721004
if (config.replicationEndpoints) {
9731005
const { replicationEndpoints } = config;

‎lib/metadata/wrapper.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ if (clientName === 'mem') {
3030
params = {
3131
mongodb: config.mongodb,
3232
replicationGroupId: config.replicationGroupId,
33+
instanceId: config.instanceId,
3334
config,
3435
};
3536
} else if (clientName === 'cdmi') {

‎tests/unit/Config.js‎

Lines changed: 90 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
const assert = require('assert');
2+
const sinon = require('sinon');
3+
const fs = require('fs');
4+
const path = require('path');
25
const {
36
azureArchiveLocationConstraintAssert,
4-
ConfigObject: ConfigObjectForTest,
7+
ConfigObject,
58
} = require('../../lib/Config');
69

710
describe('Config', () => {
@@ -25,7 +28,6 @@ describe('Config', () => {
2528
});
2629

2730
it('should emit an event when auth data is updated', done => {
28-
const { ConfigObject } = require('../../lib/Config');
2931
const config = new ConfigObject();
3032
let emitted = false;
3133
config.on('authdata-update', () => {
@@ -209,8 +211,6 @@ describe('Config', () => {
209211
});
210212

211213
describe('getAzureStorageAccountName', () => {
212-
const { ConfigObject } = require('../../lib/Config');
213-
214214
it('should return account name from config', () => {
215215
setEnv('azurebackend_AZURE_STORAGE_ACCOUNT_NAME', '');
216216
const config = new ConfigObject();
@@ -259,7 +259,7 @@ describe('Config', () => {
259259

260260
describe('time options', () => {
261261
it('should getTimeOptions', () => {
262-
const config = new ConfigObjectForTest();
262+
const config = new ConfigObject();
263263
const expectedOptions = {
264264
expireOneDayEarlier: false,
265265
transitionOneDayEarlier: false,
@@ -273,7 +273,7 @@ describe('Config', () => {
273273
it('should getTimeOptions with TIME_PROGRESSION_FACTOR', () => {
274274
setEnv('TIME_PROGRESSION_FACTOR', 2);
275275

276-
const config = new ConfigObjectForTest();
276+
const config = new ConfigObject();
277277
const expectedOptions = {
278278
expireOneDayEarlier: false,
279279
transitionOneDayEarlier: false,
@@ -287,7 +287,7 @@ describe('Config', () => {
287287
it('should getTimeOptions with EXPIRE_ONE_DAY_EARLIER', () => {
288288
setEnv('EXPIRE_ONE_DAY_EARLIER', true);
289289

290-
const config = new ConfigObjectForTest();
290+
const config = new ConfigObject();
291291
const expectedOptions = {
292292
expireOneDayEarlier: true,
293293
transitionOneDayEarlier: false,
@@ -301,7 +301,7 @@ describe('Config', () => {
301301
it('should getTimeOptions with TRANSITION_ONE_DAY_EARLIER', () => {
302302
setEnv('TRANSITION_ONE_DAY_EARLIER', true);
303303

304-
const config = new ConfigObjectForTest();
304+
const config = new ConfigObject();
305305
const expectedOptions = {
306306
expireOneDayEarlier: false,
307307
transitionOneDayEarlier: true,
@@ -316,7 +316,7 @@ describe('Config', () => {
316316
setEnv('EXPIRE_ONE_DAY_EARLIER', true);
317317
setEnv('TRANSITION_ONE_DAY_EARLIER', true);
318318

319-
const config = new ConfigObjectForTest();
319+
const config = new ConfigObject();
320320
const expectedOptions = {
321321
expireOneDayEarlier: true,
322322
transitionOneDayEarlier: true,
@@ -331,22 +331,22 @@ describe('Config', () => {
331331
setEnv('EXPIRE_ONE_DAY_EARLIER', true);
332332
setEnv('TIME_PROGRESSION_FACTOR', 2);
333333

334-
assert.throws(() => new ConfigObjectForTest());
334+
assert.throws(() => new ConfigObject());
335335
});
336336

337337
it('should throw error if TRANSITION_ONE_DAY_EARLIER and TIME_PROGRESSION_FACTOR', () => {
338338
setEnv('TRANSITION_ONE_DAY_EARLIER', true);
339339
setEnv('TIME_PROGRESSION_FACTOR', 2);
340340

341-
assert.throws(() => new ConfigObjectForTest());
341+
assert.throws(() => new ConfigObject());
342342
});
343343

344344
it('should throw error if both EXPIRE/TRANSITION_ONE_DAY_EARLIER and TIME_PROGRESSION_FACTOR', () => {
345345
setEnv('EXPIRE_ONE_DAY_EARLIER', true);
346346
setEnv('TRANSITION_ONE_DAY_EARLIER', true);
347347
setEnv('TIME_PROGRESSION_FACTOR', 2);
348348

349-
assert.throws(() => new ConfigObjectForTest());
349+
assert.throws(() => new ConfigObject());
350350
});
351351
});
352352

@@ -364,7 +364,6 @@ describe('Config', () => {
364364
});
365365

366366
it('should set up scuba', () => {
367-
const { ConfigObject } = require('../../lib/Config');
368367
const config = new ConfigObject();
369368

370369
assert.deepStrictEqual(
@@ -380,7 +379,6 @@ describe('Config', () => {
380379
setEnv('SCUBA_HOST', 'scubahost');
381380
setEnv('SCUBA_PORT', 1234);
382381

383-
const { ConfigObject } = require('../../lib/Config');
384382
const config = new ConfigObject();
385383

386384
assert.deepStrictEqual(
@@ -407,7 +405,6 @@ describe('Config', () => {
407405
});
408406

409407
it('should set up quota', () => {
410-
const { ConfigObject } = require('../../lib/Config');
411408
const config = new ConfigObject();
412409

413410
assert.deepStrictEqual(
@@ -423,7 +420,6 @@ describe('Config', () => {
423420
setEnv('QUOTA_MAX_STALENESS_MS', 1234);
424421
setEnv('QUOTA_ENABLE_INFLIGHTS', 'true');
425422

426-
const { ConfigObject } = require('../../lib/Config');
427423
const config = new ConfigObject();
428424

429425
assert.deepStrictEqual(
@@ -439,7 +435,6 @@ describe('Config', () => {
439435
setEnv('QUOTA_MAX_STALENESS_MS', 'notanumber');
440436
setEnv('QUOTA_ENABLE_INFLIGHTS', 'true');
441437

442-
const { ConfigObject } = require('../../lib/Config');
443438
const config = new ConfigObject();
444439

445440
assert.deepStrictEqual(
@@ -466,7 +461,6 @@ describe('Config', () => {
466461
});
467462

468463
it('should set up utapi local cache', () => {
469-
const { ConfigObject } = require('../../lib/Config');
470464
const config = new ConfigObject();
471465

472466
assert.deepStrictEqual(
@@ -480,7 +474,6 @@ describe('Config', () => {
480474
});
481475

482476
it('should set up utapi redis', () => {
483-
const { ConfigObject } = require('../../lib/Config');
484477
const config = new ConfigObject();
485478

486479
assert.deepStrictEqual(
@@ -668,4 +661,82 @@ describe('Config', () => {
668661
assert.throws(() => azureArchiveLocationConstraintAssert(locationObj));
669662
});
670663
});
664+
665+
describe('instanceId', () => {
666+
let defaultConfig;
667+
668+
before(() => {
669+
setEnv('S3_CONFIG_FILE', 'tests/unit/testConfigs/allOptsConfig/config.json');
670+
// Import config
671+
const defaultConfigPath = path.join(__dirname, './testConfigs/allOptsConfig/config.json');
672+
defaultConfig = require(defaultConfigPath);
673+
});
674+
675+
afterEach(() => {
676+
sinon.restore();
677+
});
678+
679+
it('should use the instance id set in the config', () => {
680+
// Stub fs.readFileSync to return a specific config.json content that includes instanceId
681+
const originalReadFileSync = fs.readFileSync;
682+
const readFileSyncStub = sinon.stub(fs, 'readFileSync');
683+
// Mock only config.json file
684+
readFileSyncStub
685+
.withArgs(sinon.match(/\/config\.json$/))
686+
.returns(JSON.stringify({ ...defaultConfig, instanceId: 'test' }));
687+
// For all other files, use the original readFileSync
688+
readFileSyncStub
689+
.callsFake((filePath, ...args) => originalReadFileSync(filePath, ...args));
690+
// Create a new ConfigObject instance
691+
const config = new ConfigObject();
692+
assert.strictEqual(config.instanceId, 'test');
693+
});
694+
695+
it('should assert if instanceId is not a string', () => {
696+
// Stub fs.readFileSync to return a specific config.json content that includes instanceId
697+
const originalReadFileSync = fs.readFileSync;
698+
const readFileSyncStub = sinon.stub(fs, 'readFileSync');
699+
// Mock only config.json file
700+
readFileSyncStub
701+
.withArgs(sinon.match(/\/config\.json$/))
702+
.returns(JSON.stringify({ ...defaultConfig, instanceId: 1234 }));
703+
// For all other files, use the original readFileSync
704+
readFileSyncStub
705+
.callsFake((filePath, ...args) => originalReadFileSync(filePath, ...args));
706+
// Create a new ConfigObject instance
707+
assert.throws(() => new ConfigObject());
708+
});
709+
710+
it('should use the instance id set in the env var', () => {
711+
setEnv('CLOUDSERVER_INSTANCE_ID', '123456');
712+
const config = new ConfigObject();
713+
assert.strictEqual(config.instanceId, '123456');
714+
});
715+
716+
it('should assert if instanceId is longer than 6 characters', () => {
717+
setEnv('CLOUDSERVER_INSTANCE_ID', '1234567');
718+
assert.throws(() => new ConfigObject());
719+
});
720+
721+
it('should generate a new instanceId for external cloudserver', () => {
722+
setEnv('HOSTNAME', 'connector-cloudserver-69958b4697-bs5pn');
723+
const config = new ConfigObject();
724+
assert.strictEqual(config.instanceId, 'ebs5pn');
725+
});
726+
727+
it('should generate a new instanceId for internal cloudserver', () => {
728+
setEnv('HOSTNAME', 'internal-cloudserver-54dc76b796-48m2x');
729+
const config = new ConfigObject();
730+
assert.strictEqual(config.instanceId, 'i48m2x');
731+
});
732+
it('should generate a random instanceId if HOSTNAME has an unexpected format', () => {
733+
setEnv('HOSTNAME', 'cldsrv1');
734+
const config = new ConfigObject();
735+
assert.strictEqual(config.instanceId.length, 6);
736+
});
737+
it('should generate a random instanceId if HOSTNAME is not set', () => {
738+
const config = new ConfigObject();
739+
assert.strictEqual(config.instanceId.length, 6);
740+
});
741+
});
671742
});

0 commit comments

Comments
 (0)