move the validation logic into executor

[yihuaf]

To allow more flexibility for the executor, we move the validate logic into the executor. The validate runs in the create step before workloads are executed. Instead of implementing the validation in the exec, to maintain backward competiability, we have to introduce an extra step. The exec is too late to fail if the spec is not validated.

[yihuaf]

@utam0k @YJDoc2 Let know what you guys think. I do have another branch that implements the optional verify_binary in the builder interface. I think either approach is OK and worth prototyping.

[codecov-commenter]

Codecov Report

Merging #2258 (ad5ef2c) into main (9cb9833) will decrease coverage by 0.12%.
The diff coverage is 46.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2258      +/-   ##
==========================================
- Coverage   64.99%   64.88%   -0.12%     
==========================================
  Files         129      129              
  Lines       15153    15178      +25     
==========================================
- Hits         9849     9848       -1     
- Misses       5304     5330      +26     

[utam0k]

validate is one of the purposes of implementing this function. How about before_exec() or something?
If I was a user of libcontainer, I would like to know the environment in which the validate() will run. e.g after the cgroup etc is applied

[yihuaf]

There are a few things to unpack here. I would group before_exec or similar functions into lifecycle hook category. OCI spec offers these. I am a little hesitant to expand this concept for the executor.

You raised a valid point that user would want to know the environment when validate runs. One potential solution is to extensively document the behavior. However, this will require the users to actually dig in the documentation (or comments) to understand this.

Currently, we are designing this based on a single usercase from runwasi. Is there another usecase we can look to?

[utam0k]

Currently, we are designing this based on a single usercase from runwasi. Is there another usecase we can look to?

I don't know for now. Why not create a new error(maybe ValidateError) to narrow down usage for this interface?

[yihuaf]

New error is hard. We provide an error interface for the trait without the actual implementation. There is no good way to know upfront what specific validation error should arise. Even with the executor error, we have a custom error as one of the type for this reason. The only error that can be handled is the can't handle error.

[utam0k]

I was a little short in my explanation. My point is this:

#[derive(Debug, thiserror::Error)]
pub enum ExecutorValidateError {
    #[error("{0} executor can't handle spec")]
    CantHandle(&'static str),
}

pub trait Executor: CloneBoxExecutor {
    /// Executes the workload
    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>;

    /// Validate if the spec can be executed by the executor. This step runs
    /// after the container init process is created, entered into the correct
    /// namespace and cgroups, and pivot_root into the rootfs. But this step
    /// runs before waiting for the container start signal.
    fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidateError>;
}

I think exec() and validate() will return the different types of errors. They should be fundamentally separated.

[yihuaf]

I see. Done.

[utam0k]

I generally agree, but I left the comment.

[utam0k]

May I ask you to write the migration guide?

[jsturtevant]

@yihuaf thanks! this approach looks like it would help address the challenges we ran into on the runwasi side

[YJDoc2]

May I ask you to write the migration guide?

@yihuaf , If you rebase with current main, I have started a doc for this in one of my PRs. You can extend that, or re-write that (the existing points in it are few) as needed.

[Mossaka]

I know that this PR just moved code but I want to raise the concern where @jprendes raised here: containerd/runwasi#156 (comment)

The issue is that the is_executable check should be inside of the for loop, not outside. @jprendes provides an example of illustrating why that's the case.

[utam0k]

@Mossaka I have created the issue to address what you mentioned #2277. I think this issue is outside the scope of this PR so we should take care of it in another PR, but I appreciate you pointed out, thanks.