Skip to content

fix runc's poststart behaviour doesn't match the runtime-spec #4348

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
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix runc's poststart behaviour doesn't match the runtime-spec #4348

wants to merge 1 commit into from

Conversation

@ningmingxiao
Copy link
Contributor

@ningmingxiao ningmingxiao commented Jul 15, 2024 •
edited
Loading

fix:#4347

@ningmingxiao ningmingxiao force-pushed the fix_poststart branch 2 times, most recently from 6ffc50a to 307c2b6 Compare July 15, 2024 21:50
cyphar
cyphar requested changes Jul 16, 2024
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

The code should be moved to container.Exec (keep it inside libcontainer).

@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 16, 2024

Please write a more detailed commit message. "fix poststart error" is not a good one.

@kolyshkin kolyshkin marked this pull request as draft July 16, 2024 01:48
@ningmingxiao ningmingxiao changed the title draft: fix poststart error draft: fix runc's poststart behaviour doesn't match the runtime-spec Jul 16, 2024
@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Jul 16, 2024 •
edited
Loading

Please write a more detailed commit message. "fix poststart error" is not a good one.

done.thank you. please review my pr. @kolyshkin @cyphar

@ningmingxiao ningmingxiao force-pushed the fix_poststart branch 2 times, most recently from c6c42bc to b766494 Compare July 16, 2024 06:57
@ningmingxiao ningmingxiao changed the title draft: fix runc's poststart behaviour doesn't match the runtime-spec fix runc's poststart behaviour doesn't match the runtime-spec Jul 16, 2024
lifubang
lifubang reviewed Jul 16, 2024
View reviewed changes
lifubang
lifubang reviewed Jul 16, 2024
View reviewed changes
@lifubang lifubang added this to the 1.1.14 milestone Jul 16, 2024
@lifubang lifubang added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Jul 16, 2024
@lifubang lifubang modified the milestones: 1.1.14, 1.2.0 Jul 16, 2024
lifubang
lifubang requested changes Jul 16, 2024
View reviewed changes
Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

Could you please add a test case to avoid the similar error in future.
You can see:
#4323 (comment)

libcontainer/container_linux.go
return err
}
if c.config.Hooks != nil {
if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
Copy link
Member

@lifubang lifubang Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As the runtime-spec said:
The poststart hooks MUST be invoked by the runtime. If any poststart hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded.

So maybe this is another bug, we should write a new function to run poststart and poststop hooks.
@kolyshkin WDYT

Copy link
Member

@lifubang lifubang Jul 16, 2024

Choose a reason for hiding this comment

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

If yes, please help to open a new issue to track this bug, thanks.

This comment was marked as outdated.

Sign in to view

Copy link
Contributor

@kolyshkin kolyshkin Jul 17, 2024

Choose a reason for hiding this comment

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

So, the current code logs a warning (like the spec says) but also terminates container's init and returns an error (unlike the spec says).

I'm not sure whether we should change the spec or runc behavior. In theory spec should be left as is, and runc should be changed to follow the spec. In practice, though, changing runc might result in backward incompatibilities and complexities in upper runtime. So yes, we have a choice to make here.

It's complicated 😿

Copy link
Contributor

@kolyshkin kolyshkin Jul 17, 2024

Choose a reason for hiding this comment

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

I researched the history a little bit.

  1. The initial implementation of poststart hooks was done by @mrunalp in 2015 (commit 452e8a7, PR Add poststart hooks #392, merged Nov 2015). If a hook was returning an error, the start was terminated and the error was returned.
  2. The "start must continue even if poststart hook failed" clause was added to runtime-spec by @RenaudWasTaken in Add create-runtime, create-container, start-container hooks runtime-spec#1008 (merged Dec 30 2019).
  3. More hooks were added to runc by @RenaudWasTaken (commit ccdd757, PR Add CreateRuntime, CreateContainer and StartContainer Hooks #2229, merged Jun 2020). The same commit touched the poststart code but didn't change it to not return an error if a hook failed.

In my mind, this is the spec that needs to be changed. The reasons are:

  • initial implementation predates the spec wording by a few years;
  • the wording in the spec was never implemented in runc;
  • returning an error (and stopping the container) seems like a more versatile approach (a hook can choose whether to return an error or not).

Opened opencontainers/runtime-spec#1262

Copy link
Contributor Author

@ningmingxiao ningmingxiao Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

how to stop container by using SIGTERM ,SIGKILL or both use?

Copy link
Member

@cyphar cyphar Aug 27, 2025

Choose a reason for hiding this comment

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

Probably SIGTERM followed by SIGKILL or just SIGKILL.

@lifubang lifubang mentioned this pull request Jul 16, 2024
Open
kolyshkin
kolyshkin reviewed Jul 17, 2024
View reviewed changes
@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Jul 17, 2024

already backport #4351

@kolyshkin kolyshkin mentioned this pull request Jul 17, 2024
Closed
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 17, 2024

The commit description still doesn't say what it does and why.

@kolyshkin kolyshkin mentioned this pull request Jul 17, 2024
Closed
kolyshkin
kolyshkin requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

This PR removes the part where the container is killed when a hook fails. This should have been caught by an integration test, alas, it doesn't check that.

Opened #4352

@ningmingxiao ningmingxiao force-pushed the fix_poststart branch 2 times, most recently from d2fac01 to adcf828 Compare July 18, 2024 06:39
@ningmingxiao ningmingxiao force-pushed the fix_poststart branch 5 times, most recently from 5d066a8 to df2c44a Compare July 18, 2024 11:42
@cyphar
Copy link
Member

cyphar commented Oct 21, 2024

Moved the milestone to 1.3.0. Dealing with this is going to take a while, and we should just get 1.2.0 out (this is a pre-existing issue anyway).

@cyphar cyphar modified the milestones: 1.2.0, 1.3.0 Oct 21, 2024
@kolyshkin kolyshkin removed the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Feb 12, 2025
@kolyshkin
Copy link
Contributor

kolyshkin commented Feb 12, 2025

A draft until opencontainers/runtime-spec#1262 is decided on

@cyphar
Copy link
Member

cyphar commented Mar 1, 2025

Damn, I missed this because it wasn't in the -rc.1 milestone. I guess we'll need to wait for 1.4.0-rc.1 before fixing this.

@rata
Copy link
Member

rata commented Apr 8, 2025

The spec PR is not yet merged, moving to 1.4. Now to 1.4.0-rc.1, so hopefully you don't miss it @cyphar :)

@rata rata modified the milestones: 1.3.0, 1.4.0-rc.1 Apr 8, 2025
@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Apr 8, 2025

Ok thanks

@cyphar cyphar modified the milestones: 1.4.0-rc.1, 1.5.0-rc.1 Aug 27, 2025
@cyphar cyphar mentioned this pull request Aug 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lifubang lifubang lifubang requested changes

@cyphar cyphar cyphar requested changes

@kolyshkin kolyshkin kolyshkin requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.5.0-rc.1

Development

Successfully merging this pull request may close these issues.

5 participants

@ningmingxiao @kolyshkin @cyphar @rata @lifubang