Skip to content
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.

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

containerd-shim-runhcs-v1 can't parse runtimeoptions.v1.Options #1941

Open
TBBle opened this issue Oct 22, 2023 · 0 comments
Open

containerd-shim-runhcs-v1 can't parse runtimeoptions.v1.Options #1941

TBBle opened this issue Oct 22, 2023 · 0 comments

Comments

@TBBle
Copy link
Contributor

TBBle commented Oct 22, 2023

I hit an issue where BuildKit was sending an empty runtimeoptions.v1.Options structure through, and because it doesn't know that type the shim terminates before setting up its connection back to containerd, which makes debugging super-annoying (like regular annoying, but wearing a cape) because containerd cleans up the resulting panic.log when it can't connect to the pipe.

I've submitted a PR to BuildKit to send the hcsshim-specific struct, since it supports populating the struct from the config block like containerd/cri does. That said, it might make sense for the containerd-shim-runhcs-v1 to actually support this options type.

Options include:

  • recognise it, ignore its contents, and fail once we are able to send a useful results back to containerd (assuming default config).
  • parse it, and fail (usefully) it if it's not empty, i.e. trivial cases like BuildKit work, but can't send any config through.
  • parse it, and support ConfigPath and ConfigBody as pointing to an on-disk/byte-serialised (respectively) containerd.runhcs.v1.Options.

That last option is interesting because as it happens, this would allow removing the special casing for containerd.runhcs.v1.Options in containerd/cri, and hence support a generic code-flow for any such container-executor, like BuildKit. BuildKit lacks this generic path right now anyway though, so it's not an immediate win. (Although you could write a config block in BuildKit with the literal ConfigBody that then contains the desired options, and I think it would serialise correctly.)

On the other hand, not supporting runtimeoptions.v1.Options means that the caller must ensure that the desired config block can parse into containerd.runhcs.v1.Options, and that's not a bad thing for early failure detection and useful user feedback.

I also got the impression that runtimeoptions.v1.Options's ConfigBody is legacy, and for the generic path, `ConfigPath' is less useful as the non-generic paths in both containerd/cri and BuildKit work from parsing a config block, so they'd need to write that block out to disk. (Which is not insane, but not as simple)


Anyway, whichever option is taken, including working-as-intended, it'd also be nice for callers if the hcsshim options package exposed the literal string io.containerd.runhcs.v1 so that they don't have to use local consts when picking an options structure to match a runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant