Hi @jterry75. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Should resolve: #5067

@thaJeztah - Know its been ages since you have thought about this problem :). But mind taking a look as well?

I actually already made the carry of the image spec changes and then decided to do it this way I'll open that PR right now. Maybe we can just get this formally supported in OCI and then we don't have to hack it here. It will be the exact same solution just wont need the extended structs.

Our current thinking internally was that ArgsEscaped in the image spec is kind of a hack. The reality is that Windows doesn't naturally use an argument array for launching a process, but rather a single command line. The idea was instead of formalizing ArgsEscaped on the image spec, we should instead add a CommandLine field (or perhaps EntrypointCommandLine/CmdCommandLine to match Entrypoint/Cmd), since this more naturally represents Windows process launching capabilties. This would also mirror the Args/CommandLine split on the runtime spec.

So I would suggest we consider that for the image spec change, rather than reviving my old PR. :)

By the way, thanks @jterry75 for working on this!

I saw the ping, but haven't had time yet to read up (will try to if still needed); I do recall indeed that this was a bit "hairy", and a bit of a hack.

From the implementations' standpoint, it would be great if following OCI Image Spec assures that existing container images work as expected. I see the Image Spec as a "retro" spec like HTML5, rather than an aspiring new standard like XHTML.

So while I don't have strong opinions regarding CommandLine (I honestly am not familiar about the semantics in Windows), I'd like to see ArgsEscaped documented and standardnized, even that is considered as Deprecated from the beginning.

I'm ok with continuing to drive ArgsEscaped into OCI even if deprecated just to signify that a world out there exists that already uses this. But are we ok doing that separate from this PR? Do we need that to happen before we can take this PR to support these types of images today?

I'm fine having OCI discussions later.

I like the idea of adding ArgsEscaped to OCI and immediately marking it as deprecated, and also figuring out the right long term solution (such as CommandLine). Agree we can get this in to fix these images working in containerd, and worry about OCI later.

@jterry75 I'll give this more of a look on Monday.

/ok-to-test

I like the idea of adding ArgsEscaped to OCI and immediately marking it as deprecated, and also figuring out the right long term solution (such as CommandLine). Agree we can get this in to fix these images working in containerd, and worry about OCI later.

@jterry75 I'll give this more of a look on Monday.

/ok-to-test

Sounds like a plan!

BTW would this fix #6300?

BTW would this fix #6300?

Yup, that looks like the same issue as #5067, which this will fix.

@kevpar - Still need to look at CRI WithProcessArgs you left. But this fixes all of @thaJeztah's comments.

@kevpar - Turns out this is quite complicated to fix for CRI. There are two entrypoints for CRI via for sandbox_run and container_create. For both cases they apply with opts.WithProcessArgs as you linked above. Problem is that these operate on a v1.ImageConfig which has already been pre-resolved. So we cannot do any extra logic here to deserialize and try to handle this with a forked implementation as it exists today.

I looked at instead trying to do this by updating the callers to use WithImageConfigArgs from the oci package and then WithProcessArgs if the config has overrides but again, the oci package from containerd expects a containerd.Image not an v1.Image. Because of this mismatch we would need to re-write the apply logic in CRI for both Windows and Linux which seems risky in the scope of this change. I do think it needs to be done though as there is literally zero reason that CRI should have a forked application here.

oci.WithImageConfigArgs applies Env, Cwd, User, Args and on Windows potentially CommandLine. It also takes care of appending additional args via the config passed in.

And sandbox_run and container_create do the exact same code.... and eventually the custom WithProcessArgs func that again, overloads the same code for image arg combining.

CRI should in theory be able to say:

`WithImageConfigArgs(image, config.Args)`
if (len(config.GetCommand()) > 0) {
  WithProcessArgs(append(config.Command, config.Args)) // Overwrite image default entrypoint/command
}
if (config.User != "") {
  WithUser(...) // Overwrite image spec user.
}

You get the idea, but this will be a very large change to do in this PR.

Anything else needed here? Would love to get this in if no objections. Thanks

Ok, @kevpar - We now have a test case for every combination of Entrypoint, Cmd, Args for both ArgsEscaped==false (the normal, sane, supported, actually good, way), and ArgsEscaped==true (the ...). I believe this mirrors Docker behavior and solves the problem for the exact same set of cases Docker supports. Plz take one final look

I think my only final feedback is we should probably add a comment summarizing the discussion/design decisions that went into this. Since from looking at it it's pretty unintuitive what it's doing and why.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 34s (non-voting)
• containerd-test-arm64 : FAILURE in 5m 37s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 25m 00s (non-voting)

I think my only final feedback is we should probably add a comment summarizing the discussion/design decisions that went into this. Since from looking at it it's pretty unintuitive what it's doing and why.

Done.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 59s (non-voting)
• containerd-test-arm64 : FAILURE in 5m 41s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 24m 52s (non-voting)

@kzys @thaJeztah @estesp would one of you mind re-reviewing? The logic has changed quite a bit.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 39s (non-voting)
• containerd-test-arm64 : FAILURE in 5m 50s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 27m 55s (non-voting)

IT MERGED!!!!!! Thanks all