From 16770340ea6bb59c94a647e768b837572e9027f1 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 27 Jan 2025 11:05:04 +0100 Subject: [PATCH 1/4] daemon/cluster/executor: networkCreateRequest don't shadow config c is used as name for the containerConfig receiver; remove the intermediate variable so that we don't shadow it. There's no bug here, because a new variable is created; just to prevent confusion and to make linters happier. Signed-off-by: Sebastiaan van Stijn --- daemon/cluster/executor/container/container.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index 4e698d2d22..bc1e5bfc75 100644 --- a/daemon/cluster/executor/container/container.go +++ b/daemon/cluster/executor/container/container.go @@ -655,12 +655,11 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ Options: na.Network.IPAM.Driver.Options, } for _, ic := range na.Network.IPAM.Configs { - c := network.IPAMConfig{ + 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) + }) } } From f5f4a062a56d9f7941ec19717e6148c2d7cac430 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 27 Jan 2025 11:09:43 +0100 Subject: [PATCH 2/4] daemon/cluster/executor: networkCreateRequest: slight DRY cleanup All of this function only referenced the Network field in the attachment; use an intermediate variable to make the code less repetitive. Signed-off-by: Sebastiaan van Stijn --- .../cluster/executor/container/container.go | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index bc1e5bfc75..c1983348bf 100644 --- a/daemon/cluster/executor/container/container.go +++ b/daemon/cluster/executor/container/container.go @@ -625,36 +625,37 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ if !ok { return clustertypes.NetworkCreateRequest{}, errors.New("container: unknown network referenced") } + nw := na.Network 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 { + for _, ic := range nw.IPAM.Configs { options.IPAM.Config = append(options.IPAM.Config, network.IPAMConfig{ Subnet: ic.Subnet, IPRange: ic.Range, @@ -664,7 +665,7 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ } return clustertypes.NetworkCreateRequest{ - ID: na.Network.ID, + ID: nw.ID, CreateRequest: network.CreateRequest{ Name: name, CreateOptions: options, From 90323ae12304ca1a99cf6e3f4552b151bca6d193 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 27 Jan 2025 11:27:23 +0100 Subject: [PATCH 3/4] daemon/cluster/executor: networkCreateRequest: not a method This method was called in a loop, iterating over the container config's network-attachments. It was defined as a method, but only to lookup the same attachment we just iterated over existed, and to obtain a copy. As there were no goroutines involved, dereferencing should not be an issue and with Go 1.22+, dereferencing loop vars is no longer needed at all, so we can change this method to a regular constructor; also removing the redundant error-return in the process. Signed-off-by: Sebastiaan van Stijn --- daemon/cluster/executor/container/adapter.go | 8 ++------ daemon/cluster/executor/container/container.go | 11 ++--------- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/daemon/cluster/executor/container/adapter.go b/daemon/cluster/executor/container/adapter.go index 0208ab9975..35bd85d59a 100644 --- a/daemon/cluster/executor/container/adapter.go +++ b/daemon/cluster/executor/container/adapter.go @@ -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.networksAttachments { + ncr := networkCreateRequest(name, nw.Network) if err := c.backend.CreateManagedNetwork(ncr); err != nil { // todo name missing if _, ok := err.(libnetwork.NetworkNameError); ok { continue diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index c1983348bf..a6716068a2 100644 --- a/daemon/cluster/executor/container/container.go +++ b/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" @@ -620,13 +619,7 @@ 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") - } - nw := na.Network - +func networkCreateRequest(name string, nw *api.Network) clustertypes.NetworkCreateRequest { ipv4Enabled := true ipv6Enabled := nw.Spec.Ipv6Enabled options := network.CreateOptions{ @@ -670,7 +663,7 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ Name: name, CreateOptions: options, }, - }, nil + } } func (c *containerConfig) applyPrivileges(hc *containertypes.HostConfig) { From 87050187059e26be0796915321307afbd4e30195 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 27 Jan 2025 12:21:15 +0100 Subject: [PATCH 4/4] daemon/cluster/executor: containerConfig: store Network instead of envelope The Network field is the only field used from the NetworkAttachment within this code. Remove the NetworkAttachment envelope, and store the Network field directly instead. Signed-off-by: Sebastiaan van Stijn --- daemon/cluster/executor/container/adapter.go | 14 +++++----- .../executor/container/adapter_test.go | 26 +++++++------------ .../cluster/executor/container/container.go | 17 ++++++++---- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/daemon/cluster/executor/container/adapter.go b/daemon/cluster/executor/container/adapter.go index 35bd85d59a..b4ac57bede 100644 --- a/daemon/cluster/executor/container/adapter.go +++ b/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,8 +207,8 @@ func (c *containerAdapter) waitNodeAttachments(ctx context.Context) error { } func (c *containerAdapter) createNetworks(ctx context.Context) error { - for name, nw := range c.container.networksAttachments { - ncr := networkCreateRequest(name, nw.Network) + 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 @@ -231,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 diff --git a/daemon/cluster/executor/container/adapter_test.go b/daemon/cluster/executor/container/adapter_test.go index 03b3b9ab45..8db2b0949d 100644 --- a/daemon/cluster/executor/container/adapter_test.go +++ b/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", }, }, }, diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index a6716068a2..6bbdec4ecc 100644 --- a/daemon/cluster/executor/container/container.go +++ b/daemon/cluster/executor/container/container.go @@ -36,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 @@ -64,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 @@ -660,7 +667,7 @@ func networkCreateRequest(name string, nw *api.Network) clustertypes.NetworkCrea return clustertypes.NetworkCreateRequest{ 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, }, }