Merge pull request #49343 from thaJeztah/cluster_cleanup_networkattachment

daemon/cluster/executor: simplify handling of Network Attachments

daemon/cluster/executor/container/adapter.go

@@ -180,13 +180,13 @@ func (c *containerAdapter) waitNodeAttachments(ctx context.Context) error {
 		// set a flag ready to true. if we try to get a network IP that doesn't
 		// exist yet, we will set this flag to "false"
 		ready := true
-		for _, attachment := range c.container.networksAttachments {
+		for _, nw := range c.container.networks {
 			// we only need node attachments (IP address) for overlay networks
 			// TODO(dperny): unsure if this will work with other network
 			// drivers, but i also don't think other network drivers use the
 			// node attachment IP address.
-			if attachment.Network.DriverState.Name == "overlay" {
-				if _, exists := attachmentStore.GetIPForNetwork(attachment.Network.ID); !exists {
+			if nw.DriverState.Name == "overlay" {
+				if _, exists := attachmentStore.GetIPForNetwork(nw.ID); !exists {
 					ready = false
 				}
 			}
@@ -207,12 +207,8 @@ func (c *containerAdapter) waitNodeAttachments(ctx context.Context) error {
 }
 
 func (c *containerAdapter) createNetworks(ctx context.Context) error {
-	for name := range c.container.networksAttachments {
-		ncr, err := c.container.networkCreateRequest(name)
-		if err != nil {
-			return err
-		}
-
+	for name, nw := range c.container.networks {
+		ncr := networkCreateRequest(name, nw)
 		if err := c.backend.CreateManagedNetwork(ncr); err != nil { // todo name missing
 			if _, ok := err.(libnetwork.NetworkNameError); ok {
 				continue
@@ -235,8 +231,8 @@ func (c *containerAdapter) removeNetworks(ctx context.Context) error {
 		errNoSuchNetwork     libnetwork.ErrNoSuchNetwork
 	)
 
-	for name, v := range c.container.networksAttachments {
-		if err := c.backend.DeleteManagedNetwork(v.Network.ID); err != nil {
+	for name, nw := range c.container.networks {
+		if err := c.backend.DeleteManagedNetwork(nw.ID); err != nil {
 			switch {
 			case errors.As(err, &activeEndpointsError):
 				continue

daemon/cluster/executor/container/adapter_test.go

@@ -34,35 +34,29 @@ func TestWaitNodeAttachment(t *testing.T) {
 	// actually; only the networkAttachments are needed.
 	container := &containerConfig{
 		task: nil,
-		networksAttachments: map[string]*api.NetworkAttachment{
+		networks: map[string]*api.Network{
 			// network1 is already present in the attachment store.
 			"network1": {
-				Network: &api.Network{
-					ID: "network1",
-					DriverState: &api.Driver{
-						Name: "overlay",
-					},
+				ID: "network1",
+				DriverState: &api.Driver{
+					Name: "overlay",
 				},
 			},
 			// network2 is not yet present in the attachment store, and we
 			// should block while waiting for it.
 			"network2": {
-				Network: &api.Network{
-					ID: "network2",
-					DriverState: &api.Driver{
-						Name: "overlay",
-					},
+				ID: "network2",
+				DriverState: &api.Driver{
+					Name: "overlay",
 				},
 			},
 			// localnetwork is not and will never be in the attachment store,
 			// but we should not block on it, because it is not an overlay
 			// network
 			"localnetwork": {
-				Network: &api.Network{
-					ID: "localnetwork",
-					DriverState: &api.Driver{
-						Name: "bridge",
-					},
+				ID: "localnetwork",
+				DriverState: &api.Driver{
+					Name: "bridge",
 				},
 			},
 		},

daemon/cluster/executor/container/container.go

@@ -3,7 +3,6 @@ package container // import "github.com/docker/docker/daemon/cluster/executor/co
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -37,8 +36,8 @@ const (
 // containerConfig converts task properties into docker container compatible
 // components.
 type containerConfig struct {
-	task                *api.Task
-	networksAttachments map[string]*api.NetworkAttachment
+	task     *api.Task
+	networks map[string]*api.Network
 }
 
 // newContainerConfig returns a validated container config. No methods should
@@ -65,9 +64,16 @@ func (c *containerConfig) setTask(t *api.Task, node *api.NodeDescription) error
 	}
 
 	// index the networks by name
-	c.networksAttachments = make(map[string]*api.NetworkAttachment, len(t.Networks))
+	c.networks = make(map[string]*api.Network, len(t.Networks))
 	for _, attachment := range t.Networks {
-		c.networksAttachments[attachment.Network.Spec.Annotations.Name] = attachment
+		// It looks like using a map is only for convenience, but not used
+		// for validation, nor for looking up the network by name. The name
+		// is part of the Network's properties (Network.Spec.Annotations.Name),
+		// and effectively only used for debugging; we should consider to
+		// change it to a slice.
+		//
+		// TODO(thaJeztah): should this check for empty and duplicate names?
+		c.networks[attachment.Network.Spec.Annotations.Name] = attachment.Network
 	}
 
 	c.task = t
@@ -620,57 +626,51 @@ func (c *containerConfig) serviceConfig() *clustertypes.ServiceConfig {
 	return svcCfg
 }
 
-func (c *containerConfig) networkCreateRequest(name string) (clustertypes.NetworkCreateRequest, error) {
-	na, ok := c.networksAttachments[name]
-	if !ok {
-		return clustertypes.NetworkCreateRequest{}, errors.New("container: unknown network referenced")
-	}
-
+func networkCreateRequest(name string, nw *api.Network) clustertypes.NetworkCreateRequest {
 	ipv4Enabled := true
-	ipv6Enabled := na.Network.Spec.Ipv6Enabled
+	ipv6Enabled := nw.Spec.Ipv6Enabled
 	options := network.CreateOptions{
-		// ID:     na.Network.ID,
-		Labels:     na.Network.Spec.Annotations.Labels,
-		Internal:   na.Network.Spec.Internal,
-		Attachable: na.Network.Spec.Attachable,
-		Ingress:    convert.IsIngressNetwork(na.Network),
+		// ID:     nw.ID,
+		Labels:     nw.Spec.Annotations.Labels,
+		Internal:   nw.Spec.Internal,
+		Attachable: nw.Spec.Attachable,
+		Ingress:    convert.IsIngressNetwork(nw),
 		EnableIPv4: &ipv4Enabled,
 		EnableIPv6: &ipv6Enabled,
 		Scope:      scope.Swarm,
 	}
 
-	if na.Network.Spec.GetNetwork() != "" {
+	if nw.Spec.GetNetwork() != "" {
 		options.ConfigFrom = &network.ConfigReference{
-			Network: na.Network.Spec.GetNetwork(),
+			Network: nw.Spec.GetNetwork(),
 		}
 	}
 
-	if na.Network.DriverState != nil {
-		options.Driver = na.Network.DriverState.Name
-		options.Options = na.Network.DriverState.Options
+	if nw.DriverState != nil {
+		options.Driver = nw.DriverState.Name
+		options.Options = nw.DriverState.Options
 	}
-	if na.Network.IPAM != nil {
+	if nw.IPAM != nil {
 		options.IPAM = &network.IPAM{
-			Driver:  na.Network.IPAM.Driver.Name,
-			Options: na.Network.IPAM.Driver.Options,
+			Driver:  nw.IPAM.Driver.Name,
+			Options: nw.IPAM.Driver.Options,
 		}
-		for _, ic := range na.Network.IPAM.Configs {
-			c := network.IPAMConfig{
+		for _, ic := range nw.IPAM.Configs {
+			options.IPAM.Config = append(options.IPAM.Config, network.IPAMConfig{
 				Subnet:  ic.Subnet,
 				IPRange: ic.Range,
 				Gateway: ic.Gateway,
-			}
-			options.IPAM.Config = append(options.IPAM.Config, c)
+			})
 		}
 	}
 
 	return clustertypes.NetworkCreateRequest{
-		ID: na.Network.ID,
+		ID: nw.ID,
 		CreateRequest: network.CreateRequest{
-			Name:          name,
+			Name:          name, // TODO(thaJeztah): this is the same as [nw.Spec.Annotations.Name]; consider using that instead
 			CreateOptions: options,
 		},
-	}, nil
+	}
 }
 
 func (c *containerConfig) applyPrivileges(hc *containertypes.HostConfig) {