search

opencontainers / runtime-spec

OCI Runtime Specification
http://www.opencontainers.org
Apache License 2.0
3.13k stars 535 forks source link

config: change prestart hook spec to match reality #1169

Closed corhere closed 1 year ago

corhere commented 1 year ago

runC originally implemented prestart hooks contrary to the spec. And it still implements them the same way today, as it would break a lot of projects which have come to rely on the existing behaviour. Any OCI runtime implementations which want to be compatible with projects that have come to rely on the existing runC behaviour must also implement them contrary to the spec. Furthermore, the Lifecycle section of the spec requires the existing runC behaviour for the prestart hook, directly contradicting the section of the spec which defines the prestart hook in config.md!

https://github.com/opencontainers/runtime-spec/blob/494a5a6aca782455c0fbfc35af8e12f04e98a55e/runtime.md?plain=1#L52-L77 https://github.com/opencontainers/runtime-spec/blob/494a5a6aca782455c0fbfc35af8e12f04e98a55e/config.md?plain=1#L445

Given that existing implementations cannot be changed, the spec contradicts existing implementations, and the spec contradicts itself, amending the spec to align with the existing runC behaviour is the only viable way to resolve the contradiction.

AkihiroSuda commented 1 year ago

Alternative to https://github.com/opencontainers/runtime-spec/pull/1167 for resolving the conflict within the spec

Do you want which of #1167 and #1169 to be merged?

corhere commented 1 year ago

Do you want which of #1167 and #1169 to be merged?

I just want clarity on when in the container lifecycle hooks are expected to be executed. I have no strong preference on how.

h-vetinari commented 1 year ago

Do you want which of #1167 and #1169 to be merged?

I just want clarity on when in the container lifecycle hooks are expected to be executed. I have no strong preference on how.

I think this PR is clearly preferable to #1167, as it codifies existing behaviour (which was the hole point of the hooks saga), rather than break them through the spec (and have implementations not follow because it'd break their consumers).

AkihiroSuda commented 1 year ago

code-review/pullapprove Pending

The PR is not mergable due to the pending pullapprove but the pullapprove link doesn't work. Can we just remove it? cc @caniszczyk

AkihiroSuda commented 1 year ago

cc @opencontainers/runc-maintainers to confirm that we are fine to amend the spec to match the reality of runc

AkihiroSuda commented 1 year ago

@opencontainers/runc-maintainers @opencontainers/runtime-spec-maintainers Ready to merge this?