-
Notifications
You must be signed in to change notification settings - Fork 153
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
Make controller's os.RemoveAll single-threaded #5217
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5217 +/- ##
==========================================
- Coverage 22.83% 22.81% -0.03%
==========================================
Files 420 420
Lines 45302 45316 +14
==========================================
- Hits 10344 10338 -6
- Misses 34163 34183 +20
Partials 795 795 ☔ View full report in Codecov by Sentry. |
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.
Thank you for the fix! I left a comment.
go func() { | ||
for ws := range c.workingDirRemovalCh { |
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.
[Ask] It looks like there is nothing to close c.workingDirRemovalCh
. Does this lead to the goroutine leak?
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'm not sure if we would call this pattern a goroutine leak.
This goroutine's lifetime is the same as that of the piped.
As there is only one controller for each piped, there is also only one instance of this goroutine.
I believe this code is acceptable because the number of goroutines doesn't increase.
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.
This goroutine's lifetime is the same as that of the piped.
As there is only one controller for each piped, there is also only one instance of this goroutine.
I see. I got your point 👍
But I think it would be better to make the lifetime of the channel explicit.
It would be better for the person calling controller.Run to not be aware of that.
WDYT?
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.
It would be better for the person calling controller.Run to not be aware of that.
I got your point. I'll try to make the lifetime explicit.
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.
Fixed in this commit
efa4e6d
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
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 🚀
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 👍
What this PR does / why we need it:
This PR modifies the planner/scheduler cleanup to be single-threaded in order to reduce the file IO loads.
I did a load test with the following scenario and discovered that os.RemoveAll creates a high load for piped:
In this scenario, the piped requires about 20 minutes to complete all deployments.
After this PR, the piped requires about 15 minutes to complete.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: Yes
How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):