From 0fc42296bfd4653f70acd6086ff83fd40dcb70e2 Mon Sep 17 00:00:00 2001 From: Benjamin Elder Date: Sat, 12 Sep 2020 00:53:41 -0700 Subject: [PATCH] fix minor parsing bug, add unit test for network sorting, minor cleanup and docs --- .../internal/providers/docker/network.go | 100 ++++++++++------- .../internal/providers/docker/network_test.go | 102 +++++++++++++++++- 2 files changed, 163 insertions(+), 39 deletions(-) diff --git a/pkg/cluster/internal/providers/docker/network.go b/pkg/cluster/internal/providers/docker/network.go index c99cc1cd94..d73347294c 100644 --- a/pkg/cluster/internal/providers/docker/network.go +++ b/pkg/cluster/internal/providers/docker/network.go @@ -76,7 +76,8 @@ func ensureNetwork(name string) error { if isIPv6UnavailableError(err) { // only one attempt, IPAM is automatic in ipv4 only return createNetworkNoDuplicates(name, "") - } else if isPoolOverlapError(err) { + } + if isPoolOverlapError(err) { // pool overlap suggests perhaps another process created the network // check if network exists already and remove any duplicate networks exists, err := checkIfNetworkExists(name) @@ -100,7 +101,8 @@ func ensureNetwork(name string) error { if err == nil { // success! return nil - } else if isPoolOverlapError(err) { + } + if isPoolOverlapError(err) { // pool overlap suggests perhaps another process created the network // check if network exists already and remove any duplicate networks exists, err := checkIfNetworkExists(name) @@ -111,10 +113,10 @@ func ensureNetwork(name string) error { return nil } // otherwise we'll try again - } else { - // unknown error ... - return err + continue } + // unknown error ... + return err } return errors.New("exhausted attempts trying to find a non-overlapping subnet") } @@ -152,52 +154,78 @@ func createNetwork(name, ipv6Subnet string) error { } func sortedNetworksWithName(name string) ([]string, error) { - // list all networks by this name - lsOut, err := exec.Output(exec.Command( - "docker", "network", "ls", - "--filter=name=^"+regexp.QuoteMeta(name)+"$", - "--format={{.ID}}", // output as unambiguous IDs - )) + // query which networks exist with the name + ids, err := networksWithName(name) + if err != nil { + return nil, err + } + // we can skip sorting if there are less than 2 + if len(ids) < 2 { + return ids, nil + } + // inspect them to get more detail for sorting + networks, err := inspectNetworks(ids) if err != nil { return nil, err } + // deterministically sort networks + // NOTE: THIS PART IS IMPORTANT! + sortNetworkInspectEntries(networks) + // return network IDs + sortedIDs := make([]string, 0, len(networks)) + for i := range networks { + sortedIDs = append(sortedIDs, networks[i].ID) + } + return sortedIDs, nil +} + +func sortNetworkInspectEntries(networks []networkInspectEntry) { + sort.Slice(networks, func(i, j int) bool { + // we want networks with active containers first + if len(networks[i].Containers) > len(networks[j].Containers) { + return true + } + return networks[i].ID < networks[j].ID + }) +} - // now inspect each - ids := strings.Split(strings.TrimSuffix(string(lsOut), "\n"), "\n") - inspectOut, err := exec.Output(exec.Command("docker", append([]string{"network", "inspect"}, ids...)...)) - // NOTE: a network could be deleted between the ls and inspect calls, that's fine +func inspectNetworks(networkIDs []string) ([]networkInspectEntry, error) { + inspectOut, err := exec.Output(exec.Command("docker", append([]string{"network", "inspect"}, networkIDs...)...)) + // NOTE: the caller can detect if the network isn't present in the output anyhow + // we don't want to fail on this here. if err != nil && !isOnlyErrorNoSuchNetwork(err) { return nil, err } - // parse - type networkInspectEntry struct { - ID string `json:"Id"` - // NOTE: we don't care about the contents here but we need to parse - // how many entries exist in the containers map - Containers map[string]map[string]string `json:"Containers"` - } - networks := []networkInspectEntry{} if err := json.Unmarshal(inspectOut, &networks); err != nil { return nil, errors.Wrap(err, "failed to decode networks list") } + return networks, nil +} - // deterministically sort networks - // NOTE: THIS PART IS IMPORTANT! - sort.Slice(networks, func(i, j int) bool { - if len(networks[i].Containers) < len(networks[j].Containers) { - return true - } - return networks[i].ID < networks[j].ID - }) +type networkInspectEntry struct { + ID string `json:"Id"` + // NOTE: we don't care about the contents here but we need to parse + // how many entries exist in the containers map + Containers map[string]map[string]string `json:"Containers"` +} - // return network IDs - sortedIDs := make([]string, 0, len(networks)) - for i := range networks { - sortedIDs = append(sortedIDs, networks[i].ID) +// networksWithName returns a list of network IDs for networks with this name +func networksWithName(name string) ([]string, error) { + lsOut, err := exec.Output(exec.Command( + "docker", "network", "ls", + "--filter=name=^"+regexp.QuoteMeta(name)+"$", + "--format={{.ID}}", // output as unambiguous IDs + )) + if err != nil { + return nil, err } - return sortedIDs, nil + cleaned := strings.TrimSuffix(string(lsOut), "\n") + if cleaned == "" { // avoid returning []string{""} + return nil, nil + } + return strings.Split(cleaned, "\n"), nil } func checkIfNetworkExists(name string) (bool, error) { diff --git a/pkg/cluster/internal/providers/docker/network_test.go b/pkg/cluster/internal/providers/docker/network_test.go index 11b2601809..73e0e326de 100644 --- a/pkg/cluster/internal/providers/docker/network_test.go +++ b/pkg/cluster/internal/providers/docker/network_test.go @@ -19,12 +19,12 @@ package docker import ( "fmt" "regexp" - "strings" "testing" "sigs.k8s.io/kind/pkg/errors" "sigs.k8s.io/kind/pkg/exec" + "sigs.k8s.io/kind/pkg/internal/assert" "sigs.k8s.io/kind/pkg/internal/integration" ) @@ -35,7 +35,10 @@ func TestIntegrationEnsureNetworkConcurrent(t *testing.T) { // cleanup cleanup := func() { - _ = exec.Command("docker", "network", "rm", testNetworkName).Run() + ids, _ := networksWithName(testNetworkName) + if len(ids) > 0 { + _ = deleteNetworks(ids...) + } } cleanup() defer cleanup() @@ -56,7 +59,6 @@ func TestIntegrationEnsureNetworkConcurrent(t *testing.T) { rerr := exec.RunErrorForError(err) if rerr != nil { t.Errorf("%q", rerr.Output) - t.Errorf("%v", strings.HasPrefix(string(rerr.Output), "Error response from daemon: network with name") && strings.HasSuffix(string(rerr.Output), "already exists\n")) } t.Errorf("%+v", errors.StackTrace(err)) } @@ -122,3 +124,97 @@ func Test_generateULASubnetFromName(t *testing.T) { }) } } + +func Test_sortNetworkInspectEntries(t *testing.T) { + cases := []struct { + Name string + Networks []networkInspectEntry + Sorted []networkInspectEntry + }{ + { + Name: "simple ID sort", + Networks: []networkInspectEntry{ + { + ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e", + }, + { + ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae", + }, + }, + Sorted: []networkInspectEntry{ + { + ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae", + }, + { + ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e", + }, + }, + }, + { + Name: "containers attached sort", + Networks: []networkInspectEntry{ + { + ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae", + }, + { + ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e", + Containers: map[string]map[string]string{ + "a37779e06f3b694eba491dd450aad18bbbaa0a0fce2952e7c9195ea45ae79d41": { + "Name": "buildx_buildkit_kind-builder0", + "EndpointID": "8f6411fb4360059b2f91028f91ef03130abc96d6381afc265ce53c9df89d5a3d", + }, + }, + }, + { + ID: "f0445f08b9989921da00250d778975202267fbab364e5fbad0ceb6db24f3f91e", + }, + { + ID: "128154205c7d88c7bb9c255d389bc9e222b58a48cf83619976e7665a48e79918", + Containers: map[string]map[string]string{ + "aad18bbbaa0a0fce2952e7c9195ea45ae79d41a37779e06f3b694eba491dd450": { + "Name": "fakey-fake", + "EndpointID": "f03130abc96d6381afc265ce53c9df89d5a3d8f6411fb4360059b2f91028f91e", + }, + }, + }, + }, + Sorted: []networkInspectEntry{ + { + ID: "128154205c7d88c7bb9c255d389bc9e222b58a48cf83619976e7665a48e79918", + Containers: map[string]map[string]string{ + "aad18bbbaa0a0fce2952e7c9195ea45ae79d41a37779e06f3b694eba491dd450": { + "Name": "fakey-fake", + "EndpointID": "f03130abc96d6381afc265ce53c9df89d5a3d8f6411fb4360059b2f91028f91e", + }, + }, + }, + { + ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e", + Containers: map[string]map[string]string{ + "a37779e06f3b694eba491dd450aad18bbbaa0a0fce2952e7c9195ea45ae79d41": { + "Name": "buildx_buildkit_kind-builder0", + "EndpointID": "8f6411fb4360059b2f91028f91ef03130abc96d6381afc265ce53c9df89d5a3d", + }, + }, + }, + { + ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae", + }, + { + ID: "f0445f08b9989921da00250d778975202267fbab364e5fbad0ceb6db24f3f91e", + }, + }, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + toSort := make([]networkInspectEntry, len(tc.Networks)) + for i := range tc.Networks { + toSort[i] = tc.Networks[i] + } + sortNetworkInspectEntries(toSort) + assert.DeepEqual(t, tc.Sorted, toSort) + }) + } +}