- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: retry embedded postgres port allocation#20371
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // EmbeddedPostgres.Start in case of a race condition where the port we quickly | ||
| // listen on and close in embeddedPostgresURL() is not free by the time the embedded | ||
| // postgres starts up. | ||
| maxAttempts:=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is the intention here to reduce the number of flakes in CI?
If so, I'd suggest usingos.Getenv("CI") == "true" for setting this conditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes. I have updated this comment to more clearly indicate the motivation.
I have also adopted the CI environment variable check, TIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought about this a little more and there's actually a better way:flag.Lookup("test.v") != nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
While looking up how that works under the hood, I also discoveredtesting.Testing().https://pkg.go.dev/testing#Testing. Seems to be the stdlib approach for this problem. Some context I foundhttps://redirect.github.com/golang/go/issues/52600
I've adopted the stdlib approach
cli/server.go Outdated
| } | ||
| returnconnectionURL,ep.Stop,nil | ||
| return"",nil,xerrors.Errorf("failed to start built-in PostgreSQL after %d attempts. Optionally, specify an external deployment with `--postgres-url`: %w",maxAttempts,startErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should link tohttps://coder.com/docs/tutorials/external-database here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added!
Uh oh!
There was an error while loading.Please reload this page.
cli/server.go Outdated
| ) | ||
| // Since a retry is needed, we wipe the port stored here at the beginning of the loop. | ||
| _=cfg.PostgresPort().Delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If a coder admin relies on embedded postgres in their production deployment, and they previously started it and depend on the port being stable, this line will change the port assignment when embedded postgres fails to start for any reason. I'd prefer that we not change the port stability behavior in this PR.
I like Cian's suggestion to limit the retry behavior to CI environments - an easy fix would be to put this line insideif (os.Getenv("CI") == "true") { ... }.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good thinking! I've adopted Cian's suggestion for this.
Sometimes tests would fail because the port embedded postgres tries to useis already in use. This is because there's no way to tell postgres to usean ephemeral port. This change adds retries to starting embedded postgreswhen the port is not explicitly defined (e.g. tests), which should rid, orat least significantly reduce, these flakes.
b4a281d to25e8620Compare
johnstcn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, one suggestion for improvement which will handle running tests outside of CI as well.
| // EmbeddedPostgres.Start in case of a race condition where the port we quickly | ||
| // listen on and close in embeddedPostgresURL() is not free by the time the embedded | ||
| // postgres starts up. | ||
| maxAttempts:=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought about this a little more and there's actually a better way:flag.Lookup("test.v") != nil.
e73f9d3 intomainUh oh!
There was an error while loading.Please reload this page.
Sometimes tests would fail because the port embedded postgres tries to use is already in use. This is because there's no way to tell postgres to use an ephemeral port. This change adds retries to starting embedded postgres when the port is not explicitly defined (e.g. tests) which should rid of, or at least significantly reduce, these flakes.
coder/internal#658