-
Notifications
You must be signed in to change notification settings - Fork 144
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
check the status of the state passed to hooks over stdin #608
Conversation
It is a successor PR of #589. The poststop status is not meaningful since the container will be deleted and the id might be reused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to pass the *tap.T
into stdinStateCheck
so it can add multiple errors instead of dying on the first.
validation/hooks_stdin.go
Outdated
return fmt.Errorf("wrong state %q in the stdin of %s hook, expected %q", state.Status, hookName, "running") | ||
} | ||
case "poststop": | ||
// Not defined by runtime spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current spec wants this to be stopped
(more discussion in opencontainers/runtime-spec#958), although more spec clarity would be nice. But what the spec is clear about is that status
must be set to something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the code since 'status' must be set to something code.
and wait for runtime-spec#958.
Now we only output three 'OK or NOT' results, each result may have the detailed property errors information. I join the state property errors of a hook together by using multiple errors. |
@liangchenye
It looks like |
That's opencontainers/runc#1741, which I'd also linked from here. I think we should land these better checks here withought waiting for runc to fix its behavior. |
@wking Thanks for the info. |
Signed-off-by: Liang Chenye <[email protected]>
rebased PTAL @q384566678 |
looks good, But when I run the test, I get the error.
|
@q384566678 Yes, it does not work because of the runc bug |
Signed-off-by: Liang Chenye [email protected]