Merge pull request #46206 from thaJeztah/libnetwork_no_walk

libnetwork: implement Controller.GetSandbox, remove Controller.WalkSandboxes and related code

daemon/container_operations.go

@@ -532,7 +532,11 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
 	// If the container is not to be connected to any network,
 	// create its network sandbox now if not present
 	if len(networks) == 0 {
-		if nil == daemon.getNetworkSandbox(container) {
+		if _, err := daemon.netController.GetSandbox(container.ID); err != nil {
+			if !errdefs.IsNotFound(err) {
+				return err
+			}
+
 			sbOptions, err := daemon.buildSandboxOptions(cfg, container)
 			if err != nil {
 				return err
@@ -557,18 +561,6 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C
 	return nil
 }
 
-func (daemon *Daemon) getNetworkSandbox(container *container.Container) *libnetwork.Sandbox {
-	var sb *libnetwork.Sandbox
-	daemon.netController.WalkSandboxes(func(s *libnetwork.Sandbox) bool {
-		if s.ContainerID() == container.ID {
-			sb = s
-			return true
-		}
-		return false
-	})
-	return sb
-}
-
 // hasUserDefinedIPAddress returns whether the passed IPAM configuration contains IP address configuration
 func hasUserDefinedIPAddress(ipamConfig *networktypes.EndpointIPAMConfig) bool {
 	return ipamConfig != nil && (len(ipamConfig.IPv4Address) > 0 || len(ipamConfig.IPv6Address) > 0)
@@ -715,7 +707,8 @@ func (daemon *Daemon) connectToNetwork(cfg *config.Config, container *container.
 		return err
 	}
 
-	sb := daemon.getNetworkSandbox(container)
+	// TODO(thaJeztah): should this fail early if no sandbox was found?
+	sb, _ := daemon.netController.GetSandbox(container.ID)
 	createOptions, err := buildCreateEndpointOptions(container, n, endpointConfig, sb, cfg.DNS)
 	if err != nil {
 		return err
@@ -1072,9 +1065,9 @@ func (daemon *Daemon) ActivateContainerServiceBinding(containerName string) erro
 	if err != nil {
 		return err
 	}
-	sb := daemon.getNetworkSandbox(ctr)
-	if sb == nil {
-		return fmt.Errorf("network sandbox does not exist for container %s", containerName)
+	sb, err := daemon.netController.GetSandbox(ctr.ID)
+	if err != nil {
+		return fmt.Errorf("failed to activate service binding for container %s: %w", containerName, err)
 	}
 	return sb.EnableService()
 }
@@ -1085,10 +1078,10 @@ func (daemon *Daemon) DeactivateContainerServiceBinding(containerName string) er
 	if err != nil {
 		return err
 	}
-	sb := daemon.getNetworkSandbox(ctr)
-	if sb == nil {
+	sb, err := daemon.netController.GetSandbox(ctr.ID)
+	if err != nil {
 		// If the network sandbox is not found, then there is nothing to deactivate
-		log.G(context.TODO()).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName)
+		log.G(context.TODO()).WithError(err).Debugf("Could not find network sandbox for container %s on service binding deactivation request", containerName)
 		return nil
 	}
 	return sb.DisableService()

libnetwork/controller.go

@@ -78,18 +78,12 @@ import (
 // When the function returns true, the walk will stop.
 type NetworkWalker func(nw *Network) bool
 
-// SandboxWalker is a client provided function which will be used to walk the Sandboxes.
-// When the function returns true, the walk will stop.
-type SandboxWalker func(sb *Sandbox) bool
-
-type sandboxTable map[string]*Sandbox
-
 // Controller manages networks.
 type Controller struct {
 	id               string
 	drvRegistry      drvregistry.Networks
 	ipamRegistry     drvregistry.IPAMs
-	sandboxes        sandboxTable
+	sandboxes        map[string]*Sandbox
 	cfg              *config.Config
 	store            *datastore.Store
 	extKeyListener   net.Listener
@@ -115,7 +109,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) {
 	c := &Controller{
 		id:               stringid.GenerateRandomID(),
 		cfg:              config.New(cfgOptions...),
-		sandboxes:        sandboxTable{},
+		sandboxes:        map[string]*Sandbox{},
 		svcRecords:       make(map[string]*svcInfo),
 		serviceBindings:  make(map[serviceKey]*service),
 		agentInitDone:    make(chan struct{}),
@@ -972,31 +966,30 @@ func (c *Controller) NewSandbox(containerID string, options ...SandboxOption) (*
 	return sb, nil
 }
 
-// Sandboxes returns the list of Sandbox(s) managed by this controller.
-func (c *Controller) Sandboxes() []*Sandbox {
+// GetSandbox returns the Sandbox which has the passed id.
+//
+// It returns an [ErrInvalidID] when passing an invalid ID, or an
+// [types.NotFoundError] if no Sandbox was found for the container.
+func (c *Controller) GetSandbox(containerID string) (*Sandbox, error) {
+	if containerID == "" {
+		return nil, ErrInvalidID("id is empty")
+	}
 	c.mu.Lock()
 	defer c.mu.Unlock()
-
-	list := make([]*Sandbox, 0, len(c.sandboxes))
-	for _, s := range c.sandboxes {
-		// Hide stub sandboxes from libnetwork users
-		if s.isStub {
-			continue
+	if runtime.GOOS == "windows" {
+		// fast-path for Windows, which uses the container ID as sandbox ID.
+		if sb := c.sandboxes[containerID]; sb != nil && !sb.isStub {
+			return sb, nil
 		}
-
-		list = append(list, s)
-	}
-
-	return list
-}
-
-// WalkSandboxes uses the provided function to walk the Sandbox(s) managed by this controller.
-func (c *Controller) WalkSandboxes(walker SandboxWalker) {
-	for _, sb := range c.Sandboxes() {
-		if walker(sb) {
-			return
+	} else {
+		for _, sb := range c.sandboxes {
+			if sb.containerID == containerID && !sb.isStub {
+				return sb, nil
+			}
 		}
 	}
+
+	return nil, types.NotFoundErrorf("network sandbox for container %s not found", containerID)
 }
 
 // SandboxByID returns the Sandbox which has the passed id.
@@ -1034,28 +1027,6 @@ func (c *Controller) SandboxDestroy(id string) error {
 	return sb.Delete()
 }
 
-// SandboxContainerWalker returns a Sandbox Walker function which looks for an existing Sandbox with the passed containerID
-func SandboxContainerWalker(out **Sandbox, containerID string) SandboxWalker {
-	return func(sb *Sandbox) bool {
-		if sb.ContainerID() == containerID {
-			*out = sb
-			return true
-		}
-		return false
-	}
-}
-
-// SandboxKeyWalker returns a Sandbox Walker function which looks for an existing Sandbox with the passed key
-func SandboxKeyWalker(out **Sandbox, key string) SandboxWalker {
-	return func(sb *Sandbox) bool {
-		if sb.Key() == key {
-			*out = sb
-			return true
-		}
-		return false
-	}
-}
-
 func (c *Controller) loadDriver(networkType string) error {
 	var err error
 

libnetwork/libnetwork_linux_test.go

@@ -2089,24 +2089,17 @@ type parallelTester struct {
 }
 
 func (pt parallelTester) Do(t *testing.T, thrNumber int) error {
-	var (
-		ep  *libnetwork.Endpoint
-		sb  *libnetwork.Sandbox
-		err error
-	)
-
 	teardown, err := pt.osctx.Set()
 	if err != nil {
 		return err
 	}
 	defer teardown(t)
 
-	epName := fmt.Sprintf("pep%d", thrNumber)
-
+	var ep *libnetwork.Endpoint
 	if thrNumber == 1 {
-		ep, err = pt.net1.EndpointByName(epName)
+		ep, err = pt.net1.EndpointByName(fmt.Sprintf("pep%d", thrNumber))
 	} else {
-		ep, err = pt.net2.EndpointByName(epName)
+		ep, err = pt.net2.EndpointByName(fmt.Sprintf("pep%d", thrNumber))
 	}
 
 	if err != nil {
@@ -2117,9 +2110,9 @@ func (pt parallelTester) Do(t *testing.T, thrNumber int) error {
 	}
 
 	cid := fmt.Sprintf("%drace", thrNumber)
-	pt.controller.WalkSandboxes(libnetwork.SandboxContainerWalker(&sb, cid))
-	if sb == nil {
-		return errors.Errorf("got nil sandbox for container: %s", cid)
+	sb, err := pt.controller.GetSandbox(cid)
+	if err != nil {
+		return err
 	}
 
 	for i := 0; i < pt.iterCnt; i++ {

libnetwork/sandbox_dns_unix.go

@@ -147,7 +147,10 @@ func (sb *Sandbox) updateParentHosts() error {
 	var pSb *Sandbox
 
 	for _, update := range sb.config.parentUpdates {
-		sb.controller.WalkSandboxes(SandboxContainerWalker(&pSb, update.cid))
+		// 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.
+		if s, _ := sb.controller.GetSandbox(update.cid); s != nil {
+			pSb = s
+		}
 		if pSb == nil {
 			continue
 		}

libnetwork/sandbox_externalkey_unix.go

@@ -164,15 +164,11 @@ func (c *Controller) processExternalKey(conn net.Conn) error {
 	if err = json.Unmarshal(buf[0:nr], &s); err != nil {
 		return err
 	}
-
-	var sandbox *Sandbox
-	search := SandboxContainerWalker(&sandbox, s.ContainerID)
-	c.WalkSandboxes(search)
-	if sandbox == nil {
-		return types.InvalidParameterErrorf("no sandbox present for %s", s.ContainerID)
+	sb, err := c.GetSandbox(s.ContainerID)
+	if err != nil {
+		return types.InvalidParameterErrorf("failed to get sandbox for %s", s.ContainerID)
 	}
-
-	return sandbox.SetKey(s.Key)
+	return sb.SetKey(s.Key)
 }
 
 func (c *Controller) stopExternalKeyListener() {

libnetwork/sandbox_unix_test.go

@@ -6,12 +6,15 @@ import (
 	"strconv"
 	"testing"
 
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/internal/testutils/netnsutils"
 	"github.com/docker/docker/libnetwork/config"
 	"github.com/docker/docker/libnetwork/ipamapi"
 	"github.com/docker/docker/libnetwork/netlabel"
 	"github.com/docker/docker/libnetwork/options"
 	"github.com/docker/docker/libnetwork/osl"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 )
 
 func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network) {
@@ -51,6 +54,42 @@ func getTestEnv(t *testing.T, opts ...[]NetworkOption) (*Controller, []*Network)
 	return c, nwList
 }
 
+func TestControllerGetSandbox(t *testing.T) {
+	ctrlr, _ := getTestEnv(t)
+	t.Run("invalid id", func(t *testing.T) {
+		const cID = ""
+		sb, err := ctrlr.GetSandbox(cID)
+		_, ok := err.(ErrInvalidID)
+		assert.Check(t, ok, "expected ErrInvalidID, got %[1]v (%[1]T)", err)
+		assert.Check(t, is.Nil(sb))
+	})
+	t.Run("not found", func(t *testing.T) {
+		const cID = "container-id-with-no-sandbox"
+		sb, err := ctrlr.GetSandbox(cID)
+		assert.Check(t, errdefs.IsNotFound(err), "expected  a ErrNotFound, got %[1]v (%[1]T)", err)
+		assert.Check(t, is.Nil(sb))
+	})
+	t.Run("existing sandbox", func(t *testing.T) {
+		const cID = "test-container-id"
+		expected, err := ctrlr.NewSandbox(cID)
+		assert.Check(t, err)
+
+		sb, err := ctrlr.GetSandbox(cID)
+		assert.Check(t, err)
+		assert.Check(t, is.Equal(sb.ContainerID(), cID))
+		assert.Check(t, is.Equal(sb.ID(), expected.ID()))
+		assert.Check(t, is.Equal(sb.Key(), expected.Key()))
+		assert.Check(t, is.Equal(sb.ContainerID(), expected.ContainerID()))
+
+		err = sb.Delete()
+		assert.Check(t, err)
+
+		sb, err = ctrlr.GetSandbox(cID)
+		assert.Check(t, errdefs.IsNotFound(err), "expected  a ErrNotFound, got %[1]v (%[1]T)", err)
+		assert.Check(t, is.Nil(sb))
+	})
+}
+
 func TestSandboxAddEmpty(t *testing.T) {
 	ctrlr, _ := getTestEnv(t)