Skip to content

feat(NODE-6605): add error message when invalidating primary #4340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
wip
  • Loading branch information
W-A-James committed Nov 20, 2024
commit 31386152a5be157825a9f6ae1199ac1eb9d76903
15 changes: 11 additions & 4 deletions src/sdam/topology_description.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { EJSON, type ObjectId } from '../bson';
import * as WIRE_CONSTANTS from '../cmap/wire_protocol/constants';
import { type MongoError, MongoRuntimeError } from '../error';
import { MongoError, MongoRuntimeError } from '../error';
import { compareObjectId, shuffle } from '../utils';
import { ServerType, TopologyType } from './common';
import { ServerDescription } from './server_description';
Expand Down Expand Up @@ -400,7 +400,9 @@ function updateRsFromPrimary(
// replace serverDescription with a default ServerDescription of type "Unknown"
serverDescriptions.set(
serverDescription.address,
new ServerDescription(serverDescription.address)
new ServerDescription(serverDescription.address, undefined, {
error: new MongoError('stale')
})
);

return [checkHasPrimary(serverDescriptions), setName, maxSetVersion, maxElectionId];
Expand All @@ -416,7 +418,9 @@ function updateRsFromPrimary(
// this primary is stale, we must remove it
serverDescriptions.set(
serverDescription.address,
new ServerDescription(serverDescription.address)
new ServerDescription(serverDescription.address, undefined, {
error: new MongoError('stale')
})
);

return [checkHasPrimary(serverDescriptions), setName, maxSetVersion, maxElectionId];
Expand All @@ -438,7 +442,10 @@ function updateRsFromPrimary(
for (const [address, server] of serverDescriptions) {
if (server.type === ServerType.RSPrimary && server.address !== serverDescription.address) {
// Reset old primary's type to Unknown.
serverDescriptions.set(address, new ServerDescription(server.address));
serverDescriptions.set(
address,
new ServerDescription(server.address, undefined, { error: new MongoError('stale') })
);

// There can only be one primary
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"servers": {
"a:27017": {
"type": "Unknown",
"error": "stale",
"setName": null
},
"b:27017": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"servers": {
"a:27017": {
"type": "Unknown",
"error": {"message": "stale"},
"setName": null,
"electionId": null
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"servers": {
"a:27017": {
"type": "Unknown",
"error": { "message": "stale" },
"setName": null,
"electionId": null
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ phases: [
servers: {
"a:27017": {
type: "Unknown",
error: "stale",
setName: ,
electionId:
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@
"servers": {
"a:27017": {
"type": "Unknown",
"error": {
"message": "stale"
},
"setName": null,
"electionId": null
},
Expand Down
152 changes: 82 additions & 70 deletions test/unit/assorted/server_discovery_and_monitoring.spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
isRecord,
MongoClient,
MongoCompatibilityError,
MongoError,
MongoNetworkError,
MongoNetworkTimeoutError,
MongoServerError,
Expand All @@ -25,6 +26,7 @@ import {
ServerHeartbeatStartedEvent,
ServerHeartbeatSucceededEvent,
ServerOpeningEvent,
squashError,
Topology,
TOPOLOGY_CLOSED,
TOPOLOGY_DESCRIPTION_CHANGED,
Expand Down Expand Up @@ -108,6 +110,7 @@ interface MonitoringOutcome {
interface OutcomeServerDescription {
type?: string;
setName?: string;
error?: { message: string };
setVersion?: number;
electionId?: ObjectId | null;
logicalSessionTimeoutMinutes?: number;
Expand Down Expand Up @@ -268,6 +271,7 @@ function normalizeServerDescription(serverDescription) {
// it as `Unknown`.
serverDescription.type = 'Unknown';
}
serverDescription.error == serverDescription?.error?.message;

return serverDescription;
}
Expand Down Expand Up @@ -322,84 +326,88 @@ async function executeSDAMTest(testData: SDAMTest) {
// connect the topology
await client.connect();

for (const phase of testData.phases) {
// Determine which of the two kinds of phases we're running
if ('responses' in phase && phase.responses != null) {
// phase with responses for hello simulations
for (const [address, hello] of phase.responses) {
client.topology.serverUpdateHandler(new ServerDescription(address, hello));
}
} else if ('applicationErrors' in phase && phase.applicationErrors) {
// phase with applicationErrors simulating error's from network, timeouts, server
for (const appError of phase.applicationErrors) {
// Stub will return appError to SDAM machinery
const checkOutStub = sinon
.stub(ConnectionPool.prototype, 'checkOut')
.callsFake(checkoutStubImpl(appError));

const server = client.topology.s.servers.get(appError.address);

// Run a dummy command to encounter the error
const res = server.command.bind(server)(ns('admin.$cmd'), { ping: 1 }, {});
const thrownError = await res.catch(error => error);

// Restore the stub before asserting anything in case of errors
checkOutStub.restore();

const isApplicationError = error => {
// These errors all come from the withConnection stub
return (
error instanceof MongoNetworkError ||
error instanceof MongoNetworkTimeoutError ||
error instanceof MongoServerError
);
};
expect(
thrownError,
`expected the error thrown to be one of MongoNetworkError, MongoNetworkTimeoutError or MongoServerError (referred to in the spec as an "Application Error") got ${thrownError.name} ${thrownError.stack}`
).to.satisfy(isApplicationError);
try {
for (const phase of testData.phases) {
// Determine which of the two kinds of phases we're running
if ('responses' in phase && phase.responses != null) {
// phase with responses for hello simulations
for (const [address, hello] of phase.responses) {
client.topology.serverUpdateHandler(new ServerDescription(address, hello));
}
} else if ('applicationErrors' in phase && phase.applicationErrors) {
// phase with applicationErrors simulating error's from network, timeouts, server
for (const appError of phase.applicationErrors) {
// Stub will return appError to SDAM machinery
const checkOutStub = sinon
.stub(ConnectionPool.prototype, 'checkOut')
.callsFake(checkoutStubImpl(appError));

const server = client.topology.s.servers.get(appError.address);

// Run a dummy command to encounter the error
const res = server.command.bind(server)(ns('admin.$cmd'), { ping: 1 }, {});
const thrownError = await res.catch(error => error);

// Restore the stub before asserting anything in case of errors
checkOutStub.restore();

const isApplicationError = error => {
// These errors all come from the withConnection stub
return (
error instanceof MongoNetworkError ||
error instanceof MongoNetworkTimeoutError ||
error instanceof MongoServerError
);
};
expect(
thrownError,
`expected the error thrown to be one of MongoNetworkError, MongoNetworkTimeoutError or MongoServerError (referred to in the spec as an "Application Error") got ${thrownError.name} ${thrownError.stack}`
).to.satisfy(isApplicationError);
}
} else if (phase.outcome != null && Object.keys(phase).length === 1) {
// Load Balancer SDAM tests have no "work" to be done for the phase
} else {
expect.fail(ejson`Unknown phase shape - ${phase}`);
}
} else if (phase.outcome != null && Object.keys(phase).length === 1) {
// Load Balancer SDAM tests have no "work" to be done for the phase
} else {
expect.fail(ejson`Unknown phase shape - ${phase}`);
}

if ('outcome' in phase && phase.outcome != null) {
if (isMonitoringOutcome(phase.outcome)) {
// Test for monitoring events
const expectedEvents = convertOutcomeEvents(phase.outcome.events);
if ('outcome' in phase && phase.outcome != null) {
if (isMonitoringOutcome(phase.outcome)) {
// Test for monitoring events
const expectedEvents = convertOutcomeEvents(phase.outcome.events);

expect(events).to.have.length(expectedEvents.length);
for (const [i, actualEvent] of Object.entries(events)) {
const actualEventClone = cloneForCompare(actualEvent);
expect(actualEventClone).to.matchMongoSpec(expectedEvents[i]);
}
} else if (isTopologyDescriptionOutcome(phase.outcome)) {
// Test for SDAM machinery correctly changing the topology type among other properties
assertTopologyDescriptionOutcomeExpectations(client.topology, phase.outcome);
if (phase.outcome.compatible === false) {
// driver specific error throwing
if (testData.description === 'Multiple mongoses with large minWireVersion') {
// TODO(DRIVERS-2250): There is test bug that causes two errors
// this will start failing when the test is synced and fixed
expect(errorsThrown).to.have.lengthOf(2);
expect(events).to.have.length(expectedEvents.length);
for (const [i, actualEvent] of Object.entries(events)) {
const actualEventClone = cloneForCompare(actualEvent);
expect(actualEventClone).to.matchMongoSpec(expectedEvents[i]);
}
} else if (isTopologyDescriptionOutcome(phase.outcome)) {
// Test for SDAM machinery correctly changing the topology type among other properties
assertTopologyDescriptionOutcomeExpectations(client.topology, phase.outcome);
if (phase.outcome.compatible === false) {
// driver specific error throwing
if (testData.description === 'Multiple mongoses with large minWireVersion') {
// TODO(DRIVERS-2250): There is test bug that causes two errors
// this will start failing when the test is synced and fixed
expect(errorsThrown).to.have.lengthOf(2);
} else {
expect(errorsThrown).to.have.lengthOf(1);
}
expect(errorsThrown[0]).to.be.instanceOf(MongoCompatibilityError);
expect(errorsThrown[0].message).to.match(/but this version of the driver/);
} else {
expect(errorsThrown).to.have.lengthOf(1);
// unset or true means no errors should be thrown
expect(errorsThrown).to.be.empty;
}
expect(errorsThrown[0]).to.be.instanceOf(MongoCompatibilityError);
expect(errorsThrown[0].message).to.match(/but this version of the driver/);
} else {
// unset or true means no errors should be thrown
expect(errorsThrown).to.be.empty;
expect.fail(ejson`Unknown outcome shape - ${phase.outcome}`);
}
} else {
expect.fail(ejson`Unknown outcome shape - ${phase.outcome}`);
}

events = [];
errorsThrown = [];
events = [];
errorsThrown = [];
}
}
} finally {
await client.close().catch(squashError);
}
}

Expand Down Expand Up @@ -464,8 +472,12 @@ function assertTopologyDescriptionOutcomeExpectations(
if (WIRE_VERSION_KEYS.has(expectedKey) && expectedValue === null) {
// For wireVersion keys we default to zero instead of null
expect(actualServer).to.have.property(expectedKey, 0);
} else {
} else if (expectedKey !== 'error') {
expect(actualServer).to.have.deep.property(expectedKey, expectedValue);
} else {
// Check that if we have
expect(actualServer).to.have.deep.property(expectedKey).instanceof(MongoError);
expect(actualServer).property(expectedKey).property('message').includes(expectedValue);
}
}
}
Expand Down