Prevent ingress deletion when endpoint count == 1#2101
Prevent ingress deletion when endpoint count == 1#2101fcrisciani merged 1 commit intomoby:masterfrom
Conversation
We should not delete an ingress network just because its endpoint count drops to 1 (the IP address of the sandbox). This addresses a regression where the ingress sandbox could be deleted on workers when the last container leave said sandbox. Signed-off-by: Chris Telfer <[email protected]>
|
This is intended as the first part of a fix for #2097. |
Codecov Report
@@ Coverage Diff @@
## master #2101 +/- ##
==========================================
+ Coverage 40.38% 40.44% +0.05%
==========================================
Files 139 139
Lines 22356 22356
==========================================
+ Hits 9029 9042 +13
+ Misses 12000 11983 -17
- Partials 1327 1331 +4
Continue to review full report at Codecov.
|
|
LGTM |
|
@ctelfer since this is a regression, could you pls point to the PR that brought in this breakage ? Please edit the description of this PR and clearly mention that this PR is dependent on another fix in moby as you described in #2097 (comment) That also makes it appropriate to justify the lack of test in libnetwork and the need for it in moby/moby. |
|
This regression was introduced with this PR: #2011. |
|
Re: a test in integration-cli ... according to @selansen no further tests are allowed to be added there. They now have to be added to integration/ in moby. Putting a test in integration/ seems reasonable. I do have a concern that the test might be unreliable if there is no way to guarantee that docker has completely cleaned up any endpoints in the ingress network from previous tests. (i.e. the test may have completed but the container/endpoint might still be in the process of removal and so the ingress network reference count could be > 1) This would apply to any test framework that isn't paying close attention to clean-up in every single test. |
|
LGTM |
|
Moby PR for this along with integration test is in moby/moby#36538. |
6 participants
Libnetwork should not delete an ingress network just because its endpoint count
drops to 1 (the IP address of the sandbox). This addresses a regression
where the ingress sandbox could be deleted on workers when the last
container leave said sandbox.
Adding this fix will ultimately require and accompanying moby/moby fix in order to
preserve the ability to remove the ingress network manually. As it turns out, moby
already has the code required to remove the load balancing endpoint in the
daemon.deleteLoadBalancerSandbox() method. Since removing the ingress network
in moby invokes the daemon.releaseIngress() function to help it, the only change
required to reinstate ingress network removal along with this fix is to invoke
deleteLoadBalancerSandbox() within releaseIngress(). This will obviously be in a
moby PR that also adjusts for the libnetwork vendoring (once this fix is in).
Signed-off-by: Chris Telfer [email protected]