Add prestart_fail hook test

[fspv]

Description

This implements a test similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/prestart_fail/prestart_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 Go test tests only one piece of behaviour: when the prestart hook fails the runtime must generate an error. This test also tests that:

• Hooks before the failed hook execute successfully
• Hooks after the failed hook don't execute

[fspv]

Also added a create_runtime failed test #3408 similar to #3396 which has been added in addition to the prestart test

[YJDoc2]

Hey, thanks for the PR. I have something I'd like your opinion on -

• In original go test we would explicitly have a file created v/s not created check. Here we don't use file for that, but just use container creation. I'd prefer if the container command also wrote to the file similar to hooks, instead of just true
• If I'm not wrong, then OCI spec imposes order on hooks, so here we can also check that hook 1 ran before hook 2, not just that their output is present or not.

Apart from that, overall looks good to me.

[fspv]

@YJDoc2 replying to your comments.

In original go test we would explicitly have a file created v/s not created check. Here we don't use file for that, but just use container creation. I'd prefer if the container command also wrote to the file similar to hooks, instead of just true

I'm not sure it makes sense to test container startup logic here, as it is part of the "start" command, not "create". We already test that hooks execute during the "create" stage here: https://github.com/youki-dev/youki/blob/main/tests/contest/contest/src/tests/prestart/mod.rs#L103 So it kind of guaranteed that we don't execute start. So testing start functionality here will be redundant.

I hope I understood your question correctly, so let me know if I didn't.

If I'm not wrong, then OCI spec imposes order on hooks, so here we can also check that hook 1 ran before hook 2, not just that their output is present or not.

Yeah, this makes sense. Will also add order check.

[YJDoc2]

@fspv , let me try to elaborate more, I think the way I wrote was confusing.

The original test sets the container process to create a file, and in assertions we check that the file does not exist

g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("touch %s", "/output")})

In our test, we only have true as the container command -

            ProcessBuilder::default()
                .args(vec![
                    "/bin/sh".to_string(),
                    "-c".to_string(),
                    "true".to_string(),
                ])

What I'm suggesting is that we should have the container command similar to hooks, which writes something to the same output file, and assert explicitly that that string is not present.

My reasoning is that, as of now youki behaves correctly, and will not start container process, but we might have some regression in future (or some other runtime might use this test suit) where it might start the container process even though a prestart hook fails. Adding a check here of the container process o/p would also assert that runtime is not starting the container process if one of the prestart hook fails. I checked the test you are mentioning, but I don't think that considers the prestart fail case, right?

What do you think?

[fspv]

My reasoning is that, as of now youki behaves correctly, and will not start container process

I still think this is covered by other tests, so we will duplicate the existing coverage here. But anyway having more tests never hurt anyone, so added this check as well.

edit: also added an ordering assertion