Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix deadlock waiting for attached-dependencies #10029

Merged
merged 1 commit into from
Dec 7, 2022
Merged

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 29, 2022

What I did
Fixed 2 race conditions as we can configure up to attach additional containers :

  • we need to explicitly tell the printer to stop, as the additional services might still run
  • we missed a way to stop watching containers for restart. This replaces use of a background context with a cancelable one

Related issue
closes #10021

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

@ndeloof ndeloof marked this pull request as draft November 29, 2022 17:21
@ndeloof ndeloof force-pushed the 10021 branch 2 times, most recently from eea1f22 to d4b37dc Compare November 30, 2022 13:54
@ndeloof ndeloof marked this pull request as ready for review November 30, 2022 14:12
Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM other than the errant log that Guillaume already caught

@glours glours enabled auto-merge November 30, 2022 16:23
@milas milas disabled auto-merge December 2, 2022 19:56
@milas
Copy link
Contributor

milas commented Dec 2, 2022

I canceled auto-merge - it looks like the E2E tests are legitimately hanging now and timing out after 15m

@ndeloof ndeloof force-pushed the 10021 branch 2 times, most recently from ca70312 to 4f6ce85 Compare December 5, 2022 10:23
@ndeloof ndeloof marked this pull request as draft December 5, 2022 15:22
@ndeloof
Copy link
Contributor Author

ndeloof commented Dec 5, 2022

still buggy, switching back to draft to prevent merge

@ndeloof ndeloof force-pushed the 10021 branch 9 times, most recently from 5051c1d to 46568f4 Compare December 7, 2022 11:32
@ndeloof ndeloof force-pushed the 10021 branch 5 times, most recently from 0523eff to 9fa1945 Compare December 7, 2022 11:55
@ndeloof ndeloof marked this pull request as ready for review December 7, 2022 11:55
@ndeloof ndeloof force-pushed the 10021 branch 10 times, most recently from 76c3208 to 0ed6ec0 Compare December 7, 2022 15:11
@codecov-commenter
Copy link

Codecov Report

Base: 77.02% // Head: 75.74% // Decreases project coverage by -1.27% ⚠️

Coverage data is based on head (0ed6ec0) compared to base (6b4ad0d).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10029      +/-   ##
==========================================
- Coverage   77.02%   75.74%   -1.28%     
==========================================
  Files           2        2              
  Lines         235      235              
==========================================
- Hits          181      178       -3     
- Misses         48       50       +2     
- Partials        6        7       +1     
Impacted Files Coverage Δ
pkg/e2e/framework.go 73.85% <0.00%> (-1.38%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

explicit API to stop the log printer

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Docker compose >=2.10 stuck on stopping containers started with --attach-dependencies option
5 participants