Skip to content

Fix potential IP overlapping with node LB#40997

Closed
xinfengliu wants to merge 2 commits into
moby:masterfrom
xinfengliu:fix-ip-overlap
Closed

Fix potential IP overlapping with node LB#40997
xinfengliu wants to merge 2 commits into
moby:masterfrom
xinfengliu:fix-ip-overlap

Conversation

@xinfengliu

@xinfengliu xinfengliu commented May 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #40989

Signed-off-by: Xinfeng Liu [email protected]

- What I did
This PR removes node LB after container exits itself or starting failure to avoid IP overlapping.

- How I did it
Add removeNetworks() in daemon/cluster/executor/container/controller.go

- How to verify it
Manual verification using the test case in #40989
Also added test cases in CI in this PR.

- Description for the changelog

Fix potential IP overlapping with node LB when container exits itself

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah

thaJeztah commented May 19, 2020

Copy link
Copy Markdown
Member

@arkodg @dperny PTAL

@thaJeztah thaJeztah mentioned this pull request May 19, 2020
Closed
@xinfengliu
xinfengliu marked this pull request as draft May 20, 2020 02:22
@xinfengliu

xinfengliu commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

I just think of another scenario of IP overlapping:
Container starting failure (not because of networking)

Example:
docker service create --network net0 --name bb1 --restart-max-attempts=1 --restart-delay=10s busybox non-existing-file

I will make some modification to the codes.

@xinfengliu xinfengliu changed the title Fix potential IP overlapping with node LB when container exits itself Fix potential IP overlapping with node LB May 20, 2020
@xinfengliu
xinfengliu force-pushed the fix-ip-overlap branch 2 times, most recently from 379170b to e351dd5 Compare May 20, 2020 10:24
@thaJeztah thaJeztah added this to the 20.03.0 milestone May 20, 2020
@xinfengliu

xinfengliu commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

If we handle the network cleanup well in Start() and Wait(), then we don't have to remove networks in Shutdown() if the container is not running (either already stopped or just in created state).

This will avoid unnecessary removing networks in below swarmkit scenarios:

  • Desired task status is changed from READY to SHUTDOWN
  • Remove tasks in some situations, e.g. task history limit is reached.

And this will in turn help mitigate widely reported issue: failed to get network during CreateEndpoint (docker/for-linux#888 (comment)) .

@xinfengliu
xinfengliu force-pushed the fix-ip-overlap branch 6 times, most recently from dfb8a3b to a6b64f8 Compare May 21, 2020 07:39
@xinfengliu
xinfengliu marked this pull request as ready for review May 21, 2020 08:48
@xinfengliu

xinfengliu commented May 21, 2020

Copy link
Copy Markdown
Contributor Author

Hi,
I feel it's ready for review. Thanks.
I have tried to consider various scenarios when I made code changes in the past 2 days and did manual tests to verify them.

@xinfengliu xinfengliu mentioned this pull request May 22, 2020
Open
@mightydok

mightydok commented May 24, 2020

Copy link
Copy Markdown

Why we need remove whole network instead of just not give used for lb IP address to new containers?

thaJeztah
thaJeztah requested changes May 25, 2020
View reviewed changes
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment on lines 233 to 230

@thaJeztah thaJeztah May 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and swarmkit will not call Shutdown() after task state is FAIL

Was this by design? Should this be changed? @dperny

@xinfengliu xinfengliu May 25, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is how current swarmkit works:
https://github.com/docker/swarmkit/blob/master/agent/exec/controller.go#L295

	if task.DesiredState >= api.TaskStateShutdown {
		if status.State >= api.TaskStateCompleted {
			return noop()
		}
...

Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/adapter.go Outdated
Comment thread daemon/cluster/executor/container/adapter.go Outdated
@xinfengliu

xinfengliu commented May 25, 2020

Copy link
Copy Markdown
Contributor Author

@mightydok

Why we need remove whole network instead of just not give used for lb IP address to new containers?

We remove the network only when there's no containers running on this network on this node. The logic is in delete() in vendor/github.com/docker/libnetwork/network.go
If there's no container running on the network on the worker node, then this node should not be a peer node for this overlay network. So we need to remove the network on this node.

cpuguy83
cpuguy83 requested changes May 28, 2020
View reviewed changes
Comment thread daemon/cluster/executor/container/adapter.go Outdated
cpuguy83
cpuguy83 requested changes May 29, 2020
View reviewed changes
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/controller.go Outdated
Comment thread daemon/cluster/executor/container/controller.go Outdated
xinfengliu added 2 commits June 3, 2020 06:09
@xinfengliu
Remove node LB to avoid IP overlapping
560fffd 
Remove node LB after task container exits itself or startup failure.
For detailed reason, see moby#40989

Signed-off-by: Xinfeng Liu <[email protected]>
@xinfengliu
add tests for verifying node LB removal
f006b4a 
add test cases for verifying node LB removal under some conditions.
Also add:
- `ServiceWithRestartAttempts` and `ServiceWithHealthCheck` for service creation in tests.
- Waiting conditions for `CompletedTasksCount` and `FailedTasksCount`

Signed-off-by: Xinfeng Liu <[email protected]>
@xinfengliu

xinfengliu commented Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

Added a new commit for tests:

  • test NormalService
  • test StartServiceFail
  • test ServiceCompletion
  • test ServiceExitFail
  • test ServiceHealthCheckFail
  • test ServiceHealthCheckExitEarly

@xinfengliu

xinfengliu commented Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

Hi @thaJeztah
I created another PR #41062 using a different way to address the same issue. I believe that way is much cleaner. Wish you to take a look. Thanks.
(Test cases are added to CI too)

@thaJeztah thaJeztah removed this from the 20.03.0 milestone Jul 16, 2020
@thaJeztah

thaJeztah commented Nov 11, 2021

Copy link
Copy Markdown
Member

Looks like this was addressed in #42432

@thaJeztah thaJeztah closed this Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 requested changes
@thaJeztah thaJeztah thaJeztah requested changes

Assignees

No one assigned

Labels

area/networking Networking kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Node LB not deleted after container exits itself (cause IP overlapping)

4 participants

@xinfengliu @thaJeztah @mightydok @cpuguy83