Skip to content

Conversation

@fspv
Copy link
Contributor

@fspv fspv commented Dec 7, 2025

Description

This implements a test similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/poststart_fail/poststart_fail.go as a part of the #361

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test updates
  • CI/CD related changes
  • Other (please describe):

Testing

  • Added new unit tests
  • Added new integration tests
  • Ran existing test suite
  • Tested manually (please provide steps)

Related Issues

#361

Additional Context

The test is similar to the happy case test landed in #3292 but it focuses on the error handling.

I made it a bit different to the original go test to make sure we also test a couple of more things. I create three hooks, with the second hook failing. We expect that the first two hooks to run and exit with an error. The hook after the failed hook should not run as implied by the lifecycle spec https://github.com/opencontainers/runtime-spec/blob/main/runtime.md#lifecycle.

One assumption I make here is that the hooks are executed in the same order they're defined in a spec, which is not explicitly stated anywhere in the runtime spec but anyway I think it is a relatively safe assumption and it is nice to guarantee this order even if the spec doesn't require it.

Also, this test is affected by the same bug as a happy case scenario opencontainers/runc#4347 so an extra branch has been added to handle the incorrect runc behaviour.

Signed-off-by: Pavel Safronov <pv.safronov@gmail.com>
@fspv
Copy link
Contributor Author

fspv commented Dec 7, 2025

Hey @YJDoc2

Can you help me with containerd-integration-tests failure?

It fails on

--- FAIL: TestContainerPids (0.19s)

I can't see how it could be related to the change I made here.

@saku3 saku3 added the kind/test label Dec 7, 2025
@saku3
Copy link
Member

saku3 commented Dec 7, 2025

As you said, this PR only changes test code, so the containerd-test error is unrelated.

Copy link
Member

@saku3 saku3 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

Overall, it looks good to me, but I left two comments.

Could you please take a look?

Signed-off-by: Pavel Safronov <pv.safronov@gmail.com>
@fspv
Copy link
Contributor Author

fspv commented Dec 15, 2025

I've addressed the comments, but one of the tests timed out. Pretty sure it is not related to my change, so will be happy if someone can trigger a re-run.

@utam0k
Copy link
Member

utam0k commented Dec 29, 2025

LGTM w/ one nit

Signed-off-by: Pavel Safronov <pv.safronov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants