network_mode now necessary in host_config
#698
Conversation
Signed-off-by: John Howard <jhoward@microsoft.com>
…rve blank host_config in start if no option is provided
|
Oh that's much better! LGTM (but not verified against docker and my docker change yet). |
|
Do you mind giving it a whirl against your branch when you have some time? Thanks! |
|
Absolutely. Will try and get to it this evening. |
|
@shin- Verified against my branch for PR 14530 in docker. LGTM 👍 Thanks for following through :) |
|
What's the purpose of all the |
|
Hi @aanand - The reason relates to moby/moby#14530 and mentioned in #691. The daemon with 14530 will do network mode validation - previously it was done by the CLI. This is necessary as part of the port of the daemon to Windows where network modes are different between Linux and Windows. Hence, a more complete host config is necessary to be passed to the daemon. Strictly speaking, it probably should have been there before as many of the tests aren't passing a "complete" host config structure, but instead are passing the minimal set to allow the docker-py tests to pass. Hope that makes sense :) |
docker/client.py
Outdated
There was a problem hiding this comment.
@shin- can correct me, I'm not a python person at all. I believe its just a shortcut to ensure that network_mode is always present when calling create_host_config. Therefore you don't need to have the code I originally had to add to the dictionary if it isn't present.
There was a problem hiding this comment.
Is there a compelling reason not to do it in create_host_config?
It looks like the fallback check there is for network_mode is None. This will be false if network_mode is the empty string. So it seems like the behaviour of start and create_host_config will differ when the network_mode kwarg is omitted:
client.start(container)will start a container with anetwork_modeofdefault.create_host_config()will return a host config object without aNetworkModekey.
If NetworkMode is now required in host config objects, I feel create_host_config should always set it.
There was a problem hiding this comment.
It's so that if the user doesn't provide an explicit value, we avoid creating a non-empty config in start (see https://github.com/docker/docker-py/blob/jhowardmsft-14530-netmode/docker/utils/utils.py#L431 - not passing None will prevent this field from being populated). It's kind of a hack, but the problem is that if a host_config is provided in start it will override the one provided in create_container, so we need to prevent that from happening (see 09a5eb8)
There's an argument for removing config from start entirely but I don't know if we're quite there yet.
There was a problem hiding this comment.
OK, that makes sense. (I'd be in favour of removing start's arguments soon.)
Could we make it clearer what's happening? Perhaps:
start_config_kwargs = dict(
binds=binds, port_bindings=port_bindings, lxc_conf=lxc_conf,
publish_all_ports=publish_all_ports, links=links, dns=dns,
privileged=privileged, dns_search=dns_search, cap_add=cap_add,
cap_drop=cap_drop, volumes_from=volumes_from, devices=devices,
network_mode=network_mode, restart_policy=restart_policy,
extra_hosts=extra_hosts, read_only=read_only, pid_mode=pid_mode,
ipc_mode=ipc_mode, security_opt=security_opt, ulimits=ulimits
)
start_config = None
if any(v is not None for v in start_config_kwargs.values()):
start_config = create_host_config(**start_config_kwargs)
`network_mode` now necessary in `host_config`
|
Merged, sorry for the delay! |
Follow-up to #691 - Tested with docker/docker master.
@aanand @jhowardmsft - please review!