From 43a50b06187e324e43b26003cace0f351a5fb78c Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 30 Mar 2015 17:14:46 -0700 Subject: [PATCH 1/3] Refactor port allocator to not have ANY global state Signed-off-by: Michael Crosby --- .../portallocator/portallocator.go | 139 ++++++++---------- .../portallocator/portallocator_test.go | 25 ++-- 2 files changed, 73 insertions(+), 91 deletions(-) diff --git a/daemon/networkdriver/portallocator/portallocator.go b/daemon/networkdriver/portallocator/portallocator.go index e2bb9ee56..c1f414b67 100644 --- a/daemon/networkdriver/portallocator/portallocator.go +++ b/daemon/networkdriver/portallocator/portallocator.go @@ -16,59 +16,14 @@ const ( DefaultPortRangeEnd = 65535 ) -var ( - beginPortRange = DefaultPortRangeStart - endPortRange = DefaultPortRangeEnd -) - -type portMap struct { - p map[int]struct{} - last int -} - -func newPortMap() *portMap { - return &portMap{ - p: map[int]struct{}{}, - last: endPortRange, - } -} - -type protoMap map[string]*portMap - -func newProtoMap() protoMap { - return protoMap{ - "tcp": newPortMap(), - "udp": newPortMap(), - } -} - type ipMapping map[string]protoMap var ( ErrAllPortsAllocated = errors.New("all ports are allocated") ErrUnknownProtocol = errors.New("unknown protocol") + defaultIP = net.ParseIP("0.0.0.0") ) -var ( - defaultIP = net.ParseIP("0.0.0.0") - - DefaultPortAllocator = New() - RequestPort = DefaultPortAllocator.RequestPort - ReleasePort = DefaultPortAllocator.ReleasePort - ReleaseAll = DefaultPortAllocator.ReleaseAll -) - -type PortAllocator struct { - mutex sync.Mutex - ipMap ipMapping -} - -func New() *PortAllocator { - return &PortAllocator{ - ipMap: ipMapping{}, - } -} - type ErrPortAlreadyAllocated struct { ip string port int @@ -81,32 +36,6 @@ func NewErrPortAlreadyAllocated(ip string, port int) ErrPortAlreadyAllocated { } } -func init() { - const portRangeKernelParam = "/proc/sys/net/ipv4/ip_local_port_range" - portRangeFallback := fmt.Sprintf("using fallback port range %d-%d", beginPortRange, endPortRange) - - file, err := os.Open(portRangeKernelParam) - if err != nil { - logrus.Warnf("port allocator - %s due to error: %v", portRangeFallback, err) - return - } - var start, end int - n, err := fmt.Fscanf(bufio.NewReader(file), "%d\t%d", &start, &end) - if n != 2 || err != nil { - if err == nil { - err = fmt.Errorf("unexpected count of parsed numbers (%d)", n) - } - logrus.Errorf("port allocator - failed to parse system ephemeral port range from %s - %s: %v", portRangeKernelParam, portRangeFallback, err) - return - } - beginPortRange = start - endPortRange = end -} - -func PortRange() (int, int) { - return beginPortRange, endPortRange -} - func (e ErrPortAlreadyAllocated) IP() string { return e.ip } @@ -123,6 +52,51 @@ func (e ErrPortAlreadyAllocated) Error() string { return fmt.Sprintf("Bind for %s:%d failed: port is already allocated", e.ip, e.port) } +type ( + PortAllocator struct { + mutex sync.Mutex + ipMap ipMapping + Begin int + End int + } + portMap struct { + p map[int]struct{} + begin, end int + last int + } + protoMap map[string]*portMap +) + +func New() *PortAllocator { + start, end, err := getDynamicPortRange() + if err != nil { + logrus.Warn(err) + start, end = DefaultPortRangeStart, DefaultPortRangeEnd + } + return &PortAllocator{ + ipMap: ipMapping{}, + Begin: start, + End: end, + } +} + +func getDynamicPortRange() (start int, end int, err error) { + const portRangeKernelParam = "/proc/sys/net/ipv4/ip_local_port_range" + portRangeFallback := fmt.Sprintf("using fallback port range %d-%d", DefaultPortRangeStart, DefaultPortRangeEnd) + file, err := os.Open(portRangeKernelParam) + if err != nil { + return 0, 0, fmt.Errorf("port allocator - %s due to error: %v", portRangeFallback, err) + } + n, err := fmt.Fscanf(bufio.NewReader(file), "%d\t%d", &start, &end) + if n != 2 || err != nil { + if err == nil { + err = fmt.Errorf("unexpected count of parsed numbers (%d)", n) + } + return 0, 0, fmt.Errorf("port allocator - failed to parse system ephemeral port range from %s - %s: %v", portRangeKernelParam, portRangeFallback, err) + } + return start, end, nil +} + // RequestPort requests new port from global ports pool for specified ip and proto. // If port is 0 it returns first free port. Otherwise it cheks port availability // in pool and return that port or error if port is already busy. @@ -140,7 +114,11 @@ func (p *PortAllocator) RequestPort(ip net.IP, proto string, port int) (int, err ipstr := ip.String() protomap, ok := p.ipMap[ipstr] if !ok { - protomap = newProtoMap() + protomap = protoMap{ + "tcp": p.newPortMap(), + "udp": p.newPortMap(), + } + p.ipMap[ipstr] = protomap } mapping := protomap[proto] @@ -175,6 +153,15 @@ func (p *PortAllocator) ReleasePort(ip net.IP, proto string, port int) error { return nil } +func (p *PortAllocator) newPortMap() *portMap { + return &portMap{ + p: map[int]struct{}{}, + begin: p.Begin, + end: p.End, + last: p.End, + } +} + // ReleaseAll releases all ports for all ips. func (p *PortAllocator) ReleaseAll() error { p.mutex.Lock() @@ -185,10 +172,10 @@ func (p *PortAllocator) ReleaseAll() error { func (pm *portMap) findPort() (int, error) { port := pm.last - for i := 0; i <= endPortRange-beginPortRange; i++ { + for i := 0; i <= pm.end-pm.begin; i++ { port++ - if port > endPortRange { - port = beginPortRange + if port > pm.end { + port = pm.begin } if _, ok := pm.p[port]; !ok { diff --git a/daemon/networkdriver/portallocator/portallocator_test.go b/daemon/networkdriver/portallocator/portallocator_test.go index f6f122bbd..17201235e 100644 --- a/daemon/networkdriver/portallocator/portallocator_test.go +++ b/daemon/networkdriver/portallocator/portallocator_test.go @@ -5,11 +5,6 @@ import ( "testing" ) -func init() { - beginPortRange = DefaultPortRangeStart - endPortRange = DefaultPortRangeEnd -} - func TestRequestNewPort(t *testing.T) { p := New() @@ -18,7 +13,7 @@ func TestRequestNewPort(t *testing.T) { t.Fatal(err) } - if expected := beginPortRange; port != expected { + if expected := p.Begin; port != expected { t.Fatalf("Expected port %d got %d", expected, port) } } @@ -101,13 +96,13 @@ func TestUnknowProtocol(t *testing.T) { func TestAllocateAllPorts(t *testing.T) { p := New() - for i := 0; i <= endPortRange-beginPortRange; i++ { + for i := 0; i <= p.End-p.Begin; i++ { port, err := p.RequestPort(defaultIP, "tcp", 0) if err != nil { t.Fatal(err) } - if expected := beginPortRange + i; port != expected { + if expected := p.Begin + i; port != expected { t.Fatalf("Expected port %d got %d", expected, port) } } @@ -122,7 +117,7 @@ func TestAllocateAllPorts(t *testing.T) { } // release a port in the middle and ensure we get another tcp port - port := beginPortRange + 5 + port := p.Begin + 5 if err := p.ReleasePort(defaultIP, "tcp", port); err != nil { t.Fatal(err) } @@ -152,13 +147,13 @@ func BenchmarkAllocatePorts(b *testing.B) { p := New() for i := 0; i < b.N; i++ { - for i := 0; i <= endPortRange-beginPortRange; i++ { + for i := 0; i <= p.End-p.Begin; i++ { port, err := p.RequestPort(defaultIP, "tcp", 0) if err != nil { b.Fatal(err) } - if expected := beginPortRange + i; port != expected { + if expected := p.Begin + i; port != expected { b.Fatalf("Expected port %d got %d", expected, port) } } @@ -230,15 +225,15 @@ func TestPortAllocation(t *testing.T) { func TestNoDuplicateBPR(t *testing.T) { p := New() - if port, err := p.RequestPort(defaultIP, "tcp", beginPortRange); err != nil { + if port, err := p.RequestPort(defaultIP, "tcp", p.Begin); err != nil { t.Fatal(err) - } else if port != beginPortRange { - t.Fatalf("Expected port %d got %d", beginPortRange, port) + } else if port != p.Begin { + t.Fatalf("Expected port %d got %d", p.Begin, port) } if port, err := p.RequestPort(defaultIP, "tcp", 0); err != nil { t.Fatal(err) - } else if port == beginPortRange { + } else if port == p.Begin { t.Fatalf("Acquire(0) allocated the same port twice: %d", port) } } From 62522c98539e1591017cf0d4f28e6a58f3b1ec6b Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 30 Mar 2015 17:31:21 -0700 Subject: [PATCH 2/3] Refactor portmapper to remove ALL global state Signed-off-by: Michael Crosby --- daemon/networkdriver/portmapper/mapper.go | 9 +-------- daemon/networkdriver/portmapper/mapper_test.go | 3 +-- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index a01b60416..7092352f5 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/daemon/networkdriver/portmapper/mapper.go @@ -18,14 +18,7 @@ type mapping struct { container net.Addr } -var ( - NewProxy = NewProxyCommand - - DefaultPortMapper = NewWithPortAllocator(portallocator.DefaultPortAllocator) - SetIptablesChain = DefaultPortMapper.SetIptablesChain - Map = DefaultPortMapper.Map - Unmap = DefaultPortMapper.Unmap -) +var NewProxy = NewProxyCommand var ( ErrUnknownBackendAddressType = errors.New("unknown container address type not supported") diff --git a/daemon/networkdriver/portmapper/mapper_test.go b/daemon/networkdriver/portmapper/mapper_test.go index 4082a6002..d5d10d8cb 100644 --- a/daemon/networkdriver/portmapper/mapper_test.go +++ b/daemon/networkdriver/portmapper/mapper_test.go @@ -4,7 +4,6 @@ import ( "net" "testing" - "github.com/docker/docker/daemon/networkdriver/portallocator" "github.com/docker/docker/pkg/iptables" ) @@ -126,7 +125,7 @@ func TestMapAllPortsSingleInterface(t *testing.T) { }() for i := 0; i < 10; i++ { - start, end := portallocator.PortRange() + start, end := pm.allocator.Begin, pm.allocator.End for i := start; i < end; i++ { if host, err = pm.Map(srcAddr1, dstIp1, 0); err != nil { t.Fatal(err) From d8c628cf082a50c0a2a5e381a21da8279a5462b4 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 30 Mar 2015 18:06:16 -0700 Subject: [PATCH 3/3] Ensure that bridge driver does not use global mappers This has a few hacks in it but it ensures that the bridge driver does not use global state in the mappers, atleast as much as possible at this point without further refactoring. Some of the exported fields are hacks to handle the daemon port mapping but this results in a much cleaner approach and completely remove the global state from the mapper and allocator. Signed-off-by: Michael Crosby --- api/server/server.go | 4 ++-- daemon/daemon.go | 7 ------- daemon/networkdriver/bridge/driver.go | 12 +++++++++--- daemon/networkdriver/portmapper/mapper.go | 16 ++++++++-------- daemon/networkdriver/portmapper/mapper_test.go | 2 +- 5 files changed, 20 insertions(+), 21 deletions(-) diff --git a/api/server/server.go b/api/server/server.go index a7d0a58b2..96abf5c3b 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -27,7 +27,7 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/docker/api" "github.com/docker/docker/api/types" - "github.com/docker/docker/daemon/networkdriver/portallocator" + "github.com/docker/docker/daemon/networkdriver/bridge" "github.com/docker/docker/engine" "github.com/docker/docker/pkg/listenbuffer" "github.com/docker/docker/pkg/parsers" @@ -1542,7 +1542,7 @@ func allocateDaemonPort(addr string) error { } for _, hostIP := range hostIPs { - if _, err := portallocator.RequestPort(hostIP, "tcp", intPort); err != nil { + if _, err := bridge.RequestPort(hostIP, "tcp", intPort); err != nil { return fmt.Errorf("failed to allocate daemon listening port %d (err: %v)", intPort, err) } } diff --git a/daemon/daemon.go b/daemon/daemon.go index ba9c73ce6..36cf438bb 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -25,7 +25,6 @@ import ( "github.com/docker/docker/daemon/graphdriver" _ "github.com/docker/docker/daemon/graphdriver/vfs" _ "github.com/docker/docker/daemon/networkdriver/bridge" - "github.com/docker/docker/daemon/networkdriver/portallocator" "github.com/docker/docker/engine" "github.com/docker/docker/graph" "github.com/docker/docker/image" @@ -818,12 +817,6 @@ func NewDaemonFromDirectory(config *Config, eng *engine.Engine) (*Daemon, error) } config.DisableNetwork = config.BridgeIface == disableNetworkBridge - // register portallocator release on shutdown - eng.OnShutdown(func() { - if err := portallocator.ReleaseAll(); err != nil { - logrus.Errorf("portallocator.ReleaseAll(): %s", err) - } - }) // Claim the pidfile first, to avoid any and all unexpected race conditions. // Some of the init doesn't need a pidfile lock - but let's not try to be smart. if config.Pidfile != "" { diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index c5f5704f9..21d8c4455 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/daemon/networkdriver/bridge/driver.go @@ -77,6 +77,7 @@ var ( bridgeIPv4Network *net.IPNet bridgeIPv6Addr net.IP globalIPv6Network *net.IPNet + portMapper *portmapper.PortMapper defaultBindingIP = net.ParseIP("0.0.0.0") currentInterfaces = ifaces{c: make(map[string]*networkInterface)} @@ -99,6 +100,7 @@ func InitDriver(job *engine.Job) error { fixedCIDR = job.Getenv("FixedCIDR") fixedCIDRv6 = job.Getenv("FixedCIDRv6") ) + portMapper = portmapper.New() if defaultIP := job.Getenv("DefaultBindingIP"); defaultIP != "" { defaultBindingIP = net.ParseIP(defaultIP) @@ -235,7 +237,7 @@ func InitDriver(job *engine.Job) error { if err != nil { return err } - portmapper.SetIptablesChain(chain) + portMapper.SetIptablesChain(chain) } bridgeIPv4Network = networkv4 @@ -350,6 +352,10 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error { return nil } +func RequestPort(ip net.IP, proto string, port int) (int, error) { + return portMapper.Allocator.RequestPort(ip, proto, port) +} + // configureBridge attempts to create and configure a network bridge interface named `bridgeIface` on the host // If bridgeIP is empty, it will try to find a non-conflicting IP from the Docker-specified private ranges // If the bridge `bridgeIface` already exists, it will only perform the IP address association with the existing @@ -587,7 +593,7 @@ func Release(job *engine.Job) error { } for _, nat := range containerInterface.PortMappings { - if err := portmapper.Unmap(nat); err != nil { + if err := portMapper.Unmap(nat); err != nil { logrus.Infof("Unable to unmap port %s: %s", nat, err) } } @@ -644,7 +650,7 @@ func AllocatePort(job *engine.Job) error { var host net.Addr for i := 0; i < MaxAllocatedPortAttempts; i++ { - if host, err = portmapper.Map(container, ip, hostPort); err == nil { + if host, err = portMapper.Map(container, ip, hostPort); err == nil { break } // There is no point in immediately retrying to map an explicitly diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index 7092352f5..8f79bae3f 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/daemon/networkdriver/portmapper/mapper.go @@ -33,7 +33,7 @@ type PortMapper struct { currentMappings map[string]*mapping lock sync.Mutex - allocator *portallocator.PortAllocator + Allocator *portallocator.PortAllocator } func New() *PortMapper { @@ -43,7 +43,7 @@ func New() *PortMapper { func NewWithPortAllocator(allocator *portallocator.PortAllocator) *PortMapper { return &PortMapper{ currentMappings: make(map[string]*mapping), - allocator: allocator, + Allocator: allocator, } } @@ -65,7 +65,7 @@ func (pm *PortMapper) Map(container net.Addr, hostIP net.IP, hostPort int) (host switch container.(type) { case *net.TCPAddr: proto = "tcp" - if allocatedHostPort, err = pm.allocator.RequestPort(hostIP, proto, hostPort); err != nil { + if allocatedHostPort, err = pm.Allocator.RequestPort(hostIP, proto, hostPort); err != nil { return nil, err } @@ -78,7 +78,7 @@ func (pm *PortMapper) Map(container net.Addr, hostIP net.IP, hostPort int) (host proxy = NewProxy(proto, hostIP, allocatedHostPort, container.(*net.TCPAddr).IP, container.(*net.TCPAddr).Port) case *net.UDPAddr: proto = "udp" - if allocatedHostPort, err = pm.allocator.RequestPort(hostIP, proto, hostPort); err != nil { + if allocatedHostPort, err = pm.Allocator.RequestPort(hostIP, proto, hostPort); err != nil { return nil, err } @@ -96,7 +96,7 @@ func (pm *PortMapper) Map(container net.Addr, hostIP net.IP, hostPort int) (host // release the allocated port on any further error during return. defer func() { if err != nil { - pm.allocator.ReleasePort(hostIP, proto, allocatedHostPort) + pm.Allocator.ReleasePort(hostIP, proto, allocatedHostPort) } }() @@ -114,7 +114,7 @@ func (pm *PortMapper) Map(container net.Addr, hostIP net.IP, hostPort int) (host // need to undo the iptables rules before we return proxy.Stop() pm.forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) - if err := pm.allocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil { + if err := pm.Allocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil { return err } @@ -154,9 +154,9 @@ func (pm *PortMapper) Unmap(host net.Addr) error { switch a := host.(type) { case *net.TCPAddr: - return pm.allocator.ReleasePort(a.IP, "tcp", a.Port) + return pm.Allocator.ReleasePort(a.IP, "tcp", a.Port) case *net.UDPAddr: - return pm.allocator.ReleasePort(a.IP, "udp", a.Port) + return pm.Allocator.ReleasePort(a.IP, "udp", a.Port) } return nil } diff --git a/daemon/networkdriver/portmapper/mapper_test.go b/daemon/networkdriver/portmapper/mapper_test.go index d5d10d8cb..729fe5607 100644 --- a/daemon/networkdriver/portmapper/mapper_test.go +++ b/daemon/networkdriver/portmapper/mapper_test.go @@ -125,7 +125,7 @@ func TestMapAllPortsSingleInterface(t *testing.T) { }() for i := 0; i < 10; i++ { - start, end := pm.allocator.Begin, pm.allocator.End + start, end := pm.Allocator.Begin, pm.Allocator.End for i := start; i < end; i++ { if host, err = pm.Map(srcAddr1, dstIp1, 0); err != nil { t.Fatal(err)