From c2ea56d296ea0efa88d0976304d1c90e292d0450 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Tue, 17 Jan 2017 14:53:05 -0800 Subject: [PATCH 1/2] Fix issue in service update of published ports in host mode This fix tries to fix the issue in docker/docker/30199 where service update will not work when published ports is host publish mode. The reason for the issue is that `isPortsAllocated()` does not capture host publish mode. This fix address the issue by updating allocated ports if there is any change in ports of host (non ingress) mode as well. Unit tests have been added. Signed-off-by: Yong Tang --- manager/allocator/network.go | 45 +++++++- manager/allocator/network_test.go | 40 +++++++ .../networkallocator/networkallocator.go | 6 + .../networkallocator/portallocator.go | 27 +++++ .../networkallocator/portallocator_test.go | 104 ++++++++++++++++++ 5 files changed, 216 insertions(+), 6 deletions(-) create mode 100644 manager/allocator/network_test.go diff --git a/manager/allocator/network.go b/manager/allocator/network.go index 6e12c5a0a4..b30de9ad8f 100644 --- a/manager/allocator/network.go +++ b/manager/allocator/network.go @@ -371,12 +371,15 @@ func (a *Allocator) doNetworkAlloc(ctx context.Context, ev events.Event) { s := v.Service.Copy() if nc.nwkAllocator.IsServiceAllocated(s) { - break - } - - if err := a.allocateService(ctx, s); err != nil { - log.G(ctx).WithError(err).Errorf("Failed allocation during update of service %s", s.ID) - break + if nc.nwkAllocator.PortsAllocatedInHostPublishMode(s) { + break + } + updatePortsInHostPublishMode(s) + } else { + if err := a.allocateService(ctx, s); err != nil { + log.G(ctx).WithError(err).Errorf("Failed allocation during update of service %s", s.ID) + break + } } if _, err := a.store.Batch(func(batch *store.Batch) error { @@ -641,6 +644,36 @@ func (a *Allocator) commitAllocatedNode(ctx context.Context, batch *store.Batch, return nil } +// This function prepares the service object for being updated when the change regards +// the published ports in host mode: It resets the runtime state ports (s.Endpoint.Ports) +// to the current ingress mode runtime state ports plus the newly configured publish mode ports, +// so that the service allocation invoked on this new service object will trigger the deallocation +// of any old publish mode port and allocation of any new one. +func updatePortsInHostPublishMode(s *api.Service) { + if s.Endpoint != nil { + var portConfigs []*api.PortConfig + for _, portConfig := range s.Endpoint.Ports { + if portConfig.PublishMode == api.PublishModeIngress { + portConfigs = append(portConfigs, portConfig) + } + } + s.Endpoint.Ports = portConfigs + } + + if s.Spec.Endpoint != nil { + if s.Endpoint == nil { + s.Endpoint = &api.Endpoint{} + } + for _, portConfig := range s.Spec.Endpoint.Ports { + if portConfig.PublishMode == api.PublishModeIngress { + continue + } + s.Endpoint.Ports = append(s.Endpoint.Ports, portConfig.Copy()) + } + s.Endpoint.Spec = s.Spec.Endpoint.Copy() + } +} + func (a *Allocator) allocateService(ctx context.Context, s *api.Service) error { nc := a.netCtx diff --git a/manager/allocator/network_test.go b/manager/allocator/network_test.go new file mode 100644 index 0000000000..0522cc6c0a --- /dev/null +++ b/manager/allocator/network_test.go @@ -0,0 +1,40 @@ +package allocator + +import ( + "testing" + + "github.com/docker/swarmkit/api" + "github.com/stretchr/testify/assert" +) + +func TestUpdatePortsInHostPublishMode(t *testing.T) { + service := api.Service{ + Spec: api.ServiceSpec{ + Endpoint: &api.EndpointSpec{ + Ports: []*api.PortConfig{ + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 10000, + PublishMode: api.PublishModeHost, + }, + }, + }, + }, + Endpoint: &api.Endpoint{ + Ports: []*api.PortConfig{ + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 15000, + PublishMode: api.PublishModeHost, + }, + }, + }, + } + updatePortsInHostPublishMode(&service) + + assert.Equal(t, len(service.Endpoint.Ports), 1) + assert.Equal(t, service.Endpoint.Ports[0].PublishedPort, uint32(10000)) + assert.Equal(t, service.Endpoint.Spec.Ports[0].PublishedPort, uint32(10000)) +} diff --git a/manager/allocator/networkallocator/networkallocator.go b/manager/allocator/networkallocator/networkallocator.go index c7780f8c05..68820c8f24 100644 --- a/manager/allocator/networkallocator/networkallocator.go +++ b/manager/allocator/networkallocator/networkallocator.go @@ -283,6 +283,12 @@ func (na *NetworkAllocator) IsTaskAllocated(t *api.Task) bool { return true } +// PortsAllocatedInHostPublishMode returns if the passed service has its published ports in +// host (non ingress) mode allocated +func (na *NetworkAllocator) PortsAllocatedInHostPublishMode(s *api.Service) bool { + return na.portAllocator.portsAllocatedInHostPublishMode(s) +} + // IsServiceAllocated returns if the passed service has its network resources allocated or not. func (na *NetworkAllocator) IsServiceAllocated(s *api.Service) bool { // If endpoint mode is VIP and allocator does not have the diff --git a/manager/allocator/networkallocator/portallocator.go b/manager/allocator/networkallocator/portallocator.go index a7391eba6c..de5412e777 100644 --- a/manager/allocator/networkallocator/portallocator.go +++ b/manager/allocator/networkallocator/portallocator.go @@ -191,6 +191,33 @@ func (pa *portAllocator) serviceDeallocatePorts(s *api.Service) { s.Endpoint.Ports = nil } +func (pa *portAllocator) portsAllocatedInHostPublishMode(s *api.Service) bool { + if s.Endpoint == nil && s.Spec.Endpoint == nil { + return true + } + + portStates := allocatedPorts{} + if s.Endpoint != nil { + for _, portState := range s.Endpoint.Ports { + if portState.PublishMode == api.PublishModeHost { + portStates.addState(portState) + } + } + } + + if s.Spec.Endpoint != nil { + for _, portConfig := range s.Spec.Endpoint.Ports { + if portConfig.PublishMode == api.PublishModeHost { + if portStates.delState(portConfig) == nil { + return false + } + } + } + } + + return true +} + func (pa *portAllocator) isPortsAllocated(s *api.Service) bool { // If service has no user-defined endpoint and allocated endpoint, // we assume it is allocated and return true. diff --git a/manager/allocator/networkallocator/portallocator_test.go b/manager/allocator/networkallocator/portallocator_test.go index ed20d05934..7178ece078 100644 --- a/manager/allocator/networkallocator/portallocator_test.go +++ b/manager/allocator/networkallocator/portallocator_test.go @@ -264,6 +264,110 @@ func TestServiceAllocatePorts(t *testing.T) { assert.Error(t, err) } +func TestPortsAllocatedInHostPublishMode(t *testing.T) { + pa, err := newPortAllocator() + assert.NoError(t, err) + + type Data struct { + input *api.Service + expect bool + } + + testCases := []Data{ + { + // both Endpoint and Spec.Endpoint are nil + input: &api.Service{ + Spec: api.ServiceSpec{ + Endpoint: nil, + }, + Endpoint: nil, + }, + expect: true, + }, + { + // non host mode does not impact + input: &api.Service{ + Spec: api.ServiceSpec{ + Endpoint: &api.EndpointSpec{ + Ports: []*api.PortConfig{ + { + Name: "test1", + Protocol: api.ProtocolTCP, + TargetPort: 10000, + PublishedPort: 10000, + }, + }, + }, + }, + Endpoint: nil, + }, + expect: true, + }, + { + // publish mode is different + input: &api.Service{ + Spec: api.ServiceSpec{ + Endpoint: &api.EndpointSpec{ + Ports: []*api.PortConfig{ + { + Name: "test1", + Protocol: api.ProtocolTCP, + TargetPort: 10000, + PublishedPort: 10000, + PublishMode: api.PublishModeHost, + }, + }, + }, + }, + Endpoint: &api.Endpoint{ + Ports: []*api.PortConfig{ + { + Name: "test1", + Protocol: api.ProtocolTCP, + TargetPort: 10000, + PublishedPort: 10000, + }, + }, + }, + }, + expect: false, + }, + { + input: &api.Service{ + Spec: api.ServiceSpec{ + Endpoint: &api.EndpointSpec{ + Ports: []*api.PortConfig{ + { + Name: "test1", + Protocol: api.ProtocolTCP, + TargetPort: 10000, + PublishedPort: 10000, + PublishMode: api.PublishModeHost, + }, + }, + }, + }, + Endpoint: &api.Endpoint{ + Ports: []*api.PortConfig{ + { + Name: "test1", + Protocol: api.ProtocolTCP, + TargetPort: 10000, + PublishedPort: 10000, + PublishMode: api.PublishModeHost, + }, + }, + }, + }, + expect: true, + }, + } + for _, singleTest := range testCases { + expect := pa.portsAllocatedInHostPublishMode(singleTest.input) + assert.Equal(t, expect, singleTest.expect) + } +} + func TestIsPortsAllocated(t *testing.T) { pa, err := newPortAllocator() assert.NoError(t, err) From fc8a2325b101dcbef968d6db78ee6e6924fa610f Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 30 Jan 2017 12:09:03 -0800 Subject: [PATCH 2/2] Add `allocatedPorts` definition for cherry-pick This commit adds the `allocatedPorts` for cherry-pick. The `allocatedPorts` was added in PR #1835, which is not part of the v1.13.1 cherry pick. Signed-off-by: Yong Tang --- .../networkallocator/portallocator.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/manager/allocator/networkallocator/portallocator.go b/manager/allocator/networkallocator/portallocator.go index de5412e777..8c56f7c910 100644 --- a/manager/allocator/networkallocator/portallocator.go +++ b/manager/allocator/networkallocator/portallocator.go @@ -38,6 +38,71 @@ type portSpace struct { dynamicPortSpace *idm.Idm } +type allocatedPorts map[api.PortConfig]map[uint32]*api.PortConfig + +// addState add the state of an allocated port to the collection. +// `allocatedPorts` is a map of portKey:publishedPort:portState. +// In case the value of the portKey is missing, the map +// publishedPort:portState is created automatically +func (ps allocatedPorts) addState(p *api.PortConfig) { + portKey := getPortConfigKey(p) + if _, ok := ps[portKey]; !ok { + ps[portKey] = make(map[uint32]*api.PortConfig) + } + ps[portKey][p.PublishedPort] = p +} + +// delState delete the state of an allocated port from the collection. +// `allocatedPorts` is a map of portKey:publishedPort:portState. +// +// If publishedPort is non-zero, then it is user defined. We will try to +// remove the portState from `allocatedPorts` directly and return +// the portState (or nil if no portState exists) +// +// If publishedPort is zero, then it is dynamically allocated. We will try +// to remove the portState from `allocatedPorts`, as long as there is +// a portState associated with a non-zero publishedPort. +// Note multiple dynamically allocated ports might exists. In this case, +// we will remove only at a time so both allocated ports are tracked. +// +// Note because of the potential co-existence of user-defined and dynamically +// allocated ports, delState has to be called for user-defined port first. +// dynamically allocated ports should be removed later. +func (ps allocatedPorts) delState(p *api.PortConfig) *api.PortConfig { + portKey := getPortConfigKey(p) + + portStateMap, ok := ps[portKey] + + // If name, port, protocol values don't match then we + // are not allocated. + if !ok { + return nil + } + + if p.PublishedPort != 0 { + // If SwarmPort was user defined but the port state + // SwarmPort doesn't match we are not allocated. + v := portStateMap[p.PublishedPort] + + // Delete state from allocatedPorts + delete(portStateMap, p.PublishedPort) + + return v + } + + // If PublishedPort == 0 and we don't have non-zero port + // then we are not allocated + for publishedPort, v := range portStateMap { + if publishedPort != 0 { + // Delete state from allocatedPorts + delete(portStateMap, publishedPort) + return v + } + } + + return nil +} + func newPortAllocator() (*portAllocator, error) { portSpaces := make(map[api.PortConfig_Protocol]*portSpace) for _, protocol := range []api.PortConfig_Protocol{api.ProtocolTCP, api.ProtocolUDP} {