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

Support feature subcommand #2837

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

musaprg
Copy link
Contributor

@musaprg musaprg commented Jun 29, 2024

derived from #2395, and fixes #815

This PR introduces a new youki features subcommand, which returns the Features Structure1 defined in the OCI runtime spec. Features Structure is written in JSON format and contains runtime features supported by the youki.

TODO

Footnotes

  1. https://github.com/opencontainers/runtime-spec/blob/main/features.md

@musaprg musaprg force-pushed the issue-815-support-feature-subcommand branch 2 times, most recently from 64ebe28 to 804e48b Compare July 7, 2024 03:23
@musaprg musaprg force-pushed the issue-815-support-feature-subcommand branch from c4a0450 to 6646793 Compare September 9, 2024 16:12
crates/youki/Cargo.toml Outdated Show resolved Hide resolved
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 20, 2024

Hey @musaprg , just wanted to confirm if you are following up on this, or might be busy with something else. No worries if you can't continue, but let us know. Thanks!

@musaprg
Copy link
Contributor Author

musaprg commented Sep 21, 2024

@YJDoc2 Hi. sorry I have been a bit busy these days, but I'm still working on it. I'll update the dependency and add missing implementations.

@musaprg musaprg force-pushed the issue-815-support-feature-subcommand branch from 4fda17a to f6f7ee8 Compare September 22, 2024 10:17
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.56962% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.04%. Comparing base (ea96160) to head (e28d8ac).
Report is 14 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2837      +/-   ##
==========================================
+ Coverage   66.76%   67.04%   +0.28%     
==========================================
  Files         131      131              
  Lines       16673    16831     +158     
==========================================
+ Hits        11131    11285     +154     
- Misses       5542     5546       +4     

@musaprg musaprg force-pushed the issue-815-support-feature-subcommand branch 2 times, most recently from e28d8ac to 491e79d Compare September 22, 2024 16:19
crates/youki/Cargo.toml Outdated Show resolved Hide resolved
DarrellTang and others added 2 commits September 28, 2024 23:53
Signed-off-by: Darrell Tang <darrelltang@gmail.com>

change struct name to resolve conflict

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix annotation references

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

set as Strings

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

use serde

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

pretty print

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

clean up names to match runc features output

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

rearrange structs and constants

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix lint issues

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

try to source caps dynamically

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

try to source namespaces dynamically

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix query_caps

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix match statements

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix linting issues

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

fix extra line for linting

Signed-off-by: Darrell Tang <darrelltang@gmail.com>

Fix format

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
@musaprg musaprg force-pushed the issue-815-support-feature-subcommand branch from 491e79d to 96a4557 Compare September 28, 2024 15:07
@musaprg musaprg marked this pull request as ready for review September 28, 2024 15:11
@musaprg musaprg requested a review from YJDoc2 September 28, 2024 15:18
@musaprg
Copy link
Contributor Author

musaprg commented Sep 28, 2024

@YJDoc2 @utam0k Hi, this PR is ready for review now. Could you check it at your convenience?
I don't think supporting seccomp support information in this PR would be necessary. I can implement it in another PR once #2924 gets merged. Thanks!

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

Couple of comments.

crates/youki/src/commands/features.rs Outdated Show resolved Hide resolved
crates/youki/src/commands/features.rs Outdated Show resolved Hide resolved
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
@musaprg musaprg requested a review from YJDoc2 September 29, 2024 13:46
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 30, 2024

Hey @musaprg , I ran this on my system and compared with runc's output, and there are some differences -

  • youki's output
`./youki features` { "ociVersionMin": "1.0.0", "ociVersionMax": "1.0.2", "hooks": [ "prestart", "createRuntime", "createContainer", "startContainer", "poststart", "poststop" ], "mountOptions": [ "async", "atime", "bind", "defaults", "dev", "diratime", "dirsync", "exec", "mand", "noatime", "nodev", "nodiratime", "noexec", "nomand", "norelatime", "nosuid", "nostrictatime", "private", "rbind", "rdev", "relatime", "remount", "rnoatime", "rnodev", "rnodiratime", "rnoexec", "rnorelatime", "rnosuid", "rnostrictatime", "ro", "rprivate", "rrw", "rshared", "rsuid", "rsymfollow", "rslave", "rstrictatime", "runbindable", "rw", "shared", "slave", "strictatime", "suid", "sync", "unbindable" ], "linux": { "namespaces": [ "pid", "network", "uts", "ipc", "mount", "user", "cgroup", "time" ], "capabilities": [], "cgroup": { "v1": false, "v2": false, "systemd": false, "systemdUser": false, "rdma": false }, "seccomp": null, "apparmor": { "enabled": true }, "selinux": { "enabled": false }, "intelRdt": { "enabled": true }, "mountExtensions": { "idmap": { "enabled": false } } }, "annotations": null, "potentiallyUnsafeConfigAnnotations": null }
  • runc's output
