Actually starting to use the deallocator to clean up services #2814
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wk8
force-pushed
the
wk8/opt_in_for_soft_delete
branch
from
93c6711 to
07d79cd
Compare
**- What I did** This patch allows users to choose to make removals of services and networks asynchronous, which means that they only get deleted once all the containers belonging to them or using them are safely shut down. **- How I did it** A previous patch (moby#2759) introduced a new component, the deallocator, responsible for cleaning up services and service-level resources. This patch is actually starting to make use of that component. Since the deallocator used to rely on the reaper deleting the tasks belonging to services that had been marked for removal, a previous version of this patch was modifying the task reaper quite heavily to also keep track of such services (needed since otherwise the reaper would fail to clean up all of them, instead keeping some for history tracking purposes). However, it soon appeared that this was not the best approach: * this creates a hidden coupling between the reaper and the deallocator * it's also not the best user experience to suddenly remove all of a service's task history while shutting down, for not apparent reason to the user For these reasons, this patch instead amends the deallocator to also look at tasks status when keeping track of how many alive tasks remain to a service. Part of this test was already reviewed in moby#2778, which subsequently got reverted following the discussion in moby/moby#38525. The main difference between this PR and moby#2778 is that asynchronous removal is an opt-in behaviour here instead of changing the existing behaviour. **- How to test it** Updated tests. **- Description for the changelog** Services & networks can be deleted asynchronously, in which case the request for their removal simply marks them for removal, and they are then actually deleted only when all their tasks are actually shut down. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
wk8
force-pushed
the
wk8/opt_in_for_soft_delete
branch
from
07d79cd to
9f80f21
Compare
Codecov Report
@@ Coverage Diff @@
## master #2814 +/- ##
==========================================
+ Coverage 61.82% 61.88% +0.05%
==========================================
Files 139 137 -2
Lines 22120 22168 +48
==========================================
+ Hits 13676 13718 +42
- Misses 6973 6976 +3
- Partials 1471 1474 +3 |
|
LGTM, but needs a rebase evidently. |
Signed-off-by: Jean Rouge <rougej+github@gmail.com>
|
Thanks @dperny , rebased :) |
Signed-off-by: Jean Rouge <rougej+github@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
This patch allows users to choose to make removals of services and networks
asynchronous, which means that they only get deleted once all the containers
belonging to them or using them are safely shut down.
- How I did it
A previous patch (#2759) introduced a
new component, the deallocator, responsible for cleaning up services and
service-level resources. This patch is actually starting to make use of that
component.
Since the deallocator used to rely on the reaper deleting the tasks belonging
to services that had been marked for removal, a previous version of this patch
was modifying the task reaper quite heavily to also keep track of such services
(needed since otherwise the reaper would fail to clean up all of them, instead
keeping some for history tracking purposes). However, it soon appeared that
this was not the best approach:
task history while shutting down, for not apparent reason to the user
For these reasons, this patch instead amends the deallocator to also look at tasks status when keeping track of how many alive tasks remain to a service.
Part of this test was already reviewed in
#2778, which subsequently got reverted
following the discussion in moby/moby#38525.
The main difference between this PR and #2778 is that asynchronous removal is
an opt-in behaviour here instead of changing the existing behaviour.
- How to test it
Updated tests.
- Description for the changelog
Services & networks can be deleted asynchronously, in which case the request
for their removal simply marks them for removal, and they are then actually
deleted only when all their tasks are actually shut down.
Signed-off-by: Jean Rouge rougej+github@gmail.com
- What I did
- How I did it
- How to test it
- Description for the changelog