From 7917a36cc787ada58987320e67cc6d96858f3b55 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 31 Jan 2017 21:03:51 -0500 Subject: [PATCH] Fix some data races After running the test suite with the race detector enabled I found these gems that need to be fixed. This is just round one, sadly lost my test results after I built the binary to test this... (whoops) Signed-off-by: Brian Goff --- api/types/network/network.go | 27 +++++++++++++++++++++++++++ daemon/container.go | 2 ++ daemon/create.go | 2 +- daemon/inspect.go | 11 +++++++++-- daemon/monitor.go | 5 ++++- daemon/start.go | 5 ----- runconfig/config_unix.go | 2 +- runconfig/hostconfig.go | 3 +-- 8 files changed, 45 insertions(+), 12 deletions(-) diff --git a/api/types/network/network.go b/api/types/network/network.go index 07ea05035b..d04deae6de 100644 --- a/api/types/network/network.go +++ b/api/types/network/network.go @@ -28,6 +28,14 @@ type EndpointIPAMConfig struct { LinkLocalIPs []string `json:",omitempty"` } +// Copy makes a copy of the endpoint ipam config +func (cfg *EndpointIPAMConfig) Copy() *EndpointIPAMConfig { + cfgCopy := *cfg + cfgCopy.LinkLocalIPs = make([]string, 0, len(cfg.LinkLocalIPs)) + cfgCopy.LinkLocalIPs = append(cfgCopy.LinkLocalIPs, cfg.LinkLocalIPs...) + return &cfgCopy +} + // PeerInfo represents one peer of an overlay network type PeerInfo struct { Name string @@ -52,6 +60,25 @@ type EndpointSettings struct { MacAddress string } +// Copy makes a deep copy of `EndpointSettings` +func (es *EndpointSettings) Copy() *EndpointSettings { + epCopy := *es + if es.IPAMConfig != nil { + epCopy.IPAMConfig = es.IPAMConfig.Copy() + } + + if es.Links != nil { + links := make([]string, 0, len(es.Links)) + epCopy.Links = append(links, es.Links...) + } + + if es.Aliases != nil { + aliases := make([]string, 0, len(es.Aliases)) + epCopy.Aliases = append(aliases, es.Aliases...) + } + return &epCopy +} + // NetworkingConfig represents the container's networking configuration for each of its interfaces // Carries the networking configs specified in the `docker run` and `docker network connect` commands type NetworkingConfig struct { diff --git a/daemon/container.go b/daemon/container.go index 53705c9a48..81b6e00ed5 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/system" "github.com/docker/docker/pkg/truncindex" + "github.com/docker/docker/runconfig" "github.com/docker/go-connections/nat" ) @@ -201,6 +202,7 @@ func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig * return err } + runconfig.SetDefaultNetModeIfBlank(hostConfig) container.HostConfig = hostConfig return container.ToDisk() } diff --git a/daemon/create.go b/daemon/create.go index 7bfe9f120c..f2e97f9a79 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -143,7 +143,7 @@ func (daemon *Daemon) create(params types.ContainerCreateConfig, managed bool) ( } // Make sure NetworkMode has an acceptable value. We do this to ensure // backwards API compatibility. - container.HostConfig = runconfig.SetDefaultNetModeIfBlank(container.HostConfig) + runconfig.SetDefaultNetModeIfBlank(container.HostConfig) daemon.updateContainerNetworkSettings(container, endpointsConfigs) diff --git a/daemon/inspect.go b/daemon/inspect.go index 557f639de1..73bc12b2f6 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/api/types/versions/v1p20" "github.com/docker/docker/container" "github.com/docker/docker/daemon/network" + "github.com/docker/go-connections/nat" ) // ContainerInspect returns low-level information about a @@ -45,7 +46,8 @@ func (daemon *Daemon) ContainerInspectCurrent(name string, size bool) (*types.Co apiNetworks := make(map[string]*networktypes.EndpointSettings) for name, epConf := range container.NetworkSettings.Networks { if epConf.EndpointSettings != nil { - apiNetworks[name] = epConf.EndpointSettings + // We must make a copy of this pointer object otherwise it can race with other operations + apiNetworks[name] = epConf.EndpointSettings.Copy() } } @@ -57,7 +59,6 @@ func (daemon *Daemon) ContainerInspectCurrent(name string, size bool) (*types.Co HairpinMode: container.NetworkSettings.HairpinMode, LinkLocalIPv6Address: container.NetworkSettings.LinkLocalIPv6Address, LinkLocalIPv6PrefixLen: container.NetworkSettings.LinkLocalIPv6PrefixLen, - Ports: container.NetworkSettings.Ports, SandboxKey: container.NetworkSettings.SandboxKey, SecondaryIPAddresses: container.NetworkSettings.SecondaryIPAddresses, SecondaryIPv6Addresses: container.NetworkSettings.SecondaryIPv6Addresses, @@ -66,6 +67,12 @@ func (daemon *Daemon) ContainerInspectCurrent(name string, size bool) (*types.Co Networks: apiNetworks, } + ports := make(nat.PortMap, len(container.NetworkSettings.Ports)) + for k, pm := range container.NetworkSettings.Ports { + ports[k] = pm + } + networkSettings.NetworkSettingsBase.Ports = ports + return &types.ContainerJSON{ ContainerJSONBase: base, Mounts: mountPoints, diff --git a/daemon/monitor.go b/daemon/monitor.go index ee0d1fcce0..bb06421332 100644 --- a/daemon/monitor.go +++ b/daemon/monitor.go @@ -31,7 +31,10 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error { case libcontainerd.StateExit: // if container's AutoRemove flag is set, remove it after clean up autoRemove := func() { - if c.HostConfig.AutoRemove { + c.Lock() + ar := c.HostConfig.AutoRemove + c.Unlock() + if ar { if err := daemon.ContainerRm(c.ID, &types.ContainerRmConfig{ForceRemove: true, RemoveVolume: true}); err != nil { logrus.Errorf("can't remove container %s: %v", c.ID, err) } diff --git a/daemon/start.go b/daemon/start.go index cc9d614fb7..eddb5d3d50 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -15,7 +15,6 @@ import ( "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" - "github.com/docker/docker/runconfig" ) // ContainerStart starts a container. @@ -138,10 +137,6 @@ func (daemon *Daemon) containerStart(container *container.Container, checkpoint return err } - // Make sure NetworkMode has an acceptable value. We do this to ensure - // backwards API compatibility. - container.HostConfig = runconfig.SetDefaultNetModeIfBlank(container.HostConfig) - if err := daemon.initializeNetworking(container); err != nil { return err } diff --git a/runconfig/config_unix.go b/runconfig/config_unix.go index 4ccfc73be2..b4fbfb2799 100644 --- a/runconfig/config_unix.go +++ b/runconfig/config_unix.go @@ -53,7 +53,7 @@ func (w *ContainerConfigWrapper) getHostConfig() *container.HostConfig { // Make sure NetworkMode has an acceptable value. We do this to ensure // backwards compatible API behavior. - hc = SetDefaultNetModeIfBlank(hc) + SetDefaultNetModeIfBlank(hc) return hc } diff --git a/runconfig/hostconfig.go b/runconfig/hostconfig.go index 2b81d02c20..a95fac6823 100644 --- a/runconfig/hostconfig.go +++ b/runconfig/hostconfig.go @@ -25,11 +25,10 @@ func DecodeHostConfig(src io.Reader) (*container.HostConfig, error) { // to default if it is not populated. This ensures backwards compatibility after // the validation of the network mode was moved from the docker CLI to the // docker daemon. -func SetDefaultNetModeIfBlank(hc *container.HostConfig) *container.HostConfig { +func SetDefaultNetModeIfBlank(hc *container.HostConfig) { if hc != nil { if hc.NetworkMode == container.NetworkMode("") { hc.NetworkMode = container.NetworkMode("default") } } - return hc }