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

move the validation logic into executor #2258

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Aug 13, 2023

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 yihuaf added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. breaking change labels Aug 13, 2023
@yihuaf
Copy link
Collaborator Author

yihuaf commented Aug 13, 2023

@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.

@yihuaf yihuaf requested review from utam0k and YJDoc2 August 13, 2023 19:45
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2023

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     

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

fn validate(&self, spec: &Spec) -> Result<(), ExecutorError>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Done.

Copy link
Collaborator

@utam0k utam0k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree, but I left the comment.

@utam0k
Copy link
Collaborator

utam0k commented Aug 14, 2023

May I ask you to write the migration guide?

@jsturtevant
Copy link

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

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Aug 16, 2023

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.

if name.contains('/') && PathBuf::from(name).exists() {
return Some(PathBuf::from(name));
}
for path in path_var.split(':') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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.

Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
@yihuaf yihuaf requested a review from utam0k August 20, 2023 17:22
Signed-off-by: yihuaf <yihuaf@unkies.org>
@utam0k utam0k merged commit 5f3f4ce into youki-dev:main Aug 22, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants