Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: Re-enable parallel run of Postgres-backed tests#119

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

Merged
bryphe-coder merged 6 commits intomainfrombryphe/fix/docker-port-overlap
Feb 1, 2022

Conversation

bryphe-coder
Copy link
Contributor

@bryphe-coderbryphe-coder commentedFeb 1, 2022
edited
Loading

@kylecarbs and I were debugging a gnarly postgres issue over the weekend, and unfortunately it looks like it is still coming up occassionally:https://github.com/coder/coder/runs/5014420662?check_suite_focus=true#step:8:35 - so thought this might be a good testing Monday task.

Intermittently, the test would fail with something like a401 - invalid e-mail, or a409 - initial user already created. This was quite surprising, because the tests are designed to spin up their own, isolated database.

We tried a few things to debug this...

Attempt 1: Log out the generated port numbers when running the docker image.

Based on the errors, it seemed like one test must be connecting to another test's database - that would explain why we'd get these conflicts! However, logging out the port number that came from docker always gave a unique number... and we couldn't find evidence of one database connecting to another.

Attempt 2: Store the database in unique, temporary folder.

@kylecarbs and I found that the there was avolume for the postgres data... so@kylecarbs implemented mounting the volume to a unique, per-test temporary folder in#89

It sounded really promising... but unfortunately we hit the issue again!

Attempt 3... this PR

After we hit the failure again, we noticed in thedocker ps logs something quite strange:
image

When the docker image is run - it creates two port bindings, an IPv4 and an IPv6 one. Theseshould be the same - but surprisingly, they can sometimes be different. It isn't deterministic, and seems to be more common when there are multiple containers running. Importantly,they can overlap as in the above image.

Turns out, it seems this is a docker bug:moby/moby#42442 - which may be fixed in newer versions.

To work around this bug, we have to manipulate the port bindings (like you would with-p) at the command line. We can do this withdocker/dockertest, but it means we have to get a free port ahead of time to know which port to map.

With that fix in - thedocker ps is a little more sane:
image

...and hopefully means we can safely run the containers in parallel again.

kylecarbs reacted with heart emoji
@bryphe-coderbryphe-coder self-assigned thisFeb 1, 2022
@codecov
Copy link

codecovbot commentedFeb 1, 2022
edited
Loading

Codecov Report

Merging#119 (02aaf40) intomain (c54d61e) willdecrease coverage by2.58%.
The diff coverage is72.72%.

Impacted file tree graph

@@            Coverage Diff             @@##             main     #119      +/-   ##==========================================- Coverage   74.72%   72.14%   -2.59%==========================================  Files          53       91      +38       Lines         550     3773    +3223       Branches       59       59              ==========================================+ Hits          411     2722    +2311- Misses        127      831     +704- Partials       12      220     +208
FlagCoverage Δ
unittest-go-macos-latest67.89% <ø> (?)
unittest-go-ubuntu-latest70.64% <72.72%> (?)
unittest-go-windows-latest67.55% <ø> (?)
unittest-js74.72% <ø> (ø)
Impacted FilesCoverage Δ
database/postgres/postgres.go70.73% <72.72%> (ø)
httpmw/userparam.go76.66% <0.00%> (ø)
httpapi/httpapi.go68.11% <0.00%> (ø)
database/db.go0.00% <0.00%> (ø)
database/migrate.go52.38% <0.00%> (ø)
httpmw/organizationparam.go69.81% <0.00%> (ø)
peerbroker/listen.go84.80% <0.00%> (ø)
provisionersdk/serve.go63.33% <0.00%> (ø)
peer/conn.go79.42% <0.00%> (ø)
site/embed.go72.31% <0.00%> (ø)
... and29 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatec54d61e...02aaf40. Read thecomment docs.

Copy link
Member

@kylecarbskylecarbs left a comment

Choose a reason for hiding this comment

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

This makes me exceptionally happy!

Just two minor things.

bryphe-coder reacted with hooray emoji
@bryphe-coderbryphe-coder merged commit38867b0 intomainFeb 1, 2022
@bryphe-coderbryphe-coder deleted the bryphe/fix/docker-port-overlap branchFebruary 1, 2022 17:22
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kylecarbskylecarbskylecarbs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bryphe-coder@kylecarbs

[8]ページ先頭

©2009-2025 Movatter.jp