Rmirica/add port mapping #119
Conversation
|
Thank @mrares I'll take a look at this later today. Also looks like you got some rewritten old commits in the PR could you rebase on top of master and drop those? |
| var portResources []*mesos.Value_Range | ||
| var portMapping []*mesos.ContainerInfo_DockerInfo_PortMapping | ||
|
|
||
| if(len(task.Ports) > 0) { |
There was a problem hiding this comment.
This is a quite big block of code to inline in this function we should try to break it up.
For an initial suggestion it could be broken down into these steps (which each could be their own function)
- a) Get N ports from an offer
- b) Construct portMappings from the ports definition and a list of ports
- c) Construct ranges resource of ports from list of ports
as an added bonus a could also be reused to implement the matcher
There was a problem hiding this comment.
The problem I had was with C... if it's not done in the same context as A it may lead to non-overlapping ranges (ie: you get offer (3001:3003),(3007:3010) and you need 5 ports, the list will be [3001, 2, 3, 7, 8] and the resulting range (3001, 3008) which will violate the offer)
There was a problem hiding this comment.
Of course a helper function that finds contiguous sequences in a list would do...
There was a problem hiding this comment.
Hmm. that's true. However reconstructing that into two ranges by finding sequences should be doable. There's also the really simple method of mapping that into 5 1-length ranges like (3001-3001, 3002-3002, ...) which I think would still be legal?
mrares
force-pushed
the
rmirica/add_portMapping
branch
from
043bee4 to
a704135
Compare
|
Hi, sorry that my merge introduced some conflicts in your branch, but I think they should be fairly easily solvable. I tried to reduce the complexity of the CreateTaskInfo function and make it easier to follow. |
mrares
force-pushed
the
rmirica/add_portMapping
branch
from
07ec553 to
a939dfa
Compare
|
I solved the conflicts, factored out the buildPorts function |
|
Ok. great stuff @mrares Some final points, could you remove the commented block in matcher.go and squash it to the commit that introduces that and then update the readme with the new api? |
|
Sure, question about the API though, should I stay with containerPort (the underlying name in the mesos API) or change it to container_port for consistency with the eremetic api? |
mrares
force-pushed
the
rmirica/add_portMapping
branch
from
a939dfa to
58c2dc5
Compare
|
ah good point. we should go with the snake_case version On Fri, 2 Sep 2016, 09:37 Rares Mirica, [email protected] wrote:
|
mrares
force-pushed
the
rmirica/add_portMapping
branch
from
58c2dc5 to
4e481f2
Compare
|
done |
|
|
I gave a shot at this naive implementation of a portMapping allocator.
It expects a
"ports":[{"containerPort":<internal port>, "protocol": "<protocol>"}]definition in the task POST json.It will select the first available ports from the mesos offer and allocate them accordingly.
I've abandoned the attempt to write an offer matcher for port-range (I don't expect this to be a problem in practice but this should be solved)