kata-ctl: Move development to main branch #5388

jodh-intel · 2022-10-10T10:26:38Z

As shown on #4499 (comment), we've been developing a replacement for the kata-runtime utility command, written in rust.

We'd now like the community to get actively involvement in this project, so this PR is a roll-up of the PRs developed on a separate branch.

Caveat emptor: kata-ctl is still a work in progress. This PR is simply moving the code into the main branch, so that we can accelerate development. kata-ctl is not yet a replacement for the existing golang kata-runtime utility command.

For further details, see:

The original issue
The kata-ctl GitHub tracking project

If you'd like to get involved, tal at the issues in the project and assign yourself or add a comment! 😄

Fixes: #4499, #5334, #5351.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

Use agent-ctl tool rust code as an example for a skeleton for the new kata-ctl tool. Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

jodh-intel · 2022-10-10T10:27:00Z

/cc @amshinde, @dborquez, @cmaf.

As we're switching to using the rust version of the kata-ctl, lets provide with its own entry in the kata-ctl command line. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

Add more unit tests cases to --version argument. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

Add framework for different architectures for check. In the existing kata-runtime check, the network checks do not appear to be architecture-specific while the kernel module, cpu, and kvm checks do have separate implementations for different architectures. Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

Remove return value for branches that call `unimplemented!()`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Automatic format updates. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Changed the `panic!()` call to a `compile_error!()` one to ensure it fires at compile time rather than runtime. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Add architecture-specific code for x86_64 and generic calls handling checks for CPU flags and attributes. Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

Switch from the functional version of `clap` to the declarative methodology. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

This kata-ctl argument returns the latest stable Kata release by hitting github.com. Adds check-version unit tests. Fixes: #11 Signed-off-by: David Esparza <david.esparza.borquez@intel.com>

Resolved a couple of clippy warnings and applied standard `rustfmt`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Build using the rust TLS implementation rather than the system ones. This resolves the `reqwest` crate build failure: it doesn't appear to build against the native libssl libraries due to Kata defaulting to using the musl libc. Fixes: kata-containers#5387. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Add a link to the `kata-ctl` tool's README. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

cmaf

@jodh-intel I'm approving the README and docs since I previously reviewed everything else, but I'll leave the official approval to others

fidencio

The code doesn't do much nowadays and the main intent of the PR is actually to have this into the main branch to have the development done there.

I've left a few questions about the version of the package / tool as it's rather confusing for me.

fidencio · 2022-10-10T16:55:20Z

src/tools/kata-ctl/Cargo.toml

+
+[package]
+name = "kata-ctl"
+version = "0.0.1"


IMHO, it's rather confusing having a tool in a different version than the rest of the project, but that's a nit.

This is just standard rust manifest versioning. If you run the tool, it shows something like this:

$ kata-ctl 3.1.0-alpha0-383b7db704fe3919841fef047bbc70a3dd605516-0.0.1

Hence, the tool contains both the Kata version and it's own tooling version (and that full version string is valid semver). We can obviously change this, but as mentioned on the description of this PR, we just want to get this landed at this stage to allow more contributions 😄

fidencio · 2022-10-10T16:58:07Z

src/tools/kata-ctl/Makefile

+
+TARGET = $(PROJECT_COMPONENT)
+
+VERSION_FILE := ./VERSION


Hmm. Doesn't this make the package version different from the tool binary version?

xyjixyjixyji · 2022-10-10T18:33:35Z

Generally nice work and LGTM! This would be nice for interaction between runtime and kata-ctl.
However, the version problem @fidencio mentioned indeed needs some discussion.

liubin · 2022-10-11T08:46:01Z

src/tools/kata-ctl/README.md

+## Install the tool
+
+```bash
+$ make install


Don't need sudo? Normally $ means a non-root user.

It currently installs to the cargo bin dir, so this is correct. However, I agree we should start thinking about a proper install so I've raised #5403.

liubin · 2022-10-11T08:55:24Z

@jodh-intel Shouldn't add kata-ctl to the top-level Makefile?

kata-containers/Makefile

Lines 18 to 21 in ffdd7e1

    
           TOOLS += agent-ctl 
        
           TOOLS += trace-forwarder 
        
           TOOLS += runk 
        
           TOOLS += log-parser

Provide a basic document explaining a little about the `kata-ctl` command. Fixes: kata-containers#5351. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Update the top-level Makefile to build the `kata-ctl` tool by default. Fixes: kata-containers#4499, kata-containers#5334. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2022-10-11T09:06:11Z

Good catch @liubin! Fixed 😄

fidencio

lgtm, thanks!

jodh-intel · 2022-10-11T13:42:05Z

/test

wainersm · 2022-10-11T15:28:14Z

src/tools/kata-ctl/Cargo.toml

+name = "kata-ctl"
+version = "0.0.0"
+authors = ["The Kata Containers community <kata-dev@lists.katacontainers.io>"]
+edition = "2021"


Hi @jodh-intel ! Amazing initiative btw!

Regarding the rust edition, it seems all the other rust components are still on 2018 edition. Could the kata-ctl be built also on that edition?

wainersm · 2022-10-11T15:33:03Z

src/tools/kata-ctl/src/main.rs

+        .long_about(DESCRIPTION_TEXT)
+        .subcommand(
+            SubCommand::with_name("check")
+            .about("tests if system can run Kata Containers")


Nit: s/tests/test to make it consistent with the other descriptions

Yep - we'll finesse this sort of stuff once it's landed :)

wainersm · 2022-10-11T15:33:39Z

src/tools/kata-ctl/src/main.rs

+        )
+        .subcommand(
+            SubCommand::with_name("help")
+            .about("shows a list of commands or help for one command")


likebreath

Generally looking good to me. Couple of comments below.

likebreath · 2022-10-11T23:16:53Z

src/tools/kata-ctl/src/arch/mod.rs

+    #[cfg(target_arch = "aarch64")]
+    let result = aarch64::check();
+
+    #[cfg(target_arch = "powerpc64le")]
+    let result = powerpc64le::check();
+
+    #[cfg(target_arch = "s390x")]
+    let result = s390x::check();
+
+    #[cfg(target_arch = "x86_64")]
+    let result = x86_64::check(global_args);
+
+    #[cfg(not(any(
+        target_arch = "aarch64",
+        target_arch = "powerpc64le",
+        target_arch = "s390x",
+        target_arch = "x86_64"
+    )))]
+    panic!("unknown architecture");


For common functions that all architecture needs to provide and share the same signature, a nicer way is exposing these common functions directly from the arch-specific module with the right cfg guard, for example:

#[cfg(target_arch = "x86_64")] pub use x86_64::check; #[cfg(target_arch = "aarch64")] pub use aarch64::check; ...

In this way, the actual interface function of the crate (e.g. arch::check()) will be better abstracted and simpler. This will also report error at compile time for unsupported architectures (instead of a panic at runtime).

Hi @likebreath - I'm confused. Firstly, that panic line was replaced on commit 7c9f9a5 so I'm not sure why/how you were seeing that. Secondly, we're already using cfg guards. The code I think you might be referring to (now) is:

#[cfg(not(any( target_arch = "aarch64", target_arch = "powerpc64le", target_arch = "s390x", target_arch = "x86_64" )))] compile_error!("unknown architecture");

... and I think that does exactly what we want: it will fail to compile on unknown platforms.

Ah - I see what you're saying about the cfg guard now. Must be time for ☕ 😄

likebreath · 2022-10-11T23:42:18Z

src/tools/kata-ctl/src/arch/x86_64/mod.rs

+        println!("INFO: check CPU: x86_64");
+
+        let cpu_info = check::get_single_cpu_info(PROC_CPUINFO, CPUINFO_DELIMITER)?;
+
+        let cpu_flags = check::get_cpu_flags(&cpu_info, CPUINFO_FLAGS_TAG)
+            .map_err(|e| anyhow!("Error parsing CPU flags, file {:?}, {:?}", PROC_CPUINFO, e))?;
+
+        // perform checks
+        // TODO: Perform checks based on hypervisor type
+        // TODO: Add more information to output (see kata-check in go tool); adjust formatting
+        let missing_cpu_attributes = check::check_cpu_attribs(&cpu_info, CPU_ATTRIBS_INTEL)?;
+        if missing_cpu_attributes.len() > 0 {
+            eprintln!("WARNING: Missing CPU attributes {:?}", missing_cpu_attributes);
+        }
+        let missing_cpu_flags = check::check_cpu_flags(&cpu_flags, CPU_FLAGS_INTEL)?;
+        if missing_cpu_flags.len() > 0 {
+            eprintln!("WARNING: Missing CPU flags {:?}", missing_cpu_flags);
+        }


The log crate is a good option for handling logging with different verbose levels, say 'warn', 'error', 'debug', etc. It can also be exposed by a CLI parameter for users to decide the logging level.

@likebreath - thanks. I've raised an issue to use the Kata logging crate (which uses slog):

kata-ctl: Switch to slog for output and logging #5401

jodh-intel · 2022-10-12T09:17:26Z

/test

Make this file conform to the standard rust layout conventions and simplify the code as recommended by `clippy`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Revert to the 2018 edition of rust for consistency with other rust components. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2022-10-12T11:11:33Z

/test

kata-ctl: Add initial rust code for kata-ctl

002b180

Use agent-ctl tool rust code as an example for a skeleton for the new kata-ctl tool. Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

jodh-intel requested a review from a team as a code owner October 10, 2022 10:26

katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Oct 10, 2022

jodh-intel force-pushed the kata-ctl branch 2 times, most recently from 7bb502f to 596df6a Compare October 10, 2022 11:27

dborquez and others added 11 commits October 10, 2022 13:42

versions: Add kata-ctl version entry

d0b33e9

As we're switching to using the rust version of the kata-ctl, lets provide with its own entry in the kata-ctl command line. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: Add unit tests cases

303fc8b

Add more unit tests cases to --version argument. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: Lint fixes to allow the branch to be built

cca7e32

Remove return value for branches that call `unimplemented!()`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: Formatting tweaks

b63ba66

Automatic format updates. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: Make arch test run at compile time

7c9f9a5

Changed the `panic!()` call to a `compile_error!()` one to ensure it fires at compile time rather than runtime. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: Add cpu check

018aa89

Add architecture-specific code for x86_64 and generic calls handling checks for CPU flags and attributes. Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>

kata-ctl: switch to use clap derive for CLI handling

eb5423c

Switch from the functional version of `clap` to the declarative methodology. Signed-off-by: David Esparza <david.esparza.borquez@intel.com> Commit-edited-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: implement CLI argument --check-version-only

1336904

This kata-ctl argument returns the latest stable Kata release by hitting github.com. Adds check-version unit tests. Fixes: #11 Signed-off-by: David Esparza <david.esparza.borquez@intel.com>

kata-ctl: clippy: Resolve warnings and reformat

c235849

Resolved a couple of clippy warnings and applied standard `rustfmt`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the kata-ctl branch from 596df6a to 3279aae Compare October 10, 2022 12:43

docs: Reference kata-ctl README

781e604

Add a link to the `kata-ctl` tool's README. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the kata-ctl branch from 3279aae to 383b7db Compare October 10, 2022 15:50

cmaf reviewed Oct 10, 2022

View reviewed changes

fidencio reviewed Oct 10, 2022

View reviewed changes

liubin reviewed Oct 11, 2022

View reviewed changes

liubin approved these changes Oct 11, 2022

View reviewed changes

jodh-intel added 2 commits October 11, 2022 10:04

kata-ctl: docs: Write basic documentation

f7010b8

Provide a basic document explaining a little about the `kata-ctl` command. Fixes: kata-containers#5351. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

build: Add kata-ctl to tools list

2b345ba

Update the top-level Makefile to build the `kata-ctl` tool by default. Fixes: kata-containers#4499, kata-containers#5334. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the kata-ctl branch from 383b7db to 2b345ba Compare October 11, 2022 09:05

fidencio approved these changes Oct 11, 2022

View reviewed changes

wainersm reviewed Oct 11, 2022

View reviewed changes

likebreath reviewed Oct 11, 2022

View reviewed changes

jodh-intel mentioned this pull request Oct 12, 2022

kata-ctl: Simplify check function #5402

Closed

jodh-intel added 2 commits October 12, 2022 11:46

kata-ctl: rustfmt + clippy fixes

fb63274

Make this file conform to the standard rust layout conventions and simplify the code as recommended by `clippy`. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

kata-ctl: cargo: 2021 -> 2018

00a42f6

Revert to the 2018 edition of rust for consistency with other rust components. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the kata-ctl branch from e4c4018 to 00a42f6 Compare October 12, 2022 10:52

jodh-intel merged commit d3ee8d9 into kata-containers:main Oct 12, 2022

jodh-intel mentioned this pull request Oct 12, 2022

Add kata-ctl tool to main repo #5334

Closed

tzY15368 mentioned this pull request Oct 13, 2022

kata-ctl: improve command descriptions #5411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kata-ctl: Move development to main branch #5388

kata-ctl: Move development to main branch #5388

jodh-intel commented Oct 10, 2022

jodh-intel commented Oct 10, 2022

cmaf left a comment

fidencio left a comment

fidencio Oct 10, 2022

jodh-intel Oct 11, 2022

fidencio Oct 10, 2022

jodh-intel Oct 11, 2022

xyjixyjixyji commented Oct 10, 2022

liubin Oct 11, 2022

jodh-intel Oct 12, 2022

liubin commented Oct 11, 2022

jodh-intel commented Oct 11, 2022

fidencio left a comment

jodh-intel commented Oct 11, 2022

wainersm Oct 11, 2022

wainersm Oct 11, 2022

jodh-intel Oct 12, 2022

wainersm Oct 11, 2022

likebreath left a comment

likebreath Oct 11, 2022

jodh-intel Oct 12, 2022

jodh-intel Oct 12, 2022

jodh-intel Oct 12, 2022

likebreath Oct 11, 2022

jodh-intel Oct 12, 2022

jodh-intel commented Oct 12, 2022

jodh-intel commented Oct 12, 2022


		TARGET = $(PROJECT_COMPONENT)

		VERSION_FILE := ./VERSION

kata-ctl: Move development to main branch #5388

kata-ctl: Move development to main branch #5388

Conversation

jodh-intel commented Oct 10, 2022

jodh-intel commented Oct 10, 2022

cmaf left a comment

Choose a reason for hiding this comment

fidencio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xyjixyjixyji commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liubin commented Oct 11, 2022

jodh-intel commented Oct 11, 2022

fidencio left a comment

Choose a reason for hiding this comment

jodh-intel commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likebreath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Oct 12, 2022

jodh-intel commented Oct 12, 2022