-
Notifications
You must be signed in to change notification settings - Fork 141
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
add hooks stdin test #589
add hooks stdin test #589
Conversation
It is a WIP PR to test the container state that passed to hooks over 'stdin':
I try to use
|
It's not possible to call I had a look at runc but it always gives an empty "status" (opencontainers/runc#1057 & opencontainers/runc#1741) and as you pointed out, it does not call the hooks at the correct time (opencontainers/runc#1710). Since the spec does not have a "status=deleted", I wonder what's the correct status to send on stdin in the poststop hook. Does the spec explain that? |
No, if it is deleted, there won't be a 'status=delete' since:
I think we can not send a status on stdin in the poststop hook. |
2b6d3bd
to
56a596f
Compare
In the updated version, I check the 'ID', 'Bundle' and 'Annotations' of the container state in 'prestart', 'poststart' and 'poststop' hooks. The 'PID','status' and 'ociVersion' is unpredictable. The 'poststop' hook is also checked here according to current spec requirement. |
This is opencontainers/runtime-spec#958. We may end up with a
Depending on how you read the spec, if state.Version != "1.0.0" && state.Version != "1.0.1" {
// complain
} The container process can check its PID, and then this test can compare with the states it recieved. |
I'm working on the PID now. |
validation/hooks_stdin.go
Outdated
g.AddPreStartHook(rspecs.Hook{ | ||
Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/sh"), | ||
Args: []string{ | ||
"sh", "-c", fmt.Sprintf("timeout -t 1 cat > %s", filepath.Join(outputDir, "prestart")), |
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.
Where is your timeout
from? It's not in POSIX, and coreutils uses -k
/ --kill-after
, not -t
.
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 is in the busybox 1.28, in the new rootfs-amd64.tar.gz.
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 is in the busybox 1.28, in the new rootfs-amd64.tar.gz.
The lack of standardization is unfortunate, because hooks are called from the runtime namespace, so the version in the container doesn't matter. We could work around that with {bundle-path}/rootfs/bin/timeout
here, but it's probably better to set Timeout
in the hook structure and skip the timeout
command.
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.
thanks, the timeout in hooks is much more rational.
if err != nil { | ||
return err | ||
} | ||
containerPid = state.Pid |
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 this would be more reliable if we dropped the state
call and collected this from the container process itself (where you currently just use true
).
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.
echo $$
will output the id inside the container. It is different with the state one - It is 'seen' by host.
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.
echo $$
will output the id inside the container. It is different with the state one -It is 'seen' by host.
Ah right, you'd need the PID namespace too to translate, and that would break down for VM-based runtimes anyway. So I'm on board with your current state
call.
I'll revoke my version test. It is wrong. |
Updated. PTAL @q384566678 |
validation/hooks_stdin.go
Outdated
} | ||
for _, file := range []string{"prestart", "poststart", "poststop"} { | ||
err := stdinStateCheck(outputDir, file, expectedState) | ||
util.SpecErrorOK(t, err == nil, specerror.NewError(specerror.PosixHooksStateToStdin, fmt.Errorf("the state of the container MUST be passed to %q hook over stdin so that they may do work appropriate to the current state of the container", file), rspecs.Version), err) |
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.
You can probably drop "so that they...of the container" from the error message. That motivation is in the spec for folks who care, but you don't need it for the check.
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.
updated.
validation/hooks_stdin.go
Outdated
} | ||
|
||
if !reflect.DeepEqual(state.Annotations, expectedState.Annotations) { | ||
return fmt.Errorf("wrong annotations \"%v\" in the stdin of %s hook, expected \"%v\"", state.Annotations, hookName, state.Annotations) |
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.
The last parameter should be expectedState.Annotations
.
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.
updated.
Signed-off-by: Liang Chenye <liangchenye@huawei.com>
The following error occurred while I was testing:
|
@q384566678 which 'runc' version do you use? Annotations are supported in mine.
|
|
It's ok when I update the |
We should really check |
My understanding of the related spec is: I'll add a new PR to check this. ##Prestart
##Poststart
##Poststop
|
Signed-off-by: Liang Chenye liangchenye@huawei.com