Skip to content
This repository was archived by the owner on Oct 27, 2023. It is now read-only.

nvidia-container-runtime-hook: Get hook stage from stdin's 'status' value#14

Closed
wking wants to merge 51 commits intoNVIDIA:masterfrom
wking:stage-from-stdin
Closed

nvidia-container-runtime-hook: Get hook stage from stdin's 'status' value#14
wking wants to merge 51 commits intoNVIDIA:masterfrom
wking:stage-from-stdin

Conversation

@wking
Copy link
Copy Markdown

@wking wking commented Feb 24, 2018

Instead of using CRI-O-specific arguments, use the state's REQUIRED status property to ensure we're being called on a created container (and not a running container or a stopped container).

Also add fallthrough statements to the no-op switch cases, since that appeared to be the intention.

This is ground-work for dropping the stage environment variable from CRI-O (cri-o/cri-o#1360), and should work for any CRI-O version. The args[0] approach I'm replacing was dropped from CRI-O in cri-o/cri-o#1244.

flx42 and others added 30 commits September 5, 2017 18:54
It gives the opportunity to perform cleanup in the main function.
Any environment variable with the prefix "NVIDIA_REQUIRE_" will have
its value passed to nvidia-container-cli.

NVIDIA_CUDA_VERSION is no longer used and the CUDA images will be
replaced by a NVIDIA_REQUIRE_CUDA variable.

Those checks can be disabled globally in the configuration file or for
each container using NVIDIA_DISABLE_REQUIRE.
flx42 and others added 18 commits December 19, 2017 22:36
Signed-off-by: Christy Norman <[email protected]>
Signed-off-by: Felix Abecassis <[email protected]>
See opencontainers/runc@db093f6
We now let debuild detect and add the dependency for older versions of runc.
Signed-off-by: Christy Norman <[email protected]>
Signed-off-by: Felix Abecassis <[email protected]>
Tools like CRI-O or Red Hat's fork of Docker only need the OCI hook,
not the full runtime.

The hook is also likely to move at a different cadence than the full
runtime, which is tied to the upstream Docker releases.
…alue

Instead of using CRI-O-specific arguments, use the state's REQUIRED
'status' property [1] to ensure we're being called on a created
container (and not a running container or a stopped container).

We could potentially have:

  args := flag.Args()
  if len(args) != 0 {
    flag.Usage()
    os.Exit(2)
  }

but to stay compatible with folks using the hook with older CRI-O
(including the 1.9.x branch), I've just ignored flag.Args() for now.

Also add 'fallthrough' statements [2] to the no-op switch cases, since
that appeared to be the intention.

[1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L16
[2]: https://golang.org/ref/spec#Fallthrough_statements
@flx42
Copy link
Copy Markdown
Member

flx42 commented Feb 24, 2018

Instead of using CRI-O-specific arguments

It's not, really. As long as it's setup properly, for instance it works fine with docker since we inject the hook + prestart.

Initially the syntax was nvidia-container-runtime-hook -prestart. Then I saw CRI-O and realized it made sense to drop the -, like a subcommand/action. But I also complained that passing the stage as the first argument was indeed undocumented, so I asked for the possibility to pass custom arguments to hooks and suggested the environment variable as an alternative. If prestart stops being injected by CRI-O, you should add it to your hook configuration file like I initially wanted to do.

@flx42
Copy link
Copy Markdown
Member

flx42 commented Feb 24, 2018

In addition, runc doesn't seem to set this field.

@wking
Copy link
Copy Markdown
Author

wking commented Feb 24, 2018

Some notes on runc support here.

@flx42
Copy link
Copy Markdown
Member

flx42 commented Feb 24, 2018

Uh, the right struct from the OCI spec was pulled in, but it still doesn't seem to be set inside of runc before the prestart hooks, right?
https://github.com/opencontainers/runc/blob/master/libcontainer/process_linux.go#L345-L351

@wking
Copy link
Copy Markdown
Author

wking commented Feb 24, 2018

... but it still doesn't seem to be set...

Heh, nice. I'll file runc and runtime-tools validation PRs later. Probably table this PR for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants