REP-5475 - mongodump --internalOnlySourceWritesDoneBarrier #768

rcownie · 2025-03-12T20:02:21Z

REP-5475 - Changes required for the mongodump passthrough testing

New mongodump option --internalOnlySourceWritesDoneBarrier to synchronize with resmoke tests

mongodump/barrier.go implements the wait-until-named-file-exists barrier

rcownie · 2025-03-13T15:03:01Z

https://jira.mongodb.org/browse/REP-5475

mongodump/barrier.go

ajayvijayakumar123

Some suggestions!

ajayvijayakumar123 · 2025-03-17T13:11:33Z

mongodump/barrier.go

+// test infrastructure.  The tests will create the barrier file when they have
+// finshed writes to the source cluster.
+func waitForSourceWritesDoneBarrier(barrierName string) {
+	log.Logvf(log.Always, "waitForSourceWritesDoneBarrier: %s", barrierName)


Suggested change

log.Logvf(log.Always, "waitForSourceWritesDoneBarrier: %s", barrierName)

log.Logvf(log.DebugHigh, "waitForSourceWritesDoneBarrier: %s", barrierName)

I'm not too familiar with how we do logging in the tools. But, should this be at the debug level since we won't use this flag in production?

Can we update the log message so that it's clear that this log means we are entering the blocking state and the later one means we are still in the blocking state.

Done and done

Going to disagree with @ajayvijayakumar123: if this should never run in production this it's actually critical that it is as noisy as possible when running in production. Basically, if we hide the logs when this is running in production then we'll not be able to detect that something which should never run in production is running in production until it breaks.

If possible, we should not have this code compile into production binaries so that it's actually impossible to run in production :).

Ah yup good point. I agree

Undone and undone, and I added a comment to explain the choice of log.Always.

mongodump/barrier.go

ege-mgdb · 2025-03-17T20:53:50Z

mongodump/barrier.go

@@ -0,0 +1,44 @@
+// Copyright (C) MongoDB, Inc. 2014-present.


Is this barrier system only used to provide a signal between mongodump and the integration testing framework?

Yes. The testing framework has to signal when the source cluster is in its final state. Otherwise there will be a race and mongodump may capture the state too early, when it's still being changed.

(this may already be addressed with discussions you had with Dave)
My opinion here is that we should have avoid having code that exists just for testing get compiled into the binaries that are released. So, is it possible to have this mechanism only be a part of the integration testing build?

Go build tags would facilitate this, FYI.

I don’t feel terribly strongly about it, though, because it’s clearly marked as “internal-only”, and even if a user did use it, the worst that happens is that their backup stalls.

This was discussed in the review of the technical design and the consensus was to use a command-line option that is always present, but has an obscure name and isn't visible in the command-line help documentation.

mongodump/barrier.go

ege-mgdb · 2025-03-17T21:03:11Z

mongodump/barrier.go

+			return
+		}
+		if os.IsNotExist(err) {
+			if time.Since(prevLogTime) >= logInterval {


just to make sure I understand, this setup will not print a log statement until 1 minute after it has actually started waiting on the barrier file. And then this works in tandem with the log statement at the top of the function to indicate we have entered and are still in the blocking state?

Yes. We get a log message after 1 minute, and (roughly) once a minute from then on, to say that we're waiting. Enough to see what's happening, but without flooding the logfile with the same message on every poll.

ege-mgdb · 2025-03-17T21:11:39Z

mongodump/barrier.go

+// test infrastructure.  The tests will create the barrier file when they have
+// finshed writes to the source cluster.
+func waitForSourceWritesDoneBarrier(barrierName string) {
+	log.Logvf(log.Always, "waitForSourceWritesDoneBarrier: %s", barrierName)


Can we update the log message so that it's clear that this log means we are entering the blocking state and the later one means we are still in the blocking state.

mongodump/barrier.go

FGasper

This generally looks good. I’ve made some notes; see what you think.

Thanks!

FGasper · 2025-03-24T00:28:39Z

mongodump/barrier.go

+			// Poll for existence of the barrier file every 500msec
+			time.Sleep(500 * time.Millisecond)
+		} else {
+			panic(errors.Wrapf(err, "failed to open barrier file %s", barrierName))


Panicking seems a bit extreme here. Can we return an error instead?

(And use %#q in the error string to get a nicely-quoted name?)

I think panic() is appropriate here, since this code is only used in the resmoke test environment and anything strange happening to that file would indicate some level of chaos in the test. I added a comment to make that clear.

Changed %s to %#q throughout.

General Go practice is to avoid panic() except for indicating purely-internal errors, usually programmer mistakes.

Maybe ping another reviewer for a third opinion?

Made the change.

mongodump/barrier.go

mongodump/mongodump.go

mongodump/barrier.go

tdq45gj · 2025-03-21T21:02:44Z

mongodump/mongodump.go

 		return nil
 	}

+	if !dump.OutputOptions.Oplog && (dump.InputOptions.SourceWritesDoneBarrier != "") {


Since we're checking collection exists before we're sure that writes are done, it seems that if a collection created after verify collection exists but before the barrier will be ignored. Should the barrier be before verifyCollectionExists?

Quibble: verifyCollectionExists is for backups of a specific collection. It’s a no-op otherwise. So either the backup will fail, or that collection will be included.

That said, I agree that the existence check should probably follow the barrier.

For the non-oplog case, moved the waitForBarrier before the check.

I added a comment about the oplog case.

// A test with the combination of // 1. --oplog and // 2. --internalSourceWritesOnly and // 3. a specified collection // 4. the collection didn't exist at the time mongodump was started but is // created later and possibly captured in the oplog // would do this check too early and thus fail. // // That's out of scope for mongodump passthrough testing so we don't try to handle it. exists, err := dump.verifyCollectionExists()

Can we note this as a non-goal on the scope? Can we create a ticket to support this case eventually so we don't lose track of it?

The medium/long-term goal is to get rid of mongodump and mongorestore, by supporting dump-to-file(s) and restore-from-file(s) in mongosync. So I don't think there's a need to plan for extending the mongodump testing.

Gotcha. I still think we note this as a non-goal on the scope to call out something that is supported but not tested

I added "mongodump --collection" as a non-goal on the scope.

FGasper

I’d still like to see the panic converted to a returned error. @ajayvijayakumar123, @tdq45gj? (Erich, you don’t pop up as easily in @ completion …)

mongodump/barrier.go

FGasper · 2025-03-26T19:42:44Z

mongodump/barrier.go

+			// Poll for existence of the barrier file every 500msec
+			time.Sleep(500 * time.Millisecond)
+		} else {
+			panic(errors.Wrapf(err, "failed to open barrier file %s", barrierName))


General Go practice is to avoid panic() except for indicating purely-internal errors, usually programmer mistakes.

Maybe ping another reviewer for a third opinion?

mongodump/barrier.go

ege-mgdb · 2025-03-27T03:54:20Z

I’d still like to see the panic converted to a returned error. @ajayvijayakumar123, @tdq45gj? (Erich, you don’t pop up as easily in @ completion …)

I've kind of intentionally backed off these PRs bc there were getting to be too many people involved and I didn't to create distractions. I'm reading the comments to learn but not asking questions or making comments so that the discussion can stay focused on getting things across the line.

My general opinion on panics or faults is that they should be used when you have entered a state that is unrecoverable (that is, the program has entered a state that should be impossible and where it can no longer trust any decision it makes after that).

ajayvijayakumar123

LGTM % a suggestion !

tdq45gj

LGTM!

tdq45gj · 2025-03-27T17:00:14Z

mongodump/barrier.go

+			time.Sleep(500 * time.Millisecond)
+		} else {
+			// Any other error implies that the resmoke test environment is
+			// irretrievably confused, so it is appropriate to panic here.


The comment might need to be updated

FGasper

LGTM % @tdq45gj’s request to update the comment about panicking.

rcownie force-pushed the rcownie/REP-5475 branch from 9b77f70 to ff0d8d3 Compare March 12, 2025 20:19

REP-5475 - mongodump --internalOnlySourceWritesDoneBarrier

dfe4cb3

rcownie force-pushed the rcownie/REP-5475 branch from ff0d8d3 to dfe4cb3 Compare March 13, 2025 15:01

rcownie marked this pull request as ready for review March 13, 2025 15:04

rcownie requested review from ajayvijayakumar123 and ege-mgdb March 13, 2025 15:30

ege-mgdb reviewed Mar 13, 2025

View reviewed changes

mongodump/barrier.go Outdated Show resolved Hide resolved

ajayvijayakumar123 requested changes Mar 17, 2025

View reviewed changes

ege-mgdb requested changes Mar 17, 2025

View reviewed changes

fixed review issues

aa0ce43

rcownie requested review from ajayvijayakumar123 and ege-mgdb March 21, 2025 17:00

fix formatting

eebae02

ajayvijayakumar123 requested changes Mar 21, 2025

View reviewed changes

mongodump/barrier.go Show resolved Hide resolved

rcownie requested a review from tdq45gj March 21, 2025 20:37

FGasper requested changes Mar 24, 2025

View reviewed changes

tdq45gj reviewed Mar 24, 2025

View reviewed changes

fixes for remaining review issues

4f931b7

rcownie requested review from FGasper, ajayvijayakumar123 and tdq45gj March 26, 2025 16:16

FGasper requested changes Mar 26, 2025

View reviewed changes

barrier code returns error

3a26df4

rcownie requested a review from FGasper March 26, 2025 23:01

ajayvijayakumar123 approved these changes Mar 27, 2025

View reviewed changes

tdq45gj approved these changes Mar 27, 2025

View reviewed changes

FGasper approved these changes Mar 27, 2025

View reviewed changes

edited comment

294dec2

ege-mgdb approved these changes Mar 28, 2025

View reviewed changes

rcownie merged commit 505d575 into master Mar 31, 2025
36 of 38 checks passed

	log.Logvf(log.Always, "waitForSourceWritesDoneBarrier: %s", barrierName)
	log.Logvf(log.DebugHigh, "waitForSourceWritesDoneBarrier: %s", barrierName)

		@@ -0,0 +1,44 @@
		// Copyright (C) MongoDB, Inc. 2014-present.

REP-5475 - mongodump --internalOnlySourceWritesDoneBarrier #768

REP-5475 - mongodump --internalOnlySourceWritesDoneBarrier #768

Uh oh!

Conversation

rcownie commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcownie commented Mar 13, 2025

Uh oh!

Uh oh!

ajayvijayakumar123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FGasper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcownie Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcownie Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajayvijayakumar123 Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rcownie commented Mar 12, 2025 •

edited

Loading

rcownie Mar 26, 2025 •

edited

Loading

rcownie Mar 26, 2025 •

edited

Loading

ajayvijayakumar123 Mar 27, 2025 •

edited

Loading

rcownie Mar 27, 2025 •

edited

Loading

ege-mgdb commented Mar 27, 2025 •

edited

Loading