Browse code

libnetwork: implement Controller.GetSandbox(containerID)

Various parts of the code were using "walkers" to iterate over the
controller's sandboxes, and the only condition for all of them was
to find the sandbox for a given container-ID. Iterating over all
sandboxes was also sub-optimal, because on Windows, the ContainerID
is used as Sandbox-ID, which can be used to lookup the sandbox from
the "sandboxes" map on the controller.

This patch implements a GetSandbox method on the controller that
looks up the sandbox for a given container-ID, using the most optimal
approach (depending on the platform).

The new method can return errors for invalid (empty) container-IDs, and
a "not found" error to allow consumers to detect non-existing sandboxes,
or potentially invalid IDs.

This new method replaces the (non-exported) Daemon.getNetworkSandbox(),
which was only used internally, in favor of directly accessing the
controller's method.

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

Sebastiaan van Stijn authored on 2023/08/12 21:38:43
Showing 6 changed files
... ...
@@ -532,7 +532,11 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
532 532
 	// If the container is not to be connected to any network,
533 533
 	// create its network sandbox now if not present
534 534
 	if len(networks) == 0 {
535
-		if nil == daemon.getNetworkSandbox(container) {
535
+		if _, err := daemon.netController.GetSandbox(container.ID); err != nil {
536
+			if !errdefs.IsNotFound(err) {
537
+				return err
538
+			}
539
+
536 540
 			sbOptions, err := daemon.buildSandboxOptions(cfg, container)
537 541
 			if err != nil {
538 542
 				return err
... ...
@@ -557,18 +561,6 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
557 557
 	return nil
558 558
 }
559 559
 
560
-func (daemon *Daemon) getNetworkSandbox(container *container.Container) *libnetwork.Sandbox {
561
-	var sb *libnetwork.Sandbox
562
-	daemon.netController.WalkSandboxes(func(s *libnetwork.Sandbox) bool {
563
-		if s.ContainerID() == container.ID {
564
-			sb = s
565
-			return true
566
-		}
567
-		return false
568
-	})
569
-	return sb
570
-}
571
-
572 560
 // hasUserDefinedIPAddress returns whether the passed IPAM configuration contains IP address configuration
573 561
 func hasUserDefinedIPAddress(ipamConfig *networktypes.EndpointIPAMConfig) bool {
574 562
 	return ipamConfig != nil && (len(ipamConfig.IPv4Address) > 0 || len(ipamConfig.IPv6Address) > 0)
... ...
@@ -715,7 +707,8 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
715 715
 		return err
716 716
 	}
717 717
 
718
-	sb := daemon.getNetworkSandbox(container)
718
+	// TODO(thaJeztah): should this fail early if no sandbox was found?
719
+	sb, _ := daemon.netController.GetSandbox(container.ID)
719 720
 	createOptions, err := buildCreateEndpointOptions(container, n, endpointConfig, sb, cfg.DNS)
720 721
 	if err != nil {
721 722
 		return err
... ...
@@ -1072,9 +1065,9 @@ func (daemon *Daemon) ActivateContainerServiceBinding(containerName string) erro
1072 1072
 	if err != nil {
1073 1073
 		return err
1074 1074
 	}
1075
-	sb := daemon.getNetworkSandbox(ctr)
1076
-	if sb == nil {
1077
-		return fmt.Errorf("network sandbox does not exist for container %s", containerName)
1075
+	sb, err := daemon.netController.GetSandbox(ctr.ID)
1076
+	if err != nil {
1077
+		return fmt.Errorf("failed to activate service binding for container %s: %w", containerName, err)
1078 1078
 	}
1079 1079
 	return sb.EnableService()
1080 1080
 }
... ...
@@ -1085,10 +1078,10 @@ func (daemon *Daemon) DeactivateContainerServiceBinding(containerName string) er
1085 1085
 	if err != nil {
1086 1086
 		return err
1087 1087
 	}
1088
-	sb := daemon.getNetworkSandbox(ctr)
1089
-	if sb == nil {
1088
+	sb, err := daemon.netController.GetSandbox(ctr.ID)
1089
+	if err != nil {
1090 1090
 		// If the network sandbox is not found, then there is nothing to deactivate
1091
-		log.G(context.TODO()).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName)
1091
+		log.G(context.TODO()).WithError(err).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName)
1092 1092
 		return nil
1093 1093
 	}
1094 1094
 	return sb.DisableService()
... ...
@@ -997,6 +997,32 @@ func (c *Controller) WalkSandboxes(walker SandboxWalker) {
997 997
 	}
998 998
 }
999 999
 
1000
+// GetSandbox returns the Sandbox which has the passed id.
1001
+//
1002
+// It returns an [ErrInvalidID] when passing an invalid ID, or an
1003
+// [types.NotFoundError] if no Sandbox was found for the container.
1004
+func (c *Controller) GetSandbox(containerID string) (*Sandbox, error) {
1005
+	if containerID == "" {
1006
+		return nil, ErrInvalidID("id is empty")
1007
+	}
1008
+	c.mu.Lock()
1009
+	defer c.mu.Unlock()
1010
+	if runtime.GOOS == "windows" {
1011
+		// fast-path for Windows, which uses the container ID as sandbox ID.
1012
+		if sb := c.sandboxes[containerID]; sb != nil && !sb.isStub {
1013
+			return sb, nil
1014
+		}
1015
+	} else {
1016
+		for _, sb := range c.sandboxes {
1017
+			if sb.containerID == containerID && !sb.isStub {
1018
+				return sb, nil
1019
+			}
1020
+		}
1021
+	}
1022
+
1023
+	return nil, types.NotFoundErrorf("network sandbox for container %s not found", containerID)
1024
+}
1025
+
1000 1026
 // SandboxByID returns the Sandbox which has the passed id.
1001 1027
 // If not found, a [types.NotFoundError] is returned.
1002 1028
 func (c *Controller) SandboxByID(id string) (*Sandbox, error) {
... ...
@@ -2109,11 +2109,10 @@ func (pt parallelTester) Do(t *testing.T, thrNumber int) error {
2109 2109
 		return errors.New("got nil ep with no error")
2110 2110
 	}
2111 2111
 
2112
-	var sb *libnetwork.Sandbox
2113 2112
 	cid := fmt.Sprintf("%drace", thrNumber)
2114
-	pt.controller.WalkSandboxes(libnetwork.SandboxContainerWalker(&sb, cid))
2115
-	if sb == nil {
2116
-		return errors.Errorf("got nil sandbox for container: %s", cid)
2113
+	sb, err := pt.controller.GetSandbox(cid)
2114
+	if err != nil {
2115
+		return err
2117 2116
 	}
2118 2117
 
2119 2118
 	for i := 0; i < pt.iterCnt; i++ {
... ...
@@ -147,7 +147,10 @@ func (sb *Sandbox) updateParentHosts() error {
147 147
 	var pSb *Sandbox
148 148
 
149 149
 	for _, update := range sb.config.parentUpdates {
150
-		sb.controller.WalkSandboxes(SandboxContainerWalker(&pSb, update.cid))
150
+		// TODO(thaJeztah): was it intentional for this loop to re-use prior results of pSB? If not, we should make pSb local and always replace here.
151
+		if s, _ := sb.controller.GetSandbox(update.cid); s != nil {
152
+			pSb = s
153
+		}
151 154
 		if pSb == nil {
152 155
 			continue
153 156
 		}
... ...
@@ -164,15 +164,11 @@ func (c *Controller) processExternalKey(conn net.Conn) error {
164 164
 	if err = json.Unmarshal(buf[0:nr], &s); err != nil {
165 165
 		return err
166 166
 	}
167
-
168
-	var sandbox *Sandbox
169
-	search := SandboxContainerWalker(&sandbox, s.ContainerID)
170
-	c.WalkSandboxes(search)
171
-	if sandbox == nil {
172
-		return types.InvalidParameterErrorf("no sandbox present for %s", s.ContainerID)
167
+	sb, err := c.GetSandbox(s.ContainerID)
168
+	if err != nil {
169
+		return types.InvalidParameterErrorf("failed to get sandbox for %s", s.ContainerID)
173 170
 	}
174
-
175
-	return sandbox.SetKey(s.Key)
171
+	return sb.SetKey(s.Key)
176 172
 }
177 173
 
178 174
 func (c *Controller) stopExternalKeyListener() {
... ...
@@ -6,12 +6,15 @@ import (
6 6
 	"strconv"
7 7
 	"testing"
8 8
 
9
+	"github.com/docker/docker/errdefs"
9 10
 	"github.com/docker/docker/internal/testutils/netnsutils"
10 11
 	"github.com/docker/docker/libnetwork/config"
11 12
 	"github.com/docker/docker/libnetwork/ipamapi"
12 13
 	"github.com/docker/docker/libnetwork/netlabel"
13 14
 	"github.com/docker/docker/libnetwork/options"
14 15
 	"github.com/docker/docker/libnetwork/osl"
16
+	"gotest.tools/v3/assert"
17
+	is "gotest.tools/v3/assert/cmp"
15 18
 )
16 19
 
17 20
 func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network) {
... ...
@@ -51,6 +54,42 @@ func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network)
51 51
 	return c, nwList
52 52
 }
53 53
 
54
+func TestControllerGetSandbox(t *testing.T) {
55
+	ctrlr, _ := getTestEnv(t)
56
+	t.Run("invalid id", func(t *testing.T) {
57
+		const cID = ""
58
+		sb, err := ctrlr.GetSandbox(cID)
59
+		_, ok := err.(ErrInvalidID)
60
+		assert.Check(t, ok, "expected ErrInvalidID, got %[1]v (%[1]T)", err)
61
+		assert.Check(t, is.Nil(sb))
62
+	})
63
+	t.Run("not found", func(t *testing.T) {
64
+		const cID = "container-id-with-no-sandbox"
65
+		sb, err := ctrlr.GetSandbox(cID)
66
+		assert.Check(t, errdefs.IsNotFound(err), "expected  a ErrNotFound, got %[1]v (%[1]T)", err)
67
+		assert.Check(t, is.Nil(sb))
68
+	})
69
+	t.Run("existing sandbox", func(t *testing.T) {
70
+		const cID = "test-container-id"
71
+		expected, err := ctrlr.NewSandbox(cID)
72
+		assert.Check(t, err)
73
+
74
+		sb, err := ctrlr.GetSandbox(cID)
75
+		assert.Check(t, err)
76
+		assert.Check(t, is.Equal(sb.ContainerID(), cID))
77
+		assert.Check(t, is.Equal(sb.ID(), expected.ID()))
78
+		assert.Check(t, is.Equal(sb.Key(), expected.Key()))
79
+		assert.Check(t, is.Equal(sb.ContainerID(), expected.ContainerID()))
80
+
81
+		err = sb.Delete()
82
+		assert.Check(t, err)
83
+
84
+		sb, err = ctrlr.GetSandbox(cID)
85
+		assert.Check(t, errdefs.IsNotFound(err), "expected  a ErrNotFound, got %[1]v (%[1]T)", err)
86
+		assert.Check(t, is.Nil(sb))
87
+	})
88
+}
89
+
54 90
 func TestSandboxAddEmpty(t *testing.T) {
55 91
 	ctrlr, _ := getTestEnv(t)
56 92