fix(shell-api): improve rs.reconfig helper for 5.0 changes MONGOSH-785

packages/shell-api/src/replica-set.spec.ts

@@ -1,5 +1,5 @@
 import { CommonErrors, MongoshInvalidInputError, MongoshRuntimeError } from '@mongosh/errors';
-import { bson, FindCursor as ServiceProviderCursor, ServiceProvider, Document } from '@mongosh/service-provider-core';
+import { bson, Document, FindCursor as ServiceProviderCursor, ServiceProvider } from '@mongosh/service-provider-core';
 import chai, { expect } from 'chai';
 import { EventEmitter } from 'events';
 import semver from 'semver';
@@ -17,7 +17,7 @@ import {
 } from './enums';
 import { signatures, toShellResult } from './index';
 import Mongo from './mongo';
-import ReplicaSet, { ReplSetMemberConfig, ReplSetConfig } from './replica-set';
+import ReplicaSet, { ReplSetConfig, ReplSetMemberConfig } from './replica-set';
 import ShellInternalState, { EvaluationListener } from './shell-internal-state';
 chai.use(sinonChai);
 
@@ -186,7 +186,7 @@ describe('ReplicaSet', () => {
     });
 
     describe('reconfig', () => {
-      const configDoc = {
+      const configDoc: Partial<ReplSetConfig> = {
         _id: 'my_replica_set',
         members: [
           { _id: 0, host: 'rs1.example.net:27017' },
@@ -237,6 +237,69 @@ describe('ReplicaSet', () => {
           }
         );
       });
+
+      describe('retry on errors', () => {
+        let oldConfig: Partial<ReplSetConfig>;
+        let reconfigCalls: ReplSetConfig[];
+        let reconfigResults: Document[];
+        let sleepStub: any;
+
+        beforeEach(() => {
+          sleepStub = sinon.stub();
+          internalState.shellApi.sleep = sleepStub;
+          reconfigCalls = [];
+
+          serviceProvider.runCommandWithCheck.callsFake(async(db: string, cmd: Document) => {
+            if (cmd.replSetGetConfig) {
+              return { config: { ...oldConfig, version: oldConfig.version ?? 1 } };
+            }
+            if (cmd.replSetReconfig) {
+              const result = reconfigResults.shift();
+              reconfigCalls.push(deepClone(cmd.replSetReconfig));
+              if (result.ok) {
+                return result;
+              }
+              oldConfig = { ...oldConfig, version: (oldConfig.version ?? 1) + 1 };
+              throw new Error(`Reconfig failed: ${JSON.stringify(result)}`);
+            }
+          });
+        });
+
+        it('does three reconfigs if the first two fail due to known issue', async() => {
+          oldConfig = deepClone(configDoc);
+          reconfigResults = [ { ok: 0 }, { ok: 0 }, { ok: 1 } ];
+
+          const origConfig = deepClone(configDoc);
+          await rs.reconfig(configDoc);
+          expect(reconfigCalls).to.deep.equal([
+            { ...origConfig, version: 2 },
+            { ...origConfig, version: 3 },
+            { ...origConfig, version: 4 }
+          ]);
+          expect(sleepStub).to.have.been.calledWith(1000);
+          expect(sleepStub).to.have.been.calledWith(1300);
+        });
+
+        it('gives up after a number of attempts', async() => {
+          oldConfig = deepClone(configDoc);
+          reconfigResults = [...Array(20).keys()].map(() => ({ ok: 0 }));
+          try {
+            await rs.reconfig(configDoc);
+            expect.fail('missed exception');
+          } catch (err) {
+            expect(err.message).to.equal('Reconfig failed: {"ok":0}');
+          }
+          expect(evaluationListener.onPrint).to.have.been.calledWith([
+            await toShellResult('Reconfig did not succeed yet, starting new attempt...')
+          ]);
+          const totalSleepLength = sleepStub.getCalls()
+            .map(({ firstArg }) => firstArg)
+            .reduce((x, y) => x + y, 0);
+          // Expect to spend about a minute sleeping here.
+          expect(totalSleepLength).to.be.closeTo(60_000, 5_000);
+          expect(reconfigCalls).to.have.lengthOf(12);
+        });
+      });
     });
     describe('status', () => {
       it('calls serviceProvider.runCommandWithCheck', async() => {
@@ -727,7 +790,7 @@ describe('ReplicaSet', () => {
           await toShellResult('Running second reconfig to give member at index 2 { priority: 1 }')
         ]);
         expect(evaluationListener.onPrint).to.have.been.calledWith([
-          await toShellResult('Second reconfig did not succeed yet, starting new attempt...')
+          await toShellResult('Reconfig did not succeed yet, starting new attempt...')
         ]);
         expect(evaluationListener.onPrint).to.have.been.calledWith([
           await toShellResult('Second reconfig did not succeed, giving up')
@@ -755,8 +818,8 @@ describe('ReplicaSet', () => {
     let cfg: Partial<ReplSetConfig>;
     let additionalServer: MongodSetup;
     let serviceProvider: CliServiceProvider;
-    let internalState;
-    let rs;
+    let internalState: ShellInternalState;
+    let rs: ReplicaSet;
 
     before(async function() {
       this.timeout(100_000);
@@ -843,9 +906,7 @@ describe('ReplicaSet', () => {
     });
 
     describe('add member', () => {
-      // TODO: Fix these tests? They are currently failing with
-      // MongoError: Cannot run replSetReconfig because the node is currently updating its configuration
-      skipIfServerVersion(srv0, '> 4.4');
+      skipIfServerVersion(srv0, '< 4.4');
       it('adds a regular member to the config', async() => {
         const version = (await rs.conf()).version;
         const result = await rs.add(`${await additionalServer.hostport()}`);
@@ -855,6 +916,12 @@ describe('ReplicaSet', () => {
         expect(conf.version).to.equal(version + 1);
       });
       it('adds a arbiter member to the config', async() => {
+        if (semver.gte(await internalState.currentDb.version(), '4.4.0')) { // setDefaultRWConcern is 4.4+ only
+          await internalState.currentDb.getSiblingDB('admin').runCommand({
+            setDefaultRWConcern: 1,
+            defaultWriteConcern: { w: 'majority' }
+          });
+        }
         const version = (await rs.conf()).version;
         const result = await rs.addArb(`${await additionalServer.hostport()}`);
         expect(result.ok).to.equal(1);
@@ -871,9 +938,6 @@ describe('ReplicaSet', () => {
     });
 
     describe('remove member', () => {
-      // TODO: Fix these tests? They are currently failing with
-      // MongoError: Cannot run replSetReconfig because the node is currently updating its configuration
-      skipIfServerVersion(srv0, '> 4.4');
       it('removes a member of the config', async() => {
         const version = (await rs.conf()).version;
         const result = await rs.remove(cfg.members[2].host);

packages/shell-api/src/replica-set.ts

@@ -1,19 +1,17 @@
+import { CommonErrors, MongoshDeprecatedError, MongoshInvalidInputError, MongoshRuntimeError } from '@mongosh/errors';
+import { redactCredentials } from '@mongosh/history';
+import { Document } from '@mongosh/service-provider-core';
+import Mongo from './mongo';
 import Database from './database';
 import {
-  shellApiClassDefault,
-  returnsPromise,
   deprecated,
+  returnsPromise,
+  shellApiClassDefault,
   ShellApiWithMongoClass
 } from './decorators';
-import {
-  Document
-} from '@mongosh/service-provider-core';
 import { asPrintable } from './enums';
 import { assertArgsDefinedType } from './helpers';
-import { CommonErrors, MongoshDeprecatedError, MongoshInvalidInputError, MongoshRuntimeError } from '@mongosh/errors';
 import { CommandResult } from './result';
-import { redactCredentials } from '@mongosh/history';
-import { Mongo } from '.';
 
 export type ReplSetMemberConfig = {
   _id: number;
@@ -106,29 +104,53 @@ export default class ReplicaSet extends ShellApiWithMongoClass {
     assertArgsDefinedType([ config, options ], ['object', [undefined, 'object']], 'ReplicaSet.reconfig');
     this._emitReplicaSetApiCall('reconfig', { config, options });
 
-    const conf = await this._getConfig();
+    const runReconfig = async(): Promise<Document> => {
+      const conf = await this._getConfig();
+      config.version = conf.version ? conf.version + 1 : 1;
+      config.protocolVersion ??= conf.protocolVersion; // Needed on mongod 4.0.x
+      const cmd = { replSetReconfig: config, ...options };
+      return await this._database._runAdminCommand(cmd);
+    };
 
-    config.version = conf.version ? conf.version + 1 : 1;
-    config.protocolVersion ??= conf.protocolVersion; // Needed on mongod 4.0.x
-    const cmd = { replSetReconfig: config, ...options };
+    let result: [ 'error', Error ] | [ 'success', Document ] = [ 'success', {} ];
+    let sleepInterval = 1000;
+    for (let i = 0; i < 12; i++) {
+      try {
+        if (result[0] === 'error') {
+          // Do a mild exponential backoff. If it's been a while since the last
+          // update, also tell the user that we're actually still working on
+          // the reconfig.
+          await this._internalState.shellApi.sleep(sleepInterval);
+          sleepInterval *= 1.3;
+          if (sleepInterval > 2500) {
+            await this._internalState.shellApi.print('Reconfig did not succeed yet, starting new attempt...');
+          }
+        }
+        result = [ 'success', await runReconfig() ];
+        break;
+      } catch (err) {
+        result = [ 'error', err ];
+      }
+    }
 
-    return this._database._runAdminCommand(cmd);
+    if (result[0] === 'error') {
+      throw result[1];
+    }
+    return result[1];
   }
 
   /**
    * A more involved version specifically for transitioning from a Primary-Arbiter
    * to a Primary-Secondary-Arbiter set (PA to PSA for short).
    */
   @returnsPromise
-  // eslint-disable-next-line complexity
   async reconfigForPSASet(newMemberIndex: number, config: Partial<ReplSetConfig>, options = {}): Promise<Document> {
     assertArgsDefinedType(
       [ newMemberIndex, config, options ],
       [ 'number', 'object', [undefined, 'object'] ],
       'ReplicaSet.reconfigForPSASet');
     this._emitReplicaSetApiCall('reconfigForPSASet', { newMemberIndex, config, options });
     const print = (msg: string) => this._internalState.shellApi.print(msg);
-    const sleep = (duration: number) => this._internalState.shellApi.sleep(duration);
 
     // First, perform some validation on the combination of newMemberIndex + config.
     const newMemberConfig = config.members?.[newMemberIndex];
@@ -180,38 +202,15 @@ export default class ReplicaSet extends ShellApiWithMongoClass {
     await print(`Running second reconfig to give member at index ${newMemberIndex} { priority: ${newMemberPriority} }`);
     newMemberConfig.priority = newMemberPriority;
 
-    // If the first reconfig added a new node, the second config will not succeed until the
-    // automatic reconfig to remove the 'newlyAdded' field is completed. Retry the second reconfig
-    // until it succeeds in that case.
-    let result: [ 'error', Error ] | [ 'success', Document ] = [ 'success', {} ];
-    let sleepInterval = 1000;
-    for (let i = 0; i < 12; i++) {
-      try {
-        if (result[0] === 'error') {
-          // Do a mild exponential backoff. If it's been a while since the last
-          // update, also tell the user that we're actually still working on
-          // the reconfig.
-          await sleep(sleepInterval);
-          sleepInterval *= 1.3;
-          if (sleepInterval > 2500) {
-            await print('Second reconfig did not succeed yet, starting new attempt...');
-          }
-        }
-        result = [ 'success', await this.reconfig(config, options) ];
-        break;
-      } catch (err) {
-        result = [ 'error', err ];
-      }
-    }
-
-    if (result[0] === 'error') {
+    try {
+      return await this.reconfig(config, options);
+    } catch (e) {
       // If this did not work out, print the attempted command to give the user
       // a chance to complete the second reconfig manually.
       await print('Second reconfig did not succeed, giving up');
       await print(`Attempted command: rs.reconfig(${JSON.stringify(config, null, '  ')}, ${JSON.stringify(options)})`);
-      throw result[1];
+      throw e;
     }
-    return result[1];
   }
 
   @returnsPromise

testing/integration-testing-hooks.ts

@@ -461,9 +461,7 @@ export function startTestCluster(...argLists: string[][]): MongodSetup[] {
 }
 
 function skipIfVersion(test: any, testServerVersion: string, semverCondition: string): void {
-  // Strip -rc.0, -alpha, etc. from the server version because semver rejects those otherwise.
-  testServerVersion = testServerVersion.replace(/-.*$/, '');
-  if (semver.satisfies(testServerVersion, semverCondition)) {
+  if (semver.satisfies(testServerVersion, semverCondition, { includePrerelease: true })) {
     test.skip();
   }
 }