Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issues of multiple published ports mapping to the same target port #1835

Merged
merged 1 commit into from
Jan 10, 2017
Merged

Fix issues of multiple published ports mapping to the same target port #1835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 113 additions & 44 deletions manager/allocator/networkallocator/portallocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It maybe only me but I feel it will help if we add a comment here saying what this method does and what it means if it return a nil or a non nil object. So to spare the reader from reading the implementation to understand that.

// 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 becasue 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} {
Expand Down Expand Up @@ -91,40 +156,53 @@ func reconcilePortConfigs(s *api.Service) []*api.PortConfig {
return s.Spec.Endpoint.Ports
}

allocatedPorts := make(map[api.PortConfig]*api.PortConfig)
portStates := allocatedPorts{}
for _, portState := range s.Endpoint.Ports {
if portState.PublishMode != api.PublishModeIngress {
continue
if portState.PublishMode == api.PublishModeIngress {
portStates.addState(portState)
}

allocatedPorts[getPortConfigKey(portState)] = portState
}

var portConfigs []*api.PortConfig

// Process the portConfig with portConfig.PublishMode != api.PublishModeIngress
// and PublishedPort != 0 (high priority)
for _, portConfig := range s.Spec.Endpoint.Ports {
// If the PublishMode is not Ingress simply pick up
// the port config.
if portConfig.PublishMode != api.PublishModeIngress {
// If the PublishMode is not Ingress simply pick up the port config.
portConfigs = append(portConfigs, portConfig)
continue
}
} else if portConfig.PublishedPort != 0 {
// Otherwise we only process PublishedPort != 0 in this round

portState, ok := allocatedPorts[getPortConfigKey(portConfig)]

// If the portConfig is exactly the same as portState
// except if SwarmPort is not user-define then prefer
// portState to ensure sticky allocation of the same
// port that was allocated before.
if ok && portConfig.Name == portState.Name &&
portConfig.TargetPort == portState.TargetPort &&
portConfig.Protocol == portState.Protocol &&
portConfig.PublishedPort == 0 {
portConfigs = append(portConfigs, portState)
continue
// Remove record from portState
portStates.delState(portConfig)

// For PublishedPort != 0 prefer the portConfig
portConfigs = append(portConfigs, portConfig)
}
}

// Iterate portConfigs with PublishedPort == 0 (low priority)
for _, portConfig := range s.Spec.Endpoint.Ports {
// Ignore ports which are not PublishModeIngress (already processed)
// And we only process PublishedPort == 0 in this round
// So the following:
// `portConfig.PublishMode == api.PublishModeIngress && portConfig.PublishedPort == 0`
if portConfig.PublishMode == api.PublishModeIngress && portConfig.PublishedPort == 0 {
// If the portConfig is exactly the same as portState
// except if SwarmPort is not user-define then prefer
// portState to ensure sticky allocation of the same
// port that was allocated before.

// Remove record from portState
if portState := portStates.delState(portConfig); portState != nil {
portConfigs = append(portConfigs, portState)
continue
}

// For all other cases prefer the portConfig
portConfigs = append(portConfigs, portConfig)
// For all other cases prefer the portConfig
portConfigs = append(portConfigs, portConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here; an if/else formulation might make the code ever so slightly clearer.

I apologize for picking nits over this kind of trivial thing. Normally I don't care very much about something like else vs continue. But this function is relatively complicated, and I'm trying to do all I can to make it intuitive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aaronlehmann. Also added additional comments to help explain the logic of the function.

}
}

return portConfigs
Expand Down Expand Up @@ -213,40 +291,31 @@ func (pa *portAllocator) isPortsAllocated(s *api.Service) bool {
return false
}

allocatedPorts := make(map[api.PortConfig]*api.PortConfig)
portStates := allocatedPorts{}
for _, portState := range s.Endpoint.Ports {
if portState.PublishMode != api.PublishModeIngress {
continue
if portState.PublishMode == api.PublishModeIngress {
portStates.addState(portState)
}

allocatedPorts[getPortConfigKey(portState)] = portState
}

// Iterate portConfigs with PublishedPort != 0 (high priority)
for _, portConfig := range s.Spec.Endpoint.Ports {
// Ignore ports which are not PublishModeIngress
if portConfig.PublishMode != api.PublishModeIngress {
continue
}

portState, ok := allocatedPorts[getPortConfigKey(portConfig)]

// If name, port, protocol values don't match then we
// are not allocated.
if !ok {
if portConfig.PublishedPort != 0 && portStates.delState(portConfig) == nil {
return false
}
}

// If SwarmPort was user defined but the port state
// SwarmPort doesn't match we are not allocated.
if portConfig.PublishedPort != portState.PublishedPort &&
portConfig.PublishedPort != 0 {
return false
// Iterate portConfigs with PublishedPort == 0 (low priority)
for _, portConfig := range s.Spec.Endpoint.Ports {
// Ignore ports which are not PublishModeIngress
if portConfig.PublishMode != api.PublishModeIngress {
continue
}

// If SwarmPort was not defined by user and port state
// is not initialized with a valid SwarmPort value then
// we are not allocated.
if portConfig.PublishedPort == 0 && portState.PublishedPort == 0 {
if portConfig.PublishedPort == 0 && portStates.delState(portConfig) == nil {
return false
}
}
Expand Down
57 changes: 57 additions & 0 deletions manager/allocator/networkallocator/portallocator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,63 @@ func TestIsPortsAllocated(t *testing.T) {
},
expect: true,
},
{
// Endpoint and Spec.Endpoint have multiple PublishedPort
// See docker/docker#29730
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Ports: []*api.PortConfig{
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 5000,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 5001,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 0,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 0,
},
},
},
},
Endpoint: &api.Endpoint{
Ports: []*api.PortConfig{
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 5000,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 5001,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 30000,
},
{
Protocol: api.ProtocolTCP,
TargetPort: 80,
PublishedPort: 30001,
},
},
},
},
expect: true,
},
}

for _, singleTest := range testCases {
Expand Down