Skip to content
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

Reconfigure Mongo replica set when --port differs #7840

Merged
merged 5 commits into from Sep 29, 2016

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Sep 28, 2016

This comment by @abernix explains the problem, and this PR fixes it.

Fixes #7563 and also the bundler-{assets,options} test failures that have been plaguing the Release 1.4.2 branch.

@benjamn benjamn added this to the Release 1.4.2 milestone Sep 28, 2016
@@ -213,8 +213,7 @@ if (process.platform === 'win32') {
var foundPort = parseInt(m[2], 10);
var foundPath = m[3];

if ( (! port || port === foundPort) &&
(! dbDir || dbDir === foundPath)) {
if (! dbDir || dbDir === foundPath) {
Copy link
Member Author

@benjamn benjamn Sep 28, 2016

Choose a reason for hiding this comment

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

Without this change, the only mongod processes we'll find are those using the same port we're trying to use, which is not the case if we've changed the --port.

Since we're still checking the --dbPath, this search won't return any mongod processes that don't belong to this app.

Loading

Copy link
Contributor

@tmeasday tmeasday Sep 29, 2016

Choose a reason for hiding this comment

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

This seems incorrect. findMongoPids is used in two places:

  1. To find the mongo port as findMongoPids(dbDir)
  2. To kill the running mongo as findMongoPids(null, port).

In the first case, this change won't do anything as port is undefined. In the second case, this will now match all mongo processes, as dbDir is null.

I don't understand the reason for this change I guess?

Loading

Copy link
Contributor

@tmeasday tmeasday Sep 29, 2016

Choose a reason for hiding this comment

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

Also, shouldn't we make the same change on line 134 (the windows version). Although looking at that code, I don't see how it works in case 1. above either (it has the reverse problem).

Loading

Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

When I was debugging this issue, I didn't recall running into this issue, but yes, the original check raises some question and as @benjamn says:

Since we're still checking the --dbPath, this search won't return any mongod processes that don't belong to this app.

Loading

Copy link
Member Author

@benjamn benjamn Sep 29, 2016

Choose a reason for hiding this comment

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

@tmeasday The findMongoPids(null, port) version is broken because we pass the new port, though mongod is still running on the old port, which we don't know. There seems to be an old assumption that mongod can run with the same dbPath but different ports, so if you kill the one running on the port you're about to use, then all's well. That's just not true any more, if it ever was.

Loading

if (e.message === 'already initialized') {
yieldingMethod(db.admin(), 'command', {
replSetReconfig: configuration,
force: true,
Copy link
Member Author

@benjamn benjamn Sep 29, 2016

Choose a reason for hiding this comment

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

I believe the use of force: true is safe here because this code runs in development, while Mongo is starting up, before any Meteor code has a chance to modify the database, so the previous primary (now secondary) should be in the same state it was in when the app last shut down.

Reference: https://docs.mongodb.com/manual/reference/command/replSetReconfig/

Loading

Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

Agree force is fine, but perhaps catching this error is all that's necessary?

Loading

Copy link
Member Author

@benjamn benjamn Sep 29, 2016

Choose a reason for hiding this comment

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

Without force: true, the exception indicates that no changes were made against the secondary (because it wasn't primary).

Loading

Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

Sorry, that was poorly worded on my part and separate conversations – yes, force is fine. 😉

Loading

@@ -213,8 +213,7 @@ if (process.platform === 'win32') {
var foundPort = parseInt(m[2], 10);
var foundPath = m[3];

if ( (! port || port === foundPort) &&
(! dbDir || dbDir === foundPath)) {
if (! dbDir || dbDir === foundPath) {
Copy link
Contributor

@tmeasday tmeasday Sep 29, 2016

Choose a reason for hiding this comment

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

This seems incorrect. findMongoPids is used in two places:

  1. To find the mongo port as findMongoPids(dbDir)
  2. To kill the running mongo as findMongoPids(null, port).

In the first case, this change won't do anything as port is undefined. In the second case, this will now match all mongo processes, as dbDir is null.

I don't understand the reason for this change I guess?

Loading

@@ -213,8 +213,7 @@ if (process.platform === 'win32') {
var foundPort = parseInt(m[2], 10);
var foundPath = m[3];

if ( (! port || port === foundPort) &&
(! dbDir || dbDir === foundPath)) {
if (! dbDir || dbDir === foundPath) {
Copy link
Contributor

@tmeasday tmeasday Sep 29, 2016

Choose a reason for hiding this comment

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

Also, shouldn't we make the same change on line 134 (the windows version). Although looking at that code, I don't see how it works in case 1. above either (it has the reverse problem).

Loading

@@ -213,8 +213,7 @@ if (process.platform === 'win32') {
var foundPort = parseInt(m[2], 10);
var foundPath = m[3];

if ( (! port || port === foundPort) &&
(! dbDir || dbDir === foundPath)) {
if (! dbDir || dbDir === foundPath) {
Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

When I was debugging this issue, I didn't recall running into this issue, but yes, the original check raises some question and as @benjamn says:

Since we're still checking the --dbPath, this search won't return any mongod processes that don't belong to this app.

Loading

@@ -537,6 +536,11 @@ var launchMongo = function (options) {
maybeReadyToTalk();
}

if (/[ReplicationExecutor] Locally stored replica set configuration does not have a valid entry for the current node/) {
Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

Hmm... the [ and ] aren't escaped so it would never match the actual line but the regex itself doesn't test against anything (à la .test(data)) so it would be setting replSetReadyToBeInitiated on every log line. Maybe it's actually fine to just always reconfig? Heh.

Loading

Copy link
Member Author

@benjamn benjamn Sep 29, 2016

Choose a reason for hiding this comment

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

Oh, hah, wow, this is terrible, thanks.

Loading

if (e.message === 'already initialized') {
yieldingMethod(db.admin(), 'command', {
replSetReconfig: configuration,
force: true,
Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

Agree force is fine, but perhaps catching this error is all that's necessary?

Loading

benjamn added 4 commits Sep 29, 2016
We try to kill any mongod processes before starting new ones, but this
change kills it when the development server shuts down, too.

Killing mongo on shutdown is particularly important for tests that run
meteor multiple times in a row, and for whatever reason fail to find and
kill running mongod processes on startup, e.g. because the --port has
changed (#7563).

This comment by @glasser seems to suggest this is a reasonable idea:
#2182 (comment)

Fixes #2182 and possibly other related bugs.
@benjamn benjamn force-pushed the reconfigure-mongo-when-port-differs branch from b484edf to 295d3d5 Sep 29, 2016
@benjamn
Copy link
Member Author

@benjamn benjamn commented Sep 29, 2016

Reverted the changes to findMongoPids in favor of a more general solution: 295d3d5

Loading

Copy link
Member

@abernix abernix left a comment

LGTM

Loading

if (e.message === 'already initialized') {
yieldingMethod(db.admin(), 'command', {
replSetReconfig: configuration,
force: true,
Copy link
Member

@abernix abernix Sep 29, 2016

Choose a reason for hiding this comment

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

Sorry, that was poorly worded on my part and separate conversations – yes, force is fine. 😉

Loading

@benjamn benjamn merged commit c829bf5 into devel Sep 29, 2016
4 checks passed
Loading
@abernix abernix mentioned this pull request Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants