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..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} { @@ -191,6 +256,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)