Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: Make 'process.args' optional #489

Closed
wants to merge 2 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 5, 2016

Since be59415 (Split create and start, 2016-04-01, #384), it's possible for a container process to never execute user-specified code (e.g. you can call ‘create’, ‘kill’, ‘delete’ without calling ‘start’). For folks who expect to do that, there’s no reason to defineprocess.args.

The only other process property required for all platforms is cwd, but the runtime's idler code isn't specified in sufficient detail for the configuration author to have an opinion about what its working directory should be.

On Linux and Solaris, uid and gid are also required. My preferred approach here is to make those optional and use platform defaults:

If unset, the runtime will not attempt to manipulate the user ID (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly required properties. With the current spec, one option could be to make process optional (with the idler's working directory unspecified) for OSes besides Linux and Solaris, but the main reason that Windows doesn't have a user property is that we don't know how to specify it. I expect all platforms will have some sort of required user field, which means they'll all have to define process.

While I'm indenting the sub-properties, also wrap them to one line per sentence.

There was also some related discussion in opencontainers/runc#827, although I'm not clear on the intended policy after that discussion.

wking added 2 commits June 4, 2016 23:53
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris, but the main reason
that Windows doesn't have a user property is that we don't know how to
specify it [6].  I expect all platforms will have some sort of
required user field, which means they'll all have to define 'process'.

While I'm indenting the sub-properties, also wrap them to one line per
sentence (style.md).

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#96 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
Linux and Solaris both use the same POSIX-based structure (which I've
moved to defs-linux.json).  Windows likely needs a string-based
structure, but we're punting on that until we have more feedback from
the Windows folks [1].  Regardless of whether we have a Windows user
structure yet, the maintainer consensus is that the property is
required [2,3,4].

[1]: opencontainers#96 (comment)
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[3]: opencontainers#417 (comment)
[4]: opencontainers#417 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the optional-process.args branch from bf042d9 to f025239 Compare June 5, 2016 07:06
@wking
Copy link
Contributor Author

wking commented Jun 5, 2016

I added a second commit that makes process.user required, since that was the reason I used above to make process required.

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jun 18, 2016
I'd added some omitempties in 5c2193f (specs-go/config: Make Linux
and Solaris omitempty, 2016-05-06, opencontainers#431), but it turns out to not have
the intended effect unless the field is also a pointer type.  Before
this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
    "linux": {
      "cgroupsPath": ""
    },
    "solaris": {
      "cappedCPU": {},
      "cappedMemory": {}
    }
  }

And after this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
  }

The remaining useless properties are addressed by other in-flight pull
requests:

* 5ca74df (config: Make 'process.args' optional, 2016-06-04, opencontainers#489)
* ad33f9c (config: Explicitly list 'hooks' as optional, 2016-05-06,
  opencontainers#427)

So I've left them alone here.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jun 18, 2016
I'd added some omitempties in 5c2193f (specs-go/config: Make Linux
and Solaris omitempty, 2016-05-06, opencontainers#431), but it turns out to not have
the intended effect unless the field is also a pointer type (even
after I shifted the 'omitempty' from the platform tag to the json tag

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
    "linux": {
      "cgroupsPath": ""
    },
    "solaris": {
      "cappedCPU": {},
      "cappedMemory": {}
    }
  }

And after this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
  }

The remaining useless properties are addressed by other in-flight pull
requests:

* 5ca74df (config: Make 'process.args' optional, 2016-06-04, opencontainers#489)
* ad33f9c (config: Explicitly list 'hooks' as optional, 2016-05-06,
  opencontainers#427)

So I've left them alone here.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jun 18, 2016
I'd added some omitempties in 5c2193f (specs-go/config: Make Linux
and Solaris omitempty, 2016-05-06, opencontainers#431), but it turns out to not have
the intended effect unless the field is also a pointer type (even
after I shifted the 'omitempty' from the platform tag to the json
tag).  Before this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
    "linux": {
      "cgroupsPath": ""
    },
    "solaris": {
      "cappedCPU": {},
      "cappedMemory": {}
    }
  }

And after this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
  }

The remaining useless properties are addressed by other in-flight pull
requests:

* 5ca74df (config: Make 'process.args' optional, 2016-06-04, opencontainers#489)
* ad33f9c (config: Explicitly list 'hooks' as optional, 2016-05-06,
  opencontainers#427)

So I've left them alone here.

Signed-off-by: W. Trevor King <wking@tremily.us>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
I'd added some omitempties in 5c2193f (specs-go/config: Make Linux
and Solaris omitempty, 2016-05-06, opencontainers#431), but it turns out to not have
the intended effect unless the field is also a pointer type (even
after I shifted the 'omitempty' from the platform tag to the json
tag).  Before this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
    "linux": {
      "cgroupsPath": ""
    },
    "solaris": {
      "cappedCPU": {},
      "cappedMemory": {}
    }
  }

And after this commit:

  $ ./ocitools generate --template <(echo '{}')
  $ jq . config.json
  {
    "ociVersion": "1.0.0-rc1-dev",
    "platform": {
      "os": "linux",
      "arch": "amd64"
    },
    "process": {
      "user": {
        "uid": 0,
        "gid": 0
      },
      "args": [],
      "cwd": "/"
    },
    "root": {
      "path": "rootfs"
    },
    "hooks": {},
  }

The remaining useless properties are addressed by other in-flight pull
requests:

* 5ca74df (config: Make 'process.args' optional, 2016-06-04, opencontainers#489)
* ad33f9c (config: Explicitly list 'hooks' as optional, 2016-05-06,
  opencontainers#427)

So I've left them alone here.

Signed-off-by: W. Trevor King <wking@tremily.us>
@crosbymichael
Copy link
Member

NOTLGTM

I don't like the proposal but I can be swayed if other maintainers feel strongly for it.

@wking
Copy link
Contributor Author

wking commented Nov 4, 2016

On Fri, Nov 04, 2016 at 3:37pm -0700, Michael Crosby wrote:

I don't like the proposal…

Are you rejecting the “it's possible for a container process to never execute user-specified code (e.g. you can call ‘create’, ‘kill’, ‘delete’ without calling ‘start’)” use case? Or do you accept that use case but feel they should insert a dummy process.args? Or do you accept the use case and the optional process.args but have some technical quibbles with this PR?

@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2016

I am in favor of having the user specify the args. Even though the flow where the command is never executed is supported, that isn't a normal flow but more for scheduling or gc where one removes the container without ever starting the container after creating it.

@wking
Copy link
Contributor Author

wking commented Nov 10, 2016

On Thu, Nov 10, 2016 at 11:25:48AM -0800, Mrunal Patel wrote:

I am in favor of having the user specify the args. Even though the
flow where the command is never executed is supported, that isn't a
normal flow…

So “specify dummy args” for those folks? The current:

The executable is the first element and MUST be available at the
given path inside of the rootfs.

language makes this:

{"process": {"args": ["/foo"], …}, …}

illegal, because /foo doesn't exist. If the user is just holding
mount namespace open, they may have an empty rootfs. I guess they
could use:

{"process": {"args": ["/"], …}, …}

That seems like a silly hoop to jump through (like using an empty
tarball layer 1).

The only benefit I see to keeping process.args required is to get
validation errors when folks forget to specify it. But it seems like
you could accomplish that same goal in validation, without making it a
strict requirement in the spec itself.

But required hoop-jumping is not the end of the world ;). If there
are two maintainers in favor with the status quo and none in favor of
an optional process.args, then one of you should probably close this.

@crosbymichael
Copy link
Member

Well its hard to tell what really changed in this PR because you changed an entire block instead of what you were talking about in the description. There are more changes in this PR than just making args optional, therefore, I'm going to close it.

If you want to propose a single change that is easy for us to review and understand please open a new, small, and concise PR with only the change you are proposing and we can continue the discussion there for optional args. You are making the process hard on the maintainers and yourself by your current strategy. Multiple small, focused PRs are much better for everyone than big ones with unrelated changes.

@wking
Copy link
Contributor Author

wking commented Nov 11, 2016

On Fri, Nov 11, 2016 at 05:23PM -0800, Michael Crosby wrote:

If you want to propose a single change that is easy for us to review and understand please open a new, small, and concise PR with only the change you are proposing and we can continue the discussion there for optional args.

Filed in #620.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants