Remove deprecated host-discovery and overlay networks with external k/v #42247

thaJeztah · 2021-04-02T16:59:35Z

~~currently vendoring libnetwork changes from moby/libnetwork#2521~~

- Description for the changelog

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

thaJeztah · 2021-04-02T22:07:42Z

Two tests failing that could be related

=== RUN   TestDockerDaemonSuite/TestDaemonICCLinkExpose
    --- FAIL: TestDockerDaemonSuite/TestDaemonICCLinkExpose (134.86s)
        docker_cli_daemon_test.go:909: assertion failed: error is not nil: exit status 1
=== RUN   TestDockerDaemonSuite/TestDaemonICCPing
    --- FAIL: TestDockerDaemonSuite/TestDaemonICCPing (7.99s)
        docker_cli_daemon_test.go:885: assertion failed: error is not nil: exit status 1: PING 192.171.1.1 (192.171.1.1): 56 data bytes
            
            --- 192.171.1.1 ping statistics ---
            1 packets transmitted, 0 packets received, 100% packet loss

thaJeztah · 2021-06-28T14:26:16Z

[2021-06-28T10:25:56.637Z] libnetwork/controller.go:549:22: func `(*controller).activeCallback` is unused (unused)
[2021-06-28T10:25:56.638Z] libnetwork/controller.go:556:22: func `(*controller).hostJoinCallback` is unused (unused)
[2021-06-28T10:25:56.638Z] libnetwork/controller.go:560:22: func `(*controller).hostLeaveCallback` is unused (unused)

=== RUN   TestDockerDaemonSuite/TestDaemonICCLinkExpose
    docker_cli_daemon_test.go:908: assertion failed: error is not nil: exit status 1
    check_test.go:309: [deacc764e3b6f] daemon is not started
    --- FAIL: TestDockerDaemonSuite/TestDaemonICCLinkExpose (134.36s)

TestDockerDaemonSuite/TestDaemonICCPing
    docker_cli_daemon_test.go:864: [d9d50010d717e] failed to start daemon with arguments [--data-root /go/src/github.com/docker/docker/bundles/test-integration/TestDockerDaemonSuite/TestDaemonICCPing/d9d50010d717e/root --exec-root /tmp/dxr/d9d50010d717e --pidfile /go/src/github.com/docker/docker/bundles/test-integration/TestDockerDaemonSuite/TestDaemonICCPing/d9d50010d717e/docker.pid --userland-proxy=true --containerd-namespace d9d50010d717e --containerd-plugins-namespace d9d50010d717ep --containerd /var/run/docker/containerd/containerd.sock --host unix:///tmp/docker-integration/d9d50010d717e.sock --debug --storage-driver overlay2 --bridge external-bridge --iccdaemon.New=false] : [d9d50010d717e] daemon exited during startup: exit status 1
    check_test.go:309: [db5b083321b9c] daemon is not started
    --- FAIL: TestDockerDaemonSuite/TestDaemonICCPing (0.57s)

This part looks fishy; did I mess up a copy/paste? --iccdaemon.New=false

errordeveloper · 2021-12-21T13:22:23Z

This part looks fishy; did I mess up a copy/paste? --iccdaemon.New=false

I think you meant --icc=false ;)

thaJeztah · 2021-12-21T15:26:33Z

libnetwork/datastore/datastore.go

@@ -114,7 +114,7 @@ type ScopeClientCfg struct {
 const (
 	// LocalScope indicates to store the KV object in local datastore such as boltdb
 	LocalScope = "local"
-	// GlobalScope indicates to store the KV object in global datastore such as etcd
+	// GlobalScope indicates to store the KV object in global datastore
 	GlobalScope = "global"


Possibly, this const (and its use) can also be removed, but leaving that for a separate PR (as it's used in quite some places)

thaJeztah · 2021-12-21T15:27:42Z

libnetwork/store.go

 	"github.com/sirupsen/logrus"
 )

 func registerKVStores() {
-	etcd.Register()
 	boltdb.Register()


Also need to check if this store is actually used (I thought it was, because I know libnetwork uses boltdb for some things), but not sure if this code is actually used for that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Use unique names to prevent tests from interfering, using a shorter name, as there's a maximum length for these. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Anca Iordache <anca.iordache@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Based on randomLocalStore() in libnetwork/ipam/allocator_test.go Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cpuguy83

LGTM

tianon

LGTM

tianon · 2022-01-06T19:37:22Z

libnetwork/test/integration/dnet/helpers.bash

 	if [ "$nip" != "" ]; then
 	    neighbors=${nip}
 	fi

 	discovery=""
 	provider=""
 	address=""


This indentation is technically "wrong" now, but IMO it's not important enough to put this through CI/edits again (maybe something someone can fix as a separate PR).

oh, yeah, quite likely; I mostly tried to make my "remote X" commits clear on what I was removing, which I why I removed the bits that were related, but I think none of these are used anymore, so we likely can just remove everything here. now (I'll keep that for a follow-up)

thaJeztah · 2022-01-06T21:15:18Z

okay, i'm gonna "YOLO" and bring this one in 🙈, then look at the follow-up rebasing to make the go.mod / vendor.mod work

Thanks for reviewing!! ❤️

thaJeztah added status/2-code-review area/networking impact/changelog impact/deprecation labels Apr 2, 2021

thaJeztah added this to the 21.xx milestone Apr 2, 2021

thaJeztah mentioned this pull request Apr 2, 2021

Proposal: deprecated (and remove) "--cluster-xx" options (legacy overlay networks) #40383

Closed

11 tasks

thaJeztah force-pushed the remove_discovery branch 2 times, most recently from f2c836b to 2f6b9fc Compare April 2, 2021 19:09

thaJeztah mentioned this pull request Apr 7, 2021

[do not merge] remove "discovery" options moby/libnetwork#2521

Closed

thaJeztah mentioned this pull request Jun 8, 2021

Convert "vendor.conf" to "go.mod" with minimal changes [DNM] #42489

Closed

thaJeztah force-pushed the remove_discovery branch 4 times, most recently from 430a285 to 5a3e155 Compare June 28, 2021 08:47

thaJeztah force-pushed the remove_discovery branch from ac28201 to 6b739e4 Compare June 28, 2021 14:37

thaJeztah force-pushed the remove_discovery branch from 498e593 to 103c0d8 Compare August 10, 2021 14:11

thaJeztah force-pushed the remove_discovery branch from 103c0d8 to adf76de Compare December 15, 2021 15:21

thaJeztah mentioned this pull request Dec 15, 2021

Remove deprecated host-discovery and overlay networks with external k/v [step 1] #43085

Closed

thaJeztah force-pushed the remove_discovery branch 3 times, most recently from bd33afa to 1b19325 Compare December 16, 2021 13:48

thaJeztah mentioned this pull request Dec 21, 2021

integration-cli: delete existing docker0 bridge to prevent it interfering #43097

Merged

thaJeztah force-pushed the remove_discovery branch from 1b19325 to 2e96e81 Compare December 21, 2021 15:25

thaJeztah commented Dec 21, 2021

View reviewed changes

thaJeztah mentioned this pull request Dec 22, 2021

libnetwork: remove more config bits related to external k/v stores #43099

Closed

thaJeztah changed the title ~~[WIP] Remove deprecated host-discovery and overlay networks with external k/v~~ Remove deprecated host-discovery and overlay networks with external k/v Dec 22, 2021

thaJeztah force-pushed the remove_discovery branch from 2e96e81 to 574dc56 Compare December 22, 2021 16:24

thaJeztah marked this pull request as ready for review December 22, 2021 16:25

This was referenced Dec 22, 2021

vendor: use go mod (rebase after removal of deprecated host-discovery) #43101

Merged

vendor: use go mod #43075

Closed

thaJeztah force-pushed the remove_discovery branch from 574dc56 to 998bc96 Compare January 5, 2022 09:23

thaJeztah and others added 15 commits January 6, 2022 18:28

daemon: remove discovery related tests

702cb7f

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

integration(-cli): remove discovery related tests

65b92a7

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cmd/dockerd: produce error when using discovery options

9f2240c

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: remove discovery-related config handling

ff2a530

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

integration-cli: use unique names for custom bridges

d2c6602

Use unique names to prevent tests from interfering, using a shorter name, as there's a maximum length for these. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: remove discovery inits

f28fc8b

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: remove daemon.discoveryWatcher

9492354

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: remove daemon/discovery as it's now unused

c741ab0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: remove external DS-based host discovery

00f9b23

Signed-off-by: Anca Iordache <anca.iordache@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

remove pkg/discovery as it's now unused

e202ac3

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: remove zookeeper-related code and tests

a7d0f30

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: replace consul with boltdb in test

25594c3

Based on randomLocalStore() in libnetwork/ipam/allocator_test.go Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: remove consul-related code and tests

147173b

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: remove etcd-related code and tests

745ba3e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

libnetwork: remove more config bits related to external k/v stores

7b692a4

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the remove_discovery branch from 998bc96 to 7b692a4 Compare January 6, 2022 17:46

cpuguy83 approved these changes Jan 6, 2022

View reviewed changes

tianon approved these changes Jan 6, 2022

View reviewed changes

thaJeztah merged commit 0f1d65b into moby:master Jan 6, 2022

thaJeztah deleted the remove_discovery branch January 6, 2022 21:15

thaJeztah mentioned this pull request Jan 6, 2022

Cleanup host discovery moby/libnetwork#2509

Closed

thaJeztah mentioned this pull request Aug 27, 2023

libnetwork: node-discovery: fix error-handling, and some refactor #46346

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated host-discovery and overlay networks with external k/v #42247

Remove deprecated host-discovery and overlay networks with external k/v #42247

thaJeztah commented Apr 2, 2021 •

edited

thaJeztah commented Apr 2, 2021

thaJeztah commented Jun 28, 2021 •

edited

errordeveloper commented Dec 21, 2021

thaJeztah Dec 21, 2021

thaJeztah Dec 21, 2021

cpuguy83 left a comment

tianon left a comment

tianon Jan 6, 2022

thaJeztah Jan 6, 2022

thaJeztah commented Jan 6, 2022

Remove deprecated host-discovery and overlay networks with external k/v #42247

Remove deprecated host-discovery and overlay networks with external k/v #42247

Conversation

thaJeztah commented Apr 2, 2021 • edited

thaJeztah commented Apr 2, 2021

thaJeztah commented Jun 28, 2021 • edited

errordeveloper commented Dec 21, 2021

thaJeztah Dec 21, 2021

Choose a reason for hiding this comment

thaJeztah Dec 21, 2021

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

tianon left a comment

Choose a reason for hiding this comment

tianon Jan 6, 2022

Choose a reason for hiding this comment

thaJeztah Jan 6, 2022

Choose a reason for hiding this comment

thaJeztah commented Jan 6, 2022

thaJeztah commented Apr 2, 2021 •

edited

thaJeztah commented Jun 28, 2021 •

edited