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

Get hook stage from stdin's 'status' value #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wking
Copy link
Contributor

@wking wking commented Feb 22, 2018

Instead of using CRI-O-specific arguments, environment variables, or PID positive-ness, 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, do not error if pid is unset (it isn't REQUIRED for stopped on Linux) in general, but do error if pid is not set positive for created containers (where it's REQUIRED on Linux).

This is ground-work for dropping the stage environment variable from CRI-O (cri-o/cri-o#1360).

Instead of using CRI-O-specific arguments, environment variables, or
PID positive-ness, 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).  Also, do not error if 'pid' is
unset (it isn't REQUIRED for 'stopped' on Linux [2]) in general, but
do error if pid is not set positive for 'created' containers (where
it's REQUIRED on Linux [2]).

[1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L16
[2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L25
@rhatdan
Copy link
Member

rhatdan commented Feb 24, 2018

My concern here is that this will work correctly with an older docker-runc that we are using in docker-1.12 and docker-1.13, Did these versions of runc follow those rules. If not we would need to keep the checking of argv1.

@rhatdan
Copy link
Member

rhatdan commented Feb 24, 2018

@rhvgoyal PTAL

@wking
Copy link
Contributor Author

wking commented Feb 24, 2018

Docker 1.12.0 passed in the state, but the state did not include status. So you'll want to wait until you can rely on runtime-spec v1.0.0-rc1+ (cut 2016-06-05). Looks like runc got created in its 1.0.0-rc1 (cut 2016-06-03), but didn't pass it to the hooks until opencontainers/runc#1201 (in 1.0.0-rc3, cut 2017-03-21).

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.

2 participants