Repository navigation
runtime: allow delete of a created container - #1324
Conversation
utam0k
left a comment
There was a problem hiding this comment.
LGTM. We should also update runtime-tools to reflect this change. The current test requires deleting a container in the created state to fail.
https://github.com/opencontainers/runtime-tools/blob/8a4db579f5c88af5a0d036fad34bddc9c1f703f3/validation/delete/delete.go#L51-L80
| This operation MUST [generate an error](#errors) if it is not provided the container ID. | ||
| Attempting to `delete` a container that is not [`stopped`](#state) MUST have no effect on the container and MUST [generate an error](#errors). | ||
| Attempting to `delete` a container that is neither [`created`](#state) nor [`stopped`](#state) MUST have no effect on the container and MUST [generate an error](#errors). | ||
| Attempting to `delete` a container that is [`created`](#state) MAY either have no effect on the container and [generate an error](#errors), or stop the container and then delete it. |
There was a problem hiding this comment.
I got confused for a bit because created in Moby means "no processes running", but in the OCI runtime, I guess in this stage, the init process may be present.
Wondering if it should be more explicit, e.g.
When deleting a container in the created state, the runtime MAY terminate the container's init process and then delete the container.
There was a problem hiding this comment.
I'm not sure about the terminology here. What we have in created container is, strictly speaking, not container's init yet, but rather "proto-init" (runc init or something, which just waits to exec the real container init). It is "init" in a sense that it's PID is 1 inside a container, but not "init" by any other means (not what's specified in spec nor what is expected by a user).
Referring to that proto-init as init it makes it less clear to me (☝🏻 "but hey, there's no init yet at this stage").
There was a problem hiding this comment.
(wrote a comment and lost it; trying to recreate a short version)
in other parts of spec (definition of "created") it is called "container process" so I'm following the trend.
thaJeztah
left a comment
There was a problem hiding this comment.
overall LGTM, but left a suggestion for consideration.
|
Looks like GolangCi-lint needs an update in this repository; |
Since before runtime-spec v1.0.0, both runc and crun implement `delete` of a container in the `created` state by terminating the container process and then deleting the container, rather than generating an error. This is useful in practice, since a container which was created but never started can not otherwise reach the `stopped` state without an explicit `kill`. Relax the spec to allow this behavior, while keeping the current one (generating an error) valid as well. Deleting a container in any state other than `created` or `stopped` still MUST generate an error. See also: opencontainers/runc#1774 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
e60a91c to
ed05122
Compare
|
Since runtime-spec PR #1324, attempting to delete a container in the created state MAY either have no effect and generate an error (as before), or terminate the container process and delete the container (as runc and crun do). Add a new DeleteCreatedNoEffectOrDelete spec error code, and modify the delete test so that the created case passes in either of the two cases. Update DeleteNonStop* comments and messages accordingly. See opencontainers/runtime-spec#1324 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
Since runtime-spec PR #1324, attempting to delete a container in the created state MAY either have no effect and generate an error (as before), or terminate the container process and delete the container (as runc and crun do). Add a new DeleteCreatedNoEffectOrDelete spec error code, and modify the delete test so that the created case passes in either of the two cases. Update DeleteNonStop* comments and messages accordingly. See opencontainers/runtime-spec#1324 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since before runtime-spec v1.0.0, both runc and crun implement
deleteof a container in thecreatedstate by terminating the container process and then deleting the container, rather than generating an error.Relax the spec to allow this behavior, while keeping the current one (generating an error) valid as well. Deleting a container in any state other than
createdorstoppedstill MUST generate an error.See also: