Build libcontainer's container on Instance::new to be spec compliant

[jprendes]

Fixes #158 and #326

The issue behind #326 is that we are asking youki to create and start the container on the start call.
We don't do any meaningful work on the create call.

This missmach means that containerd is assuming that extra setup has happened successfully. but youki hasn't done it yet. When youki fails to do it at start, we are breaking those assumptions (e.g., being able to provide a PID for the init process).
The solution is to build (create) the youki container on the create call (i.e., the new method of runwasi's instance).
This also requires that we change the signature of new to return a Result<Self, Error>, which was previously mentioned in #158.

I haven't added any test for this here as testing it is rather involved.
The sideeffect of this issue on the first invocation is that we leave a dangling process, and on a second invocation is a hang.
I'm open to suggestions on how we could test it. Thanks @jsturtevant for suggesting testing approach.

[jprendes]

@squillace FYI

[jsturtevant]

The sideeffect of this issue on the first invocation is that we leave a dangling process, and on a second invocation is a hang.

this is the side affect of this change?

I'm open to suggestions on how we could test it.

Would it be something like:

fn test_custom_entrypoint() -> anyhow::Result<()> {
    let (exit_code, stdout, _) = WasiTest::<WasiInstance>::builder()?
        .with_start_fn("foo")?
        .with_wasm(INVALID_ENTRYPOINT)?
        .build()?
    assert!(error occurred);

    Ok(())
}

[jprendes]

this is the side affect of this change?

No, this is what I observe on linux before this change. With this change I do not observe any dangling process and no hanging.

[jprendes]

Would it be something like:

Yes, that's 100% correct, thanks!. I'll add test points for all shims.

[jprendes]

I gave the test a go. The right place for it would be in the containerd-shim-wasm crate, as there's where the logic is implemented.

However, that test uses the containerd-shim-wasm-test crate. Adding that crate as a dependency creates a cycle which causes some weird interaction with types being doubly defined (e.g., there's a crate::sandbox::Instance and a containerd_shim_wasm::sandbox::Instance, and they are not the same trait for the compiler)

A solution is moving the testing utility from containerd-shim-wasm-test to containerd-shim-wasm behind a feature flag. Doing that significantly increases the number of files touched by this PR, but shouldn't add much complexity.

[jprendes]

I recommend reviewing the commits separately :-)

[jsturtevant]

great find! Creating the container during the new aligns with containerd's "task_create" which makes sense. left a few minor comments

[jsturtevant]

since these are under dev-dependencies are they needed here?

[jprendes]

For consumers of the testing feature of the library we need to specify it as a normal dependencies as well.
All 3 dev-dependencies are also specified as optional normal dependencies that get enabled by the testing feature.
I'm open to suggestions 😄

[jsturtevant]

LGTM

[jsturtevant]

this is safe since Nop::new() always returns Ok. might be work a comment incase things change in future?

[devigned]

@jprendes this needs a rebase, but lgtm after that.

[devigned]

lgtm!