config: change prestart hook spec to match reality#1169
config: change prestart hook spec to match reality#1169cyphar merged 1 commit intoopencontainers:mainfrom
Conversation
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!_ 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. Signed-off-by: Cory Snider <[email protected]>
corhere
requested review from
AkihiroSuda,
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
kolyshkin,
mrunalp,
thaJeztah,
tianon and
vbatts
as code owners
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). |
|
|
||
| The definition of `createRuntime` hooks is currently underspecified and hooks authors, should only expect from the runtime that the mount namespace have been created and the mount operations performed. Other operations such as cgroups and SELinux/AppArmor labels might not have been performed by the runtime. | ||
|
|
||
| Note: `runc` originally implemented `prestart` hooks contrary to the spec, namely as part of the `create` operation (instead of during the `start` operation). This incorrect implementation actually corresponds to `createRuntime` hooks. For runtimes that implement the deprecated `prestart` hooks as `createRuntime` hooks, `createRuntime` hooks MUST be called after the `prestart` hooks. |
There was a problem hiding this comment.
Maybe we can add a note that runc was contrary to the spec v1.0 but now conforms to this spec v1.1.
The PR is not mergable due to the pending pullapprove but the pullapprove link doesn't work. |
|
cc @opencontainers/runc-maintainers to confirm that we are fine to amend the spec to match the reality of runc |
|
@opencontainers/runc-maintainers @opencontainers/runtime-spec-maintainers Ready to merge this? |
7 participants
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!
runtime-spec/runtime.md
Lines 52 to 77 in 494a5a6
runtime-spec/config.md
Line 445 in 494a5a6
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.