Skip to content

feat(shell-api): add automerge information to sh.status() MONGOSH-1649 #2422

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.

Sign up for GitHub

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
gagik merged 6 commits into from
Apr 2, 2025
Merged

feat(shell-api): add automerge information to sh.status() MONGOSH-1649 #2422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c3f5dd
feat(shell-api): add automerge information to sh.status() MONGOSH-1649
gagik Mar 31, 2025
cfb1c3a
test: add start and stop automerge tests
gagik Mar 31, 2025
721736a
fix: do not show if not explicitly enabled
gagik Apr 1, 2025
5615062
fix: test automerge separately
gagik Apr 1, 2025
4b531d9
fix: do not assume not explicitly enabled by default for 7.0+
gagik Apr 1, 2025
3496b4b
fix: delete automerge settings before checking for omission
gagik Apr 2, 2025
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
13 changes: 13 additions & 0 deletions packages/shell-api/src/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,15 @@ export async function getPrintableShardStatus(
autosplit === null || autosplit.enabled ? 'yes' : 'no',
};
})(),
(async (): Promise<void> => {
// Is automerge currently enabled, available since >= 7.0
Copy link
Contributor Author

@gagik gagik Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using null scenario to return true to match the mongod behavior. But this is available since 7.0 which means in older versions it'd also similarly say 'yes' which is weird.
The solution to this is to check for the server version and compare with semver but I don't see this being used anywhere

  1. Do we want to introduce this kind of differentiation and hide this field < 7.0?
  2. If yes, what does a server version mean for automerge in sharded cluster? If I understand right it's possible for shards to have i.e. mix of 6.0 and 7.0 Mongo versions, does one get it from i.e. adminDB or something of sort?

Copy link
Collaborator

@addaleax addaleax Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering – what does automerge === null correspond to? Is that the case in which we would want to not print this information?

Copy link
Contributor Author

@gagik gagik Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going off https://github.com/mongodb/mongo/blob/a79ad6aa8b026d511227c4ece3d8c80265578831/src/mongo/shell/utils_sh.js#L264 my guess is that the idea here is that automerge wasn't explicitly enabled but it is enabled by default in 7.x+ so they print it out as such

Copy link
Contributor Author

@gagik gagik Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just confirmed this more or less with start/stop autoMerger is when that'd appear.

We could omit it when the information isn't explicitly set and show only if it has been configured; users looking for it would probably end up starting/stopping automerger so seems reasonable, though a bit of divergence from what we usually do

const automerge = await settingsColl.findOne({ _id: 'automerge' });
if (automerge) {
result.automerge = {
'Currently enabled': automerge.enabled ? 'yes' : 'no',
};
}
})(),
(async (): Promise<void> => {
// Is the balancer currently enabled
const balancerEnabled = await settingsColl.findOne({ _id: 'balancer' });
Expand Down Expand Up @@ -713,6 +722,10 @@ export type ShardingStatusResult = {
autosplit: {
'Currently enabled': 'yes' | 'no';
};
/** Shown if explicitly set, available and enabled by default from 7.0.0 */
automerge?: {
'Currently enabled': 'yes' | 'no';
};
balancer: {
'Currently enabled': 'yes' | 'no';
'Currently running': 'yes' | 'no' | 'unknown';
Expand Down
41 changes: 41 additions & 0 deletions packages/shell-api/src/shard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2468,6 +2468,18 @@ describe('Shard', function () {
]);
});
});
describe('with a 7.0+ server', function () {
skipIfServerVersion(mongos, '< 7.0');

it('displays automerge status, if explicitly set', async function () {
await sh.startAutoMerger();
const result = await sh.status();

expect(result.value.automerge).to.deep.equal({
'Currently enabled': 'yes',
});
});
});
});
describe('turn on sharding', function () {
it('enableSharding for a db', async function () {
Expand Down Expand Up @@ -2516,6 +2528,35 @@ describe('Shard', function () {
);
});
});
describe('automerge', function () {
it('not shown if sh.status() if not explicitly enabled', async function () {
// It might be explicitly set from 7.0
skipIfServerVersion(mongos, '>= 7.0');

// Ensure no previous automerge settings are present
await instanceState.currentDb
.getSiblingDB('config')
.getCollection('settings')
.deleteOne({ _id: 'automerge' });

expect((await sh.status()).value.automerge).is.undefined;
Copy link
Contributor Author

@gagik gagik Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works but does kind of mess with the sh.status integrity a little as we don't really hide fields beyond that.

I'm unsure how problematic would determining the implicit default status based on the version would be but that seems to be the only other option (or setting the status to 'unknown' but that is also a bit weird and would be shown to a lot of users by default). If we went that route, the version would probably be derived from the admin DB right?

Copy link
Collaborator

@addaleax addaleax Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we went that route, the version would probably be derived from the admin DB right?

The server version? The only reliable version that clients are supposed to make decisions on is the wire version reported by hello

Copy link
Contributor Author

@gagik gagik Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm okay, if omitting the field is fine then we could just go with that.
Thinking about it more, it's also a weird precedent to assume that because it's a certain version then lack of a setting means it is some set default from our end

});
describe('from 7.0', function () {
skipIfServerVersion(mongos, '< 7.0'); // Available from 7.0
it('stops correctly', async function () {
expect((await sh.stopAutoMerger()).acknowledged).to.equal(true);
expect(
((await sh.status()).value.automerge ?? {})['Currently enabled']
).to.equal('no');
});
it('enables correctly', async function () {
expect((await sh.startAutoMerger()).acknowledged).to.equal(true);
expect(
((await sh.status()).value.automerge ?? {})['Currently enabled']
).to.equal('yes');
});
});
});
describe('autosplit', function () {
skipIfServerVersion(mongos, '> 6.x'); // Auto-splitter is removed in 7.0
it('disables correctly', async function () {
Expand Down