Allow different services to use the same host-mode port #2177
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2177 +/- ##
=========================================
+ Coverage 59.91% 60.1% +0.19%
=========================================
Files 119 119
Lines 19709 19762 +53
=========================================
+ Hits 11808 11878 +70
+ Misses 6558 6541 -17
Partials 1343 1343 |
|
Looks good to me |
manager/controlapi/service.go
Outdated
| // If the publish mode is host, it is better to leave the collision check | ||
| // to the scheduler, given multiple services with same port in host publish | ||
| // mode can coexist. | ||
| if pc.PublishMode == api.PublishModeHost { |
There was a problem hiding this comment.
Do you need to add the same check at line 539?
There was a problem hiding this comment.
Fixed, thanks
|
How do you handle mode host conflicts with mode ingress? If a port is already taken by mode ingress, do you fail the new request for mode host? And vice versa? |
aaronlehmann
force-pushed
the
duplicate-host-mode-ports
branch
from
4303643
to
9644340
Compare
Currently the PR does not do this, but we could disallow host-mode ports from conflicting with ingress ports. |
|
Agree. I think mode host and mode ingress conflicts should fail right away. |
Relax the validation in controlapi to allow two services to use the same host-mode published port. Add a scheduler filter so that tasks from these services land on different nodes. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
aaronlehmann
force-pushed
the
duplicate-host-mode-ports
branch
from
9644340
to
dfc30fd
Compare
|
@dongluochen: Updated, PTAL |
| switch pc.PublishMode { | ||
| case api.PublishModeHost: | ||
| if _, ok := ingressPorts[pcToStruct(pc)]; ok { | ||
| return grpc.Errorf(codes.InvalidArgument, "port '%d' is already in use by service '%s' (%s) as a host-published port", pc.PublishedPort, service.Spec.Annotations.Name, service.ID) |
There was a problem hiding this comment.
as a host-published port
should be as an ingress port.
There was a problem hiding this comment.
I believe it is correct as is. Since we are in the api.PublishModeHost case, this is a newly-added ingress port conflicting with an existing host-published port.
| _, ingressConflict := ingressPorts[pcToStruct(pc)] | ||
| _, hostModeConflict := hostModePorts[pcToStruct(pc)] | ||
| if ingressConflict || hostModeConflict { | ||
| return grpc.Errorf(codes.InvalidArgument, "port '%d' is already in use by service '%s' (%s) as an ingress port", pc.PublishedPort, service.Spec.Annotations.Name, service.ID) |
There was a problem hiding this comment.
as an ingress port
Should remove this.
There was a problem hiding this comment.
I think you are confusing the new service (which the maps reflect) with the existing services we iterate over.
Relax the validation in controlapi to allow two services to use the same host-mode published port.
Add a scheduler filter so that tasks from these services land on different nodes.
cc @aboch @pvnovarese @aluzzardi @dongluochen