Skip to content

Commit 80c44b4

Browse files
akerouantonakerouanton
committed
daemon: rename: don't reload endpoint from datastore
Commit 8b7af1d added some code to update the DNSNames of all endpoints attached to a sandbox by loading a new instance of each affected endpoints from the datastore through a call to `Network.EndpointByID()`. This method then calls `Network.getEndpointFromStore()`, that in turn calls `store.GetObject()`, which then calls `cache.get()`, which calls `o.CopyTo(kvObject)`. This effectively creates a fresh new instance of an Endpoint. However, endpoints are already kept in memory by Sandbox, meaning we now have two in-memory instances of the same Endpoint. As it turns out, libnetwork is built around the idea that no two objects representing the same thing should leave in-memory, otherwise breaking mutex locking and optimistic locking (as both instances will have a drifting version tracking ID -- dbIndex in libnetwork parliance). In this specific case, this bug materializes by container rename failing when applied a second time for a given container. An integration test is added to make sure this won't happen again. Signed-off-by: Albin Kerouanton <[email protected]>
1 parent f19f233 commit 80c44b4

7 files changed

Lines changed: 34 additions & 11 deletions

File tree

‎daemon/rename.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package daemon // import "github.com/docker/docker/daemon"
22

33
import (
44
"context"
5+
"fmt"
56
"strings"
67

78
"github.com/containerd/log"
@@ -127,9 +128,9 @@ func (daemon *Daemon) ContainerRename(oldName, newName string) (retErr error) {
127128
return err
128129
}
129130

130-
ep, err := nw.EndpointByID(epConfig.EndpointID)
131-
if err != nil {
132-
return err
131+
ep := sb.GetEndpoint(epConfig.EndpointID)
132+
if ep == nil {
133+
return fmt.Errorf("no endpoint attached to network %s found", nw.Name())
133134
}
134135

135136
oldDNSNames := make([]string, len(epConfig.DNSNames))

‎integration/container/rename_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,3 +192,25 @@ func TestRenameContainerWithLinkedContainer(t *testing.T) {
192192
assert.NilError(t, err)
193193
assert.Check(t, is.Equal(db1ID, inspect.ID))
194194
}
195+
196+
// Regression test for https://github.com/moby/moby/issues/47186
197+
func TestRenameContainerTwice(t *testing.T) {
198+
ctx := setupTest(t)
199+
apiClient := testEnv.APIClient()
200+
201+
ctrName := "c0"
202+
container.Run(ctx, t, apiClient, container.WithName("c0"))
203+
defer func() {
204+
container.Remove(ctx, t, apiClient, ctrName, containertypes.RemoveOptions{
205+
Force: true,
206+
})
207+
}()
208+
209+
err := apiClient.ContainerRename(ctx, "c0", "c1")
210+
assert.NilError(t, err)
211+
ctrName = "c1"
212+
213+
err = apiClient.ContainerRename(ctx, "c1", "c2")
214+
assert.NilError(t, err)
215+
ctrName = "c2"
216+
}

‎libnetwork/agent.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,7 @@ func (ep *Endpoint) addServiceInfoToCluster(sb *Sandbox) error {
623623
// In case the deleteServiceInfoToCluster arrives first, this one is happening after the endpoint is
624624
// removed from the list, in this situation the delete will bail out not finding any data to cleanup
625625
// and the add will bail out not finding the endpoint on the sandbox.
626-
if err := sb.getEndpoint(ep.ID()); err == nil {
626+
if err := sb.GetEndpoint(ep.ID()); err == nil {
627627
log.G(context.TODO()).Warnf("addServiceInfoToCluster suppressing service resolution ep is not anymore in the sandbox %s", ep.ID())
628628
return nil
629629
}
@@ -692,7 +692,7 @@ func (ep *Endpoint) deleteServiceInfoFromCluster(sb *Sandbox, fullRemove bool, m
692692
// get caught in disableServceInNetworkDB, but we check here to make the
693693
// nature of the condition more clear.
694694
// See comment in addServiceInfoToCluster()
695-
if err := sb.getEndpoint(ep.ID()); err == nil {
695+
if err := sb.GetEndpoint(ep.ID()); err == nil {
696696
log.G(context.TODO()).Warnf("deleteServiceInfoFromCluster suppressing service resolution ep is not anymore in the sandbox %s", ep.ID())
697697
return nil
698698
}

‎libnetwork/endpoint_info.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func (ep *Endpoint) Info() EndpointInfo {
194194
return ep
195195
}
196196

197-
return sb.getEndpoint(ep.ID())
197+
return sb.GetEndpoint(ep.ID())
198198
}
199199

200200
// Iface returns information about the interface which was assigned to

‎libnetwork/network.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,8 +1276,8 @@ func (n *Network) EndpointByName(name string) (*Endpoint, error) {
12761276
return e, nil
12771277
}
12781278

1279-
// EndpointByID returns the Endpoint which has the passed id. If not found,
1280-
// the error ErrNoSuchEndpoint is returned.
1279+
// EndpointByID should *never* be called as it's going to create a 2nd instance of an Endpoint. The first one lives in
1280+
// the Sandbox the endpoint is attached to. Instead, the endpoint should be retrieved by calling [Sandbox.Endpoints()].
12811281
func (n *Network) EndpointByID(id string) (*Endpoint, error) {
12821282
if id == "" {
12831283
return nil, ErrInvalidID(id)

‎libnetwork/sandbox.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ func (sb *Sandbox) removeEndpointRaw(ep *Endpoint) {
334334
}
335335
}
336336

337-
func (sb *Sandbox) getEndpoint(id string) *Endpoint {
337+
func (sb *Sandbox) GetEndpoint(id string) *Endpoint {
338338
sb.mu.Lock()
339339
defer sb.mu.Unlock()
340340

@@ -554,7 +554,7 @@ func (sb *Sandbox) DisableService() (err error) {
554554
}
555555

556556
func (sb *Sandbox) clearNetworkResources(origEp *Endpoint) error {
557-
ep := sb.getEndpoint(origEp.id)
557+
ep := sb.GetEndpoint(origEp.id)
558558
if ep == nil {
559559
return fmt.Errorf("could not find the sandbox endpoint data for endpoint %s",
560560
origEp.id)

‎libnetwork/service_linux.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func (n *Network) findLBEndpointSandbox() (*Endpoint, *Sandbox, error) {
5757
if !ok {
5858
return nil, nil, fmt.Errorf("Unable to get sandbox for %s(%s) in for %s", ep.Name(), ep.ID(), n.ID())
5959
}
60-
sep := sb.getEndpoint(ep.ID())
60+
sep := sb.GetEndpoint(ep.ID())
6161
if sep == nil {
6262
return nil, nil, fmt.Errorf("Load balancing endpoint %s(%s) removed from %s", ep.Name(), ep.ID(), n.ID())
6363
}

0 commit comments

Comments
 (0)