Haven't had a chance to play with this, but it looks like a promising fix to the originally thorny problem of figuring out the intent vs. distro differences that brought up the original bug.

When you say docker inspect is changed, I assume you mean it will just show the entire FQDN as Hostname with a blank Domainname for the CLI usecase of --host=hostname.domainname, which would have been shown separately due to the splitting logic being at the "front-end" prior to this PR.

I think the only downside is that for non-API uses with a FQDN in --host, this PR doesn't fix the max length problem for the SetHostname syscall. I remember one of the difficulties when I worked on the original "fix" is that even though the Docker container config has the concept of a separate hostname/domainname, libcontainer does not, and the passthrough is the somewhat clunky env. variable HOSTNAME set into the process config buffer, which the execdriver then uses to set up the libcontainer variant of the container's config. However, at least this allows for proper use via the API and with the libnetwork changes, gives you a more appropriate setup for differentiating hostname and domainname, and more importantly fixes the hostname length max issue for that use case.

Yes, your assessment of the This doesn't fix the length issue

On Wed, Feb 10, 2016 at 11:51 AM, Phil Estes notifications@github.com
wrote:

Haven't had a chance to play with this, but it looks like a promising fix
to the originally thorny problem of figuring out the intent vs. distro
differences that brought up the original bug.

When you say docker inspect is changed, I assume you mean it will just
show the entire FQDN as Hostname with a blank Domainname for the CLI
usecase of --host=hostname.domainname, which would have been shown
separately due to the splitting logic being at the "front-end" prior to
this PR.

I think the only downside is that for non-API uses with a FQDN in --host,
this PR doesn't fix the max length problem for the SetHostname syscall. I
remember one of the difficulties when I worked on the original "fix" is
that even though the Docker container config has the concept of a separate
hostname/domainname, libcontainer does not
https://github.com/opencontainers/runc/blob/master/libcontainer/configs/config.go#L108-L109,
and the passthrough is the somewhat clunky env. variable HOSTNAME set
into the process config buffer, which the execdriver then uses
https://github.com/docker/docker/blob/master/daemon/execdriver/driver_unix.go#L140
to set up the libcontainer variant of the container's config. However, at
least this allows for proper use via the API and with the libnetwork
changes, gives you a more appropriate setup for differentiating hostname
and domainname, and more importantly fixes the h ostname length max issue
for that use case.

—
Reply to this email directly or view it on GitHub
#20200 (comment).

@thockin can you sign-off your commit? 😇

Now that the initial overall WIP has been floated for discussion, I would suggest the libnetwork PR can be created for review as well, and those 2 file changes dropped from this PR. Don't know if @mavenugo or others want to weigh in here, or wait for a libnetwork PR. I personally think this is a reasonable fix for a long-standing issue around hostname vs. fqdn.

thanks @thockin @estesp. yes, we will look into this mainly to make sure it doesn't introduce any serious regression.
ping @sanimej

Thanks.

I will prep a "real" patch against libnetwork and sign this and update docs
in the next couple days

On Thu, Feb 11, 2016 at 9:15 AM, Madhu Venugopal notifications@github.com
wrote:

thanks @thockin https://github.com/thockin @estesp
https://github.com/estesp. yes, we will look into this mainly to make
sure it doesn't introduce any serious regression.
ping @sanimej https://github.com/sanimej

—
Reply to this email directly or view it on GitHub
#20200 (comment).

Some missing CI jobs here. They need kicking off again. Signing the commit and force pushing should resolve that.

@thockin The change looks ok for fixing the API. But in the current form it breaks the existing behavior for net host. I think we can retain the current behavior by not splitting the os.Hostname() into host and domain name in initializeNetworking

        if container.HostConfig.NetworkMode.IsHost() {
                container.Config.Hostname, err = os.Hostname()
                if err != nil {
                        return err
                }

                parts := strings.SplitN(container.Config.Hostname, ".", 2) 
                if len(parts) > 1 {
                        container.Config.Hostname = parts[0]
                        container.Config.Domainname = parts[1]
                }       

        }

@sanimej re --net-host : Do you think it is sufficient to set the hostname and not the domainname? Or should we actually get the domainname (exec hostname -d) and set the domainname field?

Also - thoughts on what docs need to be updated?

@thockin For --net=host mode its sufficient to set only the hostname. This will cover the case where hostname is an fqdn. When the hostname is not an fqdn, /etc/hosts should typically have the fqdn. Since the /etc/hosts file is copied in the --net=host mode container should get the right dnsdomainname.

@thaJeztah Can you please help in identifying the docs that need to be updated for this change ? I don't see hostname & fqdn being specifically discussed in any of the docs. But probably we have to update the API documentation to clarify the behavior ?

@sanimej I'll have a look at the docs; from a design perspective, does this LGTY (the networking team?) if it does, we can move this forward to code review

@thaJeztah Yes, This is a reasonable solution without adding any UI or API changes.

There is one expected change in docker inspect output though; as mentioned earlier by @estesp

When you say docker inspect is changed, I assume you mean it will just show the entire FQDN as Hostname with a blank Domainname for the CLI usecase of --host=hostname.domainname, which would have been shown separately due to the splitting logic being at the "front-end" prior to this PR.

For the documentation, I think we need;

• A mention in the API changes, because specifying both hostname and domainname gives a different result; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v123-api-changes
• Possibly update the example output of the inspect response as mentioned above

IIUC, the --hostname on the CLI will keep the same behavior?

Rebased and test fixed, I think

Thanks @thockin

oh cr*p, @thockin looks like it needs another rebase already 😢

rebased

On Tue, Mar 15, 2016 at 3:48 AM, Sebastiaan van Stijn <
notifications@github.com> wrote:

oh cr*p, @thockin https://github.com/thockin looks like it needs
another rebase already [image: 😢]

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#20200 (comment)

ping @estesp @cpuguy83 PTAL

LGTM. Thanks @thockin

Anything I need to do regarding the failed windows tests?

Can we "fix" the inspect output for older clients?

@cpuguy83 I think I am out of my depth on that question.

LGTM, let's move this forward to docs review.

thanks, LGTM - not sure if there's anything else to document 😄

Docs LGTM too.