fix ambiguous networks #1831

BenTheElder · 2020-09-01T05:06:43Z

Rough algorithm:

check if network exists if it does, end here
create network
check if there are now > 1 networks
sort networks based on:
- # of attached containers (available when inspecting the network)
- CreatedAt (creation timestamp, standard docker object metadata)
- Id comparison (dockerd assigns UIDs as the primary identifier centrally)
delete N-1 networks, keeping the one that sorts first

This ensures that we will remove the ambiguous extra networks when we have two kind create cluster "win the race" of docker network create and simultaneously create identically named networks within the timeframe that docker doesn't catch that the name already exists during the internal non-atomic check for an existing network by the name.

This should be safe because we deterministically sort based on "is this network being used somehow" then "when was this network created" and finally if those are identical on the UID docker assigns them.

TODO:

I need to rework the network list / sort logic to list then inspect and sort by # of attached containers, currently it only sorts by ID and CreatedAt
[ ] There's still perhaps a race when doing the cleanup, where process A and B win the race, A wins it faster and somehow continues on to creating containers attached to this while B is going to cleanup. I'm not sure if this can actually happen and it's difficult to reproduce, but if it did we might get an error during container creation, in which case we should retry because the cleanup is coming. I think this isn't feasible as in all my experiments when I pull out the network logic and "win the race" by running it simultaneously in two goroutines I see that the following list check observes both networks and handles it, but this is probably still worth adding as a mitigation

EDIT: The second case should not happen now. If somehow I'm wrong it will be a simple mitigation in the future though and this already provides a significant improvement (complete?) in avoiding #1596.

k8s-ci-robot · 2020-09-01T05:06:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2020-09-01T05:16:38Z

pkg/cluster/internal/providers/docker/network.go

+
+	// deterministically sort networks
+	// NOTE: THIS PART IS IMPORTANT!
+	// TODO(fixme): we should be sorting on active usage first!


this TODO is the one thing keeping this WIP.
I will come back and rework this later. the overall code will be more or less the same otherwise.

aojea · 2020-09-01T16:23:57Z

let's verify it

diff --git a/pkg/cluster/internal/providers/docker/network_test.go b/pkg/cluster/internal/providers/docker/network_test.go
index 789a0d93..a5fb1d2b 100644
--- a/pkg/cluster/internal/providers/docker/network_test.go
+++ b/pkg/cluster/internal/providers/docker/network_test.go
@@ -19,8 +19,46 @@ package docker
 import (
        "fmt"
        "testing"
+
+       "sigs.k8s.io/kind/pkg/exec"
 )
 
