libnetwork/overlay: remove dead code #44965

akerouanton · 2023-02-09T16:49:29Z

- What I did

All the code related to Classic Swarm is removed from the overlay driver, as well as all the code paths becoming dead because of that.

- How to verify it

CI should be green. It can also be tested by forming a swarm cluster with two dev containers:

dev1$ docker network create --driver overlay --scope global --attachable testnet
dev1$ docker run -d --rm --name whoami --network testnet traefik/whoami
dev2$ docker run --rm -t --name netshoot --network testnet nicolaka/netshoot curl http://whoami

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

corhere

Almost everything LGTM. Has overlay host mode been documented as deprecated? We might not be able to rip it out unless it's gone through a deprecation cycle.

libnetwork/drivers/overlay/overlay.go

corhere · 2023-03-02T18:14:21Z

libnetwork/drivers/overlay/overlay.go

@@ -205,7 +188,7 @@ func (d *driver) initializeVxlanIdm() error {
 		return nil
 	}

-	d.vxlanIdm, err = idm.New(d.store, "vxlan-id", vxlanIDStart, vxlanIDEnd)
+	d.vxlanIdm, err = idm.New(nil, "vxlan-id", vxlanIDStart, vxlanIDEnd)


Woohoo, datastore support can be ripped out of package idm, which is the only user of bitseq.Handle... WE CAN DELETE PACKAGE bitseq SOON! 🎉 🎉

libnetwork/drivers/overlay/ov_network.go

libnetwork/drivers/windows/overlay/overlay_windows.go

corhere · 2023-03-27T22:22:37Z

Since host mode is an implementation detail rather than a user-facing feature, and is only needed to support overlay networks on long-deprecated kernels, that code can be dropped without first going through a deprecation cycle. It'll need a changelog entry, though.

corhere

Could we please save the new code and behaviour changes for a follow-up PR? Let's keep this PR on topic: removing dead code. Lazily cleaning up stale sandboxes may appear dumb, and it might be, but it also might have been done that way for a good reason. (Containers can survive a daemon restart, e.g. over an upgrade. Lazy clean-up might be a way to not disrupt the containers before Swarm has a chance to replace them with new ones.)

corhere · 2023-03-31T22:32:44Z

libnetwork/drivers/overlay/ov_network.go

-	if n.secure {
-		for _, vni := range vnis {
-			programMangle(vni, false)
-			programInput(vni, false)
-		}
-	}


Interesting! So failing to clean up those rules is a long-standing bug. We should fix that. Follow-up PR?

Yes, I'll submit a follow-up PR to properly clean-up those rules, once this one got merged.

libnetwork/drivers/overlay/ov_network.go

libnetwork/drivers/overlay/overlay.go

akerouanton · 2023-04-06T17:07:52Z

Could we please save the new code and behaviour changes for a follow-up PR? Let's keep this PR on topic: removing dead code.

Right, I'll move the incriminated commits to another branch for now.

Lazily cleaning up stale sandboxes may appear dumb, and it might be, but it also might have been done that way for a good reason. (Containers can survive a daemon restart, e.g. over an upgrade. Lazy clean-up might be a way to not disrupt the containers before Swarm has a chance to replace them with new ones.)

I think it was meaningful when Classic Swarm was supported, but I don't see any reason to do that anymore with only Swarm Mode supported since it doesn't support live-restore:

dockerd won't start if it's part of a swarm cluster when live-restore is enabled (dockerd errors out with: Error starting cluster component: --live-restore daemon configuration is incompatible with swarm mode) ;
It returns an error if you try to join/init a cluster whereas live-restore is enabled (it fails with: Error response from daemon: --live-restore daemon configuration is incompatible with swarm mode) ;

Now that being said, I think that's not enough to prove my point. I don't want to spend time on this right now but I'll resubmit those changes if I come up with stronger arguments.

