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.

Sign up for GitHub

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

Jump to bottom

clarify kill and delete operation for shared pid namespace container #1234

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

clarify kill and delete operation for shared pid namespace container #1234

wants to merge 1 commit into from

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Oct 6, 2023 •
edited
Loading

There was no description for delete a container which doesn't have it's own private pid namespace, so it may cause some break changes when we do code refactor in this area.
Maybe we should add some descriptions for this.

In fact, container-runtime kill [-a,--all] container_id signal] and container-runtime delete [-f,--force] container_id has been used by upstream projects for many years, but it isn't defined in runtime-spec. It may cause some break changes when we do code refactor in this area.
So, maybe we should define it clearly in runtime-spec.

background: opencontainers/runc#4049

giuseppe
giuseppe reviewed Oct 6, 2023
View reviewed changes
runtime.md Outdated Show resolved Hide resolved
@lifubang
Copy link
Member Author

lifubang commented Oct 8, 2023

@opencontainers/runtime-spec-maintainers PTAL, we need this PR to make a decision how to fix a bug in runc(opencontainers/runc#4047).
Looking forward your feedback, thanks.

@lifubang lifubang mentioned this pull request Oct 10, 2023
Closed
kolyshkin
kolyshkin reviewed Oct 10, 2023
View reviewed changes
runtime.md Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Oct 10, 2023
View reviewed changes
runtime.md Outdated
@@ -129,18 +129,19 @@ This operation MUST run the user-specified program as specified by [`process`](c
This operation MUST generate an error if `process` was not set.

### <a name="runtimeKill" />Kill
`kill <container-id> <signal>`
`kill <container-id> [-a,--all] <signal>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1190 (review)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed this PR to clarify kill and delete operation for shared pid namespace container, let's discuss -a in #1190 .

AkihiroSuda
AkihiroSuda requested changes Oct 10, 2023
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime-spec/runtime.md

Line 95 in 6331715

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

@lifubang lifubang changed the title clarify kill and delete operation clarify kill and delete operation for shared pid namespace container Oct 10, 2023
@lifubang
Copy link
Member Author

runtime-spec/runtime.md

Line 95 in 6331715

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

I wonder we should change this rule or not?
Before we change this rule, I changed my PR to clarify kill and delete operation for shared pid namespace container, if we don't think this type container should be described in runtime-spec, feel free to close this PR.

@fuweid
Copy link
Member

fuweid commented Oct 11, 2023 •
edited
Loading

@lifubang the operation can be function or command-line. -a is kind of command-line api. I think the maintainer can accept <all> here.

However, the runc kill -a has been used for many years. <all> can be -a in the command-line implementation, is it correct? @AkihiroSuda

@AkihiroSuda
Copy link
Member

However, the runc kill -a has been used for many years. can be -a in the command-line implementation, is it correct? @AkihiroSuda

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

@lifubang
Copy link
Member Author

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

@AkihiroSuda
Copy link
Member

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

Adding the CLI spec to the Runtime Spec was once rejected in 2017 (#513 (comment)) and accepted in the runtime-tools repo: https://github.com/opencontainers/runtime-tools/blob/master/docs/command-line-interface.md

I guess we can revisit this, but it is beyond the scope of this PR. (Feel free to open an issue/PR)

@fuweid
Copy link
Member

fuweid commented Oct 11, 2023

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

Adding the CLI spec to the Runtime Spec was once rejected in 2017 (#513 (comment)) and accepted in the runtime-tools repo: https://github.com/opencontainers/runtime-tools/blob/master/docs/command-line-interface.md

I guess we can revisit this, but it is beyond the scope of this PR. (Feel free to open an issue/PR)

Thanks. We should discuss it in other pr (I also received the runc maintainer's comment about introducing new flag opencontainers/runc#4045)

kolyshkin
kolyshkin reviewed Nov 28, 2023
View reviewed changes
runtime.md Outdated

This operation MUST send the specified signal to the container's init process.

Specially, if the signal is `SIGKILL` and the container does not use its own private PID namespace, this operation MUST send the `SIGKILL` signal to all the processes in the container, even if the container's state is `stopped`. If there is no process left in this type container, the operation MUST [generate an error](#errors).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. s/does not use/does not have/
  2. Traditionally, runc kill -a does not result in an error even if there are no processes left. I'm not sure if we want to change it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Traditionally, runc kill -a does not result in an error even if there are no processes left. I'm not sure if we want to change it here.

Yes, I think we should not result in an error here, I'll remove these words later.

kolyshkin
kolyshkin reviewed Nov 28, 2023
View reviewed changes
runtime.md Outdated
Deleting a container MUST delete the resources that were created during the `create` step.

Specially, when deleting a container, which does not use its own private PID namespace, the operation should ensure kill all processes in this type container, and ensure no process left in it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/does not use/does not have/

s/should ensure kill all processes/SHOULD kill all processes/

s/and ensure no process left in it./and return an error if those processes can not be killed./

@lifubang
clarify kill and delete operation for shared pid namespace container
9eadaa0 
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
@lifubang
Copy link
Member Author

lifubang commented Dec 6, 2023

@opencontainers/runtime-spec-maintainers PTAL

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

@kolyshkin kolyshkin kolyshkin left review comments

@crosbymichael crosbymichael Awaiting requested review from crosbymichael crosbymichael is a code owner

@cyphar cyphar Awaiting requested review from cyphar cyphar is a code owner

@dqminh dqminh Awaiting requested review from dqminh dqminh is a code owner

@hqhq hqhq Awaiting requested review from hqhq hqhq is a code owner

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@thaJeztah thaJeztah Awaiting requested review from thaJeztah thaJeztah is a code owner

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@vbatts vbatts Awaiting requested review from vbatts

@utam0k utam0k Awaiting requested review from utam0k utam0k is a code owner

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda AkihiroSuda is a code owner

@giuseppe giuseppe Awaiting requested review from giuseppe giuseppe is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lifubang @fuweid @AkihiroSuda @giuseppe @kolyshkin