just check hostname with UTS namespace when platform set as linux

[Mashimiao]

Signed-off-by: Ma Shimiao [email protected]

[wking]

On Mon, May 23, 2016 at 08:02:26PM -0700, Ma Shimiao wrote:

• checkLinux executed when platform set as linux

This is how I'd expected things would work. But see 1 where @vbatts
suggests setting ‘solaris’ for Linux configs (and so maybe ‘linux’ for
Solaris configs?).

[Mashimiao]

@wking IMO, @vbatts means setting 'linux' for Solaris configs. As you know, Linux does not support Solaris's specific settings.
I'm not familiar with Solaris system, so I'm not sure if Solaris is always in Linux compatibility modes.
If so, that will be easy we just need to check if platform.OS is 'linux' or 'solaris', then determine to execute checkLinux or not. But if Solaris needs some specific settings to make itself in Linux compatibility modes, then that may be a bit complicated.

[liangchenye]

I think Vincent is talking about why use 'SHOULD' but not 'MUST', for now.
Linux specific settings could also be used in other platform, like Solaris.
Thus it maybe false negative to have this PR.
And since checkLinux will not return error if there is no Linux specific setting,
I suggest not to check os == 'linux' at present.

If runtime spec says “This MUST NOT be set if platform.os is not $OS”, we need to have codes like this:

if  os == 'linux' {
   checkLinux()
} else {
   checkLinuxConfigNotExist()
}

[Mashimiao]

I'm not sure except Solaris, which platform also need Linux specific settings.
But, checkLinux checks some specific settings on Linux, like when set hostname, must create a new UTS namespace.
If the above specific setting is not needed for other platform and just set hostname, then checkLinux will fail with like 'need new UTS namespace'.
If only Solaris and Linux need linux specific settings and as I said before Solaris is always in Linux compatibility modes, then I think
if os == 'linux' || os == 'solaris' { checkLinux() }
is better.

[wking]

On Tue, May 24, 2016 at 01:15:07AM -0700, Ma Shimiao wrote:

But, checkLinux checks some specific settings on Linux, like when set hostname, must create a new UTS namespace. If the above
specific setting is not needed for other platform and just set
hostname, then checkLinux will fail with like 'need new UTS
namespace'.

Instead of guessing what makes sense here, I suggest we push for more
clarity in the spec. For example, if there are some configs where a
platform-specific section that does not match platform.os makes sense,
we should have at least an example to that effect in the spec.

Until then, a narrow reading of the spec language after
opencontainers/runtime-spec#444 is:

• All platform-specific sections are allowed regardless of platform.os
  [1,2].
• There are no prerequisites for ‘hostname’ unless platform.os is
  ‘linux’ 3.

That sounds like:

• Run checkLinux whenever the ‘linux’ object is set.
• Pass the full rspec.Spec through to checkLinux so it can apply the
  UTS/hostname check if and only if platform.os is ‘linux’.

But personally I'd rather table this until the spec gets clearer,
because I don't see much in ‘linux’ that makes sense on non-Linux
platforms.

[Mashimiao]

• Run checkLinux whenever the ‘linux’ object is set.
• Pass the full rspec.Spec through to checkLinux so it can apply the UTS/hostname check if and only if platform.os is ‘linux’.

@wking @liangchenye
I think this is OK, but I don't agree to table this. We may temporarily implement as this before spec becomes clearer.
As you know, checkLinux() has already validate hostname, if trying to set hostname but not create new UTS namespace on non-linux platform, ocitools validate will fail.
If you agree with me, I can update the PR.

[wking]

On Tue, May 24, 2016 at 07:34:38PM -0700, Ma Shimiao wrote:

As you know, checkLinux() has already validate hostname, if trying
to set hostname but not create new UTS namespace on non-linux
platform, ocitools validate will fail.

I don't see grounds for keeping that behavior in the current spec (see
my “if and only if” wording in 1).

If you're interested in spending the time, I don't mind helping review
versions of this that do match the current spec wording, but I don't
think the current spec wording makes a lot of sense ;).

[Mashimiao]

ping @wking @liangchenye

[wking]

With 3514b32 the commit subject looks stale. And it feels like there should be an earlier check and bail if the ‘linux’ property is completely unset. But since all of linux's child properties are optional, the way you have it (without an initial “was ‘linux’ set?” check) should work fine.

[liangchenye]

The PR looks good. But I suggest to change this message Hostname requires a new UTS namespace to be specified as well.
How about On Linux, hostname requires a UTS namespace to be specified as well?

[Mashimiao]

@liangchenye I think that's OK.

[wking]

On Wed, May 25, 2016 at 08:10:18PM -0700, 梁辰晔 (Liang Chenye) wrote:

• if !utsExists && hostname != "" {
• if spec.Platform.OS == "linux" && !utsExists && spec.Hostname != "" {
  logrus.Fatalf("Hostname requires a new UTS namespace to be specified as well")

The PR looks good. But I suggest to change this message Hostname requires a new UTS namespace to be specified as well. How about
On Linux, hostname requires a UTS namespace to be specified as well?

You want to keep “new” in the error message too, since 1 has:

Also, when a path is specified, a runtime MUST assume that the setup
for that particular namespace has already been done and error out if
the config specifies anything else related to that namespace.

The UTS hostname condition is just a special case of that general rule
2, and I added it to the spec to avoid surprising people who had
only read the hostname section.

[liangchenye]

LGTM

[Mashimiao]

ping @mrunalp

[mrunalp]

LGTM