This command was useful when overlay networks based on external KV store was developed but is unused nowadays. As the last reference to OverlayBindInterface and OverlayNeighborIP netlabels are in the ovrouter cmd, they're removed too. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The overlay driver was creating a global store whenever netlabel.GlobalKVClient was specified in its config argument. This specific label is not used anymore since 142b522 (moby#44875). golangci-lint now detects dead code. This will be fixed in subsequent commits. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The overlay driver was creating a global store whenever netlabel.GlobalKVClient was specified in its config argument. This specific label is unused anymore since 142b522 (moby#44875). It was also creating a local store whenever netlabel.LocalKVClient was specificed in its config argument. This store is unused since moby/libnetwork@9e72136 (moby/libnetwork#1636). Finally, the sync.Once properties are never used and thus can be deleted. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- GetIfaceAddr is unused since moby/libnetwork@e51ead59 (moby/libnetwork#670). - ValidateAlias and ParseAlias are unused since moby/moby@0645eb84 (moby#42539). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

- LocalKVProvider, LocalKVProviderURL, LocalKVProviderConfig, GlobalKVProvider, GlobalKVProviderURL and GlobalKVProviderConfig are all unused since moby/libnetwork@be2b6962 (moby/libnetwork#908). - GlobalKVClient is unused since 0fa873c and c8d2c6e. - MakeKVProvider, MakeKVProviderURL and MakeKVProviderConfig are unused since 96cfb07 (moby#44683). - MakeKVClient is unused since 142b522 (moby#44875). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Prior to 0fa873c, the serf-based event loop was started when a global store was available. Since there's no more global store, this event loop and all its associated code is dead. Most dead code detected by golangci-lint in prior commits is now gone. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

VNI allocations made by the overlay driver were only used by Classic Swarm. With Swarm v2 mode, the driver ovmanager is responsible of allocating & releasing them. Previously, vxlanIdm was initialized when a global store was available but since 142b522, no global store can be instantiated. As such, releaseVxlanID actually does actually nothing and iptables rules are never removed. The last line of dead code detected by golangci-lint is now gone. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The overlay driver in Swarm v2 mode doesn't support live-restore, ie. the daemon won't even start if the node is part of a Swarm cluster and live-restore is enabled. This feature was only used by Swarm Classic. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Since the previous commit, data from the local store are never read, thus proving it was only used for Classic Swarm. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Since a few commits, subnet's vni don't change during the lifetime of the subnet struct, so there's no need to lock the network before accessing it. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Since 0fa873c, there's no function writing overlay networks to some datastore. As such, overlay network struct doesn't need to implement KVObject interface. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Linux kernel prior to v3.16 was not supporting netns for vxlan interfaces. As such, moby/libnetwork#821 introduced a "host mode" to the overlay driver. The related kernel fix is available for rhel7 users since v7.2. This mode could be forced through the use of the env var _OVERLAY_HOST_MODE. However this env var has never been documented and is not referenced in any blog post, so there's little chance many people rely on it. Moreover, this host mode is deemed as an implementation details by maintainers. As such, we can consider it dead and we can remove it without a prior deprecation warning. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

corhere

LGTM!

thaJeztah · 2023-04-10T17:06:48Z

libnetwork/netutils/utils.go

-// GetIfaceAddr returns the first IPv4 address and slice of IPv6 addresses for the specified network interface
-func GetIfaceAddr(name string) (net.Addr, []net.Addr, error) {
-	iface, err := net.InterfaceByName(name)
-	if err != nil {
-		return nil, nil, err
-	}
-	addrs, err := iface.Addrs()


Just a random comment 🤔

Haven't been through all changes yet, but I saw this being removed when I looked from my phone, and I was curious why it wasn't used (as some options accept either an IP-address or an interface name). But it looks like we have various "variations" on a a theme (some are test-utilities, so perhaps irrelevant). I haven't compared them fully (to see if there's specific differences between them), but was wondering if there would be use for a common implementation (or if it would be worth looking into);

getBindAddr

moby/libnetwork/agent.go

Lines 58 to 62 in dd3b71d

func getBindAddr(ifaceName string) (string, error) {

iface, err := net.InterfaceByName(ifaceName)

if err != nil {

return "", fmt.Errorf("failed to find interface %s: %v", ifaceName, err)

}

resolveInterfaceAddr

moby/daemon/cluster/listen_addr.go

Lines 161 to 168 in dd3b71d

func resolveInterfaceAddr(specifiedInterface string) (net.IP, error) {

// Use a specific interface's IP address.

intf, err := net.InterfaceByName(specifiedInterface)

if err != nil {

return nil, errNoSuchInterface

}

addrs, err := intf.Addrs()

getExternalAddress

moby/integration/container/nat_test.go

Lines 121 to 126 in dd3b71d

func getExternalAddress(t *testing.T) net.IP {

t.Helper()

iface, err := net.InterfaceByName("eth0")

skip.If(t, err != nil, "Test not running with `make test-integration`. Interface eth0 not found: %s", err)

ifaceAddrs, err := iface.Addrs()

setupDriver

moby/libnetwork/drivers/overlay/overlay_test.go

Lines 66 to 70 in dd3b71d

iface, err := net.InterfaceByName("eth0")

if err != nil {

t.Fatal(err)

}

addrs, err := iface.Addrs()

libnetwork.getBindAddr returns an arbitrary interface IP address which is not an IPv6 link-local address, with no preference of IP address family. On the other hand, cluster.resolveInterfaceAddr prefers IPv4 over IPv6, neglects to skip over IPv6 link-local addresses, and returns an error if there is more than one address of either family associated with the interface. Since IPv6 requires a link-local address be assigned to each interface in addition to any routable addresses, cluster.resolveInterfaceAddr is broken for any interface which has a routable IPv6 address associated.

I question the utility of both functions. While it is kinda convenient to let the administrator specify an interface name instead of an address, specifying an interface implies a promise which the daemon can't currently keep: that the daemon will dynamically handle changes to the IP addresses associated with the interface.

question the utility of both functions. While it is kinda convenient to let the administrator specify an interface name instead of an address, specifying an interface implies a promise which the daemon can't currently keep: that the daemon will dynamically handle changes to the IP addresses associated with the interface.

Yeah, swarm doesn't handle IP changes at all. The "allow interface" was mostly out of convenience (ISTR part of that was created at the time for automated setups of clusters)

It still feels like a good thing to verify the various code paths to check if they're consistent (and do the right thing).

Definitely not for this PR though!

thaJeztah

LGTM!

thaJeztah · 2023-04-11T14:56:07Z

libnetwork/drivers/overlay/ov_network.go

Just a nit (and can't comment on commit messages);

This specific label is not used anymore since https://github.com/moby/moby/commit/142b522946f67a1884228119ebce1e7e5a740364 (https://github.com/moby/moby/pull/44875).

I tend to avoid referencing PR numbers in commit messages, as they can sometimes get noisy (forks backporting fixes etc). What I usually do;

refer to the commit in the commit message (useful, even without GitHub around)

in the PR description on GitHub I add the PR number (if useful)

Either way; not a blocker 👍

akerouanton marked this pull request as draft February 10, 2023 16:10

akerouanton force-pushed the libnetwork-dead-code branch 2 times, most recently from f182240 to 112b050 Compare February 16, 2023 10:03

akerouanton force-pushed the libnetwork-dead-code branch 2 times, most recently from 76657dc to 79a486e Compare March 2, 2023 16:13

akerouanton marked this pull request as ready for review March 2, 2023 16:39

akerouanton changed the title ~~Remove dead code from libnetwork~~ libnetwork/overlay: remove dead code Mar 2, 2023

corhere requested changes Mar 2, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

thaJeztah added status/2-code-review area/networking labels Mar 2, 2023

thaJeztah added this to the v-next milestone Mar 2, 2023

corhere mentioned this pull request Mar 15, 2023

libnetwork/d/overlay: handle peer ops directly #45164

Merged

corhere added impact/changelog kind/refactor PR's that refactor, or clean-up code labels Mar 27, 2023

akerouanton force-pushed the libnetwork-dead-code branch from 79a486e to 7bcd578 Compare March 31, 2023 20:59

corhere requested changes Mar 31, 2023

View reviewed changes

akerouanton added 11 commits April 6, 2023 19:33

libnetwork/netutils: remove dead code

f8b5fe5

- GetIfaceAddr is unused since moby/libnetwork@e51ead59 (moby/libnetwork#670). - ValidateAlias and ParseAlias are unused since moby/moby@0645eb84 (moby#42539). Signed-off-by: Albin Kerouanton <albinker@gmail.com>

libnetwork: remove local store from overlay driver

b67446a

Since the previous commit, data from the local store are never read, thus proving it was only used for Classic Swarm. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

libnetwork/overlay: don't lock network when accessing subnet vni

f32f09e

Since a few commits, subnet's vni don't change during the lifetime of the subnet struct, so there's no need to lock the network before accessing it. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

libnetwork/overlay: remove KVObject implementation

1d46597

Since 0fa873c, there's no function writing overlay networks to some datastore. As such, overlay network struct doesn't need to implement KVObject interface. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton force-pushed the libnetwork-dead-code branch from 7bcd578 to 8ed9002 Compare April 6, 2023 17:54

akerouanton requested a review from corhere April 6, 2023 18:01

corhere approved these changes Apr 10, 2023

View reviewed changes

thaJeztah reviewed Apr 10, 2023

View reviewed changes

thaJeztah approved these changes Apr 11, 2023

View reviewed changes

thaJeztah merged commit 0154746 into moby:master Apr 11, 2023
91 checks passed

akerouanton deleted the libnetwork-dead-code branch April 11, 2023 15:11

This was referenced Apr 17, 2023

libnet/overlay: clean up iptables rules on network delete #45343

Merged

libnet: remove package bitseq #45348

Closed

corhere mentioned this pull request Jul 7, 2023

libnetwork: drop cruft formerly needed by Swarmkit #45908

Merged

This was referenced Jul 11, 2023

libnet: Return proper error when overlay network can't be created #45915

Merged

[24.0 backport] libnet: Return proper error when overlay network can't be created #45974

Merged

corhere mentioned this pull request Jan 26, 2024

libnetwork/d/overlay: drop unused miss flags from peerAdd #47234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnetwork/overlay: remove dead code #44965

libnetwork/overlay: remove dead code #44965

akerouanton commented Feb 9, 2023 •

edited

corhere left a comment

corhere Mar 2, 2023

This comment was marked as outdated.

corhere commented Mar 27, 2023

corhere left a comment

corhere Mar 31, 2023

akerouanton Apr 6, 2023 •

edited

akerouanton commented Apr 6, 2023

corhere left a comment

thaJeztah Apr 10, 2023

corhere Apr 10, 2023

thaJeztah Apr 11, 2023

thaJeztah left a comment

thaJeztah Apr 11, 2023

	func getBindAddr(ifaceName string) (string, error) {
	iface, err := net.InterfaceByName(ifaceName)
	if err != nil {
	return "", fmt.Errorf("failed to find interface %s: %v", ifaceName, err)
	}

	func resolveInterfaceAddr(specifiedInterface string) (net.IP, error) {
	// Use a specific interface's IP address.
	intf, err := net.InterfaceByName(specifiedInterface)
	if err != nil {
	return nil, errNoSuchInterface
	}

	addrs, err := intf.Addrs()

	func getExternalAddress(t *testing.T) net.IP {
	t.Helper()
	iface, err := net.InterfaceByName("eth0")
	skip.If(t, err != nil, "Test not running with `make test-integration`. Interface eth0 not found: %s", err)

	ifaceAddrs, err := iface.Addrs()

	iface, err := net.InterfaceByName("eth0")
	if err != nil {
	t.Fatal(err)
	}
	addrs, err := iface.Addrs()

libnetwork/overlay: remove dead code #44965

libnetwork/overlay: remove dead code #44965

Conversation

akerouanton commented Feb 9, 2023 • edited

corhere left a comment

Choose a reason for hiding this comment

corhere Mar 2, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

corhere commented Mar 27, 2023

corhere left a comment

Choose a reason for hiding this comment

corhere Mar 31, 2023

Choose a reason for hiding this comment

akerouanton Apr 6, 2023 • edited

Choose a reason for hiding this comment

akerouanton commented Apr 6, 2023

corhere left a comment

Choose a reason for hiding this comment

thaJeztah Apr 10, 2023

Choose a reason for hiding this comment

corhere Apr 10, 2023

Choose a reason for hiding this comment

thaJeztah Apr 11, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Apr 11, 2023

Choose a reason for hiding this comment

akerouanton commented Feb 9, 2023 •

edited

akerouanton Apr 6, 2023 •

edited