Enable setting networking mode for docker #184
Conversation
|
Thanks for putting this together! How about having NetworkMode specified in the task's Config member instead, since it's driver-specific, and not a resource that is taken up? |
|
@dimfeld updated. |
|
Nice, thanks :) |
|
An another note, I wonder if you would still want to allow Resources.Networks to have entries, since that could be used to reserve ports and ensure that jobs with conflicting ports aren't scheduled on the same node. Of course this would also then require the docker client to not actually configure those ports in host mode; it would be more an advisory for the scheduler. But I'll have to defer to people who actually work at Hashicorp for this one. Also, panicking inside the client due to bad user input is probably not the best idea either, since it'll kill the client and possibly be disruptive to the network unless there's some |
|
There is no |
| @@ -118,6 +118,22 @@ func createContainer(ctx *ExecContext, task *structs.Task, logger *log.Logger) d | |||
| logger.Printf("[DEBUG] driver.docker: using %d bytes memory for %s", hostConfig.Memory, task.Config["image"]) | |||
| logger.Printf("[DEBUG] driver.docker: using %d cpu shares for %s", hostConfig.CPUShares, task.Config["image"]) | |||
|
|
|||
| mode := task.Config["network_mode"] | |||
There was a problem hiding this comment.
You can check if the value exists in the map using the tupled return type like: if mode, ok := task.Config["network_mode"]. Should make using the default cleaner.
|
Left some comments above. Thanks! |
|
@ryanuber all addressed |
|
nice job here! |
|
Thanks @zenvdeluca Looking forward to getting this merged :) |
| case "default", "bridge", "none": | ||
| logger.Printf("[DEBUG] driver.docker: using %s as network mode", mode) | ||
| case "host": | ||
| if len(task.Resources.Networks) != 0 { |
There was a problem hiding this comment.
This is incorrect behavior. If tasks bind to ports not given to them by the scheduler then there is the possibility that the scheduler will place a subsequent job with a constraint on a port onto a client that has a task bound to it (this creates a loss of information for the scheduler).
It is okay to bind to the host network namespace but you must only bind to ports specified in the task.Resources.Networks. This goes for all drivers.
So lets remove that warning.
|
@dadgar this is a task config, so I put it in the task section. Two different tasks in the same client can have different modes. |
| * `network_mode` - (Optional) The network mode to be used for the container. | ||
| Valid options are `net`, `bridge`, `host` or `none`. If nothing is | ||
| specified, the container will start in `bridge` mode. The `container` | ||
| network mode is not suuported right now. |
There was a problem hiding this comment.
suuported -> currently supported
- Small typo
This patch enables setting networking mode for the docker driver. This does not handle the `container` mode. Closes #175
Enable setting networking mode for docker
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This patch enables setting networking mode for the docker
driver. This does not handle the
containermode.Closes #175