+func TestEnsureNetworkConcurrent(t *testing.T) {
+       defer func() {
+               cmd := exec.Command(
+                       "docker", "network", "rm", "test-kind",
+               )
+               cmd.Run()
+       }()
+
+       // Create multiple networks concurrenctly
+       errCh := make(chan error, 3)
+       for i := 0; i < 3; i++ {
+               go func() {
+                       errCh <- ensureNetwork("test-kind")
+               }()
+       }
+       for i := 0; i < 3; i++ {
+               if err := <-errCh; err != nil {
+                       t.Errorf("error creating network: %v", err)
+               }
+       }
+
+       cmd := exec.Command(
+               "docker", "network", "ls",
+               "--filter=name=^test-kind$",
+               "--format={{.Name}}",
+       )
+
+       lines, err := exec.OutputLines(cmd)
+       if err != nil {
+               t.Errorf("obtaining the docker networks")
+       }
+       if len(lines) != 1 {
+               t.Errorf("wrong number of networks created: %d", len(lines))
+       }
+}
+
 func Test_generateULASubnetFromName(t *testing.T) {
        t.Parallel()
        cases := []struct {

aojea · 2020-09-01T16:25:06Z

maybe the name has not to be unique but the subnet should be

=== RUN   TestEnsureNetworkConcurrent
    TestEnsureNetworkConcurrent: /home/aojea/go/src/sigs.k8s.io/kind/pkg/cluster/internal/providers/docker/network_test.go:43: error creating network: command "docker network create -d=bridge -o com.docker.network.bridge.enable_ip_masquerade=true --ipv6 --subnet fc00:3587:1532:ea5a::/64 test-kind" failed with error: exit status 1
    TestEnsureNetworkConcurrent: /home/aojea/go/src/sigs.k8s.io/kind/pkg/cluster/internal/providers/docker/network_test.go:43: error creating network: command "docker network create -d=bridge -o com.docker.network.bridge.enable_ip_masquerade=true --ipv6 --subnet fc00:3587:1532:ea5a::/64 test-kind" failed with error: exit status 1
--- FAIL: TestEnsureNetworkConcurrent (0.36s)
FAIL
coverage: 15.7% of statements
exit status 1

pkg/cluster/internal/providers/docker/network.go

BenTheElder · 2020-09-01T17:11:03Z

#1831 (comment)

yeah, so AFAICT this only happens when we use the default IPAM (AKA the no-IPv6 case).

EDIT: ignore previous note here. see integration test discussion below.

aojea · 2020-09-01T17:17:20Z

I'm not overly inclined to add unit tests that depend on docker / aren't self-contained just yet, we probably want to consider having an integration test pattern similar to kubernetes vs our unit and e2e testing.

not asking to include it , just to run it before merge to be sure, it is not working for me ... I couldn't spend much time though

BenTheElder · 2020-09-09T07:04:13Z

picking this back up now.

also remove debug message

BenTheElder · 2020-09-11T07:53:58Z

testing with go test -count=40 -run ^TestIntegrationEnsureNetworkConcurrent -v ./pkg/cluster/internal/providers/docker is cropping up the odd occasion that no network exists when all the ensure calls are done. I haven't figured out that race yet.

BenTheElder · 2020-09-11T08:08:47Z

I think it may have been in the timestamp parse/comparison. Since dropping that I've not had any failures in 160 iterations.

BenTheElder · 2020-09-11T08:13:14Z

pkg/cluster/internal/providers/docker/network_test.go

 )

+func TestIntegrationEnsureNetworkConcurrent(t *testing.T) {


We can consider reworking the make targets based on this pattern in a follow-up.

to run only "unit": go test -short ...
to run only "integration": go test -run ^TestIntegration ...

pkg/cluster/internal/providers/docker/network.go

BenTheElder · 2020-09-11T17:51:00Z

/retest
CI flakes from being on the legacy build cluster. We should talk to CI signal about moving KIND testing over with the other jobs setting resource limits.

BenTheElder · 2020-09-11T17:51:34Z

/retitle fix ambiguous networks

amwat

Great! This should mitigate the main ambiguous docker network issue 👍
The other remaining possibilities of race conditions seem like weird corner cases which we'll most likely not run into in practice.

pkg/cluster/internal/providers/docker/network.go

aojea · 2020-09-11T22:43:31Z

SGTM
just suggesting to squash some of the first commits so we can have a safe revert if necessary, not criticizing the PR, just the opposite, I think that this is a big improvement that tries to solve a well known docker broken behavior

BenTheElder · 2020-09-11T22:50:46Z

currently refactoring to make #1831 (comment) a bit cleaner to address.

initially I didn't want to expose the inspect details but I think it's worth it to break these up and make them more testable, upon reflection.

…up and docs

BenTheElder · 2020-09-12T19:07:00Z

/retest

k8s-ci-robot · 2020-09-12T20:14:56Z

@BenTheElder: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kind-conformance-parallel-ga-only	`0fc4229`	link	`/test pull-kind-conformance-parallel-ga-only`
pull-kind-conformance-parallel-1-15	`0fc4229`	link	`/test pull-kind-conformance-parallel-1-15`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

amwat

/lgtm

BenTheElder · 2020-09-14T19:09:10Z

I profiled this before the last commit and improved the performance slightly, the happy path is just as cheap as before now

We're going to need to do something about those two CI jobs, but they're unrelated to this PR.

BenTheElder added 3 commits August 31, 2020 21:40

remove duplicated networks

30d86bb

add important TODO

8756a91

ignore network already exists errors

c57f4c3

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 1, 2020

k8s-ci-robot requested review from amwat and neolit123 September 1, 2020 05:06

k8s-ci-robot added the area/provider/docker Issues or PRs related to docker label Sep 1, 2020

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2020

BenTheElder commented Sep 1, 2020

View reviewed changes

aojea reviewed Sep 1, 2020

View reviewed changes

pkg/cluster/internal/providers/docker/network.go Show resolved Hide resolved

aojea reviewed Sep 1, 2020

View reviewed changes

pkg/cluster/internal/providers/docker/network.go Show resolved Hide resolved

BenTheElder mentioned this pull request Sep 1, 2020

Podman network #1790

Merged

BenTheElder added 3 commits September 10, 2020 23:27

improve handling of pool overlap and concurrent removal

4e41dc4

also remove debug message

add network integration test

017b19b

use inspect to sort networks

37c741d

drop parsing / comparing created time

4a98c10

BenTheElder commented Sep 11, 2020

View reviewed changes

explicitly ignore best-effort cleanup in integration test

5be1881

aojea reviewed Sep 11, 2020

View reviewed changes

pkg/cluster/internal/providers/docker/network.go Show resolved Hide resolved

k8s-ci-robot changed the title ~~WIP: fix ambiguous networks~~ fix ambiguous networks Sep 11, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2020

amwat reviewed Sep 11, 2020

View reviewed changes

pkg/cluster/internal/providers/docker/network.go Outdated Show resolved Hide resolved

aojea reviewed Sep 11, 2020

View reviewed changes

pkg/cluster/internal/providers/docker/network.go Outdated Show resolved Hide resolved

fix minor parsing bug, add unit test for network sorting, minor clean…

0fc4229

…up and docs

amwat reviewed Sep 14, 2020

View reviewed changes

k8s-ci-robot assigned amwat Sep 14, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2020

BenTheElder merged commit 295318a into kubernetes-sigs:master Sep 14, 2020

BenTheElder deleted the no-dupe-networks branch September 14, 2020 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ambiguous networks #1831

fix ambiguous networks #1831

BenTheElder commented Sep 1, 2020 •

edited

Loading

k8s-ci-robot commented Sep 1, 2020

BenTheElder Sep 1, 2020

aojea commented Sep 1, 2020

aojea commented Sep 1, 2020

BenTheElder commented Sep 1, 2020 •

edited

Loading

aojea commented Sep 1, 2020

BenTheElder commented Sep 9, 2020

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

BenTheElder Sep 11, 2020 •

edited

Loading

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

amwat left a comment

aojea commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 12, 2020

k8s-ci-robot commented Sep 12, 2020

amwat left a comment

BenTheElder commented Sep 14, 2020

		)

		func TestIntegrationEnsureNetworkConcurrent(t *testing.T) {

fix ambiguous networks #1831

fix ambiguous networks #1831

Conversation

BenTheElder commented Sep 1, 2020 • edited Loading

k8s-ci-robot commented Sep 1, 2020

BenTheElder Sep 1, 2020

Choose a reason for hiding this comment

aojea commented Sep 1, 2020

aojea commented Sep 1, 2020

BenTheElder commented Sep 1, 2020 • edited Loading

aojea commented Sep 1, 2020

BenTheElder commented Sep 9, 2020

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

BenTheElder Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

amwat left a comment

Choose a reason for hiding this comment

aojea commented Sep 11, 2020

BenTheElder commented Sep 11, 2020

BenTheElder commented Sep 12, 2020

k8s-ci-robot commented Sep 12, 2020

amwat left a comment

Choose a reason for hiding this comment

BenTheElder commented Sep 14, 2020

BenTheElder commented Sep 1, 2020 •

edited

Loading

BenTheElder commented Sep 1, 2020 •

edited

Loading

BenTheElder Sep 11, 2020 •

edited

Loading