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

[Do Not Merge]: Track runc missing options and commands #1613

Closed
wants to merge 9 commits into from

Conversation

c3d
Copy link
Contributor

@c3d c3d commented Mar 3, 2023

DO NOT MERGE - FOR REFERENCE / CHERRY-PICKING ONLY

Add the missing command-line options and subcommands in liboci-cli, as documented for runc, including:

  • Missing options for checkpoint
  • Missing no_pivot option for create
  • Missing options for exec
  • Missing options for run
  • Missing options for update
  • Missing options for spec

Also add the missing undocumented features subcommand from runc

(This does not mean that they are necessarily implemented)

@adrianreber In case you need to sync the options for this command.

Context: The ociplex tool needs the additional options to be able to pass them down to some other runtime.

@c3d
Copy link
Contributor Author

c3d commented Mar 3, 2023

@adrianreber For context, this is part of the work I am doing on ociplex, and attempt to restore podman compatibility for Kata Containers using Youki as an in-between, see https://github.com/c3d/youki/tree/ociplex.

pub lazy_pages: bool,
/// Pass a file descriptor fd to criu
#[clap(long)]
pub status_fd: Option<u32>, // TODO: Is u32 the right type?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, I am not sure how file descriptors should be passed around in that context.

@adrianreber
Copy link
Contributor

Not sure this is a good idea. Podman tries to figure out certain features of crun and runc by parsing the help output. The expectation is, that if it is listed in the help output it is supported.

In the case of youki it is not really important as checkpoint is still hidden behind checkpointt.

@c3d
Copy link
Contributor Author

c3d commented Mar 3, 2023

Not sure this is a good idea. Podman tries to figure out certain features of crun and runc by parsing the help output. The expectation is, that if it is listed in the help output it is supported.

In the case of youki it is not really important as checkpoint is still hidden behind checkpointt.

I was not aware of this behaviour of podman. That's weird 😄 . Well, I need these changes on my branch, feel free to pick them up if/when you implement that. I'll also figure out a way to have this be different for ociplex and youki.

@adrianreber
Copy link
Contributor

Not sure this is a good idea. Podman tries to figure out certain features of crun and runc by parsing the help output. The expectation is, that if it is listed in the help output it is supported.
In the case of youki it is not really important as checkpoint is still hidden behind checkpointt.

I was not aware of this behaviour of podman. That's weird smile . Well, I need these changes on my branch, feel free to pick them up if/when you implement that. I'll also figure out a way to have this be different for ociplex and youki.

It is only during restore: https://github.com/containers/podman/blob/main/pkg/checkpoint/crutils/checkpoint_restore_utils.go#L229

@c3d
Copy link
Contributor Author

c3d commented Mar 6, 2023

@adrianreber OK, I understand your constraints. In any case, the changes in this PR are not for immediate consumption. It's more "If you need to implement this or that option, feel free to grab it from this branch". I will try to maintain two parallel branches:

  • The oci-changes branch, where I'll try to keep only the OCI command-line interface changes

  • The ociplex branch where we are developing an OCI multiplexer that leverages youki's command-line parsing, the primary goal being to build a bridge between podman and shimv2-only runtimes like Kata Containers v2 and later.

I'll try to reorder and cleanup the commits in these branches to rebase ociplex on oci-changes. In case there is no negative impact of adding the missing options. It's really your call if and when you want to do that.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Merging #1613 (a191eb7) into main (74a499f) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
- Coverage   68.83%   68.74%   -0.10%     
==========================================
  Files         120      122       +2     
  Lines       13109    13197      +88     
==========================================
+ Hits         9024     9072      +48     
- Misses       4085     4125      +40     

@utam0k
Copy link
Member

utam0k commented Mar 7, 2023

Not sure this is a good idea. Podman tries to figure out certain features of crun and runc by parsing the help output. The expectation is, that if it is listed in the help output it is supported.

In the case of youki it is not really important as checkpoint is still hidden behind checkpointt.

BTW, podman should use the feature command to verify the futures of the oci runtimes
opencontainers/runtime-spec#1130

@c3d
Copy link
Contributor Author

c3d commented Mar 7, 2023 via email

Add the missing command-line options as documented for runc, and also reorder
the options to match the documentation:
https://github.com/opencontainers/runc/blob/main/man/runc-checkpoint.8.md

(This does not mean that they are necessarily implemented)

Signed-off-by: Christophe de Dinechin <[email protected]>
The --no-pivot option is documented in
https://github.com/opencontainers/runc/blob/main/man/runc-create.8.md

Also change the options order in order to match the doc, this makes the code a
bit easier to maintain.

Signed-off-by: Christophe de Dinechin <[email protected]>
@c3d c3d force-pushed the oci-changes branch 3 times, most recently from d51aeaf to 173b011 Compare March 7, 2023 18:44
@@ -0,0 +1,7 @@
use clap::Parser;
Copy link
Member

Choose a reason for hiding this comment

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

May I ask you to update the README for liboci-cli?
https://github.com/containers/youki/tree/main/crates/liboci-cli

Copy link
Member

@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 apologize for the inconvenience, but for commands that are not yet implemented, you should make it clear in the prefix in the comment section of each option.
Then they should appear in the help command.

@c3d
Copy link
Contributor Author

c3d commented Mar 8, 2023

@utam0k Not an inconvenience at all. Thanks for pointing that out, I'll work on it.

@c3d c3d changed the title checkpoint: Add the missing options DNM: Track runc missing options and commands Mar 8, 2023
c3d added 5 commits March 8, 2023 10:15
Add the missing command-line options for the exec subcommand.
Reference: https://github.com/opencontainers/runc/blob/main/man/runc-exec.8.md

Signed-off-by: Christophe de Dinechin <[email protected]>
Also change the order to match the documentation in
https://github.com/opencontainers/runc/blob/main/man/runc-run.8.md

Signed-off-by: Christophe de Dinechin <[email protected]>
Add the missing bundle option, as documented in
https://github.com/opencontainers/runc/blob/main/man/runc-spec.8.md

Signed-off-by: Christophe de Dinechin <[email protected]>
The 'features' subcommand is not publicly documented yet, but it was introduced
in `runc` in opencontainers/runc#3296.

Signed-off-by: Christophe de Dinechin <[email protected]>
c3d added 2 commits March 8, 2023 10:29
The `features` subcommand is  implemented in `runc`, but not documented.
See opencontainers/runc#3296

Signed-off-by: Christophe de Dinechin <[email protected]>
Suggested-by: Toru Komatsu <[email protected]>
@utam0k utam0k marked this pull request as draft March 10, 2023 06:47
@yihuaf yihuaf changed the title DNM: Track runc missing options and commands [Do Not Merge]: Track runc missing options and commands Jun 10, 2023
@yihuaf
Copy link
Collaborator

yihuaf commented May 20, 2024

Given this PR is stale for more than a year, I will close this. Please re-open when needed.

@yihuaf yihuaf closed this May 20, 2024
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

Successfully merging this pull request may close these issues.

5 participants