`runc features` { "ociVersionMin": "1.0.0", "ociVersionMax": "1.0.2-dev", "hooks": [ "prestart", "createRuntime", "createContainer", "startContainer", "poststart", "poststop" ], "mountOptions": [ "acl", "async", "atime", "bind", "defaults", "dev", "diratime", "dirsync", "exec", "iversion", "lazytime", "loud", "mand", "noacl", "noatime", "nodev", "nodiratime", "noexec", "noiversion", "nolazytime", "nomand", "norelatime", "nostrictatime", "nosuid", "nosymfollow", "private", "ratime", "rbind", "rdev", "rdiratime", "relatime", "remount", "rexec", "rnoatime", "rnodev", "rnodiratime", "rnoexec", "rnorelatime", "rnostrictatime", "rnosuid", "rnosymfollow", "ro", "rprivate", "rrelatime", "rro", "rrw", "rshared", "rslave", "rstrictatime", "rsuid", "rsymfollow", "runbindable", "rw", "shared", "silent", "slave", "strictatime", "suid", "symfollow", "sync", "tmpcopyup", "unbindable" ], "linux": { "namespaces": [ "cgroup", "ipc", "mount", "network", "pid", "user", "uts" ], "capabilities": [ "CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_DAC_READ_SEARCH", "CAP_FOWNER", "CAP_FSETID", "CAP_KILL", "CAP_SETGID", "CAP_SETUID", "CAP_SETPCAP", "CAP_LINUX_IMMUTABLE", "CAP_NET_BIND_SERVICE", "CAP_NET_BROADCAST", "CAP_NET_ADMIN", "CAP_NET_RAW", "CAP_IPC_LOCK", "CAP_IPC_OWNER", "CAP_SYS_MODULE", "CAP_SYS_RAWIO", "CAP_SYS_CHROOT", "CAP_SYS_PTRACE", "CAP_SYS_PACCT", "CAP_SYS_ADMIN", "CAP_SYS_BOOT", "CAP_SYS_NICE", "CAP_SYS_RESOURCE", "CAP_SYS_TIME", "CAP_SYS_TTY_CONFIG", "CAP_MKNOD", "CAP_LEASE", "CAP_AUDIT_WRITE", "CAP_AUDIT_CONTROL", "CAP_SETFCAP", "CAP_MAC_OVERRIDE", "CAP_MAC_ADMIN", "CAP_SYSLOG", "CAP_WAKE_ALARM", "CAP_BLOCK_SUSPEND", "CAP_AUDIT_READ", "CAP_PERFMON", "CAP_BPF", "CAP_CHECKPOINT_RESTORE" ], "cgroup": { "v1": true, "v2": true, "systemd": true, "systemdUser": true }, "seccomp": { "enabled": true, "actions": [ "SCMP_ACT_ALLOW", "SCMP_ACT_ERRNO", "SCMP_ACT_KILL", "SCMP_ACT_KILL_PROCESS", "SCMP_ACT_KILL_THREAD", "SCMP_ACT_LOG", "SCMP_ACT_NOTIFY", "SCMP_ACT_TRACE", "SCMP_ACT_TRAP" ], "operators": [ "SCMP_CMP_EQ", "SCMP_CMP_GE", "SCMP_CMP_GT", "SCMP_CMP_LE", "SCMP_CMP_LT", "SCMP_CMP_MASKED_EQ", "SCMP_CMP_NE" ], "archs": [ "SCMP_ARCH_AARCH64", "SCMP_ARCH_ARM", "SCMP_ARCH_MIPS", "SCMP_ARCH_MIPS64", "SCMP_ARCH_MIPS64N32", "SCMP_ARCH_MIPSEL", "SCMP_ARCH_MIPSEL64", "SCMP_ARCH_MIPSEL64N32", "SCMP_ARCH_PPC", "SCMP_ARCH_PPC64", "SCMP_ARCH_PPC64LE", "SCMP_ARCH_RISCV64", "SCMP_ARCH_S390", "SCMP_ARCH_S390X", "SCMP_ARCH_X32", "SCMP_ARCH_X86", "SCMP_ARCH_X86_64" ] }, "apparmor": { "enabled": true }, "selinux": { "enabled": true } }, "annotations": { "io.github.seccomp.libseccomp.version": "2.5.3", "org.opencontainers.runc.checkpoint.enabled": "true", "org.opencontainers.runc.commit": "v1.1.13-0-g58aa920", "org.opencontainers.runc.version": "1.1.13" } }

The major differences I see here are capabilities list, cgroup and systemd info, also seccomp info. Can you take a look?

@musaprg
Copy link
Contributor Author

musaprg commented Oct 3, 2024

@YJDoc2 IIUC, as for the systemd-related fields, it depends on the youki's feature flags indicating which feature should be compiled in. I guess executing cargo build without any feature flags won't include any cgroup features such as v1, v2, systemd, systemd-user. Could you try scripts/build.sh -o $PWD -c youki -f v2 for example?

@musaprg
Copy link
Contributor Author

musaprg commented Oct 3, 2024

As for the seccomp information, #2924 is required to be merged, so I've just left it as null, which means unknown about seccomp support.

@musaprg
Copy link
Contributor Author

musaprg commented Oct 3, 2024

As for capabilities, I probably misunderstood the spec. The capabilities listed there don't have to be actually supported on the kernel running youki. I've fixed it in b8f902a.

The recognized names of the capabilities, including capabilities that might not be supported by the host operating system. The runtime MUST recognize the elements in this array in the process.capabilities object of config.json.
https://github.com/opencontainers/runtime-spec/blob/main/features-linux.md#capabilities

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
@musaprg
Copy link
Contributor Author

musaprg commented Oct 3, 2024

I noticed that namespaces also should include all possible namespaces regardless of whether it's supported on the kernel. I've fixed it in a73d957.

The recognized names of the namespaces, including namespaces that might not be supported by the host operating system. The runtime MUST recognize the elements in this array as the type of linux.namespaces objects in config.json.

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>
}

// Return a list of known mount options supported by youki
fn known_mount_options() -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

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

This may be slightly beyond the scope of this PR, but I'm considering methods to ensure this list is regularly updated. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll try.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, unless either of you have done implementation of the updation, I think we should go ahead with hard-coded parts in this PR and merge once reviewed, and the dynamic values can be done in #2937 or so. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I have been a bit busy recently. Separating PR sounds good to me, but either would be fine. I'll continue working on #2937 in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for feature subcommand
5 participants