diff --git a/.github/workflows/kata-deploy-test.yaml b/.github/workflows/kata-deploy-test.yaml index febd4fbee..548b30959 100644 --- a/.github/workflows/kata-deploy-test.yaml +++ b/.github/workflows/kata-deploy-test.yaml @@ -5,46 +5,96 @@ on: name: test-kata-deploy jobs: - create-and-test-container: + build-asset: if: | github.event.issue.pull_request && github.event_name == 'issue_comment' && github.event.action == 'created' && startsWith(github.event.comment.body, '/test_kata_deploy') runs-on: ubuntu-latest + strategy: + matrix: + asset: + - cloud-hypervisor + - firecracker + - kernel + - qemu + - rootfs-image + - rootfs-initrd + - shim-v2 steps: - - name: get-PR-ref - id: get-PR-ref + - uses: actions/checkout@v2 + - name: Install docker run: | - ref=$(cat $GITHUB_EVENT_PATH | jq -r '.issue.pull_request.url' | sed 's#^.*\/pulls#refs\/pull#' | sed 's#$#\/merge#') - echo "reference for PR: " ${ref} - echo "##[set-output name=pr-ref;]${ref}" + curl -fsSL https://test.docker.com -o test-docker.sh + sh test-docker.sh - - name: check out - uses: actions/checkout@v2 - with: - ref: ${{ steps.get-PR-ref.outputs.pr-ref }} - - - name: build-container-image - id: build-container-image + - name: Build ${{ matrix.asset }} run: | - PR_SHA=$(git log --format=format:%H -n1) - VERSION="2.0.0" - ARTIFACT_URL="https://github.com/kata-containers/kata-containers/releases/download/${VERSION}/kata-static-${VERSION}-x86_64.tar.xz" - wget "${ARTIFACT_URL}" -O tools/packaging/kata-deploy/kata-static.tar.xz - docker build --build-arg KATA_ARTIFACTS=kata-static.tar.xz -t katadocker/kata-deploy-ci:${PR_SHA} -t quay.io/kata-containers/kata-deploy-ci:${PR_SHA} ./tools/packaging/kata-deploy - docker login -u ${{ secrets.DOCKER_USERNAME }} -p ${{ secrets.DOCKER_PASSWORD }} - docker push katadocker/kata-deploy-ci:$PR_SHA - docker login -u ${{ secrets.QUAY_DEPLOYER_USERNAME }} -p ${{ secrets.QUAY_DEPLOYER_PASSWORD }} quay.io - docker push quay.io/kata-containers/kata-deploy-ci:$PR_SHA - echo "##[set-output name=pr-sha;]${PR_SHA}" - - - name: test-kata-deploy-ci-in-aks - uses: ./tools/packaging/kata-deploy/action - with: - packaging-sha: ${{ steps.build-container-image.outputs.pr-sha }} + make "${KATA_ASSET}-tarball" + build_dir=$(readlink -f build) + # store-artifact does not work with symlink + sudo cp -r "${build_dir}" "kata-build" env: - PKG_SHA: ${{ steps.build-container-image.outputs.pr-sha }} + KATA_ASSET: ${{ matrix.asset }} + TAR_OUTPUT: ${{ matrix.asset }}.tar.gz + + - name: store-artifact ${{ matrix.asset }} + uses: actions/upload-artifact@v2 + with: + name: kata-artifacts + path: kata-build/kata-static-${{ matrix.asset }}.tar.xz + if-no-files-found: error + + create-kata-tarball: + runs-on: ubuntu-latest + needs: build-asset + steps: + - uses: actions/checkout@v2 + - name: get-artifacts + uses: actions/download-artifact@v2 + with: + name: kata-artifacts + path: kata-artifacts + - name: merge-artifacts + run: | + ./tools/packaging/kata-deploy/local-build/kata-deploy-merge-builds.sh kata-artifacts + - name: store-artifacts + uses: actions/upload-artifact@v2 + with: + name: kata-static-tarball + path: kata-static.tar.xz + + kata-deploy: + needs: create-kata-tarball + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: get-kata-tarball + uses: actions/download-artifact@v2 + with: + name: kata-static-tarball + - name: build-and-push-kata-deploy-ci + id: build-and-push-kata-deploy-ci + run: | + tag=$(echo $GITHUB_REF | cut -d/ -f3-) + pushd $GITHUB_WORKSPACE + git checkout $tag + pkg_sha=$(git rev-parse HEAD) + popd + mv kata-static.tar.xz $GITHUB_WORKSPACE/tools/packaging/kata-deploy/kata-static.tar.xz + docker build --build-arg KATA_ARTIFACTS=kata-static.tar.xz -t quay.io/kata-containers/kata-deploy-ci:$pkg_sha $GITHUB_WORKSPACE/tools/packaging/kata-deploy + docker login -u ${{ secrets.QUAY_DEPLOYER_USERNAME }} -p ${{ secrets.QUAY_DEPLOYER_PASSWORD }} quay.io + docker push quay.io/kata-containers/kata-deploy-ci:$pkg_sha + mkdir -p packaging/kata-deploy + ln -s $GITHUB_WORKSPACE/tools/packaging/kata-deploy/action packaging/kata-deploy/action + echo "::set-output name=PKG_SHA::${pkg_sha}" + - name: test-kata-deploy-ci-in-aks + uses: ./packaging/kata-deploy/action + with: + packaging-sha: ${{steps.build-and-push-kata-deploy-ci.outputs.PKG_SHA}} + env: + PKG_SHA: ${{steps.build-and-push-kata-deploy-ci.outputs.PKG_SHA}} AZ_APPID: ${{ secrets.AZ_APPID }} AZ_PASSWORD: ${{ secrets.AZ_PASSWORD }} AZ_SUBSCRIPTION_ID: ${{ secrets.AZ_SUBSCRIPTION_ID }} diff --git a/docs/README.md b/docs/README.md index 5dedec378..f5fd38eef 100644 --- a/docs/README.md +++ b/docs/README.md @@ -52,6 +52,18 @@ Documents that help to understand and contribute to Kata Containers. * [How to contribute to Kata Containers](https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md) * [Code of Conduct](../CODE_OF_CONDUCT.md) +## Help Writing a Code PR + +* [Code PR advice](code-pr-advice.md). + +## Help Writing Unit Tests + +* [Unit Test Advice](Unit-Test-Advice.md) + +## Help Improving the Documents + +* [Documentation Requirements](Documentation-Requirements.md) + ### Code Licensing * [Licensing](Licensing-strategy.md): About the licensing strategy of Kata Containers. @@ -61,10 +73,6 @@ Documents that help to understand and contribute to Kata Containers. * [Release strategy](Stable-Branch-Strategy.md) * [Release Process](Release-Process.md) -## Help Improving the Documents - -* [Documentation Requirements](Documentation-Requirements.md) - ## Website Changes If you have a suggestion for how we can improve the diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md new file mode 100644 index 000000000..5fe8b9caf --- /dev/null +++ b/docs/Unit-Test-Advice.md @@ -0,0 +1,379 @@ +# Unit Test Advice + +## Overview + +This document offers advice on writing a Unit Test (UT) in +[Golang](https://golang.org) and [Rust](https://www.rust-lang.org). + +## General advice + +### Unit test strategies + +#### Positive and negative tests + +Always add positive tests (where success is expected) *and* negative +tests (where failure is expected). + +#### Boundary condition tests + +Try to add unit tests that exercise boundary conditions such as: + +- Missing values (`null` or `None`). +- Empty strings and huge strings. +- Empty (or uninitialised) complex data structures + (such as lists, vectors and hash tables). +- Common numeric values (such as `-1`, `0`, `1` and the minimum and + maximum values). + +#### Test unusual values + +Also always consider "unusual" input values such as: + +- String values containing spaces, Unicode characters, special + characters, escaped characters or null bytes. + + > **Note:** Consider these unusual values in prefix, infix and + > suffix position. + +- String values that cannot be converted into numeric values or which + contain invalid structured data (such as invalid JSON). + +#### Other types of tests + +If the code requires other forms of testing (such as stress testing, +fuzz testing and integration testing), raise a GitHub issue and +reference it on the issue you are using for the main work. This +ensures the test team are aware that a new test is required. + +### Test environment + +#### Create unique files and directories + +Ensure your tests do not write to a fixed file or directory. This can +cause problems when running multiple tests simultaneously and also +when running tests after a previous test run failure. + +#### Assume parallel testing + +Always assume your tests will be run *in parallel*. If this is +problematic for a test, force it to run in isolation using the +`serial_test` crate for Rust code for example. + +### Running + +Ensure you run the unit tests and they all pass before raising a PR. +Ideally do this on different distributions on different architectures +to maximise coverage (and so minimise surprises when your code runs in +the CI). + +## Assertions + +### Golang assertions + +Use the `testify` assertions package to create a new assertion object as this +keeps the test code free from distracting `if` tests: + +```go +func TestSomething(t *testing.T) { + assert := assert.New(t) + + err := doSomething() + assert.NoError(err) +} +``` + +### Rust assertions + +Use the standard set of `assert!()` macros. + +## Table driven tests + +Try to write tests using a table-based approach. This allows you to distill +the logic into a compact table (rather than spreading the tests across +multiple test functions). It also makes it easy to cover all the +interesting boundary conditions: + +### Golang table driven tests + +Assume the following function: + +```go +// The function under test. +// +// Accepts a string and an integer and returns the +// result of sticking them together separated by a dash as a string. +func joinParamsWithDash(str string, num int) (string, error) { + if str == "" { + return "", errors.New("string cannot be blank") + } + + if num <= 0 { + return "", errors.New("number must be positive") + } + + return fmt.Sprintf("%s-%d", str, num), nil +} +``` + +A table driven approach to testing it: + +```go +import ( + "testing" + "github.com/stretchr/testify/assert" +) + +func TestJoinParamsWithDash(t *testing.T) { + assert := assert.New(t) + + // Type used to hold function parameters and expected results. + type testData struct { + param1 string + param2 int + expectedResult string + expectError bool + } + + // List of tests to run including the expected results + data := []testData{ + // Failure scenarios + {"", -1, "", true}, + {"", 0, "", true}, + {"", 1, "", true}, + {"foo", 0, "", true}, + {"foo", -1, "", true}, + + // Success scenarios + {"foo", 1, "foo-1", false}, + {"bar", 42, "bar-42", false}, + } + + // Run the tests + for i, d := range data { + // Create a test-specific string that is added to each assert + // call. It will be displayed if any assert test fails. + msg := fmt.Sprintf("test[%d]: %+v", i, d) + + // Call the function under test + result, err := joinParamsWithDash(d.param1, d.param2) + + // update the message for more information on failure + msg = fmt.Sprintf("%s, result: %q, err: %v", msg, result, err) + + if d.expectError { + assert.Error(err, msg) + + // If an error is expected, there is no point + // performing additional checks. + continue + } + + assert.NoError(err, msg) + assert.Equal(d.expectedResult, result, msg) + } +} +``` + +### Rust table driven tests + +Assume the following function: + +```rust +// Convenience type to allow Result return types to only specify the type +// for the true case; failures are specified as static strings. +// XXX: This is an example. In real code use the "anyhow" and +// XXX: "thiserror" crates. +pub type Result = std::result::Result; + +// The function under test. +// +// Accepts a string and an integer and returns the +// result of sticking them together separated by a dash as a string. +fn join_params_with_dash(str: &str, num: i32) -> Result { + if str.is_empty() { + return Err("string cannot be blank"); + } + + if num <= 0 { + return Err("number must be positive"); + } + + let result = format!("{}-{}", str, num); + + Ok(result) +} + +``` + +A table driven approach to testing it: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_join_params_with_dash() { + // This is a type used to record all details of the inputs + // and outputs of the function under test. + #[derive(Debug)] + struct TestData<'a> { + str: &'a str, + num: i32, + result: Result, + } + + // The tests can now be specified as a set of inputs and outputs + let tests = &[ + // Failure scenarios + TestData { + str: "", + num: 0, + result: Err("string cannot be blank"), + }, + TestData { + str: "foo", + num: -1, + result: Err("number must be positive"), + }, + + // Success scenarios + TestData { + str: "foo", + num: 42, + result: Ok("foo-42".to_string()), + }, + TestData { + str: "-", + num: 1, + result: Ok("--1".to_string()), + }, + ]; + + // Run the tests + for (i, d) in tests.iter().enumerate() { + // Create a string containing details of the test + let msg = format!("test[{}]: {:?}", i, d); + + // Call the function under test + let result = join_params_with_dash(d.str, d.num); + + // Update the test details string with the results of the call + let msg = format!("{}, result: {:?}", msg, result); + + // Perform the checks + if d.result.is_ok() { + assert!(result == d.result, msg); + continue; + } + + let expected_error = format!("{}", d.result.as_ref().unwrap_err()); + let actual_error = format!("{}", result.unwrap_err()); + assert!(actual_error == expected_error, msg); + } + } +} +``` + +## Temporary files + +Always delete temporary files on success. + +### Golang temporary files + +```go +func TestSomething(t *testing.T) { + assert := assert.New(t) + + // Create a temporary directory + tmpdir, err := ioutil.TempDir("", "") + assert.NoError(err) + + // Delete it at the end of the test + defer os.RemoveAll(tmpdir) + + // Add test logic that will use the tmpdir here... +} +``` + +### Rust temporary files + +Use the `tempfile` crate which allows files and directories to be deleted +automatically: + +```rust +#[cfg(test)] +mod tests { + use tempfile::tempdir; + + #[test] + fn test_something() { + + // Create a temporary directory (which will be deleted automatically + let dir = tempdir().expect("failed to create tmpdir"); + + let filename = dir.path().join("file.txt"); + + // create filename ... + } +} + +``` + +## Test user + +[Unit tests are run *twice*](https://github.com/kata-containers/tests/blob/main/.ci/go-test.sh): + +- as the current user +- as the `root` user (if different to the current user) + +When writing a test consider which user should run it; even if the code the +test is exercising runs as `root`, it may be necessary to *only* run the test +as a non-`root` for the test to be meaningful. Add appropriate skip +guards around code that requires `root` and non-`root` so that the test +will run if the correct type of user is detected and skipped if not. + +### Run Golang tests as a different user + +The main repository has the most comprehensive set of skip abilities. See: + +- https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/katatestutils + +### Run Rust tests as a different user + +One method is to use the `nix` crate along with some custom macros: + +``` +#[cfg(test)] +mod tests { + #[allow(unused_macros)] + macro_rules! skip_if_root { + () => { + if nix::unistd::Uid::effective().is_root() { + println!("INFO: skipping {} which needs non-root", module_path!()); + return; + } + }; + } + + #[allow(unused_macros)] + macro_rules! skip_if_not_root { + () => { + if !nix::unistd::Uid::effective().is_root() { + println!("INFO: skipping {} which needs root", module_path!()); + return; + } + }; + } + + #[test] + fn test_that_must_be_run_as_root() { + // Not running as the superuser, so skip. + skip_if_not_root!(); + + // Run test *iff* the user running the test is root + + // ... + } +} +``` diff --git a/docs/code-pr-advice.md b/docs/code-pr-advice.md new file mode 100644 index 000000000..78413ccf2 --- /dev/null +++ b/docs/code-pr-advice.md @@ -0,0 +1,246 @@ +# Code PR Advice + +Before raising a PR containing code changes, we suggest you consider +the following to ensure a smooth and fast process. + +> **Note:** +> +> - All the advice in this document is optional. However, if the +> advice provided is not followed, there is no guarantee your PR +> will be merged. +> +> - All the check tools will be run automatically on your PR by the CI. +> However, if you run them locally first, there is a much better +> chance of a successful initial CI run. + +## Assumptions + +This document assumes you have already read (and in the case of the +code of conduct agreed to): + +- The [Kata Containers code of conduct](https://github.com/kata-containers/community/blob/main/CODE_OF_CONDUCT.md). +- The [Kata Containers contributing guide](https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md). + +## Code + +### Architectures + +Do not write architecture-specific code if it is possible to write the +code generically. + +### General advice + +- Do not write code to impress: instead write code that is easy to read and understand. + +- Always consider which user will run the code. Try to minimise + the privileges the code requires. + +### Comments + +Always add comments if the intent of the code is not obvious. However, +try to avoid comments if the code could be made clearer (for example +by using more meaningful variable names). + +### Constants + +Don't embed magic numbers and strings in functions, particularly if +they are used repeatedly. + +Create constants at the top of the file instead. + +### Copyright and license + +Ensure all new files contain a copyright statement and an SPDX license +identifier in the comments at the top of the file. + +### FIXME and TODO + +If the code contains areas that are not fully implemented, make this +clear a comment which provides a link to a GitHub issue that provides +further information. + +Do not just rely on comments in this case though: if possible, return +a "`BUG: feature X not implemented see {bug-url}`" type error. + +### Functions + +- Keep functions relatively short (less than 100 lines is a good "rule of thumb"). + +- Document functions if the parameters, return value or general intent + of the function is not obvious. + +- Always return errors where possible. + + Do not discard error return values from the functions this function + calls. + +### Logging + +- Don't use multiple log calls when a single log call could be used. + +- Use structured logging where possible to allow + [standard tooling](https://github.com/kata-containers/tests/tree/main/cmd/log-parser) + be able to extract the log fields. + +### Names + +Give functions, macros and variables clear and meaningful names. + +### Structures + +#### Golang structures + +Unlike Rust, Go does not enforce that all structure members be set. +This has lead to numerous bugs in the past where code like the +following is used: + +```go +type Foo struct { + Key string + Value string +} + +// BUG: Key not set, but nobody noticed! ;( +let foo1 = Foo { + Value: "foo", +} +``` + +A much safer approach is to create a constructor function to enforce +integrity: + +```go +type Foo struct { + Key string + Value string +} + +func NewFoo(key, value string) (*Foo, error) { + if key == "" { + return nil, errors.New("Foo needs a key") + } + + if value == "" { + return nil, errors.New("Foo needs a value") + } + + return &Foo{ + Key: key, + Value: value, + }, nil +} + +func testFoo() error { + // BUG: Key not set, but nobody noticed! ;( + badFoo := Foo{Value: "value"} + + // Ok - the constructor performs needed validation + goodFoo, err := NewFoo("name", "value") + if err != nil { + return err + } + + return nil +``` + +> **Note:** +> +> The above is just an example. The *safest* approach would be to move +> `NewFoo()` into a separate package and make `Foo` and it's elements +> private. The compiler would then enforce the use of the constructor +> to guarantee correctly defined objects. + + +### Tracing + +Consider if the code needs to create a new +[trace span](https://github.com/kata-containers/kata-containers/blob/main/docs/tracing.md). + +Ensure any new trace spans added to the code are completed. + +## Tests + +### Unit tests + +Where possible, code changes should be accompanied by unit tests. + +Consider using the standard +[table-based approach](https://github.com/kata-containers/tests/blob/main/Unit-Test-Advice.md) +as it encourages you to make functions small and simple, and also +allows you to think about what types of value to test. + +### Other categories of test + +Raised a GitHub issue in the +[`tests`](https://github.com/kata-containers/tests) repository that +explains what sort of test is required along with as much detail as +possible. Ensure the original issue is referenced on the `tests` issue. + +### Unsafe code + +#### Rust language specifics + +Minimise the use of `unsafe` blocks in Rust code and since it is +potentially dangerous always write [unit tests][#unit-tests] +for this code where possible. + +`expect()` and `unwrap()` will cause the code to panic on error. +Prefer to return a `Result` on error rather than using these calls to +allow the caller to deal with the error condition. + +The table below lists the small number of cases where use of +`expect()` and `unwrap()` are permitted: + +| Area | Rationale for permitting | +|-|-| +| In test code (the `tests` module) | Panics will cause the test to fail, which is desirable. | +| `lazy_static!()` | This magic macro cannot "return" a value as it runs before `main()`. | +| `defer!()` | Similar to golang's `defer()` but doesn't allow the use of `?`. | +| `tokio::spawn(async move {})` | Cannot currently return a `Result` from an `async move` closure. | +| If an explicit test is performed before the `unwrap()` / `expect()` | *"Just about acceptable"*, but not ideal `[*]` | + + +`[*]` - There can lead to bad *future* code: consider what would +happen if the explicit test gets dropped in the future. This is easier +to happen if the test and the extraction of the value are two separate +operations. In summary, this strategy can introduce an insidious +maintenance issue. + +## Documentation + +### General requirements + +- All new features should be accompanied by documentation explaining: + + - What the new feature does + + - Why it is useful + + - How to use the feature + + - Any known issues or limitations + + Links should be provided to GitHub issues tracking the issues + +- The [documentation requirements document](Documentation-Requirements.md) + explains how the project formats documentation. + +### Markdown syntax + +Run the +[markdown checker](https://github.com/kata-containers/tests/tree/main/cmd/check-markdown) +on your documentation changes. + +### Spell check + +Run the +[spell checker](https://github.com/kata-containers/tests/tree/main/cmd/check-spelling) +on your documentation changes. + +## Finally + +You may wish to read the documentation that the +[Kata Review Team](https://github.com/kata-containers/community/blob/main/Rota-Process.md) use to help review PRs: + +- [PR review guide](https://github.com/kata-containers/community/blob/main/PR-Review-Guide.md). +- [documentation review process](https://github.com/kata-containers/community/blob/main/Documentation-Review-Process.md). diff --git a/docs/install/README.md b/docs/install/README.md index 7b5cd0229..9ad55f0f2 100644 --- a/docs/install/README.md +++ b/docs/install/README.md @@ -12,16 +12,26 @@ Containers. Packaged installation methods uses your distribution's native package format (such as RPM or DEB). -*Note:* We encourage installation methods that provides automatic updates, it ensures security updates and bug fixes are -easily applied. +> **Note:** We encourage installation methods that provides automatic updates, it ensures security updates and bug fixes are +> easily applied. -| Installation method | Description | Automatic updates | Use case | -|------------------------------------------------------|---------------------------------------------------------------------|-------------------|----------------------------------------------------------| -| [Using official distro packages](#official-packages) | Kata packages provided by Linux distributions official repositories | yes | Recommended for most users. | -| [Using snap](#snap-installation) | Easy to install | yes | Good alternative to official distro packages. | -| [Automatic](#automatic-installation) | Run a single command to install a full system | **No!** | For those wanting the latest release quickly. | -| [Manual](#manual-installation) | Follow a guide step-by-step to install a working system | **No!** | For those who want the latest release with more control. | -| [Build from source](#build-from-source-installation) | Build the software components manually | **No!** | Power users and developers only. | +| Installation method | Description | Automatic updates | Use case | +|------------------------------------------------------|----------------------------------------------------------------------------------------------|-------------------|-----------------------------------------------------------------------------------------------| +| [Using kata-deploy](#kata-deploy-installation) | The preferred way to deploy the Kata Containers distributed binaries on a Kubernetes cluster | **No!** | Best way to give it a try on kata-containers on an already up and running Kubernetes cluster. | +| [Using official distro packages](#official-packages) | Kata packages provided by Linux distributions official repositories | yes | Recommended for most users. | +| [Using snap](#snap-installation) | Easy to install | yes | Good alternative to official distro packages. | +| [Automatic](#automatic-installation) | Run a single command to install a full system | **No!** | For those wanting the latest release quickly. | +| [Manual](#manual-installation) | Follow a guide step-by-step to install a working system | **No!** | For those who want the latest release with more control. | +| [Build from source](#build-from-source-installation) | Build the software components manually | **No!** | Power users and developers only. | + +### Kata Deploy Installation + +Kata Deploy provides a Dockerfile, which contains all of the binaries and +artifacts required to run Kata Containers, as well as reference DaemonSets, +which can be utilized to install Kata Containers on a running Kubernetes +cluster. + +[Use Kata Deploy](/tools/packaging/kata-deploy/README.md) to install Kata Containers on a Kubernetes Cluster. ### Official packages @@ -48,9 +58,9 @@ Follow the [containerd installation guide](container-manager/containerd/containe ## Build from source installation -*Note:* Power users who decide to build from sources should be aware of the -implications of using an unpackaged system which will not be automatically -updated as new [releases](../Stable-Branch-Strategy.md) are made available. +> **Note:** Power users who decide to build from sources should be aware of the +> implications of using an unpackaged system which will not be automatically +> updated as new [releases](../Stable-Branch-Strategy.md) are made available. [Building from sources](../Developer-Guide.md#initial-setup) allows power users who are comfortable building software from source to use the latest component diff --git a/docs/use-cases/using-SPDK-vhostuser-and-kata.md b/docs/use-cases/using-SPDK-vhostuser-and-kata.md index 4cef647ea..ff6905a72 100644 --- a/docs/use-cases/using-SPDK-vhostuser-and-kata.md +++ b/docs/use-cases/using-SPDK-vhostuser-and-kata.md @@ -1,4 +1,4 @@ -# Setup to run SPDK vhost-user devices with Kata Containers and Docker* +# Setup to run SPDK vhost-user devices with Kata Containers > **Note:** This guide only applies to QEMU, since the vhost-user storage > device is only available for QEMU now. The enablement work on other @@ -222,26 +222,43 @@ minor `0` should be created for it, in order to be recognized by Kata runtime: $ sudo mknod /var/run/kata-containers/vhost-user/block/devices/vhostblk0 b 241 0 ``` -> **Note:** The enablement of vhost-user block device in Kata containers -> is supported by Kata Containers `1.11.0-alpha1` or newer. -> Make sure you have updated your Kata containers before evaluation. - ## Launch a Kata container with SPDK vhost-user block device -To use `vhost-user-blk` device, use Docker to pass a host `vhost-user-blk` -device to the container. In docker, `--device=HOST-DIR:CONTAINER-DIR` is used +To use `vhost-user-blk` device, use `ctr` to pass a host `vhost-user-blk` +device to the container. In your `config.json`, you should use `devices` to pass a host device to the container. -For example: +For example (only `vhost-user-blk` listed): + +```json +{ + "linux": { + "devices": [ + { + "path": "/dev/vda", + "type": "b", + "major": 241, + "minor": 0, + "fileMode": 420, + "uid": 0, + "gid": 0 + } + ] + } +} +``` + +With `rootfs` provisioned under `bundle` directory, you can run your SPDK container: ```bash -$ sudo docker run --runtime kata-runtime --device=/var/run/kata-containers/vhost-user/block/devices/vhostblk0:/dev/vda -it busybox sh +$ sudo ctr run -d --runtime io.containerd.run.kata.v2 --config bundle/config.json spdk_container ``` Example of performing I/O operations on the `vhost-user-blk` device inside container: ``` +$ sudo ctr t exec --exec-id 1 -t spdk_container sh / # ls -l /dev/vda brw-r--r-- 1 root root 254, 0 Jan 20 03:54 /dev/vda / # dd if=/dev/vda of=/tmp/ddtest bs=4k count=20 diff --git a/src/agent/Makefile b/src/agent/Makefile index 8078bb9df..bd647d448 100644 --- a/src/agent/Makefile +++ b/src/agent/Makefile @@ -114,7 +114,7 @@ $(GENERATED_FILES): %: %.in ##TARGET optimize: optimized build optimize: $(SOURCES) | show-summary show-header - @RUSTFLAGS="-C link-arg=-s $(EXTRA_RUSTFLAGS) --deny-warnings" cargo build --target $(TRIPLE) --$(BUILD_TYPE) $(EXTRA_RUSTFEATURES) + @RUSTFLAGS="-C link-arg=-s $(EXTRA_RUSTFLAGS) --deny warnings" cargo build --target $(TRIPLE) --$(BUILD_TYPE) $(EXTRA_RUSTFEATURES) ##TARGET clippy: run clippy linter clippy: $(GENERATED_CODE) diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 7d1f7bc8f..7de987400 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -112,6 +112,7 @@ lazy_static! { } #[inline(always)] +#[cfg(not(test))] pub fn mount< P1: ?Sized + NixPath, P2: ?Sized + NixPath, @@ -124,21 +125,42 @@ pub fn mount< flags: MsFlags, data: Option<&P4>, ) -> std::result::Result<(), nix::Error> { - #[cfg(not(test))] - return mount::mount(source, target, fstype, flags, data); - #[cfg(test)] - return Ok(()); + mount::mount(source, target, fstype, flags, data) } #[inline(always)] +#[cfg(test)] +pub fn mount< + P1: ?Sized + NixPath, + P2: ?Sized + NixPath, + P3: ?Sized + NixPath, + P4: ?Sized + NixPath, +>( + _source: Option<&P1>, + _target: &P2, + _fstype: Option<&P3>, + _flags: MsFlags, + _data: Option<&P4>, +) -> std::result::Result<(), nix::Error> { + Ok(()) +} + +#[inline(always)] +#[cfg(not(test))] pub fn umount2( target: &P, flags: MntFlags, ) -> std::result::Result<(), nix::Error> { - #[cfg(not(test))] - return mount::umount2(target, flags); - #[cfg(test)] - return Ok(()); + mount::umount2(target, flags) +} + +#[inline(always)] +#[cfg(test)] +pub fn umount2( + _target: &P, + _flags: MntFlags, +) -> std::result::Result<(), nix::Error> { + Ok(()) } pub fn init_rootfs( @@ -450,14 +472,20 @@ fn mount_cgroups( Ok(()) } +#[cfg(not(test))] fn pivot_root( new_root: &P1, put_old: &P2, ) -> anyhow::Result<(), nix::Error> { - #[cfg(not(test))] - return unistd::pivot_root(new_root, put_old); - #[cfg(test)] - return Ok(()); + unistd::pivot_root(new_root, put_old) +} + +#[cfg(test)] +fn pivot_root( + _new_root: &P1, + _put_old: &P2, +) -> anyhow::Result<(), nix::Error> { + Ok(()) } pub fn pivot_rootfs(path: &P) -> Result<()> { @@ -582,11 +610,15 @@ fn parse_mount_table() -> Result> { } #[inline(always)] +#[cfg(not(test))] fn chroot(path: &P) -> Result<(), nix::Error> { - #[cfg(not(test))] - return unistd::chroot(path); - #[cfg(test)] - return Ok(()); + unistd::chroot(path) +} + +#[inline(always)] +#[cfg(test)] +fn chroot(_path: &P) -> Result<(), nix::Error> { + Ok(()) } pub fn ms_move_root(rootfs: &str) -> Result { diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index df0221c74..7c73dd2ca 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -3,7 +3,6 @@ // SPDX-License-Identifier: Apache-2.0 // -use libc::{c_uint, major, minor}; use nix::sys::stat; use regex::Regex; use std::collections::HashMap; @@ -12,7 +11,7 @@ use std::fmt; use std::fs; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::MetadataExt; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::Arc; use tokio::sync::Mutex; @@ -23,7 +22,7 @@ use crate::linux_abi::*; use crate::pci; use crate::sandbox::Sandbox; use crate::uevent::{wait_for_uevent, Uevent, UeventMatcher}; -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, Context, Result}; use oci::{LinuxDeviceCgroup, LinuxResources, Spec}; use protocols::agent::Device; use tracing::instrument; @@ -53,15 +52,6 @@ pub const DRIVER_VFIO_GK_TYPE: &str = "vfio-gk"; // container as a VFIO device node pub const DRIVER_VFIO_TYPE: &str = "vfio"; -#[derive(Debug)] -struct DevIndexEntry { - idx: usize, - residx: Vec, -} - -#[derive(Debug)] -struct DevIndex(HashMap); - #[instrument] pub fn online_device(path: &str) -> Result<()> { fs::write(path, "1")?; @@ -167,20 +157,22 @@ pub fn pcipath_to_sysfs(root_bus_sysfs: &str, pcipath: &pci::Path) -> Result = fs::read_dir(&bridgebuspath)?.collect(); - if files.len() != 1 { - return Err(anyhow!( - "Expected exactly one PCI bus in {}, got {} instead", - bridgebuspath, - files.len() - )); - } - - // unwrap is safe, because of the length test above - let busfile = files.pop().unwrap()?; - bus = busfile - .file_name() - .into_string() - .map_err(|e| anyhow!("Bad filename under {}: {:?}", &bridgebuspath, e))?; + match files.pop() { + Some(busfile) if files.is_empty() => { + bus = busfile? + .file_name() + .into_string() + .map_err(|e| anyhow!("Bad filename under {}: {:?}", &bridgebuspath, e))?; + } + _ => { + return Err(anyhow!( + "Expected exactly one PCI bus in {}, got {} instead", + bridgebuspath, + // Adjust to original value as we've already popped + files.len() + 1 + )); + } + }; } Ok(relpath) @@ -228,8 +220,9 @@ impl VirtioBlkPciMatcher { fn new(relpath: &str) -> VirtioBlkPciMatcher { let root_bus = create_pci_root_bus_path(); let re = format!(r"^{}{}/virtio[0-9]+/block/", root_bus, relpath); + VirtioBlkPciMatcher { - rex: Regex::new(&re).unwrap(), + rex: Regex::new(&re).expect("BUG: failed to compile VirtioBlkPciMatcher regex"), } } } @@ -267,7 +260,7 @@ impl VirtioBlkCCWMatcher { root_bus_path, device ); VirtioBlkCCWMatcher { - rex: Regex::new(&re).unwrap(), + rex: Regex::new(&re).expect("BUG: failed to compile VirtioBlkCCWMatcher regex"), } } } @@ -423,12 +416,15 @@ fn scan_scsi_bus(scsi_addr: &str) -> Result<()> { for entry in fs::read_dir(SYSFS_SCSI_HOST_PATH)? { let host = entry?.file_name(); - let scan_path = format!( - "{}/{}/{}", - SYSFS_SCSI_HOST_PATH, - host.to_str().unwrap(), - "scan" - ); + + let host_str = host.to_str().ok_or_else(|| { + anyhow!( + "failed to convert directory entry to unicode for file {:?}", + host + ) + })?; + + let scan_path = PathBuf::from(&format!("{}/{}/{}", SYSFS_SCSI_HOST_PATH, host_str, "scan")); fs::write(scan_path, &scan_data)?; } @@ -436,91 +432,201 @@ fn scan_scsi_bus(scsi_addr: &str) -> Result<()> { Ok(()) } -// update_spec_device updates the device list in the OCI spec to make -// it include details appropriate for the VM, instead of the host. It -// is given the host path to the device (to locate the device in the -// original OCI spec) and the VM path which it uses to determine the -// VM major/minor numbers, and the final path with which to present -// the device in the (inner) container -#[instrument] -fn update_spec_device( - spec: &mut Spec, - devidx: &DevIndex, - host_path: &str, - vm_path: &str, - final_path: &str, -) -> Result<()> { - let major_id: c_uint; - let minor_id: c_uint; +#[derive(Debug, Clone)] +struct DevNumUpdate { + // the major and minor numbers for the device within the guest + guest_major: i64, + guest_minor: i64, +} - // If no container_path is provided, we won't be able to match and - // update the device in the OCI spec device list. This is an error. - if host_path.is_empty() { - return Err(anyhow!("Host path cannot empty for device")); +impl DevNumUpdate { + fn from_vm_path>(vm_path: T) -> Result { + let vm_path = vm_path.as_ref(); + + if !vm_path.exists() { + return Err(anyhow!("VM device path {:?} doesn't exist", vm_path)); + } + + let devid = fs::metadata(vm_path)?.rdev(); + let guest_major = stat::major(devid) as i64; + let guest_minor = stat::minor(devid) as i64; + + Ok(DevNumUpdate { + guest_major, + guest_minor, + }) } +} +// Represents the device-node and resource related updates to the OCI +// spec needed for a particular device +#[derive(Debug, Clone)] +struct DevUpdate { + num: DevNumUpdate, + // an optional new path to update the device to in the "inner" container + // specification + final_path: Option, +} + +impl DevUpdate { + fn from_vm_path>(vm_path: T, final_path: String) -> Result { + Ok(DevUpdate { + final_path: Some(final_path), + ..DevNumUpdate::from_vm_path(vm_path)?.into() + }) + } +} + +impl From for DevUpdate { + fn from(num: DevNumUpdate) -> Self { + DevUpdate { + num, + final_path: None, + } + } +} + +// Represents the updates to the OCI spec needed for a particular device +#[derive(Debug, Clone, Default)] +struct SpecUpdate { + dev: Option, + // optional corrections for PCI addresses + pci: Vec<(pci::Address, pci::Address)>, +} + +impl> From for SpecUpdate { + fn from(dev: T) -> Self { + SpecUpdate { + dev: Some(dev.into()), + pci: Vec::new(), + } + } +} + +// update_spec_devices updates the device list in the OCI spec to make +// it include details appropriate for the VM, instead of the host. It +// is given a map of (container_path => update) where: +// container_path: the path to the device in the original OCI spec +// update: information on changes to make to the device +#[instrument] +fn update_spec_devices(spec: &mut Spec, mut updates: HashMap<&str, DevUpdate>) -> Result<()> { let linux = spec .linux .as_mut() - .ok_or_else(|| anyhow!("Spec didn't container linux field"))?; + .ok_or_else(|| anyhow!("Spec didn't contain linux field"))?; + let mut res_updates = HashMap::<(&str, i64, i64), DevNumUpdate>::with_capacity(updates.len()); - if !Path::new(vm_path).exists() { - return Err(anyhow!("vm_path:{} doesn't exist", vm_path)); - } - - let meta = fs::metadata(vm_path)?; - let dev_id = meta.rdev(); - unsafe { - major_id = major(dev_id); - minor_id = minor(dev_id); - } - - info!( - sl!(), - "update_spec_device(): vm_path={}, major: {}, minor: {}\n", vm_path, major_id, minor_id - ); - - if let Some(idxdata) = devidx.0.get(host_path) { - let dev = &mut linux.devices[idxdata.idx]; - let host_major = dev.major; - let host_minor = dev.minor; - - dev.major = major_id as i64; - dev.minor = minor_id as i64; - dev.path = final_path.to_string(); - - info!( - sl!(), - "change the device from path: {} major: {} minor: {} to vm device path: {} major: {} minor: {}", - host_path, - host_major, - host_minor, - dev.path, - dev.major, - dev.minor, - ); - - // Resources must be updated since they are used to identify - // the device in the devices cgroup. - for ridx in &idxdata.residx { - // unwrap is safe, because residx would be empty if there - // were no resources - let res = &mut linux.resources.as_mut().unwrap().devices[*ridx]; - res.major = Some(major_id as i64); - res.minor = Some(minor_id as i64); + for specdev in &mut linux.devices { + if let Some(update) = updates.remove(specdev.path.as_str()) { + let host_major = specdev.major; + let host_minor = specdev.minor; info!( sl!(), - "set resources for device major: {} minor: {}\n", major_id, minor_id + "update_spec_devices() updating device"; + "container_path" => &specdev.path, + "type" => &specdev.r#type, + "host_major" => host_major, + "host_minor" => host_minor, + "guest_major" => update.num.guest_major, + "guest_minor" => update.num.guest_minor, + "final_path" => update.final_path.as_ref(), ); + + specdev.major = update.num.guest_major; + specdev.minor = update.num.guest_minor; + if let Some(final_path) = update.final_path { + specdev.path = final_path; + } + + if res_updates + .insert( + (specdev.r#type.as_str(), host_major, host_minor), + update.num, + ) + .is_some() + { + return Err(anyhow!( + "Conflicting resource updates for host_major={} host_minor={}", + host_major, + host_minor + )); + } } - Ok(()) - } else { - Err(anyhow!( - "Should have found a matching device {} in the spec", - vm_path - )) } + + // Make sure we applied all of our updates + if !updates.is_empty() { + return Err(anyhow!( + "Missing devices in OCI spec: {:?}", + updates + .keys() + .map(|d| format!("{:?}", d)) + .collect::>() + .join(" ") + )); + } + + if let Some(resources) = linux.resources.as_mut() { + for r in &mut resources.devices { + if let (Some(host_major), Some(host_minor)) = (r.major, r.minor) { + if let Some(update) = res_updates.get(&(r.r#type.as_str(), host_major, host_minor)) + { + info!( + sl!(), + "update_spec_devices() updating resource"; + "type" => &r.r#type, + "host_major" => host_major, + "host_minor" => host_minor, + "guest_major" => update.guest_major, + "guest_minor" => update.guest_minor, + ); + + r.major = Some(update.guest_major); + r.minor = Some(update.guest_minor); + } + } + } + } + + Ok(()) +} + +// update_spec_pci PCI addresses in the OCI spec to be guest addresses +// instead of host addresses. It is given a map of (host address => +// guest address) +#[instrument] +fn update_spec_pci(spec: &mut Spec, updates: HashMap) -> Result<()> { + // Correct PCI addresses in the environment + if let Some(process) = spec.process.as_mut() { + for envvar in process.env.iter_mut() { + let eqpos = envvar + .find('=') + .ok_or_else(|| anyhow!("Malformed OCI env entry {:?}", envvar))?; + + let (name, eqval) = envvar.split_at(eqpos); + let val = &eqval[1..]; + + if !name.starts_with("PCIDEVICE_") { + continue; + } + + let mut guest_addrs = Vec::::new(); + + for host_addr in val.split(',') { + let host_addr = pci::Address::from_str(host_addr) + .with_context(|| format!("Can't parse {} environment variable", name))?; + let guest_addr = updates + .get(&host_addr) + .ok_or_else(|| anyhow!("Unable to translate host PCI address {}", host_addr))?; + guest_addrs.push(format!("{}", guest_addr)); + } + + envvar.replace_range(eqpos + 1.., guest_addrs.join(",").as_str()); + } + } + + Ok(()) } // device.Id should be the predicted device name (vda, vdb, ...) @@ -528,43 +634,25 @@ fn update_spec_device( #[instrument] async fn virtiommio_blk_device_handler( device: &Device, - spec: &mut Spec, _sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { +) -> Result { if device.vm_path.is_empty() { return Err(anyhow!("Invalid path for virtio mmio blk device")); } - update_spec_device( - spec, - devidx, - &device.container_path, - &device.vm_path, - &device.container_path, - ) + Ok(DevNumUpdate::from_vm_path(&device.vm_path)?.into()) } // device.Id should be a PCI path string #[instrument] async fn virtio_blk_device_handler( device: &Device, - spec: &mut Spec, sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { - let mut dev = device.clone(); +) -> Result { let pcipath = pci::Path::from_str(&device.id)?; + let vm_path = get_virtio_blk_pci_device_name(sandbox, &pcipath).await?; - dev.vm_path = get_virtio_blk_pci_device_name(sandbox, &pcipath).await?; - - update_spec_device( - spec, - devidx, - &dev.container_path, - &dev.vm_path, - &dev.container_path, - ) + Ok(DevNumUpdate::from_vm_path(vm_path)?.into()) } // device.id should be a CCW path string @@ -572,30 +660,17 @@ async fn virtio_blk_device_handler( #[instrument] async fn virtio_blk_ccw_device_handler( device: &Device, - spec: &mut Spec, sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { - let mut dev = device.clone(); +) -> Result { let ccw_device = ccw::Device::from_str(&device.id)?; - dev.vm_path = get_virtio_blk_ccw_device_name(sandbox, &ccw_device).await?; - update_spec_device( - spec, - devidx, - &dev.container_path, - &dev.vm_path, - &dev.container_path, - ) + let vm_path = get_virtio_blk_ccw_device_name(sandbox, &ccw_device).await?; + + Ok(DevNumUpdate::from_vm_path(vm_path)?.into()) } #[cfg(not(target_arch = "s390x"))] #[instrument] -async fn virtio_blk_ccw_device_handler( - _: &Device, - _: &mut Spec, - _: &Arc>, - _: &DevIndex, -) -> Result<()> { +async fn virtio_blk_ccw_device_handler(_: &Device, _: &Arc>) -> Result { Err(anyhow!("CCW is only supported on s390x")) } @@ -603,39 +678,23 @@ async fn virtio_blk_ccw_device_handler( #[instrument] async fn virtio_scsi_device_handler( device: &Device, - spec: &mut Spec, sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { - let mut dev = device.clone(); - dev.vm_path = get_scsi_device_name(sandbox, &device.id).await?; - update_spec_device( - spec, - devidx, - &dev.container_path, - &dev.vm_path, - &dev.container_path, - ) +) -> Result { + let vm_path = get_scsi_device_name(sandbox, &device.id).await?; + + Ok(DevNumUpdate::from_vm_path(vm_path)?.into()) } #[instrument] async fn virtio_nvdimm_device_handler( device: &Device, - spec: &mut Spec, _sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { +) -> Result { if device.vm_path.is_empty() { return Err(anyhow!("Invalid path for nvdimm device")); } - update_spec_device( - spec, - devidx, - &device.container_path, - &device.vm_path, - &device.container_path, - ) + Ok(DevNumUpdate::from_vm_path(&device.vm_path)?.into()) } fn split_vfio_option(opt: &str) -> Option<(&str, &str)> { @@ -653,80 +712,53 @@ fn split_vfio_option(opt: &str) -> Option<(&str, &str)> { // Each option should have the form "DDDD:BB:DD.F=" // DDDD:BB:DD.F is the device's PCI address in the host // is a PCI path to the device in the guest (see pci.rs) -async fn vfio_device_handler( - device: &Device, - spec: &mut Spec, - sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { +async fn vfio_device_handler(device: &Device, sandbox: &Arc>) -> Result { let vfio_in_guest = device.field_type != DRIVER_VFIO_GK_TYPE; + let mut pci_fixups = Vec::<(pci::Address, pci::Address)>::new(); let mut group = None; for opt in device.options.iter() { - let (_, pcipath) = + let (host, pcipath) = split_vfio_option(opt).ok_or_else(|| anyhow!("Malformed VFIO option {:?}", opt))?; + let host = + pci::Address::from_str(host).context("Bad host PCI address in VFIO option {:?}")?; let pcipath = pci::Path::from_str(pcipath)?; let guestdev = wait_for_pci_device(sandbox, &pcipath).await?; if vfio_in_guest { pci_driver_override(SYSFS_BUS_PCI_PATH, guestdev, "vfio-pci")?; - let devgroup = pci_iommu_group(SYSFS_BUS_PCI_PATH, guestdev)?; - if devgroup.is_none() { - // Devices must have an IOMMU group to be usable via VFIO - return Err(anyhow!("{} has no IOMMU group", guestdev)); - } + // Devices must have an IOMMU group to be usable via VFIO + let devgroup = pci_iommu_group(SYSFS_BUS_PCI_PATH, guestdev)? + .ok_or_else(|| anyhow!("{} has no IOMMU group", guestdev))?; - if group.is_some() && group != devgroup { - // If PCI devices associated with the same VFIO device - // (and therefore group) in the host don't end up in - // the same group in the guest, something has gone - // horribly wrong - return Err(anyhow!( - "{} is not in guest IOMMU group {}", - guestdev, - group.unwrap() - )); - } - - group = devgroup; - } - } - - if vfio_in_guest { - // If there are any devices at all, logic above ensures that group is not None - let group = group.unwrap(); - let vmpath = get_vfio_device_name(sandbox, group).await?; - - update_spec_device(spec, devidx, &device.container_path, &vmpath, &vmpath)?; - } - - Ok(()) -} - -impl DevIndex { - fn new(spec: &Spec) -> DevIndex { - let mut map = HashMap::new(); - - if let Some(linux) = spec.linux.as_ref() { - for (i, d) in linux.devices.iter().enumerate() { - let mut residx = Vec::new(); - - if let Some(linuxres) = linux.resources.as_ref() { - for (j, r) in linuxres.devices.iter().enumerate() { - if r.r#type == d.r#type - && r.major == Some(d.major) - && r.minor == Some(d.minor) - { - residx.push(j); - } - } + if let Some(g) = group { + if g != devgroup { + return Err(anyhow!("{} is not in guest IOMMU group {}", guestdev, g)); } - map.insert(d.path.clone(), DevIndexEntry { idx: i, residx }); } + + group = Some(devgroup); + + pci_fixups.push((host, guestdev)); } - DevIndex(map) } + + let dev_update = if vfio_in_guest { + // If there are any devices at all, logic above ensures that group is not None + let group = group.ok_or_else(|| anyhow!("failed to get VFIO group: {:?}"))?; + + let vm_path = get_vfio_device_name(sandbox, group).await?; + + Some(DevUpdate::from_vm_path(&vm_path, vm_path.clone())?) + } else { + None + }; + + Ok(SpecUpdate { + dev: dev_update, + pci: pci_fixups, + }) } #[instrument] @@ -735,22 +767,40 @@ pub async fn add_devices( spec: &mut Spec, sandbox: &Arc>, ) -> Result<()> { - let devidx = DevIndex::new(spec); + let mut dev_updates = HashMap::<&str, DevUpdate>::with_capacity(devices.len()); + let mut pci_updates = HashMap::::new(); for device in devices.iter() { - add_device(device, spec, sandbox, &devidx).await?; + let update = add_device(device, sandbox).await?; + if let Some(dev_update) = update.dev { + if dev_updates + .insert(&device.container_path, dev_update) + .is_some() + { + return Err(anyhow!( + "Conflicting device updates for {}", + &device.container_path + )); + } + + for (host, guest) in update.pci { + if let Some(other_guest) = pci_updates.insert(host, guest) { + return Err(anyhow!( + "Conflicting guest address for host device {} ({} versus {})", + host, + guest, + other_guest + )); + } + } + } } - Ok(()) + update_spec_devices(spec, dev_updates) } #[instrument] -async fn add_device( - device: &Device, - spec: &mut Spec, - sandbox: &Arc>, - devidx: &DevIndex, -) -> Result<()> { +async fn add_device(device: &Device, sandbox: &Arc>) -> Result { // log before validation to help with debugging gRPC protocol version differences. info!(sl!(), "device-id: {}, device-type: {}, device-vm-path: {}, device-container-path: {}, device-options: {:?}", device.id, device.field_type, device.vm_path, device.container_path, device.options); @@ -768,14 +818,12 @@ async fn add_device( } match device.field_type.as_str() { - DRIVER_BLK_TYPE => virtio_blk_device_handler(device, spec, sandbox, devidx).await, - DRIVER_BLK_CCW_TYPE => virtio_blk_ccw_device_handler(device, spec, sandbox, devidx).await, - DRIVER_MMIO_BLK_TYPE => virtiommio_blk_device_handler(device, spec, sandbox, devidx).await, - DRIVER_NVDIMM_TYPE => virtio_nvdimm_device_handler(device, spec, sandbox, devidx).await, - DRIVER_SCSI_TYPE => virtio_scsi_device_handler(device, spec, sandbox, devidx).await, - DRIVER_VFIO_GK_TYPE | DRIVER_VFIO_TYPE => { - vfio_device_handler(device, spec, sandbox, devidx).await - } + DRIVER_BLK_TYPE => virtio_blk_device_handler(device, sandbox).await, + DRIVER_BLK_CCW_TYPE => virtio_blk_ccw_device_handler(device, sandbox).await, + DRIVER_MMIO_BLK_TYPE => virtiommio_blk_device_handler(device, sandbox).await, + DRIVER_NVDIMM_TYPE => virtio_nvdimm_device_handler(device, sandbox).await, + DRIVER_SCSI_TYPE => virtio_scsi_device_handler(device, sandbox).await, + DRIVER_VFIO_GK_TYPE | DRIVER_VFIO_TYPE => vfio_device_handler(device, sandbox).await, _ => Err(anyhow!("Unknown device type {}", device.field_type)), } } @@ -795,11 +843,8 @@ pub fn update_device_cgroup(spec: &mut Spec) -> Result<()> { .as_mut() .ok_or_else(|| anyhow!("Spec didn't container linux field"))?; - if linux.resources.is_none() { - linux.resources = Some(LinuxResources::default()); - } + let resources = linux.resources.get_or_insert(LinuxResources::default()); - let resources = linux.resources.as_mut().unwrap(); resources.devices.push(LinuxDeviceCgroup { allow: false, major: Some(major), @@ -815,7 +860,8 @@ pub fn update_device_cgroup(spec: &mut Spec) -> Result<()> { mod tests { use super::*; use crate::uevent::spawn_test_watcher; - use oci::Linux; + use oci::{Linux, Process}; + use std::iter::FromIterator; use tempfile::tempdir; #[test] @@ -840,28 +886,36 @@ mod tests { } #[test] - fn test_update_spec_device() { + fn test_update_spec_devices() { let (major, minor) = (7, 2); let mut spec = Spec::default(); - // container_path empty - let container_path = ""; - let vm_path = ""; - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); - assert!(res.is_err()); + // vm_path empty + let update = DevNumUpdate::from_vm_path(""); + assert!(update.is_err()); // linux is empty let container_path = "/dev/null"; - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + let vm_path = "/dev/null"; + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!(res.is_err()); spec.linux = Some(Linux::default()); - // linux.devices is empty - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + // linux.devices doesn't contain the updated device + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!(res.is_err()); spec.linux.as_mut().unwrap().devices = vec![oci::LinuxDevice { @@ -871,16 +925,14 @@ mod tests { ..oci::LinuxDevice::default() }]; - // vm_path empty - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); - assert!(res.is_err()); - - let vm_path = "/dev/null"; - // guest and host path are not the same - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!( res.is_err(), "container_path={:?} vm_path={:?} spec={:?}", @@ -892,8 +944,13 @@ mod tests { spec.linux.as_mut().unwrap().devices[0].path = container_path.to_string(); // spec.linux.resources is empty - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!(res.is_ok()); // update both devices and cgroup lists @@ -913,13 +970,18 @@ mod tests { ..oci::LinuxResources::default() }); - let devidx = DevIndex::new(&spec); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!(res.is_ok()); } #[test] - fn test_update_spec_device_guest_host_conflict() { + fn test_update_spec_devices_guest_host_conflict() { let null_rdev = fs::metadata("/dev/null").unwrap().rdev(); let zero_rdev = fs::metadata("/dev/zero").unwrap().rdev(); let full_rdev = fs::metadata("/dev/full").unwrap().rdev(); @@ -968,7 +1030,6 @@ mod tests { }), ..Spec::default() }; - let devidx = DevIndex::new(&spec); let container_path_a = "/dev/a"; let vm_path_a = "/dev/zero"; @@ -994,34 +1055,17 @@ mod tests { assert_eq!(Some(host_major_b), specresources.devices[1].major); assert_eq!(Some(host_minor_b), specresources.devices[1].minor); - let res = update_spec_device( - &mut spec, - &devidx, - container_path_a, - vm_path_a, - container_path_a, - ); - assert!(res.is_ok()); - - let specdevices = &spec.linux.as_ref().unwrap().devices; - assert_eq!(guest_major_a, specdevices[0].major); - assert_eq!(guest_minor_a, specdevices[0].minor); - assert_eq!(host_major_b, specdevices[1].major); - assert_eq!(host_minor_b, specdevices[1].minor); - - let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap(); - assert_eq!(Some(guest_major_a), specresources.devices[0].major); - assert_eq!(Some(guest_minor_a), specresources.devices[0].minor); - assert_eq!(Some(host_major_b), specresources.devices[1].major); - assert_eq!(Some(host_minor_b), specresources.devices[1].minor); - - let res = update_spec_device( - &mut spec, - &devidx, - container_path_b, - vm_path_b, - container_path_b, - ); + let updates = HashMap::from_iter(vec![ + ( + container_path_a, + DevNumUpdate::from_vm_path(vm_path_a).unwrap().into(), + ), + ( + container_path_b, + DevNumUpdate::from_vm_path(vm_path_b).unwrap().into(), + ), + ]); + let res = update_spec_devices(&mut spec, updates); assert!(res.is_ok()); let specdevices = &spec.linux.as_ref().unwrap().devices; @@ -1038,7 +1082,7 @@ mod tests { } #[test] - fn test_update_spec_device_char_block_conflict() { + fn test_update_spec_devices_char_block_conflict() { let null_rdev = fs::metadata("/dev/null").unwrap().rdev(); let guest_major = stat::major(null_rdev) as i64; @@ -1085,7 +1129,6 @@ mod tests { }), ..Spec::default() }; - let devidx = DevIndex::new(&spec); let container_path = "/dev/char"; let vm_path = "/dev/null"; @@ -1096,7 +1139,13 @@ mod tests { assert_eq!(Some(host_major), specresources.devices[1].major); assert_eq!(Some(host_minor), specresources.devices[1].minor); - let res = update_spec_device(&mut spec, &devidx, container_path, vm_path, container_path); + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevNumUpdate::from_vm_path(vm_path).unwrap().into(), + )]), + ); assert!(res.is_ok()); // Only the char device, not the block device should be updated @@ -1108,19 +1157,19 @@ mod tests { } #[test] - fn test_update_spec_device_final_path() { + fn test_update_spec_devices_final_path() { let null_rdev = fs::metadata("/dev/null").unwrap().rdev(); let guest_major = stat::major(null_rdev) as i64; let guest_minor = stat::minor(null_rdev) as i64; - let host_path = "/dev/host"; + let container_path = "/dev/original"; let host_major: i64 = 99; let host_minor: i64 = 99; let mut spec = Spec { linux: Some(Linux { devices: vec![oci::LinuxDevice { - path: host_path.to_string(), + path: container_path.to_string(), r#type: "c".to_string(), major: host_major, minor: host_minor, @@ -1130,12 +1179,17 @@ mod tests { }), ..Spec::default() }; - let devidx = DevIndex::new(&spec); let vm_path = "/dev/null"; - let final_path = "/dev/final"; + let final_path = "/dev/new"; - let res = update_spec_device(&mut spec, &devidx, host_path, vm_path, final_path); + let res = update_spec_devices( + &mut spec, + HashMap::from_iter(vec![( + container_path, + DevUpdate::from_vm_path(vm_path, final_path.to_string()).unwrap(), + )]), + ); assert!(res.is_ok()); let specdevices = &spec.linux.as_ref().unwrap().devices; @@ -1144,6 +1198,48 @@ mod tests { assert_eq!(final_path, specdevices[0].path); } + #[test] + fn test_update_spec_pci() { + let example_map = [ + // Each is a host,guest pair of pci addresses + ("0000:1a:01.0", "0000:01:01.0"), + ("0000:1b:02.0", "0000:01:02.0"), + // This one has the same host address as guest address + // above, to test that we're not double-translating + ("0000:01:01.0", "ffff:02:1f.7"), + ]; + + let mut spec = Spec { + process: Some(Process { + env: vec![ + "PCIDEVICE_x=0000:1a:01.0,0000:1b:02.0".to_string(), + "PCIDEVICE_y=0000:01:01.0".to_string(), + "NOTAPCIDEVICE_blah=abcd:ef:01.0".to_string(), + ], + ..Process::default() + }), + ..Spec::default() + }; + + let pci_fixups = example_map + .iter() + .map(|(h, g)| { + ( + pci::Address::from_str(h).unwrap(), + pci::Address::from_str(g).unwrap(), + ) + }) + .collect(); + + let res = update_spec_pci(&mut spec, pci_fixups); + assert!(res.is_ok()); + + let env = &spec.process.as_ref().unwrap().env; + assert_eq!(env[0], "PCIDEVICE_x=0000:01:01.0,0000:01:02.0"); + assert_eq!(env[1], "PCIDEVICE_y=ffff:02:1f.7"); + assert_eq!(env[2], "NOTAPCIDEVICE_blah=abcd:ef:01.0"); + } + #[test] fn test_pcipath_to_sysfs() { let testdir = tempdir().expect("failed to create tmpdir"); diff --git a/src/agent/src/main.rs b/src/agent/src/main.rs index b1eb06273..c3fa0a2e3 100644 --- a/src/agent/src/main.rs +++ b/src/agent/src/main.rs @@ -114,10 +114,10 @@ async fn create_logger_task(rfd: RawFd, vsock_port: u32, shutdown: Receiver Result { AGENT_SCRAPE_COUNT.inc(); // update agent process metrics - update_agent_metrics(); + update_agent_metrics()?; // update guest os metrics update_guest_metrics(); @@ -84,23 +85,26 @@ pub fn get_metrics(_: &protocols::agent::GetMetricsRequest) -> Result { let mut buffer = Vec::new(); let encoder = TextEncoder::new(); - encoder.encode(&metric_families, &mut buffer).unwrap(); + encoder.encode(&metric_families, &mut buffer)?; - Ok(String::from_utf8(buffer).unwrap()) + Ok(String::from_utf8(buffer)?) } #[instrument] -fn update_agent_metrics() { +fn update_agent_metrics() -> Result<()> { let me = procfs::process::Process::myself(); - if let Err(err) = me { - error!(sl!(), "failed to create process instance: {:?}", err); - return; - } + let me = match me { + Ok(p) => p, + Err(e) => { + // FIXME: return Ok for all errors? + warn!(sl!(), "failed to create process instance: {:?}", e); - let me = me.unwrap(); + return Ok(()); + } + }; - let tps = procfs::ticks_per_second().unwrap(); + let tps = procfs::ticks_per_second()?; // process total time AGENT_TOTAL_TIME.set((me.stat.utime + me.stat.stime) as f64 / (tps as f64)); @@ -109,7 +113,7 @@ fn update_agent_metrics() { AGENT_TOTAL_VM.set(me.stat.vsize as f64); // Total resident set - let page_size = procfs::page_size().unwrap() as f64; + let page_size = procfs::page_size()? as f64; AGENT_TOTAL_RSS.set(me.stat.rss as f64 * page_size); // io @@ -132,11 +136,11 @@ fn update_agent_metrics() { } match me.status() { - Err(err) => { - info!(sl!(), "failed to get process status: {:?}", err); - } + Err(err) => error!(sl!(), "failed to get process status: {:?}", err), Ok(status) => set_gauge_vec_proc_status(&AGENT_PROC_STATUS, &status), } + + Ok(()) } #[instrument] diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 3d55f874f..20fd0494c 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -139,8 +139,8 @@ pub const STORAGE_HANDLER_LIST: &[&str] = &[ #[instrument] pub fn baremount( - source: &str, - destination: &str, + source: &Path, + destination: &Path, fs_type: &str, flags: MsFlags, options: &str, @@ -148,11 +148,11 @@ pub fn baremount( ) -> Result<()> { let logger = logger.new(o!("subsystem" => "baremount")); - if source.is_empty() { + if source.as_os_str().is_empty() { return Err(anyhow!("need mount source")); } - if destination.is_empty() { + if destination.as_os_str().is_empty() { return Err(anyhow!("need mount destination")); } @@ -444,16 +444,19 @@ fn mount_storage(logger: &Logger, storage: &Storage) -> Result<()> { let options_vec = options_vec.iter().map(String::as_str).collect(); let (flags, options) = parse_mount_flags_and_options(options_vec); + let source = Path::new(&storage.source); + let mount_point = Path::new(&storage.mount_point); + info!(logger, "mounting storage"; - "mount-source:" => storage.source.as_str(), - "mount-destination" => storage.mount_point.as_str(), + "mount-source" => source.display(), + "mount-destination" => mount_point.display(), "mount-fstype" => storage.fstype.as_str(), "mount-options" => options.as_str(), ); baremount( - storage.source.as_str(), - storage.mount_point.as_str(), + source, + mount_point, storage.fstype.as_str(), flags, options.as_str(), @@ -579,7 +582,10 @@ fn mount_to_rootfs(logger: &Logger, m: &InitMount) -> Result<()> { fs::create_dir_all(Path::new(m.dest)).context("could not create directory")?; - baremount(m.src, m.dest, m.fstype, flags, &options, logger).or_else(|e| { + let source = Path::new(m.src); + let dest = Path::new(m.dest); + + baremount(source, dest, m.fstype, flags, &options, logger).or_else(|e| { if m.src != "dev" { return Err(e); } @@ -622,8 +628,7 @@ pub fn get_mount_fs_type_from_file(mount_file: &str, mount_point: &str) -> Resul let file = File::open(mount_file)?; let reader = BufReader::new(file); - let re = Regex::new(format!("device .+ mounted on {} with fstype (.+)", mount_point).as_str()) - .unwrap(); + let re = Regex::new(format!("device .+ mounted on {} with fstype (.+)", mount_point).as_str())?; // Read the file line by line using the lines() iterator from std::io::BufRead. for (_index, line) in reader.lines().enumerate() { @@ -701,20 +706,21 @@ pub fn get_cgroup_mounts( } } - if fields[0].is_empty() { + let subsystem_name = fields[0]; + + if subsystem_name.is_empty() { continue; } - if fields[0] == "devices" { + if subsystem_name == "devices" { has_device_cgroup = true; } - if let Some(value) = CGROUPS.get(&fields[0]) { - let key = CGROUPS.keys().find(|&&f| f == fields[0]).unwrap(); + if let Some((key, value)) = CGROUPS.get_key_value(subsystem_name) { cg_mounts.push(InitMount { fstype: "cgroup", src: "cgroup", - dest: *value, + dest: value, options: vec!["nosuid", "nodev", "noexec", "relatime", key], }); } @@ -767,10 +773,9 @@ fn ensure_destination_file_exists(path: &Path) -> Result<()> { return Err(anyhow!("{:?} exists but is not a regular file", path)); } - // The only way parent() can return None is if the path is /, - // which always exists, so the test above will already have caught - // it, thus the unwrap() is safe - let dir = path.parent().unwrap(); + let dir = path + .parent() + .ok_or_else(|| anyhow!("failed to find parent path for {:?}", path))?; fs::create_dir_all(dir).context(format!("create_dir_all {:?}", dir))?; @@ -937,14 +942,10 @@ mod tests { std::fs::create_dir_all(d).expect("failed to created directory"); } - let result = baremount( - &src_filename, - &dest_filename, - d.fs_type, - d.flags, - d.options, - &logger, - ); + let src = Path::new(&src_filename); + let dest = Path::new(&dest_filename); + + let result = baremount(src, dest, d.fs_type, d.flags, d.options, &logger); let msg = format!("{}: result: {:?}", msg, result); @@ -1021,15 +1022,11 @@ mod tests { .unwrap_or_else(|_| panic!("failed to create directory {}", d)); } + let src = Path::new(mnt_src_filename); + let dest = Path::new(mnt_dest_filename); + // Create an actual mount - let result = baremount( - mnt_src_filename, - mnt_dest_filename, - "bind", - MsFlags::MS_BIND, - "", - &logger, - ); + let result = baremount(src, dest, "bind", MsFlags::MS_BIND, "", &logger); assert!(result.is_ok(), "mount for test setup failed"); let tests = &[ diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 061370a46..c821a0acb 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -104,7 +104,10 @@ impl Namespace { if let Err(err) = || -> Result<()> { let origin_ns_path = get_current_thread_ns_path(ns_type.get()); - File::open(Path::new(&origin_ns_path))?; + let source = Path::new(&origin_ns_path); + let destination = new_ns_path.as_path(); + + File::open(&source)?; // Create a new netns on the current thread. let cf = ns_type.get_flags(); @@ -115,8 +118,6 @@ impl Namespace { nix::unistd::sethostname(hostname.unwrap())?; } // Bind mount the new namespace from the current thread onto the mount point to persist it. - let source: &str = origin_ns_path.as_str(); - let destination: &str = new_ns_path.as_path().to_str().unwrap_or("none"); let mut flags = MsFlags::empty(); @@ -131,7 +132,7 @@ impl Namespace { baremount(source, destination, "none", flags, "", &logger).map_err(|e| { anyhow!( - "Failed to mount {} to {} with err:{:?}", + "Failed to mount {:?} to {:?} with err:{:?}", source, destination, e diff --git a/src/agent/src/pci.rs b/src/agent/src/pci.rs index 25ad9a6a0..4cb6b521a 100644 --- a/src/agent/src/pci.rs +++ b/src/agent/src/pci.rs @@ -20,7 +20,7 @@ const FUNCTION_MAX: u8 = (1 << FUNCTION_BITS) - 1; // Represents a PCI function's slot (a.k.a. device) and function // numbers, giving its location on a single logical bus -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct SlotFn(u8); impl SlotFn { @@ -94,7 +94,7 @@ impl fmt::Display for SlotFn { } } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct Address { domain: u16, bus: u8, diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 974fafc25..9aea7bd87 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -114,11 +114,18 @@ pub struct AgentService { // ^[a-zA-Z0-9][a-zA-Z0-9_.-]+$ // pub fn verify_cid(id: &str) -> Result<()> { - let valid = id.len() > 1 - && id.chars().next().unwrap().is_alphanumeric() - && id - .chars() - .all(|c| (c.is_alphanumeric() || ['.', '-', '_'].contains(&c))); + let mut chars = id.chars(); + + let valid = match chars.next() { + Some(first) + if first.is_alphanumeric() + && id.len() > 1 + && chars.all(|c| c.is_alphanumeric() || ['.', '-', '_'].contains(&c)) => + { + true + } + _ => false, + }; match valid { true => Ok(()), @@ -195,7 +202,7 @@ impl AgentService { update_device_cgroup(&mut oci)?; // Append guest hooks - append_guest_hooks(&s, &mut oci); + append_guest_hooks(&s, &mut oci)?; // write spec to bundle path, hooks might // read ocispec @@ -217,21 +224,14 @@ impl AgentService { LinuxContainer::new(cid.as_str(), CONTAINER_BASE, opts, &sl!())?; let pipe_size = AGENT_CONFIG.read().await.container_pipe_size; - let p = if oci.process.is_some() { - Process::new( - &sl!(), - oci.process.as_ref().unwrap(), - cid.as_str(), - true, - pipe_size, - )? + + let p = if let Some(p) = oci.process { + Process::new(&sl!(), &p, cid.as_str(), true, pipe_size)? } else { info!(sl!(), "no process configurations!"); return Err(anyhow!(nix::Error::from_errno(nix::errno::Errno::EINVAL))); }; - ctr.start(p).await?; - s.update_shared_pidns(&ctr)?; s.add_container(ctr); info!(sl!(), "created container!"); @@ -253,11 +253,17 @@ impl AgentService { ctr.exec()?; + if sid == cid { + return Ok(()); + } + // start oom event loop - if sid != cid && ctr.cgroup_manager.is_some() { - let cg_path = ctr.cgroup_manager.as_ref().unwrap().get_cg_path("memory"); - if cg_path.is_some() { - let rx = notifier::notify_oom(cid.as_str(), cg_path.unwrap()).await?; + if let Some(ref ctr) = ctr.cgroup_manager { + let cg_path = ctr.get_cg_path("memory"); + + if let Some(cg_path) = cg_path { + let rx = notifier::notify_oom(cid.as_str(), cg_path.to_string()).await?; + s.run_oom_event_monitor(rx, cid.clone()).await; } } @@ -357,14 +363,13 @@ impl AgentService { let s = self.sandbox.clone(); let mut sandbox = s.lock().await; - let process = if req.process.is_some() { - req.process.as_ref().unwrap() - } else { - return Err(anyhow!(nix::Error::from_errno(nix::errno::Errno::EINVAL))); - }; + let process = req + .process + .into_option() + .ok_or_else(|| anyhow!(nix::Error::from_errno(nix::errno::Errno::EINVAL)))?; let pipe_size = AGENT_CONFIG.read().await.container_pipe_size; - let ocip = rustjail::process_grpc_to_oci(process); + let ocip = rustjail::process_grpc_to_oci(&process); let p = Process::new(&sl!(), &ocip, exec_id.as_str(), false, pipe_size)?; let ctr = sandbox @@ -382,7 +387,6 @@ impl AgentService { let eid = req.exec_id.clone(); let s = self.sandbox.clone(); let mut sandbox = s.lock().await; - let mut init = false; info!( sl!(), @@ -391,13 +395,14 @@ impl AgentService { "exec-id" => eid.clone(), ); - if eid.is_empty() { - init = true; - } + let p = sandbox.find_container_process(cid.as_str(), eid.as_str())?; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), init)?; - - let mut signal = Signal::try_from(req.signal as i32).unwrap(); + let mut signal = Signal::try_from(req.signal as i32).map_err(|e| { + anyhow!(e).context(format!( + "failed to convert {:?} to signal (container-id: {}, exec-id: {})", + req.signal, cid, eid + )) + })?; // For container initProcess, if it hasn't installed handler for "SIGTERM" signal, // it will ignore the "SIGTERM" signal sent to it, thus send it "SIGKILL" signal @@ -433,7 +438,7 @@ impl AgentService { let exit_rx = { let mut sandbox = s.lock().await; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false)?; + let p = sandbox.find_container_process(cid.as_str(), eid.as_str())?; p.exit_watchers.push(exit_send); pid = p.pid; @@ -456,7 +461,11 @@ impl AgentService { Some(p) => p, None => { // Lost race, pick up exit code from channel - resp.status = exit_recv.recv().await.unwrap(); + resp.status = exit_recv + .recv() + .await + .ok_or_else(|| anyhow!("Failed to receive exit code"))?; + return Ok(resp); } }; @@ -487,7 +496,7 @@ impl AgentService { let writer = { let s = self.sandbox.clone(); let mut sandbox = s.lock().await; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false)?; + let p = sandbox.find_container_process(cid.as_str(), eid.as_str())?; // use ptmx io if p.term_master.is_some() { @@ -498,7 +507,7 @@ impl AgentService { } }; - let writer = writer.unwrap(); + let writer = writer.ok_or_else(|| anyhow!("cannot get writer"))?; writer.lock().await.write_all(req.data.as_slice()).await?; let mut resp = WriteStreamResponse::new(); @@ -520,7 +529,7 @@ impl AgentService { let s = self.sandbox.clone(); let mut sandbox = s.lock().await; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false)?; + let p = sandbox.find_container_process(cid.as_str(), eid.as_str())?; if p.term_master.is_some() { term_exit_notifier = p.term_exit_notifier.clone(); @@ -540,7 +549,7 @@ impl AgentService { return Err(anyhow!(nix::Error::from_errno(nix::errno::Errno::EINVAL))); } - let reader = reader.unwrap(); + let reader = reader.ok_or_else(|| anyhow!("cannot get stream reader"))?; tokio::select! { _ = term_exit_notifier.notified() => { @@ -706,8 +715,8 @@ impl protocols::agent_ttrpc::AgentService for AgentService { let resp = Empty::new(); - if res.is_some() { - let oci_res = rustjail::resources_grpc_to_oci(&res.unwrap()); + if let Some(res) = res.as_ref() { + let oci_res = rustjail::resources_grpc_to_oci(res); match ctr.set(oci_res) { Err(e) => { return Err(ttrpc_error(ttrpc::Code::INTERNAL, e.to_string())); @@ -836,12 +845,14 @@ impl protocols::agent_ttrpc::AgentService for AgentService { let s = Arc::clone(&self.sandbox); let mut sandbox = s.lock().await; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false).map_err(|e| { - ttrpc_error( - ttrpc::Code::INVALID_ARGUMENT, - format!("invalid argument: {:?}", e), - ) - })?; + let p = sandbox + .find_container_process(cid.as_str(), eid.as_str()) + .map_err(|e| { + ttrpc_error( + ttrpc::Code::INVALID_ARGUMENT, + format!("invalid argument: {:?}", e), + ) + })?; p.close_stdin(); @@ -860,32 +871,33 @@ impl protocols::agent_ttrpc::AgentService for AgentService { let eid = req.exec_id.clone(); let s = Arc::clone(&self.sandbox); let mut sandbox = s.lock().await; - let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false).map_err(|e| { - ttrpc_error( - ttrpc::Code::UNAVAILABLE, - format!("invalid argument: {:?}", e), - ) - })?; + let p = sandbox + .find_container_process(cid.as_str(), eid.as_str()) + .map_err(|e| { + ttrpc_error( + ttrpc::Code::UNAVAILABLE, + format!("invalid argument: {:?}", e), + ) + })?; - if p.term_master.is_none() { + if let Some(fd) = p.term_master { + unsafe { + let win = winsize { + ws_row: req.row as c_ushort, + ws_col: req.column as c_ushort, + ws_xpixel: 0, + ws_ypixel: 0, + }; + + let err = libc::ioctl(fd, TIOCSWINSZ, &win); + Errno::result(err).map(drop).map_err(|e| { + ttrpc_error(ttrpc::Code::INTERNAL, format!("ioctl error: {:?}", e)) + })?; + } + } else { return Err(ttrpc_error(ttrpc::Code::UNAVAILABLE, "no tty".to_string())); } - let fd = p.term_master.unwrap(); - unsafe { - let win = winsize { - ws_row: req.row as c_ushort, - ws_col: req.column as c_ushort, - ws_xpixel: 0, - ws_ypixel: 0, - }; - - let err = libc::ioctl(fd, TIOCSWINSZ, &win); - Errno::result(err) - .map(drop) - .map_err(|e| ttrpc_error(ttrpc::Code::INTERNAL, format!("ioctl error: {:?}", e)))?; - } - Ok(Empty::new()) } @@ -1087,12 +1099,25 @@ impl protocols::agent_ttrpc::AgentService for AgentService { let mut sandbox = s.lock().await; // destroy all containers, clean up, notify agent to exit // etc. - sandbox.destroy().await.unwrap(); + sandbox + .destroy() + .await + .map_err(|e| ttrpc_error(ttrpc::Code::INTERNAL, e.to_string()))?; // Close get_oom_event connection, // otherwise it will block the shutdown of ttrpc. sandbox.event_tx.take(); - sandbox.sender.take().unwrap().send(1).unwrap(); + sandbox + .sender + .take() + .ok_or_else(|| { + ttrpc_error( + ttrpc::Code::INTERNAL, + "failed to get sandbox sender channel".to_string(), + ) + })? + .send(1) + .map_err(|e| ttrpc_error(ttrpc::Code::INTERNAL, e.to_string()))?; Ok(Empty::new()) } @@ -1351,11 +1376,7 @@ fn get_memory_info(block_size: bool, hotplug: bool) -> Result<(u64, bool)> { match stat::stat(SYSFS_MEMORY_HOTPLUG_PROBE_PATH) { Ok(_) => plug = true, Err(e) => { - info!( - sl!(), - "hotplug memory error: {}", - e.as_errno().unwrap().desc() - ); + info!(sl!(), "hotplug memory error: {:?}", e); match e { nix::Error::Sys(errno) => match errno { Errno::ENOENT => plug = false, @@ -1411,27 +1432,7 @@ async fn read_stream(reader: Arc>>, l: usize) -> Resu Ok(content) } -fn find_process<'a>( - sandbox: &'a mut Sandbox, - cid: &'a str, - eid: &'a str, - init: bool, -) -> Result<&'a mut Process> { - let ctr = sandbox - .get_container(cid) - .ok_or_else(|| anyhow!("Invalid container id"))?; - - if init || eid.is_empty() { - return ctr - .processes - .get_mut(&ctr.init_process_pid) - .ok_or_else(|| anyhow!("cannot find init process!")); - } - - ctr.get_process(eid).map_err(|_| anyhow!("Invalid exec id")) -} - -pub fn start(s: Arc>, server_address: &str) -> TtrpcServer { +pub fn start(s: Arc>, server_address: &str) -> Result { let agent_service = Box::new(AgentService { sandbox: s.clone() }) as Box; @@ -1451,15 +1452,14 @@ pub fn start(s: Arc>, server_address: &str) -> TtrpcServer { let image_service = protocols::image_ttrpc::create_image(Arc::new(image_service)); let server = TtrpcServer::new() - .bind(server_address) - .unwrap() + .bind(server_address)? .register_service(agent_service) .register_service(health_service) .register_service(image_service); info!(sl!(), "ttRPC server started"; "address" => server_address); - server + Ok(server) } // This function updates the container namespaces configuration based on the @@ -1504,24 +1504,28 @@ fn update_container_namespaces( // the create_sandbox request or create_container request. // Else set this to empty string so that a new pid namespace is // created for the container. - if sandbox_pidns && sandbox.sandbox_pidns.is_some() { - pid_ns.path = String::from(sandbox.sandbox_pidns.as_ref().unwrap().path.as_str()); + if sandbox_pidns { + if let Some(ref pidns) = &sandbox.sandbox_pidns { + pid_ns.path = String::from(pidns.path.as_str()); + } else { + return Err(anyhow!("failed to get sandbox pidns")); + } } linux.namespaces.push(pid_ns); Ok(()) } -fn append_guest_hooks(s: &Sandbox, oci: &mut Spec) { - if s.hooks.is_none() { - return; +fn append_guest_hooks(s: &Sandbox, oci: &mut Spec) -> Result<()> { + if let Some(ref guest_hooks) = s.hooks { + let mut hooks = oci.hooks.take().unwrap_or_default(); + hooks.prestart.append(&mut guest_hooks.prestart.clone()); + hooks.poststart.append(&mut guest_hooks.poststart.clone()); + hooks.poststop.append(&mut guest_hooks.poststop.clone()); + oci.hooks = Some(hooks); } - let guest_hooks = s.hooks.as_ref().unwrap(); - let mut hooks = oci.hooks.take().unwrap_or_default(); - hooks.prestart.append(&mut guest_hooks.prestart.clone()); - hooks.poststart.append(&mut guest_hooks.poststart.clone()); - hooks.poststop.append(&mut guest_hooks.poststop.clone()); - oci.hooks = Some(hooks); + + Ok(()) } // Check is the container process installed the @@ -1611,7 +1615,7 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { PathBuf::from("/") }; - fs::create_dir_all(dir.to_str().unwrap()).or_else(|e| { + fs::create_dir_all(&dir).or_else(|e| { if e.kind() != std::io::ErrorKind::AlreadyExists { return Err(e); } @@ -1619,10 +1623,7 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { Ok(()) })?; - std::fs::set_permissions( - dir.to_str().unwrap(), - std::fs::Permissions::from_mode(req.dir_mode), - )?; + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(req.dir_mode))?; let mut tmpfile = path.clone(); tmpfile.set_extension("tmp"); @@ -1631,10 +1632,10 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { .write(true) .create(true) .truncate(false) - .open(tmpfile.to_str().unwrap())?; + .open(&tmpfile)?; file.write_all_at(req.data.as_slice(), req.offset as u64)?; - let st = stat::stat(tmpfile.to_str().unwrap())?; + let st = stat::stat(&tmpfile)?; if st.st_size != req.file_size { return Ok(()); @@ -1643,7 +1644,7 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { file.set_permissions(std::fs::Permissions::from_mode(req.file_mode))?; unistd::chown( - tmpfile.to_str().unwrap(), + &tmpfile, Some(Uid::from_raw(req.uid as u32)), Some(Gid::from_raw(req.gid as u32)), )?; @@ -1680,10 +1681,13 @@ async fn do_add_swap(sandbox: &Arc>, req: &AddSwapRequest) -> Res // - container rootfs bind mounted at ///rootfs // - modify container spec root to point to ///rootfs fn setup_bundle(cid: &str, spec: &mut Spec) -> Result { - if spec.root.is_none() { + let spec_root = if let Some(sr) = &spec.root { + sr + } else { return Err(nix::Error::Sys(Errno::EINVAL).into()); - } - let spec_root = spec.root.as_ref().unwrap(); + }; + + let spec_root_path = Path::new(&spec_root.path); let bundle_path = Path::new(CONTAINER_BASE).join(cid); let config_path = bundle_path.join(CONFIG_JSON); @@ -1698,22 +1702,37 @@ fn setup_bundle(cid: &str, spec: &mut Spec) -> Result { if !rootfs_exists { fs::create_dir_all(&rootfs_path)?; baremount( - &spec_root.path, - rootfs_path.to_str().unwrap(), + spec_root_path, + &rootfs_path, "bind", MsFlags::MS_BIND, "", &sl!(), )?; } + + let rootfs_path_name = rootfs_path + .to_str() + .ok_or_else(|| anyhow!("failed to convert rootfs to unicode"))? + .to_string(); + spec.root = Some(Root { - path: rootfs_path.to_str().unwrap().to_owned(), + path: rootfs_path_name, readonly: spec_root.readonly, }); - let _ = spec.save(config_path.to_str().unwrap()); + + let _ = spec.save( + config_path + .to_str() + .ok_or_else(|| anyhow!("cannot convert path to unicode"))?, + ); let olddir = unistd::getcwd().context("cannot getcwd")?; - unistd::chdir(bundle_path.to_str().unwrap())?; + unistd::chdir( + bundle_path + .to_str() + .ok_or_else(|| anyhow!("cannot convert bundle path to unicode"))?, + )?; Ok(olddir) } @@ -1746,8 +1765,8 @@ fn load_kernel_module(module: &protocols::agent::KernelModule) -> Result<()> { match status.code() { Some(code) => { - let std_out: String = String::from_utf8(output.stdout).unwrap(); - let std_err: String = String::from_utf8(output.stderr).unwrap(); + let std_out = String::from_utf8_lossy(&output.stdout); + let std_err = String::from_utf8_lossy(&output.stderr); let msg = format!( "load_kernel_module return code: {} stdout:{} stderr:{}", code, std_out, std_err @@ -1810,7 +1829,7 @@ mod tests { let mut oci = Spec { ..Default::default() }; - append_guest_hooks(&s, &mut oci); + append_guest_hooks(&s, &mut oci).unwrap(); assert_eq!(s.hooks, oci.hooks); } diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index d90bfbafa..d98940aeb 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -228,6 +228,21 @@ impl Sandbox { None } + pub fn find_container_process(&mut self, cid: &str, eid: &str) -> Result<&mut Process> { + let ctr = self + .get_container(cid) + .ok_or_else(|| anyhow!("Invalid container id"))?; + + if eid.is_empty() { + return ctr + .processes + .get_mut(&ctr.init_process_pid) + .ok_or_else(|| anyhow!("cannot find init process!")); + } + + ctr.get_process(eid).map_err(|_| anyhow!("Invalid exec id")) + } + #[instrument] pub async fn destroy(&mut self) -> Result<()> { for ctr in self.containers.values_mut() { @@ -456,14 +471,19 @@ mod tests { use nix::mount::MsFlags; use oci::{Linux, Root, Spec}; use rustjail::container::LinuxContainer; + use rustjail::process::Process; use rustjail::specconv::CreateOpts; use slog::Logger; use std::fs::{self, File}; use std::os::unix::fs::PermissionsExt; + use std::path::Path; use tempfile::Builder; fn bind_mount(src: &str, dst: &str, logger: &Logger) -> Result<(), Error> { - baremount(src, dst, "bind", MsFlags::MS_BIND, "", logger) + let src_path = Path::new(src); + let dst_path = Path::new(dst); + + baremount(src_path, dst_path, "bind", MsFlags::MS_BIND, "", logger) } use serial_test::serial; @@ -783,4 +803,50 @@ mod tests { let ret = s.destroy().await; assert!(ret.is_ok()); } + + #[tokio::test] + async fn test_find_container_process() { + skip_if_not_root!(); + let logger = slog::Logger::root(slog::Discard, o!()); + let mut s = Sandbox::new(&logger).unwrap(); + let cid = "container-123"; + + let mut linux_container = create_linuxcontainer(); + linux_container.init_process_pid = 1; + linux_container.id = cid.to_string(); + // add init process + linux_container.processes.insert( + 1, + Process::new(&logger, &oci::Process::default(), "1", true, 1).unwrap(), + ); + // add exec process + linux_container.processes.insert( + 123, + Process::new(&logger, &oci::Process::default(), "exec-123", false, 1).unwrap(), + ); + + s.add_container(linux_container); + + // empty exec-id will return init process + let p = s.find_container_process(cid, ""); + assert!(p.is_ok(), "Expecting Ok, Got {:?}", p); + let p = p.unwrap(); + assert_eq!("1", p.exec_id, "exec_id should be 1"); + assert!(p.init, "init flag should be true"); + + // get exist exec-id will return the exec process + let p = s.find_container_process(cid, "exec-123"); + assert!(p.is_ok(), "Expecting Ok, Got {:?}", p); + let p = p.unwrap(); + assert_eq!("exec-123", p.exec_id, "exec_id should be exec-123"); + assert!(!p.init, "init flag should be false"); + + // get not exist exec-id will return error + let p = s.find_container_process(cid, "exec-456"); + assert!(p.is_err(), "Expecting Error, Got {:?}", p); + + // container does not exist + let p = s.find_container_process("not-exist-cid", ""); + assert!(p.is_err(), "Expecting Error, Got {:?}", p); + } } diff --git a/src/agent/src/util.rs b/src/agent/src/util.rs index 0e262e7ee..1be52f730 100644 --- a/src/agent/src/util.rs +++ b/src/agent/src/util.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -use anyhow::Result; +use anyhow::{anyhow, Result}; use futures::StreamExt; use std::io; use std::io::ErrorKind; @@ -64,8 +64,12 @@ pub fn get_vsock_incoming(fd: RawFd) -> Incoming { #[instrument] pub async fn get_vsock_stream(fd: RawFd) -> Result { - let stream = get_vsock_incoming(fd).next().await.unwrap()?; - Ok(stream) + let stream = get_vsock_incoming(fd) + .next() + .await + .ok_or_else(|| anyhow!("cannot handle incoming vsock connection"))?; + + Ok(stream?) } #[cfg(test)] @@ -124,7 +128,9 @@ mod tests { let mut vec_locked = vec_ref.lock(); - let v = vec_locked.as_deref_mut().unwrap(); + let v = vec_locked + .as_deref_mut() + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; std::io::Write::flush(v) } diff --git a/src/agent/src/watcher.rs b/src/agent/src/watcher.rs index b111aa166..b3cd3f832 100644 --- a/src/agent/src/watcher.rs +++ b/src/agent/src/watcher.rs @@ -49,7 +49,7 @@ struct Storage { /// the source becomes too large, either in number of files (>16) or total size (>1MB). watch: bool, - /// The list of files to watch from the source mount point and updated in the target one. + /// The list of files, directories, symlinks to watch from the source mount point and updated in the target one. watched_files: HashMap, } @@ -79,6 +79,20 @@ impl Drop for Storage { } } +async fn copy(from: impl AsRef, to: impl AsRef) -> Result<()> { + if fs::symlink_metadata(&from).await?.file_type().is_symlink() { + // if source is a symlink, create new symlink with same link source. If + // the symlink exists, remove and create new one: + if fs::symlink_metadata(&to).await.is_ok() { + fs::remove_file(&to).await?; + } + fs::symlink(fs::read_link(&from).await?, &to).await?; + } else { + fs::copy(from, to).await?; + } + Ok(()) +} + impl Storage { async fn new(storage: protos::Storage) -> Result { let entry = Storage { @@ -93,6 +107,16 @@ impl Storage { async fn update_target(&self, logger: &Logger, source_path: impl AsRef) -> Result<()> { let source_file_path = source_path.as_ref(); + // if we are creating a directory: just create it, nothing more to do + if source_file_path.symlink_metadata()?.file_type().is_dir() { + fs::create_dir_all(source_file_path) + .await + .with_context(|| { + format!("Unable to mkdir all for {}", source_file_path.display()) + })? + } + + // Assume we are dealing with either a file or a symlink now: let dest_file_path = if self.source_mount_point.is_file() { // Simple file to file copy // Assume target mount is a file path @@ -110,19 +134,13 @@ impl Storage { dest_file_path }; - debug!( - logger, - "Copy from {} to {}", - source_file_path.display(), - dest_file_path.display() - ); - fs::copy(&source_file_path, &dest_file_path) + copy(&source_file_path, &dest_file_path) .await .with_context(|| { format!( "Copy from {} to {} failed", source_file_path.display(), - dest_file_path.display() + dest_file_path.display(), ) })?; @@ -135,7 +153,7 @@ impl Storage { let mut remove_list = Vec::new(); let mut updated_files: Vec = Vec::new(); - // Remove deleted files for tracking list + // Remove deleted files for tracking list. self.watched_files.retain(|st, _| { if st.exists() { true @@ -147,10 +165,19 @@ impl Storage { // Delete from target for path in remove_list { - // File has been deleted, remove it from target mount let target = self.make_target_path(path)?; - debug!(logger, "Removing file from mount: {}", target.display()); - let _ = fs::remove_file(target).await; + // The target may be a directory or a file. If it is a directory that is removed, + // we'll remove all files under that directory as well. Because of this, there's a + // chance the target (a subdirectory or file under a prior removed target) was already + // removed. Make sure we check if the target exists before checking the metadata, and + // don't return an error if the remove fails + if target.exists() && target.symlink_metadata()?.file_type().is_dir() { + debug!(logger, "Removing a directory: {}", target.display()); + let _ = fs::remove_dir_all(target).await; + } else { + debug!(logger, "Removing a file: {}", target.display()); + let _ = fs::remove_file(target).await; + } } // Scan new & changed files @@ -182,15 +209,16 @@ impl Storage { let mut size: u64 = 0; debug!(logger, "Scanning path: {}", path.display()); - if path.is_file() { - let metadata = path - .metadata() - .with_context(|| format!("Failed to query metadata for: {}", path.display()))?; + let metadata = path + .symlink_metadata() + .with_context(|| format!("Failed to query metadata for: {}", path.display()))?; - let modified = metadata - .modified() - .with_context(|| format!("Failed to get modified date for: {}", path.display()))?; + let modified = metadata + .modified() + .with_context(|| format!("Failed to get modified date for: {}", path.display()))?; + // Treat files and symlinks the same: + if path.is_file() || metadata.file_type().is_symlink() { size += metadata.len(); // Insert will return old entry if any @@ -212,6 +240,16 @@ impl Storage { } ); } else { + // Handling regular directories - check to see if this directory is already being tracked, and + // track if not: + if self + .watched_files + .insert(path.to_path_buf(), modified) + .is_none() + { + update_list.push(path.to_path_buf()); + } + // Scan dir recursively let mut entries = fs::read_dir(path) .await @@ -328,8 +366,8 @@ impl SandboxStorages { } match baremount( - entry.source_mount_point.to_str().unwrap(), - entry.target_mount_point.to_str().unwrap(), + entry.source_mount_point.as_path(), + entry.target_mount_point.as_path(), "bind", MsFlags::MS_BIND, "bind", @@ -439,8 +477,8 @@ impl BindWatcher { fs::create_dir_all(WATCH_MOUNT_POINT_PATH).await?; baremount( - "tmpfs", - WATCH_MOUNT_POINT_PATH, + Path::new("tmpfs"), + Path::new(WATCH_MOUNT_POINT_PATH), "tmpfs", MsFlags::empty(), "", @@ -612,7 +650,7 @@ mod tests { .unwrap(); // setup storage3: many files, but still watchable - for i in 1..MAX_ENTRIES_PER_STORAGE + 1 { + for i in 1..MAX_ENTRIES_PER_STORAGE { fs::write(src3_path.join(format!("{}.txt", i)), "original").unwrap(); } @@ -622,6 +660,9 @@ mod tests { ..Default::default() }; + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + entries .add(std::iter::once(storage0), &logger) .await @@ -674,7 +715,7 @@ mod tests { std::fs::read_dir(entries.0[3].target_mount_point.as_path()) .unwrap() .count(), - MAX_ENTRIES_PER_STORAGE + MAX_ENTRIES_PER_STORAGE - 1 ); // Add two files to storage 0, verify it is updated without needing to run check: @@ -692,6 +733,9 @@ mod tests { "updated" ); + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + // // Prepare for second check: update mount sources // @@ -744,7 +788,7 @@ mod tests { std::fs::read_dir(entries.0[3].target_mount_point.as_path()) .unwrap() .count(), - MAX_ENTRIES_PER_STORAGE + 1 + MAX_ENTRIES_PER_STORAGE ); // verify that we can remove files as well, but that it isn't observed until check is run @@ -822,15 +866,20 @@ mod tests { fs::remove_file(source_dir.path().join("big.txt")).unwrap(); fs::remove_file(source_dir.path().join("too-big.txt")).unwrap(); - // Up to 16 files should be okay: - for i in 1..MAX_ENTRIES_PER_STORAGE + 1 { + assert_eq!(entry.scan(&logger).await.unwrap(), 0); + + // Up to 15 files should be okay (can watch 15 files + 1 directory) + for i in 1..MAX_ENTRIES_PER_STORAGE { fs::write(source_dir.path().join(format!("{}.txt", i)), "original").unwrap(); } - assert_eq!(entry.scan(&logger).await.unwrap(), MAX_ENTRIES_PER_STORAGE); + assert_eq!( + entry.scan(&logger).await.unwrap(), + MAX_ENTRIES_PER_STORAGE - 1 + ); - // 17 files is too many: - fs::write(source_dir.path().join("17.txt"), "updated").unwrap(); + // 16 files wll be too many: + fs::write(source_dir.path().join("16.txt"), "updated").unwrap(); thread::sleep(Duration::from_secs(1)); // Expect to receive a MountTooManyFiles error @@ -843,6 +892,180 @@ mod tests { } } + #[tokio::test] + async fn test_copy() { + // prepare tmp src/destination + let source_dir = tempfile::tempdir().unwrap(); + let dest_dir = tempfile::tempdir().unwrap(); + + // verify copy of a regular file + let src_file = source_dir.path().join("file.txt"); + let dst_file = dest_dir.path().join("file.txt"); + fs::write(&src_file, "foo").unwrap(); + copy(&src_file, &dst_file).await.unwrap(); + // verify destination: + assert!(!fs::symlink_metadata(dst_file) + .unwrap() + .file_type() + .is_symlink()); + + // verify copy of a symlink + let src_symlink_file = source_dir.path().join("symlink_file.txt"); + let dst_symlink_file = dest_dir.path().join("symlink_file.txt"); + tokio::fs::symlink(&src_file, &src_symlink_file) + .await + .unwrap(); + copy(src_symlink_file, &dst_symlink_file).await.unwrap(); + // verify destination: + assert!(fs::symlink_metadata(&dst_symlink_file) + .unwrap() + .file_type() + .is_symlink()); + assert_eq!(fs::read_link(&dst_symlink_file).unwrap(), src_file); + assert_eq!(fs::read_to_string(&dst_symlink_file).unwrap(), "foo") + } + + #[tokio::test] + async fn watch_directory_verify_dir_removal() { + let source_dir = tempfile::tempdir().unwrap(); + let dest_dir = tempfile::tempdir().unwrap(); + + let mut entry = Storage::new(protos::Storage { + source: source_dir.path().display().to_string(), + mount_point: dest_dir.path().display().to_string(), + ..Default::default() + }) + .await + .unwrap(); + let logger = slog::Logger::root(slog::Discard, o!()); + + // create a path we'll remove later + fs::create_dir_all(source_dir.path().join("tmp")).unwrap(); + fs::write(&source_dir.path().join("tmp/test-file"), "foo").unwrap(); + assert_eq!(entry.scan(&logger).await.unwrap(), 3); // root, ./tmp, test-file + + // Verify expected directory, file: + assert_eq!( + std::fs::read_dir(dest_dir.path().join("tmp")) + .unwrap() + .count(), + 1 + ); + assert_eq!(std::fs::read_dir(&dest_dir).unwrap().count(), 1); + + // Now, remove directory, and verify that the directory (and its file) are removed: + fs::remove_dir_all(source_dir.path().join("tmp")).unwrap(); + thread::sleep(Duration::from_secs(1)); + assert_eq!(entry.scan(&logger).await.unwrap(), 0); + + assert_eq!(std::fs::read_dir(&dest_dir).unwrap().count(), 0); + + assert_eq!(entry.scan(&logger).await.unwrap(), 0); + } + + #[tokio::test] + async fn watch_directory_with_symlinks() { + // Prepare source directory: + // ..2021_10_29_03_10_48.161654083/file.txt + // ..data -> ..2021_10_29_03_10_48.161654083 + // file.txt -> ..data/file.txt + + let source_dir = tempfile::tempdir().unwrap(); + let actual_dir = source_dir.path().join("..2021_10_29_03_10_48.161654083"); + let actual_file = actual_dir.join("file.txt"); + let sym_dir = source_dir.path().join("..data"); + let sym_file = source_dir.path().join("file.txt"); + + let relative_to_dir = PathBuf::from("..2021_10_29_03_10_48.161654083"); + + // create backing file/path + fs::create_dir_all(&actual_dir).unwrap(); + fs::write(&actual_file, "two").unwrap(); + + // create indirection symlink directory that points to the directory that holds the actual file: + tokio::fs::symlink(&relative_to_dir, &sym_dir) + .await + .unwrap(); + + // create presented data file symlink: + tokio::fs::symlink(PathBuf::from("..data/file.txt"), sym_file) + .await + .unwrap(); + + let dest_dir = tempfile::tempdir().unwrap(); + + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + + let mut entry = Storage::new(protos::Storage { + source: source_dir.path().display().to_string(), + mount_point: dest_dir.path().display().to_string(), + ..Default::default() + }) + .await + .unwrap(); + + let logger = slog::Logger::root(slog::Discard, o!()); + + assert_eq!(entry.scan(&logger).await.unwrap(), 5); + + // Should copy no files since nothing is changed since last check + assert_eq!(entry.scan(&logger).await.unwrap(), 0); + + // now what, what is updated? + fs::write(actual_file, "updated").unwrap(); + + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + + assert_eq!(entry.scan(&logger).await.unwrap(), 1); + + assert_eq!( + fs::read_to_string(dest_dir.path().join("file.txt")).unwrap(), + "updated" + ); + + // Verify that resulting file.txt is a symlink: + assert!( + tokio::fs::symlink_metadata(dest_dir.path().join("file.txt")) + .await + .unwrap() + .file_type() + .is_symlink() + ); + + // Verify that .data directory is a symlink: + assert!(tokio::fs::symlink_metadata(&dest_dir.path().join("..data")) + .await + .unwrap() + .file_type() + .is_symlink()); + + // Should copy no new files after copy happened + assert_eq!(entry.scan(&logger).await.unwrap(), 0); + + // Now, simulate configmap update. + // - create a new actual dir/file, + // - update the symlink directory to point to this one + // - remove old dir/file + let new_actual_dir = source_dir.path().join("..2021_10_31"); + let new_actual_file = new_actual_dir.join("file.txt"); + fs::create_dir_all(&new_actual_dir).unwrap(); + fs::write(&new_actual_file, "new configmap").unwrap(); + + tokio::fs::remove_file(&sym_dir).await.unwrap(); + tokio::fs::symlink(PathBuf::from("..2021_10_31"), &sym_dir) + .await + .unwrap(); + tokio::fs::remove_dir_all(&actual_dir).await.unwrap(); + + assert_eq!(entry.scan(&logger).await.unwrap(), 3); // file, file-dir, symlink + assert_eq!( + fs::read_to_string(dest_dir.path().join("file.txt")).unwrap(), + "new configmap" + ); + } + #[tokio::test] async fn watch_directory() { // Prepare source directory: @@ -853,6 +1076,9 @@ mod tests { fs::create_dir_all(source_dir.path().join("A/B")).unwrap(); fs::write(source_dir.path().join("A/B/1.txt"), "two").unwrap(); + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + let dest_dir = tempfile::tempdir().unwrap(); let mut entry = Storage::new(protos::Storage { @@ -865,13 +1091,11 @@ mod tests { let logger = slog::Logger::root(slog::Discard, o!()); - assert_eq!(entry.scan(&logger).await.unwrap(), 2); + assert_eq!(entry.scan(&logger).await.unwrap(), 5); // Should copy no files since nothing is changed since last check assert_eq!(entry.scan(&logger).await.unwrap(), 0); - // Should copy 1 file - thread::sleep(Duration::from_secs(1)); fs::write(source_dir.path().join("A/B/1.txt"), "updated").unwrap(); assert_eq!(entry.scan(&logger).await.unwrap(), 1); assert_eq!( @@ -879,6 +1103,9 @@ mod tests { "updated" ); + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + // Should copy no new files after copy happened assert_eq!(entry.scan(&logger).await.unwrap(), 0); @@ -909,7 +1136,9 @@ mod tests { assert_eq!(entry.scan(&logger).await.unwrap(), 1); - thread::sleep(Duration::from_secs(1)); + // delay 20 ms between writes to files in order to ensure filesystem timestamps are unique + thread::sleep(Duration::from_millis(20)); + fs::write(&source_file, "two").unwrap(); assert_eq!(entry.scan(&logger).await.unwrap(), 1); assert_eq!(fs::read_to_string(&dest_file).unwrap(), "two"); @@ -935,8 +1164,9 @@ mod tests { let logger = slog::Logger::root(slog::Discard, o!()); - assert_eq!(entry.scan(&logger).await.unwrap(), 1); - assert_eq!(entry.watched_files.len(), 1); + // expect the root directory and the file: + assert_eq!(entry.scan(&logger).await.unwrap(), 2); + assert_eq!(entry.watched_files.len(), 2); assert!(target_file.exists()); assert!(entry.watched_files.contains_key(&source_file)); @@ -946,7 +1176,7 @@ mod tests { assert_eq!(entry.scan(&logger).await.unwrap(), 0); - assert_eq!(entry.watched_files.len(), 0); + assert_eq!(entry.watched_files.len(), 1); assert!(!target_file.exists()); } @@ -992,6 +1222,8 @@ mod tests { watcher.mount(&logger).await.unwrap(); assert!(is_mounted(WATCH_MOUNT_POINT_PATH).unwrap()); + thread::sleep(Duration::from_millis(20)); + watcher.cleanup(); assert!(!is_mounted(WATCH_MOUNT_POINT_PATH).unwrap()); } diff --git a/src/runtime/.gitignore b/src/runtime/.gitignore index d155740b7..ffbf8a755 100644 --- a/src/runtime/.gitignore +++ b/src/runtime/.gitignore @@ -11,7 +11,6 @@ config-generated.go /pkg/containerd-shim-v2/monitor_address /data/kata-collect-data.sh /kata-monitor -/kata-netmon /kata-runtime /pkg/katautils/config-settings.go /virtcontainers/hack/virtc/virtc diff --git a/src/runtime/Makefile b/src/runtime/Makefile index a8f7ef070..9d1080780 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -55,11 +55,6 @@ RUNTIME_OUTPUT = $(CURDIR)/$(TARGET) RUNTIME_DIR = $(CLI_DIR)/$(TARGET) BINLIST += $(TARGET) -NETMON_DIR = $(CLI_DIR)/netmon -NETMON_TARGET = $(PROJECT_TYPE)-netmon -NETMON_RUNTIME_OUTPUT = $(CURDIR)/$(NETMON_TARGET) -BINLIBEXECLIST += $(NETMON_TARGET) - DESTDIR ?= / ifeq ($(PREFIX),) @@ -142,9 +137,6 @@ ACRNVALIDHYPERVISORPATHS := [\"$(ACRNPATH)\"] ACRNCTLPATH := $(ACRNBINDIR)/$(ACRNCTLCMD) ACRNVALIDCTLPATHS := [\"$(ACRNCTLPATH)\"] -NETMONCMD := $(BIN_PREFIX)-netmon -NETMONPATH := $(PKGLIBEXECDIR)/$(NETMONCMD) - # Default number of vCPUs DEFVCPUS := 1 # Default maximum number of vCPUs @@ -416,7 +408,6 @@ USER_VARS += PROJECT_PREFIX USER_VARS += PROJECT_TAG USER_VARS += PROJECT_TYPE USER_VARS += PROJECT_URL -USER_VARS += NETMONPATH USER_VARS += QEMUBINDIR USER_VARS += QEMUCMD USER_VARS += QEMUPATH @@ -509,7 +500,7 @@ define SHOW_ARCH $(shell printf "\\t%s%s\\\n" "$(1)" $(if $(filter $(ARCH),$(1))," (default)","")) endef -all: runtime containerd-shim-v2 netmon monitor +all: runtime containerd-shim-v2 monitor # Targets that depend on .git-commit can use $(shell cat .git-commit) to get a # git revision string. They will only be rebuilt if the revision string @@ -525,11 +516,6 @@ containerd-shim-v2: $(SHIMV2_OUTPUT) monitor: $(MONITOR_OUTPUT) -netmon: $(NETMON_RUNTIME_OUTPUT) - -$(NETMON_RUNTIME_OUTPUT): $(SOURCES) VERSION - $(QUIET_BUILD)(cd $(NETMON_DIR) && go build $(BUILDFLAGS) -o $@ -ldflags "-X main.version=$(VERSION)" $(KATA_LDFLAGS)) - runtime: $(RUNTIME_OUTPUT) $(CONFIGS) .DEFAULT: default @@ -638,15 +624,13 @@ coverage: go test -v -mod=vendor -covermode=atomic -coverprofile=coverage.txt ./... go tool cover -html=coverage.txt -o coverage.html -install: all install-runtime install-containerd-shim-v2 install-monitor install-netmon +install: all install-runtime install-containerd-shim-v2 install-monitor install-bin: $(BINLIST) $(QUIET_INST)$(foreach f,$(BINLIST),$(call INSTALL_EXEC,$f,$(BINDIR))) install-runtime: runtime install-scripts install-completions install-configs install-bin -install-netmon: install-bin-libexec - install-containerd-shim-v2: $(SHIMV2) $(QUIET_INST)$(call INSTALL_EXEC,$<,$(BINDIR)) @@ -678,7 +662,6 @@ clean: $(QUIET_CLEAN)rm -f \ $(CONFIGS) \ $(GENERATED_FILES) \ - $(NETMON_TARGET) \ $(MONITOR) \ $(SHIMV2) \ $(TARGET) \ @@ -706,9 +689,7 @@ show-usage: show-header @printf "\tgenerate-config : create configuration file.\n" @printf "\tinstall : install everything.\n" @printf "\tinstall-containerd-shim-v2 : only install containerd shim v2 files.\n" - @printf "\tinstall-netmon : only install netmon files.\n" @printf "\tinstall-runtime : only install runtime files.\n" - @printf "\tnetmon : only build netmon.\n" @printf "\truntime : only build runtime.\n" @printf "\tshow-arches : show supported architectures (ARCH variable values).\n" @printf "\tshow-summary : show install locations.\n" diff --git a/src/runtime/cmd/kata-runtime/kata-env.go b/src/runtime/cmd/kata-runtime/kata-env.go index 2fb8f08ba..b1421fa00 100644 --- a/src/runtime/cmd/kata-runtime/kata-env.go +++ b/src/runtime/cmd/kata-runtime/kata-env.go @@ -140,14 +140,6 @@ type HostInfo struct { SupportVSocks bool } -// NetmonInfo stores netmon details -type NetmonInfo struct { - Path string - Version VersionInfo - Debug bool - Enable bool -} - // EnvInfo collects all information that will be displayed by the // env command. // @@ -159,7 +151,6 @@ type EnvInfo struct { Initrd InitrdInfo Hypervisor HypervisorInfo Runtime RuntimeInfo - Netmon NetmonInfo Host HostInfo Agent AgentInfo } @@ -276,26 +267,6 @@ func getMemoryInfo() MemoryInfo { } } -func getNetmonInfo(config oci.RuntimeConfig) NetmonInfo { - netmonConfig := config.NetmonConfig - - var netmonVersionInfo VersionInfo - if version, err := getCommandVersion(netmonConfig.Path); err != nil { - netmonVersionInfo = unknownVersionInfo - } else { - netmonVersionInfo = constructVersionInfo(version) - } - - netmon := NetmonInfo{ - Version: netmonVersionInfo, - Path: netmonConfig.Path, - Debug: netmonConfig.Debug, - Enable: netmonConfig.Enable, - } - - return netmon -} - func getCommandVersion(cmd string) (string, error) { return utils.RunCommand([]string{cmd, "--version"}) } @@ -364,8 +335,6 @@ func getEnvInfo(configFile string, config oci.RuntimeConfig) (env EnvInfo, err e return EnvInfo{}, err } - netmon := getNetmonInfo(config) - agent, err := getAgentInfo(config) if err != nil { return EnvInfo{}, err @@ -398,7 +367,6 @@ func getEnvInfo(configFile string, config oci.RuntimeConfig) (env EnvInfo, err e Initrd: initrd, Agent: agent, Host: host, - Netmon: netmon, } return env, nil diff --git a/src/runtime/cmd/kata-runtime/kata-env_test.go b/src/runtime/cmd/kata-runtime/kata-env_test.go index 507fd0325..39d354427 100644 --- a/src/runtime/cmd/kata-runtime/kata-env_test.go +++ b/src/runtime/cmd/kata-runtime/kata-env_test.go @@ -32,7 +32,6 @@ import ( ) var ( - testNetmonVersion = "netmon version 0.1" testHypervisorVersion = "QEMU emulator version 2.7.0+git.741f430a96-6.1, Copyright (c) 2003-2016 Fabrice Bellard and the QEMU Project developers" ) @@ -41,7 +40,6 @@ var ( enableVirtioFS = false runtimeDebug = false runtimeTrace = false - netmonDebug = false agentDebug = false agentTrace = false ) @@ -83,7 +81,6 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC imagePath := filepath.Join(prefixDir, "image") kernelParams := "foo=bar xyz" machineType := "machineType" - netmonPath := filepath.Join(prefixDir, "netmon") disableBlock := true blockStorageDriver := "virtio-scsi" enableIOThreads := true @@ -107,11 +104,6 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC } } - err = makeVersionBinary(netmonPath, testNetmonVersion) - if err != nil { - return "", oci.RuntimeConfig{}, err - } - err = makeVersionBinary(hypervisorPath, testHypervisorVersion) if err != nil { return "", oci.RuntimeConfig{}, err @@ -130,7 +122,6 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC ImagePath: imagePath, KernelParams: kernelParams, MachineType: machineType, - NetmonPath: netmonPath, LogPath: logPath, DefaultGuestHookPath: hypConfig.GuestHookPath, DisableBlock: disableBlock, @@ -146,7 +137,6 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC HypervisorDebug: hypervisorDebug, RuntimeDebug: runtimeDebug, RuntimeTrace: runtimeTrace, - NetmonDebug: netmonDebug, AgentDebug: agentDebug, AgentTrace: agentTrace, SharedFS: sharedFS, @@ -169,15 +159,6 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC return configFile, config, nil } -func getExpectedNetmonDetails(config oci.RuntimeConfig) (NetmonInfo, error) { - return NetmonInfo{ - Version: constructVersionInfo(testNetmonVersion), - Path: config.NetmonConfig.Path, - Debug: config.NetmonConfig.Debug, - Enable: config.NetmonConfig.Enable, - }, nil -} - func getExpectedAgentDetails(config oci.RuntimeConfig) (AgentInfo, error) { agentConfig := config.AgentConfig @@ -352,11 +333,6 @@ func getExpectedSettings(config oci.RuntimeConfig, tmpdir, configFile string) (E return EnvInfo{}, err } - netmon, err := getExpectedNetmonDetails(config) - if err != nil { - return EnvInfo{}, err - } - hypervisor := getExpectedHypervisor(config) kernel := getExpectedKernel(config) image := getExpectedImage(config) @@ -369,7 +345,6 @@ func getExpectedSettings(config oci.RuntimeConfig, tmpdir, configFile string) (E Kernel: kernel, Agent: agent, Host: host, - Netmon: netmon, } return env, nil @@ -612,50 +587,6 @@ func TestEnvGetRuntimeInfo(t *testing.T) { assert.Equal(t, expectedRuntime, runtime) } -func TestEnvGetNetmonInfo(t *testing.T) { - tmpdir, err := ioutil.TempDir("", "") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpdir) - - _, config, err := makeRuntimeConfig(tmpdir) - assert.NoError(t, err) - - expectedNetmon, err := getExpectedNetmonDetails(config) - assert.NoError(t, err) - - netmon := getNetmonInfo(config) - assert.NoError(t, err) - - assert.Equal(t, expectedNetmon, netmon) -} - -func TestEnvGetNetmonInfoNoVersion(t *testing.T) { - tmpdir, err := ioutil.TempDir("", "") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpdir) - - _, config, err := makeRuntimeConfig(tmpdir) - assert.NoError(t, err) - - expectedNetmon, err := getExpectedNetmonDetails(config) - assert.NoError(t, err) - - // remove the netmon ensuring its version cannot be queried - err = os.Remove(config.NetmonConfig.Path) - assert.NoError(t, err) - - expectedNetmon.Version = unknownVersionInfo - - netmon := getNetmonInfo(config) - assert.NoError(t, err) - - assert.Equal(t, expectedNetmon, netmon) -} - func TestEnvGetAgentInfo(t *testing.T) { tmpdir, err := ioutil.TempDir("", "") if err != nil { @@ -1052,11 +983,13 @@ func TestGetHypervisorInfoSocket(t *testing.T) { {vc.QemuHypervisor, false}, } + config.HypervisorConfig.VMStorePath = "/foo" + config.HypervisorConfig.RunStorePath = "/bar" + for i, details := range hypervisors { msg := fmt.Sprintf("hypervisor[%d]: %+v", i, details) config.HypervisorType = details.hType - info, err := getHypervisorInfo(config) assert.NoError(err, msg) diff --git a/src/runtime/cmd/netmon/netmon.go b/src/runtime/cmd/netmon/netmon.go deleted file mode 100644 index 21798a153..000000000 --- a/src/runtime/cmd/netmon/netmon.go +++ /dev/null @@ -1,683 +0,0 @@ -// Copyright (c) 2018 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package main - -import ( - "encoding/json" - "errors" - "flag" - "fmt" - "io/ioutil" - "log/syslog" - "net" - "os" - "os/exec" - "os/signal" - "path/filepath" - "strings" - "syscall" - "time" - - "github.com/kata-containers/kata-containers/src/runtime/pkg/signals" - pbTypes "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/agent/protocols" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" - - "github.com/sirupsen/logrus" - lSyslog "github.com/sirupsen/logrus/hooks/syslog" - "github.com/vishvananda/netlink" - "golang.org/x/sys/unix" -) - -const ( - netmonName = "kata-netmon" - - kataCmd = "kata-network" - kataCLIAddIfaceCmd = "add-iface" - kataCLIDelIfaceCmd = "del-iface" - kataCLIUpdtRoutesCmd = "update-routes" - - kataSuffix = "kata" - - // sharedFile is the name of the file that will be used to share - // the data between this process and the kata-runtime process - // responsible for updating the network. - sharedFile = "shared.json" - storageFilePerm = os.FileMode(0640) - storageDirPerm = os.FileMode(0750) -) - -var ( - // version is the netmon version. This variable is populated at build time. - version = "unknown" - - // For simplicity the code will only focus on IPv4 addresses for now. - netlinkFamily = netlink.FAMILY_ALL - - storageParentPath = "/var/run/kata-containers/netmon/sbs" -) - -type netmonParams struct { - sandboxID string - runtimePath string - logLevel string - debug bool -} - -type netmon struct { - netIfaces map[int]pbTypes.Interface - - linkUpdateCh chan netlink.LinkUpdate - linkDoneCh chan struct{} - - rtUpdateCh chan netlink.RouteUpdate - rtDoneCh chan struct{} - - netHandler *netlink.Handle - - storagePath string - sharedFile string - - netmonParams -} - -var netmonLog = logrus.New() - -func printVersion() { - fmt.Printf("%s version %s\n", netmonName, version) -} - -const componentDescription = `is a network monitoring process that is intended to be started in the -appropriate network namespace so that it can listen to any event related to -link and routes. Whenever a new interface or route is created/updated, it is -responsible for calling into the kata-runtime CLI to ask for the actual -creation/update of the given interface or route. -` - -func printComponentDescription() { - fmt.Printf("\n%s %s\n", netmonName, componentDescription) -} - -func parseOptions() netmonParams { - var version, help bool - - params := netmonParams{} - - flag.BoolVar(&help, "h", false, "describe component usage") - flag.BoolVar(&help, "help", false, "") - flag.BoolVar(¶ms.debug, "d", false, "enable debug mode") - flag.BoolVar(&version, "v", false, "display program version and exit") - flag.BoolVar(&version, "version", false, "") - flag.StringVar(¶ms.sandboxID, "s", "", "sandbox id (required)") - flag.StringVar(¶ms.runtimePath, "r", "", "runtime path (required)") - flag.StringVar(¶ms.logLevel, "log", "warn", - "log messages above specified level: debug, warn, error, fatal or panic") - - flag.Parse() - - if help { - printComponentDescription() - flag.PrintDefaults() - os.Exit(0) - } - - if version { - printVersion() - os.Exit(0) - } - - if params.sandboxID == "" { - fmt.Fprintf(os.Stderr, "Error: sandbox id is empty, one must be provided\n") - flag.PrintDefaults() - os.Exit(1) - } - - if params.runtimePath == "" { - fmt.Fprintf(os.Stderr, "Error: runtime path is empty, one must be provided\n") - flag.PrintDefaults() - os.Exit(1) - } - - return params -} - -func newNetmon(params netmonParams) (*netmon, error) { - handler, err := netlink.NewHandle(netlinkFamily) - if err != nil { - return nil, err - } - - n := &netmon{ - netmonParams: params, - storagePath: filepath.Join(storageParentPath, params.sandboxID), - sharedFile: filepath.Join(storageParentPath, params.sandboxID, sharedFile), - netIfaces: make(map[int]pbTypes.Interface), - linkUpdateCh: make(chan netlink.LinkUpdate), - linkDoneCh: make(chan struct{}), - rtUpdateCh: make(chan netlink.RouteUpdate), - rtDoneCh: make(chan struct{}), - netHandler: handler, - } - - if err := os.MkdirAll(n.storagePath, storageDirPerm); err != nil { - return nil, err - } - - return n, nil -} - -func (n *netmon) cleanup() { - os.RemoveAll(n.storagePath) - n.netHandler.Close() - close(n.linkDoneCh) - close(n.rtDoneCh) -} - -// setupSignalHandler sets up signal handling, starting a go routine to deal -// with signals as they arrive. -func (n *netmon) setupSignalHandler() { - signals.SetLogger(n.logger()) - - sigCh := make(chan os.Signal, 8) - - for _, sig := range signals.HandledSignals() { - signal.Notify(sigCh, sig) - } - - go func() { - for { - sig := <-sigCh - - nativeSignal, ok := sig.(syscall.Signal) - if !ok { - err := errors.New("unknown signal") - netmonLog.WithError(err).WithField("signal", sig.String()).Error() - continue - } - - if signals.FatalSignal(nativeSignal) { - netmonLog.WithField("signal", sig).Error("received fatal signal") - signals.Die(nil) - } else if n.debug && signals.NonFatalSignal(nativeSignal) { - netmonLog.WithField("signal", sig).Debug("handling signal") - signals.Backtrace() - } - } - }() -} - -func (n *netmon) logger() *logrus.Entry { - fields := logrus.Fields{ - "name": netmonName, - "pid": os.Getpid(), - "source": "netmon", - } - - if n.sandboxID != "" { - fields["sandbox"] = n.sandboxID - } - - return netmonLog.WithFields(fields) -} - -func (n *netmon) setupLogger() error { - level, err := logrus.ParseLevel(n.logLevel) - if err != nil { - return err - } - - netmonLog.SetLevel(level) - - netmonLog.Formatter = &logrus.TextFormatter{TimestampFormat: time.RFC3339Nano} - - hook, err := lSyslog.NewSyslogHook("", "", syslog.LOG_INFO|syslog.LOG_USER, netmonName) - if err != nil { - return err - } - - netmonLog.AddHook(hook) - - announceFields := logrus.Fields{ - "runtime-path": n.runtimePath, - "debug": n.debug, - "log-level": n.logLevel, - } - - n.logger().WithFields(announceFields).Info("announce") - - return nil -} - -func (n *netmon) listenNetlinkEvents() error { - if err := netlink.LinkSubscribe(n.linkUpdateCh, n.linkDoneCh); err != nil { - return err - } - - return netlink.RouteSubscribe(n.rtUpdateCh, n.rtDoneCh) -} - -// convertInterface converts a link and its IP addresses as defined by netlink -// package, into the Interface structure format expected by kata-runtime to -// describe an interface and its associated IP addresses. -func convertInterface(linkAttrs *netlink.LinkAttrs, linkType string, addrs []netlink.Addr) pbTypes.Interface { - if linkAttrs == nil { - netmonLog.Warn("Link attributes are nil") - return pbTypes.Interface{} - } - - var ipAddrs []*pbTypes.IPAddress - - for _, addr := range addrs { - if addr.IPNet == nil { - continue - } - - netMask, _ := addr.Mask.Size() - - ipAddr := &pbTypes.IPAddress{ - Address: addr.IP.String(), - Mask: fmt.Sprintf("%d", netMask), - } - - if addr.IP.To4() != nil { - ipAddr.Family = utils.ConvertNetlinkFamily(netlink.FAMILY_V4) - } else { - ipAddr.Family = utils.ConvertNetlinkFamily(netlink.FAMILY_V6) - } - - ipAddrs = append(ipAddrs, ipAddr) - } - - iface := pbTypes.Interface{ - Device: linkAttrs.Name, - Name: linkAttrs.Name, - IPAddresses: ipAddrs, - Mtu: uint64(linkAttrs.MTU), - HwAddr: linkAttrs.HardwareAddr.String(), - Type: linkType, - } - - netmonLog.WithField("interface", iface).Debug("Interface converted") - - return iface -} - -// convertRoutes converts a list of routes as defined by netlink package, -// into a list of Route structure format expected by kata-runtime to -// describe a set of routes. -func convertRoutes(netRoutes []netlink.Route) []pbTypes.Route { - var routes []pbTypes.Route - - for _, netRoute := range netRoutes { - dst := "" - - if netRoute.Protocol == unix.RTPROT_KERNEL { - continue - } - - if netRoute.Dst != nil { - dst = netRoute.Dst.String() - if netRoute.Dst.IP.To4() != nil || netRoute.Dst.IP.To16() != nil { - dst = netRoute.Dst.String() - } else { - netmonLog.WithField("destination", netRoute.Dst.IP.String()).Warn("Unexpected network address format") - } - } - - src := "" - if netRoute.Src != nil { - if netRoute.Src.To4() != nil || netRoute.Src.To16() != nil { - src = netRoute.Src.String() - } else { - netmonLog.WithField("source", netRoute.Src.String()).Warn("Unexpected network address format") - } - } - - gw := "" - if netRoute.Gw != nil { - if netRoute.Gw.To4() != nil || netRoute.Gw.To16() != nil { - gw = netRoute.Gw.String() - } else { - netmonLog.WithField("gateway", netRoute.Gw.String()).Warn("Unexpected network address format") - } - } - - dev := "" - iface, err := net.InterfaceByIndex(netRoute.LinkIndex) - if err == nil { - dev = iface.Name - } - - route := pbTypes.Route{ - Dest: dst, - Gateway: gw, - Device: dev, - Source: src, - Scope: uint32(netRoute.Scope), - } - - routes = append(routes, route) - } - - netmonLog.WithField("routes", routes).Debug("Routes converted") - - return routes -} - -// scanNetwork lists all the interfaces it can find inside the current -// network namespace, and store them in-memory to keep track of them. -func (n *netmon) scanNetwork() error { - links, err := n.netHandler.LinkList() - if err != nil { - return err - } - - for _, link := range links { - addrs, err := n.netHandler.AddrList(link, netlinkFamily) - if err != nil { - return err - } - - linkAttrs := link.Attrs() - if linkAttrs == nil { - continue - } - - iface := convertInterface(linkAttrs, link.Type(), addrs) - n.netIfaces[linkAttrs.Index] = iface - } - - n.logger().Debug("Network scanned") - - return nil -} - -func (n *netmon) storeDataToSend(data interface{}) error { - // Marshal the data structure into a JSON bytes array. - jsonArray, err := json.Marshal(data) - if err != nil { - return err - } - - // Store the JSON bytes array at the specified path. - return ioutil.WriteFile(n.sharedFile, jsonArray, storageFilePerm) -} - -func (n *netmon) execKataCmd(subCmd string) error { - execCmd := exec.Command(n.runtimePath, kataCmd, subCmd, n.sandboxID, n.sharedFile) - - n.logger().WithField("command", execCmd).Debug("Running runtime command") - - // Make use of Run() to ensure the kata-runtime process has correctly - // terminated before to go further. - if err := execCmd.Run(); err != nil { - return err - } - - // Remove the shared file after the command returned. At this point - // we know the content of the file is not going to be used anymore, - // and the file path can be reused for further commands. - return os.Remove(n.sharedFile) -} - -func (n *netmon) addInterfaceCLI(iface pbTypes.Interface) error { - if err := n.storeDataToSend(iface); err != nil { - return err - } - - return n.execKataCmd(kataCLIAddIfaceCmd) -} - -func (n *netmon) delInterfaceCLI(iface pbTypes.Interface) error { - if err := n.storeDataToSend(iface); err != nil { - return err - } - - return n.execKataCmd(kataCLIDelIfaceCmd) -} - -func (n *netmon) updateRoutesCLI(routes []pbTypes.Route) error { - if err := n.storeDataToSend(routes); err != nil { - return err - } - - return n.execKataCmd(kataCLIUpdtRoutesCmd) -} - -func (n *netmon) updateRoutes() error { - // Get all the routes. - netlinkRoutes, err := n.netHandler.RouteList(nil, netlinkFamily) - if err != nil { - return err - } - - // Translate them into Route structures. - routes := convertRoutes(netlinkRoutes) - - // Update the routes through the Kata CLI. - return n.updateRoutesCLI(routes) -} - -func (n *netmon) handleRTMNewAddr(ev netlink.LinkUpdate) error { - n.logger().Debug("Interface update not supported") - return nil -} - -func (n *netmon) handleRTMDelAddr(ev netlink.LinkUpdate) error { - n.logger().Debug("Interface update not supported") - return nil -} - -func (n *netmon) handleRTMNewLink(ev netlink.LinkUpdate) error { - // NEWLINK might be a lot of different things. We're interested in - // adding the interface (both to our list and by calling into the - // Kata CLI API) only if this has the flags UP and RUNNING, meaning - // we don't expect any further change on the interface, and that we - // are ready to add it. - - linkAttrs := ev.Link.Attrs() - if linkAttrs == nil { - n.logger().Warn("The link attributes are nil") - return nil - } - - // First, ignore if the interface name contains "kata". This way we - // are preventing from adding interfaces created by Kata Containers. - if strings.HasSuffix(linkAttrs.Name, kataSuffix) { - n.logger().Debugf("Ignore the interface %s because found %q", - linkAttrs.Name, kataSuffix) - return nil - } - - // Check if the interface exist in the internal list. - if _, exist := n.netIfaces[int(ev.Index)]; exist { - n.logger().Debugf("Ignoring interface %s because already exist", - linkAttrs.Name) - return nil - } - - // Now, check if the interface has been enabled to UP and RUNNING. - if (ev.Flags&unix.IFF_UP) != unix.IFF_UP || - (ev.Flags&unix.IFF_RUNNING) != unix.IFF_RUNNING { - n.logger().Debugf("Ignore the interface %s because not UP and RUNNING", - linkAttrs.Name) - return nil - } - - // Get the list of IP addresses associated with this interface. - addrs, err := n.netHandler.AddrList(ev.Link, netlinkFamily) - if err != nil { - return err - } - - // Convert the interfaces in the appropriate structure format. - iface := convertInterface(linkAttrs, ev.Link.Type(), addrs) - - // Add the interface through the Kata CLI. - if err := n.addInterfaceCLI(iface); err != nil { - return err - } - - // Add the interface to the internal list. - n.netIfaces[linkAttrs.Index] = iface - - // Complete by updating the routes. - return n.updateRoutes() -} - -func (n *netmon) handleRTMDelLink(ev netlink.LinkUpdate) error { - // It can only delete if identical interface is found in the internal - // list of interfaces. Otherwise, the deletion will be ignored. - linkAttrs := ev.Link.Attrs() - if linkAttrs == nil { - n.logger().Warn("Link attributes are nil") - return nil - } - - // First, ignore if the interface name contains "kata". This way we - // are preventing from deleting interfaces created by Kata Containers. - if strings.Contains(linkAttrs.Name, kataSuffix) { - n.logger().Debugf("Ignore the interface %s because found %q", - linkAttrs.Name, kataSuffix) - return nil - } - - // Check if the interface exist in the internal list. - iface, exist := n.netIfaces[int(ev.Index)] - if !exist { - n.logger().Debugf("Ignoring interface %s because not found", - linkAttrs.Name) - return nil - } - - if err := n.delInterfaceCLI(iface); err != nil { - return err - } - - // Delete the interface from the internal list. - delete(n.netIfaces, linkAttrs.Index) - - // Complete by updating the routes. - return n.updateRoutes() -} - -func (n *netmon) handleRTMNewRoute(ev netlink.RouteUpdate) error { - // Add the route through updateRoutes(), only if the route refer to an - // interface that already exists in the internal list of interfaces. - if _, exist := n.netIfaces[ev.Route.LinkIndex]; !exist { - n.logger().Debugf("Ignoring route %+v since interface %d not found", - ev.Route, ev.Route.LinkIndex) - return nil - } - - return n.updateRoutes() -} - -func (n *netmon) handleRTMDelRoute(ev netlink.RouteUpdate) error { - // Remove the route through updateRoutes(), only if the route refer to - // an interface that already exists in the internal list of interfaces. - return n.updateRoutes() -} - -func (n *netmon) handleLinkEvent(ev netlink.LinkUpdate) error { - n.logger().Debug("handleLinkEvent: netlink event received") - - switch ev.Header.Type { - case unix.NLMSG_DONE: - n.logger().Debug("NLMSG_DONE") - return nil - case unix.NLMSG_ERROR: - n.logger().Error("NLMSG_ERROR") - return fmt.Errorf("Error while listening on netlink socket") - case unix.RTM_NEWADDR: - n.logger().Debug("RTM_NEWADDR") - return n.handleRTMNewAddr(ev) - case unix.RTM_DELADDR: - n.logger().Debug("RTM_DELADDR") - return n.handleRTMDelAddr(ev) - case unix.RTM_NEWLINK: - n.logger().Debug("RTM_NEWLINK") - return n.handleRTMNewLink(ev) - case unix.RTM_DELLINK: - n.logger().Debug("RTM_DELLINK") - return n.handleRTMDelLink(ev) - default: - n.logger().Warnf("Unknown msg type %v", ev.Header.Type) - } - - return nil -} - -func (n *netmon) handleRouteEvent(ev netlink.RouteUpdate) error { - n.logger().Debug("handleRouteEvent: netlink event received") - - switch ev.Type { - case unix.RTM_NEWROUTE: - n.logger().Debug("RTM_NEWROUTE") - return n.handleRTMNewRoute(ev) - case unix.RTM_DELROUTE: - n.logger().Debug("RTM_DELROUTE") - return n.handleRTMDelRoute(ev) - default: - n.logger().Warnf("Unknown msg type %v", ev.Type) - } - - return nil -} - -func (n *netmon) handleEvents() (err error) { - for { - select { - case ev := <-n.linkUpdateCh: - if err = n.handleLinkEvent(ev); err != nil { - return err - } - case ev := <-n.rtUpdateCh: - if err = n.handleRouteEvent(ev); err != nil { - return err - } - } - } -} - -func main() { - // Parse parameters. - params := parseOptions() - - // Create netmon handler. - n, err := newNetmon(params) - if err != nil { - netmonLog.WithError(err).Fatal("newNetmon()") - os.Exit(1) - } - defer n.cleanup() - - // Init logger. - if err := n.setupLogger(); err != nil { - netmonLog.WithError(err).Fatal("setupLogger()") - os.Exit(1) - } - - // Setup signal handlers - n.setupSignalHandler() - - // Scan the current interfaces. - if err := n.scanNetwork(); err != nil { - n.logger().WithError(err).Fatal("scanNetwork()") - os.Exit(1) - } - - // Subscribe to the link listener. - if err := n.listenNetlinkEvents(); err != nil { - n.logger().WithError(err).Fatal("listenNetlinkEvents()") - os.Exit(1) - } - - // Go into the main loop. - if err := n.handleEvents(); err != nil { - n.logger().WithError(err).Fatal("handleEvents()") - os.Exit(1) - } -} diff --git a/src/runtime/cmd/netmon/netmon_test.go b/src/runtime/cmd/netmon/netmon_test.go deleted file mode 100644 index 7e059c737..000000000 --- a/src/runtime/cmd/netmon/netmon_test.go +++ /dev/null @@ -1,701 +0,0 @@ -// Copyright (c) 2018 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package main - -import ( - "encoding/json" - "fmt" - "io/ioutil" - "net" - "os" - "os/exec" - "path/filepath" - "reflect" - "runtime" - "testing" - - ktu "github.com/kata-containers/kata-containers/src/runtime/pkg/katatestutils" - pbTypes "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/agent/protocols" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" - - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" - "github.com/vishvananda/netlink" - "github.com/vishvananda/netns" - "golang.org/x/sys/unix" -) - -const ( - testSandboxID = "123456789" - testRuntimePath = "/foo/bar/test-runtime" - testLogLevel = "info" - testStorageParentPath = "/tmp/netmon" - testSharedFile = "foo-shared.json" - testWrongNetlinkFamily = -1 - testIfaceName = "test_eth0" - testMTU = 12345 - testHwAddr = "02:00:ca:fe:00:48" - testIPAddress = "192.168.0.15" - testIPAddressWithMask = "192.168.0.15/32" - testIP6Address = "2001:db8:1::242:ac11:2" - testIP6AddressWithMask = "2001:db8:1::/64" - testScope = 1 - testTxQLen = -1 - testIfaceIndex = 5 -) - -func skipUnlessRoot(t *testing.T) { - tc := ktu.NewTestConstraint(false) - - if tc.NotValid(ktu.NeedRoot()) { - t.Skip("Test disabled as requires root user") - } -} - -func TestNewNetmon(t *testing.T) { - skipUnlessRoot(t) - - // Override storageParentPath - savedStorageParentPath := storageParentPath - storageParentPath = testStorageParentPath - defer func() { - storageParentPath = savedStorageParentPath - }() - - params := netmonParams{ - sandboxID: testSandboxID, - runtimePath: testRuntimePath, - debug: true, - logLevel: testLogLevel, - } - - expected := &netmon{ - netmonParams: params, - storagePath: filepath.Join(storageParentPath, params.sandboxID), - sharedFile: filepath.Join(storageParentPath, params.sandboxID, sharedFile), - } - - os.RemoveAll(expected.storagePath) - - got, err := newNetmon(params) - assert.Nil(t, err) - assert.True(t, reflect.DeepEqual(expected.netmonParams, got.netmonParams), - "Got %+v\nExpected %+v", got.netmonParams, expected.netmonParams) - assert.True(t, reflect.DeepEqual(expected.storagePath, got.storagePath), - "Got %+v\nExpected %+v", got.storagePath, expected.storagePath) - assert.True(t, reflect.DeepEqual(expected.sharedFile, got.sharedFile), - "Got %+v\nExpected %+v", got.sharedFile, expected.sharedFile) - - _, err = os.Stat(got.storagePath) - assert.Nil(t, err) - - os.RemoveAll(got.storagePath) -} - -func TestNewNetmonErrorWrongFamilyType(t *testing.T) { - // Override netlinkFamily - savedNetlinkFamily := netlinkFamily - netlinkFamily = testWrongNetlinkFamily - defer func() { - netlinkFamily = savedNetlinkFamily - }() - - n, err := newNetmon(netmonParams{}) - assert.NotNil(t, err) - assert.Nil(t, n) -} - -func TestCleanup(t *testing.T) { - skipUnlessRoot(t) - - // Override storageParentPath - savedStorageParentPath := storageParentPath - storageParentPath = testStorageParentPath - defer func() { - storageParentPath = savedStorageParentPath - }() - - handler, err := netlink.NewHandle(netlinkFamily) - assert.Nil(t, err) - - n := &netmon{ - storagePath: filepath.Join(storageParentPath, testSandboxID), - linkDoneCh: make(chan struct{}), - rtDoneCh: make(chan struct{}), - netHandler: handler, - } - - err = os.MkdirAll(n.storagePath, storageDirPerm) - assert.Nil(t, err) - _, err = os.Stat(n.storagePath) - assert.Nil(t, err) - - n.cleanup() - - _, err = os.Stat(n.storagePath) - assert.NotNil(t, err) - _, ok := (<-n.linkDoneCh) - assert.False(t, ok) - _, ok = (<-n.rtDoneCh) - assert.False(t, ok) -} - -func TestLogger(t *testing.T) { - fields := logrus.Fields{ - "name": netmonName, - "pid": os.Getpid(), - "source": "netmon", - "sandbox": testSandboxID, - } - - expected := netmonLog.WithFields(fields) - - n := &netmon{ - netmonParams: netmonParams{ - sandboxID: testSandboxID, - }, - } - - got := n.logger() - assert.True(t, reflect.DeepEqual(*expected, *got), - "Got %+v\nExpected %+v", *got, *expected) -} - -func TestConvertInterface(t *testing.T) { - hwAddr, err := net.ParseMAC(testHwAddr) - assert.Nil(t, err) - - addrs := []netlink.Addr{ - { - IPNet: &net.IPNet{ - IP: net.ParseIP(testIPAddress), - }, - }, - { - IPNet: &net.IPNet{ - IP: net.ParseIP(testIP6Address), - }, - }, - } - - linkAttrs := &netlink.LinkAttrs{ - Name: testIfaceName, - MTU: testMTU, - HardwareAddr: hwAddr, - } - - linkType := "link_type_test" - - expected := pbTypes.Interface{ - Device: testIfaceName, - Name: testIfaceName, - Mtu: uint64(testMTU), - HwAddr: testHwAddr, - IPAddresses: []*pbTypes.IPAddress{ - { - Family: utils.ConvertNetlinkFamily(netlink.FAMILY_V4), - Address: testIPAddress, - Mask: "0", - }, - { - Family: utils.ConvertNetlinkFamily(netlink.FAMILY_V6), - Address: testIP6Address, - Mask: "0", - }, - }, - Type: linkType, - } - - got := convertInterface(linkAttrs, linkType, addrs) - - assert.True(t, reflect.DeepEqual(expected, got), - "Got %+v\nExpected %+v", got, expected) -} - -func TestConvertRoutes(t *testing.T) { - ip, ipNet, err := net.ParseCIDR(testIPAddressWithMask) - assert.Nil(t, err) - assert.NotNil(t, ipNet) - - _, ip6Net, err := net.ParseCIDR(testIP6AddressWithMask) - assert.Nil(t, err) - assert.NotNil(t, ipNet) - - routes := []netlink.Route{ - { - Dst: ipNet, - Src: ip, - Gw: ip, - LinkIndex: -1, - Scope: testScope, - }, - { - Dst: ip6Net, - Src: nil, - Gw: nil, - LinkIndex: -1, - Scope: testScope, - }, - } - - expected := []pbTypes.Route{ - { - Dest: testIPAddressWithMask, - Gateway: testIPAddress, - Source: testIPAddress, - Scope: uint32(testScope), - }, - { - Dest: testIP6AddressWithMask, - Gateway: "", - Source: "", - Scope: uint32(testScope), - }, - } - - got := convertRoutes(routes) - assert.True(t, reflect.DeepEqual(expected, got), - "Got %+v\nExpected %+v", got, expected) -} - -type testTeardownNetwork func() - -func testSetupNetwork(t *testing.T) testTeardownNetwork { - skipUnlessRoot(t) - - // new temporary namespace so we don't pollute the host - // lock thread since the namespace is thread local - runtime.LockOSThread() - var err error - ns, err := netns.New() - if err != nil { - t.Fatal("Failed to create newns", ns) - } - - return func() { - ns.Close() - runtime.UnlockOSThread() - } -} - -func testCreateDummyNetwork(t *testing.T, handler *netlink.Handle) (int, pbTypes.Interface) { - hwAddr, err := net.ParseMAC(testHwAddr) - assert.Nil(t, err) - - link := &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - MTU: testMTU, - TxQLen: testTxQLen, - Name: testIfaceName, - HardwareAddr: hwAddr, - }, - } - - err = handler.LinkAdd(link) - assert.Nil(t, err) - err = handler.LinkSetUp(link) - assert.Nil(t, err) - - attrs := link.Attrs() - assert.NotNil(t, attrs) - - addrs, err := handler.AddrList(link, netlinkFamily) - assert.Nil(t, err) - - var ipAddrs []*pbTypes.IPAddress - - // Scan addresses for ipv6 link local address which is automatically assigned - for _, addr := range addrs { - if addr.IPNet == nil { - continue - } - - netMask, _ := addr.Mask.Size() - - ipAddr := &pbTypes.IPAddress{ - Address: addr.IP.String(), - Mask: fmt.Sprintf("%d", netMask), - } - - if addr.IP.To4() != nil { - ipAddr.Family = utils.ConvertNetlinkFamily(netlink.FAMILY_V4) - } else { - ipAddr.Family = utils.ConvertNetlinkFamily(netlink.FAMILY_V6) - } - - ipAddrs = append(ipAddrs, ipAddr) - } - - iface := pbTypes.Interface{ - Device: testIfaceName, - Name: testIfaceName, - Mtu: uint64(testMTU), - HwAddr: testHwAddr, - Type: link.Type(), - IPAddresses: ipAddrs, - } - - return attrs.Index, iface -} - -func TestScanNetwork(t *testing.T) { - tearDownNetworkCb := testSetupNetwork(t) - defer tearDownNetworkCb() - - handler, err := netlink.NewHandle(netlinkFamily) - assert.Nil(t, err) - assert.NotNil(t, handler) - defer handler.Close() - - idx, expected := testCreateDummyNetwork(t, handler) - - n := &netmon{ - netIfaces: make(map[int]pbTypes.Interface), - netHandler: handler, - } - - err = n.scanNetwork() - assert.Nil(t, err) - assert.True(t, reflect.DeepEqual(expected, n.netIfaces[idx]), - "Got %+v\nExpected %+v", n.netIfaces[idx], expected) -} - -func TestStoreDataToSend(t *testing.T) { - var got pbTypes.Interface - - expected := pbTypes.Interface{ - Device: testIfaceName, - Name: testIfaceName, - Mtu: uint64(testMTU), - HwAddr: testHwAddr, - } - - n := &netmon{ - sharedFile: filepath.Join(testStorageParentPath, testSharedFile), - } - - err := os.MkdirAll(testStorageParentPath, storageDirPerm) - defer os.RemoveAll(testStorageParentPath) - assert.Nil(t, err) - - err = n.storeDataToSend(expected) - assert.Nil(t, err) - - // Check the file has been created, check the content, and delete it. - _, err = os.Stat(n.sharedFile) - assert.Nil(t, err) - byteArray, err := ioutil.ReadFile(n.sharedFile) - assert.Nil(t, err) - err = json.Unmarshal(byteArray, &got) - assert.Nil(t, err) - assert.True(t, reflect.DeepEqual(expected, got), - "Got %+v\nExpected %+v", got, expected) -} - -func TestExecKataCmdSuccess(t *testing.T) { - trueBinPath, err := exec.LookPath("true") - assert.Nil(t, err) - assert.NotEmpty(t, trueBinPath) - - params := netmonParams{ - runtimePath: trueBinPath, - } - - n := &netmon{ - netmonParams: params, - sharedFile: filepath.Join(testStorageParentPath, testSharedFile), - } - - err = os.MkdirAll(testStorageParentPath, storageDirPerm) - assert.Nil(t, err) - defer os.RemoveAll(testStorageParentPath) - - file, err := os.Create(n.sharedFile) - assert.Nil(t, err) - assert.NotNil(t, file) - file.Close() - - _, err = os.Stat(n.sharedFile) - assert.Nil(t, err) - - err = n.execKataCmd("") - assert.Nil(t, err) - _, err = os.Stat(n.sharedFile) - assert.NotNil(t, err) -} - -func TestExecKataCmdFailure(t *testing.T) { - falseBinPath, err := exec.LookPath("false") - assert.Nil(t, err) - assert.NotEmpty(t, falseBinPath) - - params := netmonParams{ - runtimePath: falseBinPath, - } - - n := &netmon{ - netmonParams: params, - } - - err = n.execKataCmd("") - assert.NotNil(t, err) -} - -func TestActionsCLI(t *testing.T) { - trueBinPath, err := exec.LookPath("true") - assert.Nil(t, err) - assert.NotEmpty(t, trueBinPath) - - params := netmonParams{ - runtimePath: trueBinPath, - } - - n := &netmon{ - netmonParams: params, - sharedFile: filepath.Join(testStorageParentPath, testSharedFile), - } - - err = os.MkdirAll(testStorageParentPath, storageDirPerm) - assert.Nil(t, err) - defer os.RemoveAll(testStorageParentPath) - - // Test addInterfaceCLI - err = n.addInterfaceCLI(pbTypes.Interface{}) - assert.Nil(t, err) - - // Test delInterfaceCLI - err = n.delInterfaceCLI(pbTypes.Interface{}) - assert.Nil(t, err) - - // Test updateRoutesCLI - err = n.updateRoutesCLI([]pbTypes.Route{}) - assert.Nil(t, err) - - tearDownNetworkCb := testSetupNetwork(t) - defer tearDownNetworkCb() - - handler, err := netlink.NewHandle(netlinkFamily) - assert.Nil(t, err) - assert.NotNil(t, handler) - defer handler.Close() - - n.netHandler = handler - - // Test updateRoutes - err = n.updateRoutes() - assert.Nil(t, err) - - // Test handleRTMDelRoute - err = n.handleRTMDelRoute(netlink.RouteUpdate{}) - assert.Nil(t, err) -} - -func TestHandleRTMNewAddr(t *testing.T) { - n := &netmon{} - - err := n.handleRTMNewAddr(netlink.LinkUpdate{}) - assert.Nil(t, err) -} - -func TestHandleRTMDelAddr(t *testing.T) { - n := &netmon{} - - err := n.handleRTMDelAddr(netlink.LinkUpdate{}) - assert.Nil(t, err) -} - -func TestHandleRTMNewLink(t *testing.T) { - n := &netmon{} - ev := netlink.LinkUpdate{ - Link: &netlink.Dummy{}, - } - - // LinkAttrs is nil - err := n.handleRTMNewLink(ev) - assert.Nil(t, err) - - // Link name contains "kata" suffix - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo_kata", - }, - }, - } - err = n.handleRTMNewLink(ev) - assert.Nil(t, err) - - // Interface already exist in list - n.netIfaces = make(map[int]pbTypes.Interface) - n.netIfaces[testIfaceIndex] = pbTypes.Interface{} - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo0", - }, - }, - } - ev.Index = testIfaceIndex - err = n.handleRTMNewLink(ev) - assert.Nil(t, err) - - // Flags are not up and running - n.netIfaces = make(map[int]pbTypes.Interface) - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo0", - }, - }, - } - ev.Index = testIfaceIndex - err = n.handleRTMNewLink(ev) - assert.Nil(t, err) - - // Invalid link - n.netIfaces = make(map[int]pbTypes.Interface) - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo0", - }, - }, - } - ev.Index = testIfaceIndex - ev.Flags = unix.IFF_UP | unix.IFF_RUNNING - handler, err := netlink.NewHandle(netlinkFamily) - assert.Nil(t, err) - assert.NotNil(t, handler) - defer handler.Close() - n.netHandler = handler - err = n.handleRTMNewLink(ev) - assert.NotNil(t, err) -} - -func TestHandleRTMDelLink(t *testing.T) { - n := &netmon{} - ev := netlink.LinkUpdate{ - Link: &netlink.Dummy{}, - } - - // LinkAttrs is nil - err := n.handleRTMDelLink(ev) - assert.Nil(t, err) - - // Link name contains "kata" suffix - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo_kata", - }, - }, - } - err = n.handleRTMDelLink(ev) - assert.Nil(t, err) - - // Interface does not exist in list - n.netIfaces = make(map[int]pbTypes.Interface) - ev = netlink.LinkUpdate{ - Link: &netlink.Dummy{ - LinkAttrs: netlink.LinkAttrs{ - Name: "foo0", - }, - }, - } - ev.Index = testIfaceIndex - err = n.handleRTMDelLink(ev) - assert.Nil(t, err) -} - -func TestHandleRTMNewRouteIfaceNotFound(t *testing.T) { - n := &netmon{ - netIfaces: make(map[int]pbTypes.Interface), - } - - err := n.handleRTMNewRoute(netlink.RouteUpdate{}) - assert.Nil(t, err) -} - -func TestHandleLinkEvent(t *testing.T) { - n := &netmon{} - ev := netlink.LinkUpdate{} - - // Unknown event - err := n.handleLinkEvent(ev) - assert.Nil(t, err) - - // DONE event - ev.Header.Type = unix.NLMSG_DONE - err = n.handleLinkEvent(ev) - assert.Nil(t, err) - - // ERROR event - ev.Header.Type = unix.NLMSG_ERROR - err = n.handleLinkEvent(ev) - assert.NotNil(t, err) - - // NEWADDR event - ev.Header.Type = unix.RTM_NEWADDR - err = n.handleLinkEvent(ev) - assert.Nil(t, err) - - // DELADDR event - ev.Header.Type = unix.RTM_DELADDR - err = n.handleLinkEvent(ev) - assert.Nil(t, err) - - // NEWLINK event - ev.Header.Type = unix.RTM_NEWLINK - ev.Link = &netlink.Dummy{} - err = n.handleLinkEvent(ev) - assert.Nil(t, err) - - // DELLINK event - ev.Header.Type = unix.RTM_DELLINK - ev.Link = &netlink.Dummy{} - err = n.handleLinkEvent(ev) - assert.Nil(t, err) -} - -func TestHandleRouteEvent(t *testing.T) { - n := &netmon{} - ev := netlink.RouteUpdate{} - - // Unknown event - err := n.handleRouteEvent(ev) - assert.Nil(t, err) - - // RTM_NEWROUTE event - ev.Type = unix.RTM_NEWROUTE - err = n.handleRouteEvent(ev) - assert.Nil(t, err) - - trueBinPath, err := exec.LookPath("true") - assert.Nil(t, err) - assert.NotEmpty(t, trueBinPath) - - n.runtimePath = trueBinPath - n.sharedFile = filepath.Join(testStorageParentPath, testSharedFile) - - err = os.MkdirAll(testStorageParentPath, storageDirPerm) - assert.Nil(t, err) - defer os.RemoveAll(testStorageParentPath) - - tearDownNetworkCb := testSetupNetwork(t) - defer tearDownNetworkCb() - - handler, err := netlink.NewHandle(netlinkFamily) - assert.Nil(t, err) - assert.NotNil(t, handler) - defer handler.Close() - - n.netHandler = handler - - // RTM_DELROUTE event - ev.Type = unix.RTM_DELROUTE - err = n.handleRouteEvent(ev) - assert.Nil(t, err) -} diff --git a/src/runtime/config/configuration-acrn.toml.in b/src/runtime/config/configuration-acrn.toml.in index 69b203703..f9fb39e23 100644 --- a/src/runtime/config/configuration-acrn.toml.in +++ b/src/runtime/config/configuration-acrn.toml.in @@ -147,21 +147,6 @@ block_device_driver = "@DEFBLOCKSTORAGEDRIVER_ACRN@" # (default: 30) #dial_timeout = 30 -[netmon] -# If enabled, the network monitoring process gets started when the -# sandbox is created. This allows for the detection of some additional -# network being added to the existing network namespace, after the -# sandbox has been created. -# (default: disabled) -#enable_netmon = true - -# Specify the path to the netmon binary. -path = "@NETMONPATH@" - -# If enabled, netmon messages will be sent to the system log -# (default: disabled) -#enable_debug = true - [runtime] # If enabled, the runtime will log additional debug messages to the # system log @@ -217,7 +202,6 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If enabled, the runtime will not create a network namespace for shim and hypervisor processes. # This option may have some potential impacts to your host. It should only be used when you know what you're doing. -# `disable_new_netns` conflicts with `enable_netmon` # `disable_new_netns` conflicts with `internetworking_model=bridged` and `internetworking_model=macvtap`. It works only # with `internetworking_model=none`. The tap device will be in the host network namespace and can connect to a bridge # (like OVS) directly. diff --git a/src/runtime/config/configuration-clh.toml.in b/src/runtime/config/configuration-clh.toml.in index 2dc8a8322..a016e0975 100644 --- a/src/runtime/config/configuration-clh.toml.in +++ b/src/runtime/config/configuration-clh.toml.in @@ -169,22 +169,6 @@ block_device_driver = "virtio-blk" # (default: 30) #dial_timeout = 30 -[netmon] -# If enabled, the network monitoring process gets started when the -# sandbox is created. This allows for the detection of some additional -# network being added to the existing network namespace, after the -# sandbox has been created. -# (default: disabled) -#enable_netmon = true - -# Specify the path to the netmon binary. -path = "@NETMONPATH@" - -# If enabled, netmon messages will be sent to the system log -# (default: disabled) -#enable_debug = true - - [runtime] # If enabled, the runtime will log additional debug messages to the # system log @@ -240,7 +224,6 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If enabled, the runtime will not create a network namespace for shim and hypervisor processes. # This option may have some potential impacts to your host. It should only be used when you know what you're doing. -# `disable_new_netns` conflicts with `enable_netmon` # `disable_new_netns` conflicts with `internetworking_model=bridged` and `internetworking_model=macvtap`. It works only # with `internetworking_model=none`. The tap device will be in the host network namespace and can connect to a bridge # (like OVS) directly. diff --git a/src/runtime/config/configuration-fc.toml.in b/src/runtime/config/configuration-fc.toml.in index d03e452f1..8454a0a3d 100644 --- a/src/runtime/config/configuration-fc.toml.in +++ b/src/runtime/config/configuration-fc.toml.in @@ -282,21 +282,6 @@ kernel_modules=[] # (default: 30) #dial_timeout = 30 -[netmon] -# If enabled, the network monitoring process gets started when the -# sandbox is created. This allows for the detection of some additional -# network being added to the existing network namespace, after the -# sandbox has been created. -# (default: disabled) -#enable_netmon = true - -# Specify the path to the netmon binary. -path = "@NETMONPATH@" - -# If enabled, netmon messages will be sent to the system log -# (default: disabled) -#enable_debug = true - [runtime] # If enabled, the runtime will log additional debug messages to the # system log @@ -345,7 +330,6 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If enabled, the runtime will not create a network namespace for shim and hypervisor processes. # This option may have some potential impacts to your host. It should only be used when you know what you're doing. -# `disable_new_netns` conflicts with `enable_netmon` # `disable_new_netns` conflicts with `internetworking_model=tcfilter` and `internetworking_model=macvtap`. It works only # with `internetworking_model=none`. The tap device will be in the host network namespace and can connect to a bridge # (like OVS) directly. diff --git a/src/runtime/config/configuration-qemu.toml.in b/src/runtime/config/configuration-qemu.toml.in index 5bc8e9cc3..883dc6789 100644 --- a/src/runtime/config/configuration-qemu.toml.in +++ b/src/runtime/config/configuration-qemu.toml.in @@ -458,21 +458,6 @@ kernel_modules=[] # (default: 30) #dial_timeout = 30 -[netmon] -# If enabled, the network monitoring process gets started when the -# sandbox is created. This allows for the detection of some additional -# network being added to the existing network namespace, after the -# sandbox has been created. -# (default: disabled) -#enable_netmon = true - -# Specify the path to the netmon binary. -path = "@NETMONPATH@" - -# If enabled, netmon messages will be sent to the system log -# (default: disabled) -#enable_debug = true - [runtime] # If enabled, the runtime will log additional debug messages to the # system log @@ -521,7 +506,6 @@ disable_guest_seccomp=@DEFDISABLEGUESTSECCOMP@ # If enabled, the runtime will not create a network namespace for shim and hypervisor processes. # This option may have some potential impacts to your host. It should only be used when you know what you're doing. -# `disable_new_netns` conflicts with `enable_netmon` # `disable_new_netns` conflicts with `internetworking_model=tcfilter` and `internetworking_model=macvtap`. It works only # with `internetworking_model=none`. The tap device will be in the host network namespace and can connect to a bridge # (like OVS) directly. diff --git a/src/runtime/pkg/containerd-shim-v2/create_test.go b/src/runtime/pkg/containerd-shim-v2/create_test.go index 5ad96f149..7b6f9f57a 100644 --- a/src/runtime/pkg/containerd-shim-v2/create_test.go +++ b/src/runtime/pkg/containerd-shim-v2/create_test.go @@ -328,7 +328,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config string, err err kernelParams := "foo=bar xyz" imagePath := path.Join(dir, "image") shimPath := path.Join(dir, "shim") - netmonPath := path.Join(dir, "netmon") logDir := path.Join(dir, "logs") logPath := path.Join(logDir, "runtime.log") machineType := "machineType" @@ -349,7 +348,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config string, err err KernelParams: kernelParams, MachineType: machineType, ShimPath: shimPath, - NetmonPath: netmonPath, LogPath: logPath, DisableBlock: disableBlockDevice, BlockDeviceDriver: blockDeviceDriver, diff --git a/src/runtime/pkg/containerd-shim-v2/service.go b/src/runtime/pkg/containerd-shim-v2/service.go index 29cd259f9..cafbb1907 100644 --- a/src/runtime/pkg/containerd-shim-v2/service.go +++ b/src/runtime/pkg/containerd-shim-v2/service.go @@ -953,6 +953,12 @@ func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (_ * // exited when shimv2 terminated. Thus here to do the last cleanup of the hypervisor. syscall.Kill(int(s.hpid), syscall.SIGKILL) + // os.Exit() will terminate program immediately, the defer functions won't be executed, + // so we add defer functions again before os.Exit(). + // Refer to https://pkg.go.dev/os#Exit + shimLog.WithField("container", r.ID).Debug("Shutdown() end") + rpcDurationsHistogram.WithLabelValues("shutdown").Observe(float64(time.Since(start).Nanoseconds() / int64(time.Millisecond))) + os.Exit(0) // This will never be called, but this is only there to make sure the diff --git a/src/runtime/virtcontainers/persist/api/hypervisor.go b/src/runtime/pkg/hypervisors/hypervisor_state.go similarity index 98% rename from src/runtime/virtcontainers/persist/api/hypervisor.go rename to src/runtime/pkg/hypervisors/hypervisor_state.go index b2d41000f..6dae6222c 100644 --- a/src/runtime/virtcontainers/persist/api/hypervisor.go +++ b/src/runtime/pkg/hypervisors/hypervisor_state.go @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -package persistapi +package hypervisors // Bridge is a bridge where devices can be hot plugged type Bridge struct { diff --git a/src/runtime/pkg/katatestutils/utils.go b/src/runtime/pkg/katatestutils/utils.go index 617943a03..c5d79e6c4 100644 --- a/src/runtime/pkg/katatestutils/utils.go +++ b/src/runtime/pkg/katatestutils/utils.go @@ -215,7 +215,6 @@ type RuntimeConfigOptions struct { KernelParams string MachineType string ShimPath string - NetmonPath string LogPath string BlockDeviceDriver string SharedFS string @@ -237,7 +236,6 @@ type RuntimeConfigOptions struct { RuntimeDebug bool RuntimeTrace bool ShimDebug bool - NetmonDebug bool AgentDebug bool AgentTrace bool EnablePprof bool @@ -331,10 +329,6 @@ func MakeRuntimeConfigFileData(config RuntimeConfigOptions) string { enable_debug = ` + strconv.FormatBool(config.AgentDebug) + ` enable_tracing = ` + strconv.FormatBool(config.AgentTrace) + ` - [netmon] - path = "` + config.NetmonPath + `" - enable_debug = ` + strconv.FormatBool(config.NetmonDebug) + ` - [runtime] enable_debug = ` + strconv.FormatBool(config.RuntimeDebug) + ` enable_tracing = ` + strconv.FormatBool(config.RuntimeTrace) + ` diff --git a/src/runtime/pkg/katautils/config-settings.go.in b/src/runtime/pkg/katautils/config-settings.go.in index bd793e23f..8f2ae6bfd 100644 --- a/src/runtime/pkg/katautils/config-settings.go.in +++ b/src/runtime/pkg/katautils/config-settings.go.in @@ -97,5 +97,3 @@ const defaultVMCacheEndpoint string = "/var/run/kata-containers/cache.sock" // Default config file used by stateless systems. var defaultRuntimeConfiguration = "@CONFIG_PATH@" - -var defaultNetmonPath = "/usr/libexec/kata-containers/kata-netmon" diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 481ea5aa4..d93a54012 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -56,7 +56,6 @@ type tomlConfig struct { Agent map[string]agent Runtime runtime Image image - Netmon netmon Factory factory } @@ -162,12 +161,6 @@ type agent struct { DialTimeout uint32 `toml:"dial_timeout"` } -type netmon struct { - Path string `toml:"path"` - Debug bool `toml:"enable_debug"` - Enable bool `toml:"enable_netmon"` -} - func (h hypervisor) path() (string, error) { p := h.Path @@ -506,22 +499,6 @@ func (a agent) kernelModules() []string { return a.KernelModules } -func (n netmon) enable() bool { - return n.Enable -} - -func (n netmon) path() string { - if n.Path == "" { - return defaultNetmonPath - } - - return n.Path -} - -func (n netmon) debug() bool { - return n.Debug -} - func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { hypervisor, err := h.path() if err != nil { @@ -1014,12 +991,6 @@ func updateRuntimeConfig(configPath string, tomlConf tomlConfig, config *oci.Run } config.FactoryConfig = fConfig - config.NetmonConfig = vc.NetmonConfig{ - Path: tomlConf.Netmon.path(), - Debug: tomlConf.Netmon.debug(), - Enable: tomlConf.Netmon.enable(), - } - err = SetKernelParams(config) if err != nil { return err @@ -1263,9 +1234,6 @@ func checkConfig(config oci.RuntimeConfig) error { // Because it is an expert option and conflicts with some other common configs. func checkNetNsConfig(config oci.RuntimeConfig) error { if config.DisableNewNetNs { - if config.NetmonConfig.Enable { - return fmt.Errorf("config disable_new_netns conflicts with enable_netmon") - } if config.InterNetworkModel != vc.NetXConnectNoneModel { return fmt.Errorf("config disable_new_netns only works with 'none' internetworking_model") } diff --git a/src/runtime/pkg/katautils/config_test.go b/src/runtime/pkg/katautils/config_test.go index acf92efec..0c596d84e 100644 --- a/src/runtime/pkg/katautils/config_test.go +++ b/src/runtime/pkg/katautils/config_test.go @@ -29,7 +29,6 @@ var ( hypervisorDebug = false runtimeDebug = false runtimeTrace = false - netmonDebug = false agentDebug = false agentTrace = false enablePprof = true @@ -74,7 +73,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf kernelPath := path.Join(dir, "kernel") kernelParams := "foo=bar xyz" imagePath := path.Join(dir, "image") - netmonPath := path.Join(dir, "netmon") logDir := path.Join(dir, "logs") logPath := path.Join(logDir, "runtime.log") machineType := "machineType" @@ -95,7 +93,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf ImagePath: imagePath, KernelParams: kernelParams, MachineType: machineType, - NetmonPath: netmonPath, LogPath: logPath, DefaultGuestHookPath: defaultGuestHookPath, DisableBlock: disableBlockDevice, @@ -111,7 +108,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf HypervisorDebug: hypervisorDebug, RuntimeDebug: runtimeDebug, RuntimeTrace: runtimeTrace, - NetmonDebug: netmonDebug, AgentDebug: agentDebug, AgentTrace: agentTrace, SharedFS: sharedFS, @@ -180,12 +176,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf LongLiveConn: true, } - netmonConfig := vc.NetmonConfig{ - Path: netmonPath, - Debug: false, - Enable: false, - } - factoryConfig := oci.FactoryConfig{ TemplatePath: defaultTemplatePath, VMCacheEndpoint: defaultVMCacheEndpoint, @@ -197,7 +187,6 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf AgentConfig: agentConfig, - NetmonConfig: netmonConfig, DisableNewNetNs: disableNewNetNs, EnablePprof: enablePprof, JaegerEndpoint: jaegerEndpoint, @@ -493,7 +482,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { defaultHypervisorPath = hypervisorPath jailerPath := path.Join(dir, "jailer") defaultJailerPath = jailerPath - netmonPath := path.Join(dir, "netmon") imagePath := path.Join(dir, "image.img") initrdPath := path.Join(dir, "initrd.img") @@ -535,8 +523,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { [agent.kata] debug_console_enabled=true kernel_modules=["a", "b", "c"] - [netmon] - path = "` + netmonPath + `" ` orgVHostVSockDevicePath := utils.VHostVSockDevicePath @@ -561,11 +547,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { t.Error(err) } - err = createEmptyFile(netmonPath) - if err != nil { - t.Error(err) - } - _, config, err := LoadConfiguration(configPath, false) if err != nil { t.Fatal(err) @@ -597,12 +578,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { KernelModules: []string{"a", "b", "c"}, } - expectedNetmonConfig := vc.NetmonConfig{ - Path: netmonPath, - Debug: false, - Enable: false, - } - expectedFactoryConfig := oci.FactoryConfig{ TemplatePath: defaultTemplatePath, VMCacheEndpoint: defaultVMCacheEndpoint, @@ -614,8 +589,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { AgentConfig: expectedAgentConfig, - NetmonConfig: expectedNetmonConfig, - FactoryConfig: expectedFactoryConfig, } err = SetKernelParams(&expectedConfig) @@ -1553,9 +1526,6 @@ func TestCheckNetNsConfig(t *testing.T) { config := oci.RuntimeConfig{ DisableNewNetNs: true, - NetmonConfig: vc.NetmonConfig{ - Enable: true, - }, } err := checkNetNsConfig(config) assert.Error(err) diff --git a/src/runtime/pkg/katautils/create.go b/src/runtime/pkg/katautils/create.go index fc593c671..dd7056214 100644 --- a/src/runtime/pkg/katautils/create.go +++ b/src/runtime/pkg/katautils/create.go @@ -120,6 +120,9 @@ func CreateSandbox(ctx context.Context, vci vc.VC, ociSpec specs.Spec, runtimeCo return nil, vc.Process{}, err } + // setup shared path in hypervisor config: + sandboxConfig.HypervisorConfig.SharedPath = vc.GetSharePath(containerID) + if err := checkForFIPS(&sandboxConfig); err != nil { return nil, vc.Process{}, err } diff --git a/src/runtime/pkg/oci/utils.go b/src/runtime/pkg/oci/utils.go index 7d6dc4293..584927924 100644 --- a/src/runtime/pkg/oci/utils.go +++ b/src/runtime/pkg/oci/utils.go @@ -109,7 +109,6 @@ type RuntimeConfig struct { FactoryConfig FactoryConfig HypervisorConfig vc.HypervisorConfig - NetmonConfig vc.NetmonConfig AgentConfig vc.KataAgentConfig //Determines how the VM should be connected to the @@ -317,12 +316,6 @@ func networkConfig(ocispec specs.Spec, config RuntimeConfig) (vc.NetworkConfig, netConf.InterworkingModel = config.InterNetworkModel netConf.DisableNewNetNs = config.DisableNewNetNs - netConf.NetmonConfig = vc.NetmonConfig{ - Path: config.NetmonConfig.Path, - Debug: config.NetmonConfig.Debug, - Enable: config.NetmonConfig.Enable, - } - return netConf, nil } diff --git a/src/runtime/virtcontainers/acrn.go b/src/runtime/virtcontainers/acrn.go index fafc734a5..db0f6491a 100644 --- a/src/runtime/virtcontainers/acrn.go +++ b/src/runtime/virtcontainers/acrn.go @@ -7,7 +7,6 @@ package virtcontainers import ( "context" - "encoding/json" "fmt" "os" "os/exec" @@ -20,6 +19,7 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" "github.com/kata-containers/kata-containers/src/runtime/pkg/uuid" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" @@ -39,11 +39,13 @@ var acrnTracingTags = map[string]string{ // Since ACRN is using the store in a quite abnormal way, let's first draw it back from store to here +/* // UUIDPathSuffix is the suffix used for uuid storage const ( UUIDPathSuffix = "uuid" uuidFile = "uuid.json" ) +*/ // ACRN currently supports only known UUIDs for security // reasons (FuSa). When launching VM, only these pre-defined @@ -312,7 +314,7 @@ func (a *Acrn) setup(ctx context.Context, id string, hypervisorConfig *Hyperviso // The path might already exist, but in case of VM templating, // we have to create it since the sandbox has not created it yet. - if err = os.MkdirAll(filepath.Join(a.store.RunStoragePath(), id), DirMode); err != nil { + if err = os.MkdirAll(filepath.Join(a.config.RunStorePath, id), DirMode); err != nil { return err } @@ -438,7 +440,7 @@ func (a *Acrn) StartVM(ctx context.Context, timeoutSecs int) error { a.Logger().WithField("default-kernel-parameters", formatted).Debug() } - vmPath := filepath.Join(a.store.RunVMStoragePath(), a.id) + vmPath := filepath.Join(a.config.VMStorePath, a.id) err := os.MkdirAll(vmPath, DirMode) if err != nil { return err @@ -634,7 +636,7 @@ func (a *Acrn) GetVMConsole(ctx context.Context, id string) (string, string, err span, _ := katatrace.Trace(ctx, a.Logger(), "GetVMConsole", acrnTracingTags, map[string]string{"sandbox_id": a.id}) defer span.End() - consoleURL, err := utils.BuildSocketPath(a.store.RunVMStoragePath(), id, acrnConsoleSocket) + consoleURL, err := utils.BuildSocketPath(a.config.VMStorePath, id, acrnConsoleSocket) if err != nil { return consoleProtoUnix, "", err } @@ -698,14 +700,14 @@ func (a *Acrn) toGrpc(ctx context.Context) ([]byte, error) { return nil, errors.New("acrn is not supported by VM cache") } -func (a *Acrn) Save() (s persistapi.HypervisorState) { +func (a *Acrn) Save() (s hv.HypervisorState) { s.Pid = a.state.PID s.Type = string(AcrnHypervisor) s.UUID = a.state.UUID return } -func (a *Acrn) Load(s persistapi.HypervisorState) { +func (a *Acrn) Load(s hv.HypervisorState) { a.state.PID = s.Pid a.state.UUID = s.UUID } @@ -719,7 +721,7 @@ func (a *Acrn) Check() error { } func (a *Acrn) GenerateSocket(id string) (interface{}, error) { - return generateVMSocket(id, a.store.RunVMStoragePath()) + return generateVMSocket(id, a.config.VMStorePath) } // GetACRNUUIDBytes returns UUID bytes that is used for VM creation @@ -782,38 +784,36 @@ func (a *Acrn) GetMaxSupportedACRNVM() (uint8, error) { } func (a *Acrn) storeInfo() error { - relPath := filepath.Join(UUIDPathSuffix, uuidFile) + /* + relPath := filepath.Join(UUIDPathSuffix, uuidFile) - jsonOut, err := json.Marshal(a.info) - if err != nil { - return fmt.Errorf("Could not marshal data: %s", err) - } + jsonOut, err := json.Marshal(a.info) + if err != nil { + return fmt.Errorf("Could not marshal data: %s", err) + } - if err := a.store.GlobalWrite(relPath, jsonOut); err != nil { - return fmt.Errorf("failed to write uuid to file: %v", err) - } + if err := a.store.GlobalWrite(relPath, jsonOut); err != nil { + return fmt.Errorf("failed to write uuid to file: %v", err) + }*/ return nil } func (a *Acrn) loadInfo() error { - relPath := filepath.Join(UUIDPathSuffix, uuidFile) + /* + relPath := filepath.Join(UUIDPathSuffix, uuidFile) + data, err := a.store.GlobalRead(relPath) + if err != nil { + return fmt.Errorf("failed to read uuid from file: %v", err) + } - data, err := a.store.GlobalRead(relPath) - if err != nil { - return fmt.Errorf("failed to read uuid from file: %v", err) - } + if err := json.Unmarshal(data, &a.info); err != nil { + return fmt.Errorf("failed to unmarshal uuid info: %v", err) + }*/ - if err := json.Unmarshal(data, &a.info); err != nil { - return fmt.Errorf("failed to unmarshal uuid info: %v", err) - } return nil } func (a *Acrn) IsRateLimiterBuiltin() bool { return false } - -func (a *Acrn) setSandbox(sandbox *Sandbox) { - a.sandbox = sandbox -} diff --git a/src/runtime/virtcontainers/acrn_test.go b/src/runtime/virtcontainers/acrn_test.go index f3d46b7b8..bb19b45a7 100644 --- a/src/runtime/virtcontainers/acrn_test.go +++ b/src/runtime/virtcontainers/acrn_test.go @@ -199,11 +199,15 @@ func TestAcrnGetSandboxConsole(t *testing.T) { assert.NoError(err) a := &Acrn{ - ctx: context.Background(), + ctx: context.Background(), + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, store: store, } sandboxID := "testSandboxID" - expected := filepath.Join(a.store.RunVMStoragePath(), sandboxID, consoleSocket) + expected := filepath.Join(store.RunVMStoragePath(), sandboxID, consoleSocket) proto, result, err := a.GetVMConsole(a.ctx, sandboxID) assert.NoError(err) @@ -219,6 +223,10 @@ func TestAcrnCreateVM(t *testing.T) { a := &Acrn{ store: store, + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, } sandbox := &Sandbox{ diff --git a/src/runtime/virtcontainers/api.go b/src/runtime/virtcontainers/api.go index 67e7608ac..14cfbb109 100644 --- a/src/runtime/virtcontainers/api.go +++ b/src/runtime/virtcontainers/api.go @@ -35,7 +35,7 @@ var virtLog = logrus.WithField("source", "virtcontainers") func SetLogger(ctx context.Context, logger *logrus.Entry) { fields := virtLog.Data virtLog = logger.WithFields(fields) - + SetHypervisorLogger(virtLog) // TODO: this will move to hypervisors pkg deviceApi.SetLogger(virtLog) compatoci.SetLogger(virtLog) deviceConfig.SetLogger(virtLog) diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 9236c832c..a3cdf02f6 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -20,12 +20,12 @@ import ( "time" "github.com/containerd/console" - persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" chclient "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/cloud-hypervisor/client" "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" log "github.com/sirupsen/logrus" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" @@ -157,7 +157,6 @@ func (s *CloudHypervisorState) reset() { } type cloudHypervisor struct { - store persistapi.PersistDriver console console.Console virtiofsd Virtiofsd APIClient clhClient @@ -226,7 +225,7 @@ func (clh *cloudHypervisor) CreateVM(ctx context.Context, id string, networkNS N clh.Logger().WithField("function", "CreateVM").Info("Sandbox already exist, loading from state") clh.virtiofsd = &virtiofsd{ PID: clh.state.VirtiofsdPID, - sourcePath: filepath.Join(getSharePath(clh.id)), + sourcePath: hypervisorConfig.SharedPath, debug: clh.config.Debug, socketPath: virtiofsdSocketPath, } @@ -342,7 +341,7 @@ func (clh *cloudHypervisor) CreateVM(ctx context.Context, id string, networkNS N clh.virtiofsd = &virtiofsd{ path: clh.config.VirtioFSDaemon, - sourcePath: filepath.Join(getSharePath(clh.id)), + sourcePath: filepath.Join(GetSharePath(clh.id)), socketPath: virtiofsdSocketPath, extraArgs: clh.config.VirtioFSExtraArgs, debug: clh.config.Debug, @@ -374,7 +373,7 @@ func (clh *cloudHypervisor) StartVM(ctx context.Context, timeout int) error { clh.Logger().WithField("function", "StartVM").Info("starting Sandbox") - vmPath := filepath.Join(clh.store.RunVMStoragePath(), clh.id) + vmPath := filepath.Join(clh.config.VMStorePath, clh.id) err := os.MkdirAll(vmPath, DirMode) if err != nil { return err @@ -747,7 +746,7 @@ func (clh *cloudHypervisor) toGrpc(ctx context.Context) ([]byte, error) { return nil, errors.New("cloudHypervisor is not supported by VM cache") } -func (clh *cloudHypervisor) Save() (s persistapi.HypervisorState) { +func (clh *cloudHypervisor) Save() (s hv.HypervisorState) { s.Pid = clh.state.PID s.Type = string(ClhHypervisor) s.VirtiofsdPid = clh.state.VirtiofsdPID @@ -755,7 +754,7 @@ func (clh *cloudHypervisor) Save() (s persistapi.HypervisorState) { return } -func (clh *cloudHypervisor) Load(s persistapi.HypervisorState) { +func (clh *cloudHypervisor) Load(s hv.HypervisorState) { clh.state.PID = s.Pid clh.state.VirtiofsdPID = s.VirtiofsdPid clh.state.apiSocket = s.APISocket @@ -814,7 +813,7 @@ func (clh *cloudHypervisor) AddDevice(ctx context.Context, devInfo interface{}, //########################################################################### func (clh *cloudHypervisor) Logger() *log.Entry { - return virtLog.WithField("subsystem", "cloudHypervisor") + return hvLogger.WithField("subsystem", "cloudHypervisor") } // Adds all capabilities supported by cloudHypervisor implementation of hypervisor interface @@ -893,15 +892,15 @@ func (clh *cloudHypervisor) GenerateSocket(id string) (interface{}, error) { } func (clh *cloudHypervisor) virtioFsSocketPath(id string) (string, error) { - return utils.BuildSocketPath(clh.store.RunVMStoragePath(), id, virtioFsSocket) + return utils.BuildSocketPath(clh.config.VMStorePath, id, virtioFsSocket) } func (clh *cloudHypervisor) vsockSocketPath(id string) (string, error) { - return utils.BuildSocketPath(clh.store.RunVMStoragePath(), id, clhSocket) + return utils.BuildSocketPath(clh.config.VMStorePath, id, clhSocket) } func (clh *cloudHypervisor) apiSocketPath(id string) (string, error) { - return utils.BuildSocketPath(clh.store.RunVMStoragePath(), id, clhAPISocket) + return utils.BuildSocketPath(clh.config.VMStorePath, id, clhAPISocket) } func (clh *cloudHypervisor) waitVMM(timeout uint) error { @@ -1213,7 +1212,7 @@ func (clh *cloudHypervisor) cleanupVM(force bool) error { } // Cleanup vm path - dir := filepath.Join(clh.store.RunVMStoragePath(), clh.id) + dir := filepath.Join(clh.config.VMStorePath, clh.id) // If it's a symlink, remove both dir and the target. link, err := filepath.EvalSymlinks(dir) @@ -1242,7 +1241,7 @@ func (clh *cloudHypervisor) cleanupVM(force bool) error { } if clh.config.VMid != "" { - dir = filepath.Join(clh.store.RunStoragePath(), clh.config.VMid) + dir = filepath.Join(clh.config.VMStorePath, clh.config.VMid) if err := os.RemoveAll(dir); err != nil { if !force { return err @@ -1272,6 +1271,3 @@ func (clh *cloudHypervisor) vmInfo() (chclient.VmInfo, error) { func (clh *cloudHypervisor) IsRateLimiterBuiltin() bool { return false } - -func (clh *cloudHypervisor) setSandbox(sandbox *Sandbox) { -} diff --git a/src/runtime/virtcontainers/clh_test.go b/src/runtime/virtcontainers/clh_test.go index 54718dd57..b0de2ae9b 100644 --- a/src/runtime/virtcontainers/clh_test.go +++ b/src/runtime/virtcontainers/clh_test.go @@ -203,7 +203,10 @@ func TestCloudHypervisorCleanupVM(t *testing.T) { assert.NoError(err, "persist.GetDriver() unexpected error") clh := &cloudHypervisor{ - store: store, + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, } err = clh.cleanupVM(true) @@ -214,7 +217,7 @@ func TestCloudHypervisorCleanupVM(t *testing.T) { err = clh.cleanupVM(true) assert.NoError(err, "persist.GetDriver() unexpected error") - dir := filepath.Join(clh.store.RunVMStoragePath(), clh.id) + dir := filepath.Join(store.RunVMStoragePath(), clh.id) os.MkdirAll(dir, os.ModePerm) err = clh.cleanupVM(false) @@ -235,9 +238,11 @@ func TestClhCreateVM(t *testing.T) { store, err := persist.GetDriver() assert.NoError(err) + clhConfig.VMStorePath = store.RunVMStoragePath() + clhConfig.RunStorePath = store.RunStoragePath() + clh := &cloudHypervisor{ config: clhConfig, - store: store, } sandbox := &Sandbox{ @@ -261,11 +266,13 @@ func TestClooudHypervisorStartSandbox(t *testing.T) { store, err := persist.GetDriver() assert.NoError(err) + clhConfig.VMStorePath = store.RunVMStoragePath() + clhConfig.RunStorePath = store.RunStoragePath() + clh := &cloudHypervisor{ config: clhConfig, APIClient: &clhClientMock{}, virtiofsd: &virtiofsdMock{}, - store: store, } err = clh.StartVM(context.Background(), 10) @@ -379,6 +386,11 @@ func TestClhGenerateSocket(t *testing.T) { clh, ok := hypervisor.(*cloudHypervisor) assert.True(ok) + clh.config = HypervisorConfig{ + VMStorePath: "/foo", + RunStorePath: "/bar", + } + clh.addVSock(1, "path") s, err := clh.GenerateSocket("c") @@ -391,7 +403,7 @@ func TestClhGenerateSocket(t *testing.T) { assert.NotEmpty(hvsock.UdsPath) // Path must be absolute - assert.True(strings.HasPrefix(hvsock.UdsPath, "/")) + assert.True(strings.HasPrefix(hvsock.UdsPath, "/"), "failed: socket path: %s", hvsock.UdsPath) assert.NotZero(hvsock.Port) } diff --git a/src/runtime/virtcontainers/documentation/api/1.0/api.md b/src/runtime/virtcontainers/documentation/api/1.0/api.md index 4cec38137..403a71280 100644 --- a/src/runtime/virtcontainers/documentation/api/1.0/api.md +++ b/src/runtime/virtcontainers/documentation/api/1.0/api.md @@ -366,7 +366,6 @@ type NetworkConfig struct { NetNSPath string NetNsCreated bool DisableNewNetNs bool - NetmonConfig NetmonConfig InterworkingModel NetInterworkingModel } ``` diff --git a/src/runtime/virtcontainers/fc.go b/src/runtime/virtcontainers/fc.go index 1b4c055cc..c6dffb337 100644 --- a/src/runtime/virtcontainers/fc.go +++ b/src/runtime/virtcontainers/fc.go @@ -22,9 +22,9 @@ import ( "syscall" "time" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" - persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client" models "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/models" ops "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/operations" @@ -1226,13 +1226,13 @@ func (fc *firecracker) toGrpc(ctx context.Context) ([]byte, error) { return nil, errors.New("firecracker is not supported by VM cache") } -func (fc *firecracker) Save() (s persistapi.HypervisorState) { +func (fc *firecracker) Save() (s hv.HypervisorState) { s.Pid = fc.info.PID s.Type = string(FirecrackerHypervisor) return } -func (fc *firecracker) Load(s persistapi.HypervisorState) { +func (fc *firecracker) Load(s hv.HypervisorState) { fc.info.PID = s.Pid } @@ -1274,6 +1274,3 @@ func revertBytes(num uint64) uint64 { } return 1024*revertBytes(a) + b } - -func (fc *firecracker) setSandbox(sandbox *Sandbox) { -} diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 0186c397e..715b4793c 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -14,11 +14,12 @@ import ( "strconv" "strings" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist" - persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" + + "github.com/sirupsen/logrus" ) // HypervisorType describes an hypervisor type. @@ -46,14 +47,10 @@ const ( // MockHypervisor is a mock hypervisor for testing purposes MockHypervisor HypervisorType = "mock" -) -const ( procMemInfo = "/proc/meminfo" procCPUInfo = "/proc/cpuinfo" -) -const ( defaultVCPUs = 1 // 2 GiB defaultMemSzMiB = 2048 @@ -74,6 +71,10 @@ const ( MinHypervisorMemory = 256 ) +var ( + hvLogger = logrus.WithField("source", "virtcontainers/hypervisor") +) + // In some architectures the maximum number of vCPUs depends on the number of physical cores. var defaultMaxQemuVCPUs = MaxQemuVCPUs() @@ -144,6 +145,12 @@ type MemoryDevice struct { Probe bool } +// SetHypervisorLogger sets up a logger for the hypervisor part of this pkg +func SetHypervisorLogger(logger *logrus.Entry) { + fields := hvLogger.Data + hvLogger = logger.WithFields(fields) +} + // Set sets an hypervisor type based on the input string. func (hType *HypervisorType) Set(value string) error { switch value { @@ -185,28 +192,18 @@ func (hType *HypervisorType) String() string { } } -// NewHypervisor returns an hypervisor from and hypervisor type. +// NewHypervisor returns an hypervisor from a hypervisor type. func NewHypervisor(hType HypervisorType) (Hypervisor, error) { - store, err := persist.GetDriver() - if err != nil { - return nil, err - } switch hType { case QemuHypervisor: - return &qemu{ - store: store, - }, nil + return &qemu{}, nil case FirecrackerHypervisor: return &firecracker{}, nil case AcrnHypervisor: - return &Acrn{ - store: store, - }, nil + return &Acrn{}, nil case ClhHypervisor: - return &cloudHypervisor{ - store: store, - }, nil + return &cloudHypervisor{}, nil case MockHypervisor: return &mockHypervisor{}, nil default: @@ -315,13 +312,19 @@ type HypervisorConfig struct { EntropySource string // Shared file system type: - // - virtio-9p (default) - // - virtio-fs + // - virtio-9p + // - virtio-fs (default) SharedFS string + // Path for filesystem sharing + SharedPath string + // VirtioFSDaemon is the virtio-fs vhost-user daemon path VirtioFSDaemon string + // VirtioFSCache cache mode for fs version cache or "none" + VirtioFSCache string + // File based memory backend root directory FileBackedMemRootDir string @@ -339,12 +342,15 @@ type HypervisorConfig struct { // VMid is "" if the hypervisor is not created by the factory. VMid string + // VMStorePath is the location on disk where VM information will persist + VMStorePath string + + // VMStorePath is the location on disk where runtime information will persist + RunStorePath string + // SELinux label for the VM SELinuxProcessLabel string - // VirtioFSCache cache mode for fs version cache or "none" - VirtioFSCache string - // HypervisorPathList is the list of hypervisor paths names allowed in annotations HypervisorPathList []string @@ -606,7 +612,7 @@ func (conf *HypervisorConfig) AddCustomAsset(a *types.Asset) error { return fmt.Errorf("Invalid %s at %s", a.Type(), a.Path()) } - virtLog.Debugf("Using custom %v asset %s", a.Type(), a.Path()) + hvLogger.Debugf("Using custom %v asset %s", a.Type(), a.Path()) if conf.customAssets == nil { conf.customAssets = make(map[types.AssetType]*types.Asset) @@ -874,7 +880,7 @@ func RunningOnVMM(cpuInfoPath string) (bool, error) { return flags["hypervisor"], nil } - virtLog.WithField("arch", runtime.GOARCH).Info("Unable to know if the system is running inside a VM") + hvLogger.WithField("arch", runtime.GOARCH).Info("Unable to know if the system is running inside a VM") return false, nil } @@ -931,14 +937,12 @@ type Hypervisor interface { toGrpc(ctx context.Context) ([]byte, error) Check() error - Save() persistapi.HypervisorState - Load(persistapi.HypervisorState) + Save() hv.HypervisorState + Load(hv.HypervisorState) // generate the socket to communicate the host and guest GenerateSocket(id string) (interface{}, error) // check if hypervisor supports built-in rate limiter. IsRateLimiterBuiltin() bool - - setSandbox(sandbox *Sandbox) } diff --git a/src/runtime/virtcontainers/kata_agent.go b/src/runtime/virtcontainers/kata_agent.go index bbf42a715..eac03ee7d 100644 --- a/src/runtime/virtcontainers/kata_agent.go +++ b/src/runtime/virtcontainers/kata_agent.go @@ -164,7 +164,7 @@ var kataHostSharedDir = func() string { // 2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it // // 3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/ -func getSharePath(id string) string { +func GetSharePath(id string) string { return filepath.Join(kataHostSharedDir(), id, "shared") } @@ -358,7 +358,7 @@ func (k *kataAgent) setupSandboxBindMounts(ctx context.Context, sandbox *Sandbox // Create subdirectory in host shared path for sandbox mounts sandboxMountDir := filepath.Join(getMountPath(sandbox.id), sandboxMountsDir) - sandboxShareDir := filepath.Join(getSharePath(sandbox.id), sandboxMountsDir) + sandboxShareDir := filepath.Join(GetSharePath(sandbox.id), sandboxMountsDir) if err := os.MkdirAll(sandboxMountDir, DirMode); err != nil { return fmt.Errorf("Creating sandbox shared mount directory: %v: %w", sandboxMountDir, err) } @@ -475,7 +475,7 @@ func (k *kataAgent) setupSharedPath(ctx context.Context, sandbox *Sandbox) (err defer span.End() // create shared path structure - sharePath := getSharePath(sandbox.id) + sharePath := GetSharePath(sandbox.id) mountPath := getMountPath(sandbox.id) if err := os.MkdirAll(sharePath, sharedDirMode); err != nil { return err @@ -511,7 +511,7 @@ func (k *kataAgent) createSandbox(ctx context.Context, sandbox *Sandbox) error { if err := k.setupSharedPath(ctx, sandbox); err != nil { return err } - return k.configure(ctx, sandbox.hypervisor, sandbox.id, getSharePath(sandbox.id), sandbox.config.AgentConfig) + return k.configure(ctx, sandbox.hypervisor, sandbox.id, GetSharePath(sandbox.id), sandbox.config.AgentConfig) } func cmdToKataProcess(cmd types.Cmd) (process *grpc.Process, err error) { @@ -2208,7 +2208,7 @@ func (k *kataAgent) cleanup(ctx context.Context, s *Sandbox) { } // Unmount shared path - path := getSharePath(s.id) + path := GetSharePath(s.id) k.Logger().WithField("path", path).Infof("Cleanup agent") if err := syscall.Unmount(path, syscall.MNT_DETACH|UmountNoFollow); err != nil { k.Logger().WithError(err).Errorf("failed to unmount vm share path %s", path) diff --git a/src/runtime/virtcontainers/kata_agent_test.go b/src/runtime/virtcontainers/kata_agent_test.go index 6e329919e..48c0f6a73 100644 --- a/src/runtime/virtcontainers/kata_agent_test.go +++ b/src/runtime/virtcontainers/kata_agent_test.go @@ -1158,7 +1158,7 @@ func TestSandboxBindMount(t *testing.T) { assert.Nil(err) defer os.RemoveAll(dir) - sharePath := getSharePath(sandbox.id) + sharePath := GetSharePath(sandbox.id) mountPath := getMountPath(sandbox.id) err = os.MkdirAll(sharePath, DirMode) diff --git a/src/runtime/virtcontainers/mock_hypervisor.go b/src/runtime/virtcontainers/mock_hypervisor.go index 2c132f0ba..111707fd9 100644 --- a/src/runtime/virtcontainers/mock_hypervisor.go +++ b/src/runtime/virtcontainers/mock_hypervisor.go @@ -10,7 +10,7 @@ import ( "errors" "os" - persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" ) @@ -130,11 +130,11 @@ func (m *mockHypervisor) toGrpc(ctx context.Context) ([]byte, error) { return nil, errors.New("mockHypervisor is not supported by VM cache") } -func (m *mockHypervisor) Save() (s persistapi.HypervisorState) { +func (m *mockHypervisor) Save() (s hv.HypervisorState) { return } -func (m *mockHypervisor) Load(s persistapi.HypervisorState) {} +func (m *mockHypervisor) Load(s hv.HypervisorState) {} func (m *mockHypervisor) Check() error { return nil @@ -149,6 +149,3 @@ func (m *mockHypervisor) GenerateSocket(id string) (interface{}, error) { func (m *mockHypervisor) IsRateLimiterBuiltin() bool { return false } - -func (m *mockHypervisor) setSandbox(sandbox *Sandbox) { -} diff --git a/src/runtime/virtcontainers/netmon.go b/src/runtime/virtcontainers/netmon.go deleted file mode 100644 index 58483df22..000000000 --- a/src/runtime/virtcontainers/netmon.go +++ /dev/null @@ -1,96 +0,0 @@ -// Copyright (c) 2018 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package virtcontainers - -import ( - "fmt" - "os/exec" - "syscall" - - "github.com/sirupsen/logrus" -) - -// NetmonConfig is the structure providing specific configuration -// for the network monitor. -type NetmonConfig struct { - Path string - Debug bool - Enable bool -} - -// netmonParams is the structure providing specific parameters needed -// for the execution of the network monitor binary. -type netmonParams struct { - netmonPath string - logLevel string - runtime string - sandboxID string - debug bool -} - -func netmonLogger() *logrus.Entry { - return virtLog.WithField("subsystem", "netmon") -} - -func prepareNetMonParams(params netmonParams) ([]string, error) { - if params.netmonPath == "" { - return []string{}, fmt.Errorf("Netmon path is empty") - } - if params.runtime == "" { - return []string{}, fmt.Errorf("Netmon runtime path is empty") - } - if params.sandboxID == "" { - return []string{}, fmt.Errorf("Netmon sandbox ID is empty") - } - - args := []string{params.netmonPath, - "-r", params.runtime, - "-s", params.sandboxID, - } - - if params.debug { - args = append(args, "-d") - } - if params.logLevel != "" { - args = append(args, []string{"-log", params.logLevel}...) - } - - return args, nil -} - -func startNetmon(params netmonParams) (int, error) { - args, err := prepareNetMonParams(params) - if err != nil { - return -1, err - } - - cmd := exec.Command(args[0], args[1:]...) - if err := cmd.Start(); err != nil { - return -1, err - } - - return cmd.Process.Pid, nil -} - -func stopNetmon(pid int) error { - if pid <= 0 { - return nil - } - - sig := syscall.SIGKILL - - netmonLogger().WithFields( - logrus.Fields{ - "netmon-pid": pid, - "netmon-signal": sig, - }).Info("Stopping netmon") - - if err := syscall.Kill(pid, sig); err != nil && err != syscall.ESRCH { - return err - } - - return nil -} diff --git a/src/runtime/virtcontainers/netmon_test.go b/src/runtime/virtcontainers/netmon_test.go deleted file mode 100644 index 0a7f3bb53..000000000 --- a/src/runtime/virtcontainers/netmon_test.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2018 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package virtcontainers - -import ( - "reflect" - "testing" - - "github.com/stretchr/testify/assert" -) - -const ( - testNetmonPath = "/foo/bar/netmon" - testRuntimePath = "/foo/bar/runtime" -) - -func TestNetmonLogger(t *testing.T) { - got := netmonLogger() - expected := virtLog.WithField("subsystem", "netmon") - assert.True(t, reflect.DeepEqual(expected, got), - "Got %+v\nExpected %+v", got, expected) -} - -func TestPrepareNetMonParams(t *testing.T) { - // Empty netmon path - params := netmonParams{} - got, err := prepareNetMonParams(params) - assert.NotNil(t, err) - assert.Equal(t, got, []string{}) - - // Empty runtime path - params.netmonPath = testNetmonPath - got, err = prepareNetMonParams(params) - assert.NotNil(t, err) - assert.Equal(t, got, []string{}) - - // Empty sandbox ID - params.runtime = testRuntimePath - got, err = prepareNetMonParams(params) - assert.NotNil(t, err) - assert.Equal(t, got, []string{}) - - // Successful case - params.sandboxID = testSandboxID - got, err = prepareNetMonParams(params) - assert.Nil(t, err) - expected := []string{testNetmonPath, - "-r", testRuntimePath, - "-s", testSandboxID} - assert.True(t, reflect.DeepEqual(expected, got), - "Got %+v\nExpected %+v", got, expected) -} - -func TestStopNetmon(t *testing.T) { - pid := -1 - err := stopNetmon(pid) - assert.Nil(t, err) -} diff --git a/src/runtime/virtcontainers/network.go b/src/runtime/virtcontainers/network.go index 0af47f46a..2bb39a600 100644 --- a/src/runtime/virtcontainers/network.go +++ b/src/runtime/virtcontainers/network.go @@ -178,7 +178,6 @@ type NetworkInterfacePair struct { // NetworkConfig is the network configuration related to a network. type NetworkConfig struct { NetNSPath string - NetmonConfig NetmonConfig InterworkingModel NetInterworkingModel NetNsCreated bool DisableNewNetNs bool @@ -193,7 +192,6 @@ type NetworkNamespace struct { NetNsPath string Endpoints []Endpoint NetNsCreated bool - NetmonPID int } func createLink(netHandle *netlink.Handle, name string, expectedLink netlink.Link, queues int) (netlink.Link, []*os.File, error) { diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index b8fc2c8ab..3a8f70d4a 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -8,6 +8,7 @@ package virtcontainers import ( "errors" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/api" exp "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/experimental" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist" @@ -164,7 +165,6 @@ func (s *Sandbox) dumpAgent(ss *persistapi.SandboxState) { func (s *Sandbox) dumpNetwork(ss *persistapi.SandboxState) { ss.Network = persistapi.NetworkInfo{ NetNsPath: s.networkNS.NetNsPath, - NetmonPID: s.networkNS.NetmonPID, NetNsCreated: s.networkNS.NetNsCreated, } for _, e := range s.networkNS.Endpoints { @@ -315,7 +315,7 @@ func (c *Container) loadContState(cs persistapi.ContainerState) { } } -func (s *Sandbox) loadHypervisor(hs persistapi.HypervisorState) { +func (s *Sandbox) loadHypervisor(hs hv.HypervisorState) { s.hypervisor.Load(hs) } @@ -367,7 +367,6 @@ func (c *Container) loadContProcess(cs persistapi.ContainerState) { func (s *Sandbox) loadNetwork(netInfo persistapi.NetworkInfo) { s.networkNS = NetworkNamespace{ NetNsPath: netInfo.NetNsPath, - NetmonPID: netInfo.NetmonPID, NetNsCreated: netInfo.NetNsCreated, } diff --git a/src/runtime/virtcontainers/persist/api/network.go b/src/runtime/virtcontainers/persist/api/network.go index f1c1cc2f8..51c3aac62 100644 --- a/src/runtime/virtcontainers/persist/api/network.go +++ b/src/runtime/virtcontainers/persist/api/network.go @@ -98,6 +98,5 @@ type NetworkEndpoint struct { type NetworkInfo struct { NetNsPath string Endpoints []NetworkEndpoint - NetmonPID int NetNsCreated bool } diff --git a/src/runtime/virtcontainers/persist/api/sandbox.go b/src/runtime/virtcontainers/persist/api/sandbox.go index 1398cc20f..09196637c 100644 --- a/src/runtime/virtcontainers/persist/api/sandbox.go +++ b/src/runtime/virtcontainers/persist/api/sandbox.go @@ -6,6 +6,10 @@ package persistapi +import ( + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" +) + // ============= sandbox level resources ============= // AgentState save agent state data @@ -38,7 +42,7 @@ type SandboxState struct { OverheadCgroupPath string // HypervisorState saves hypervisor specific data - HypervisorState HypervisorState + HypervisorState hv.HypervisorState // AgentState saves state data of agent AgentState AgentState diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 29cd0d173..9b78fb24e 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -31,11 +31,11 @@ import ( "github.com/sirupsen/logrus" "golang.org/x/sys/unix" + hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" pkgUtils "github.com/kata-containers/kata-containers/src/runtime/pkg/utils" "github.com/kata-containers/kata-containers/src/runtime/pkg/uuid" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" - persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" vcTypes "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" @@ -67,18 +67,12 @@ type qmpChannel struct { sync.Mutex } -// CPUDevice represents a CPU device which was hot-added in a running VM -type CPUDevice struct { - // ID is used to identify this CPU in the hypervisor options. - ID string -} - // QemuState keeps Qemu's state type QemuState struct { UUID string Bridges []types.Bridge // HotpluggedCPUs is the list of CPUs that were hot-added - HotpluggedVCPUs []CPUDevice + HotpluggedVCPUs []hv.CPUDevice HotpluggedMemory int VirtiofsdPid int PCIeRootPort int @@ -92,8 +86,6 @@ type qemu struct { virtiofsd Virtiofsd - store persistapi.PersistDriver - ctx context.Context // fds is a list of file descriptors inherited by QEMU process @@ -149,7 +141,7 @@ type qmpLogger struct { func newQMPLogger() qmpLogger { return qmpLogger{ - logger: virtLog.WithField("subsystem", "qmp"), + logger: hvLogger.WithField("subsystem", "qmp"), } } @@ -171,7 +163,7 @@ func (l qmpLogger) Errorf(format string, v ...interface{}) { // Logger returns a logrus logger appropriate for logging qemu messages func (q *qemu) Logger() *logrus.Entry { - return virtLog.WithField("subsystem", "qemu") + return hvLogger.WithField("subsystem", "qemu") } func (q *qemu) kernelParameters() string { @@ -276,7 +268,7 @@ func (q *qemu) setup(ctx context.Context, id string, hypervisorConfig *Hyperviso // The path might already exist, but in case of VM templating, // we have to create it since the sandbox has not created it yet. - if err = utils.MkdirAllWithInheritedOwner(filepath.Join(q.store.RunStoragePath(), id), DirMode); err != nil { + if err = utils.MkdirAllWithInheritedOwner(filepath.Join(q.config.RunStorePath, id), DirMode); err != nil { return err } } @@ -331,7 +323,7 @@ func (q *qemu) memoryTopology() (govmmQemu.Memory, error) { } func (q *qemu) qmpSocketPath(id string) (string, error) { - return utils.BuildSocketPath(q.store.RunVMStoragePath(), id, qmpSocket) + return utils.BuildSocketPath(q.config.VMStorePath, id, qmpSocket) } func (q *qemu) getQemuMachine() (govmmQemu.Machine, error) { @@ -618,7 +610,7 @@ func (q *qemu) CreateVM(ctx context.Context, id string, networkNS NetworkNamespa GlobalParam: "kvm-pit.lost_tick_policy=discard", Bios: firmwarePath, PFlash: pflash, - PidFile: filepath.Join(q.store.RunVMStoragePath(), q.id, "pid"), + PidFile: filepath.Join(q.config.VMStorePath, q.id, "pid"), } qemuConfig.Devices, qemuConfig.Bios, err = q.arch.appendProtectionDevice(qemuConfig.Devices, firmwarePath) @@ -655,7 +647,7 @@ func (q *qemu) CreateVM(ctx context.Context, id string, networkNS NetworkNamespa q.virtiofsd = &virtiofsd{ path: q.config.VirtioFSDaemon, - sourcePath: filepath.Join(getSharePath(q.id)), + sourcePath: hypervisorConfig.SharedPath, socketPath: virtiofsdSocketPath, extraArgs: q.config.VirtioFSExtraArgs, debug: q.config.Debug, @@ -666,7 +658,7 @@ func (q *qemu) CreateVM(ctx context.Context, id string, networkNS NetworkNamespa } func (q *qemu) vhostFSSocketPath(id string) (string, error) { - return utils.BuildSocketPath(q.store.RunVMStoragePath(), id, vhostFSSocket) + return utils.BuildSocketPath(q.config.VMStorePath, id, vhostFSSocket) } func (q *qemu) setupVirtiofsd(ctx context.Context) (err error) { @@ -795,7 +787,7 @@ func (q *qemu) StartVM(ctx context.Context, timeout int) error { q.fds = []*os.File{} }() - vmPath := filepath.Join(q.store.RunVMStoragePath(), q.id) + vmPath := filepath.Join(q.config.VMStorePath, q.id) err := utils.MkdirAllWithInheritedOwner(vmPath, DirMode) if err != nil { return err @@ -1002,7 +994,7 @@ func (q *qemu) StopVM(ctx context.Context, waitOnly bool) error { func (q *qemu) cleanupVM() error { // Cleanup vm path - dir := filepath.Join(q.store.RunVMStoragePath(), q.id) + dir := filepath.Join(q.config.VMStorePath, q.id) // If it's a symlink, remove both dir and the target. // This can happen when vm template links a sandbox to a vm. @@ -1023,7 +1015,7 @@ func (q *qemu) cleanupVM() error { } if q.config.VMid != "" { - dir = filepath.Join(q.store.RunStoragePath(), q.config.VMid) + dir = filepath.Join(q.config.RunStorePath, q.config.VMid) if err := os.RemoveAll(dir); err != nil { q.Logger().WithError(err).WithField("path", dir).Warnf("failed to remove vm path") } @@ -1149,7 +1141,7 @@ func (q *qemu) dumpSandboxMetaInfo(dumpSavePath string) { dumpStatePath := filepath.Join(dumpSavePath, "state") // copy state from /run/vc/sbs to memory dump directory - statePath := filepath.Join(q.store.RunStoragePath(), q.id) + statePath := filepath.Join(q.config.RunStorePath, q.id) command := []string{"/bin/cp", "-ar", statePath, dumpStatePath} q.Logger().WithField("command", command).Info("try to Save sandbox state") if output, err := pkgUtils.RunCommandFull(command, true); err != nil { @@ -1822,7 +1814,7 @@ func (q *qemu) hotplugAddCPUs(amount uint32) (uint32, error) { } // a new vCPU was added, update list of hotplugged vCPUs and Check if all vCPUs were added - q.state.HotpluggedVCPUs = append(q.state.HotpluggedVCPUs, CPUDevice{cpuID}) + q.state.HotpluggedVCPUs = append(q.state.HotpluggedVCPUs, hv.CPUDevice{ID: cpuID}) hotpluggedVCPUs++ if hotpluggedVCPUs == amount { // All vCPUs were hotplugged @@ -2030,7 +2022,7 @@ func (q *qemu) GetVMConsole(ctx context.Context, id string) (string, string, err span, _ := katatrace.Trace(ctx, q.Logger(), "GetVMConsole", qemuTracingTags, map[string]string{"sandbox_id": q.id}) defer span.End() - consoleURL, err := utils.BuildSocketPath(q.store.RunVMStoragePath(), id, consoleSocket) + consoleURL, err := utils.BuildSocketPath(q.config.VMStorePath, id, consoleSocket) if err != nil { return consoleProtoUnix, "", err } @@ -2469,7 +2461,7 @@ func (q *qemu) toGrpc(ctx context.Context) ([]byte, error) { return json.Marshal(&qp) } -func (q *qemu) Save() (s persistapi.HypervisorState) { +func (q *qemu) Save() (s hv.HypervisorState) { // If QEMU isn't even running, there isn't any state to Save if q.stopped { @@ -2488,7 +2480,7 @@ func (q *qemu) Save() (s persistapi.HypervisorState) { s.PCIeRootPort = q.state.PCIeRootPort for _, bridge := range q.arch.getBridges() { - s.Bridges = append(s.Bridges, persistapi.Bridge{ + s.Bridges = append(s.Bridges, hv.Bridge{ DeviceAddr: bridge.Devices, Type: string(bridge.Type), ID: bridge.ID, @@ -2497,14 +2489,14 @@ func (q *qemu) Save() (s persistapi.HypervisorState) { } for _, cpu := range q.state.HotpluggedVCPUs { - s.HotpluggedVCPUs = append(s.HotpluggedVCPUs, persistapi.CPUDevice{ + s.HotpluggedVCPUs = append(s.HotpluggedVCPUs, hv.CPUDevice{ ID: cpu.ID, }) } return } -func (q *qemu) Load(s persistapi.HypervisorState) { +func (q *qemu) Load(s hv.HypervisorState) { q.state.UUID = s.UUID q.state.HotpluggedMemory = s.HotpluggedMemory q.state.HotplugVFIOOnRootBus = s.HotplugVFIOOnRootBus @@ -2516,7 +2508,7 @@ func (q *qemu) Load(s persistapi.HypervisorState) { } for _, cpu := range s.HotpluggedVCPUs { - q.state.HotpluggedVCPUs = append(q.state.HotpluggedVCPUs, CPUDevice{ + q.state.HotpluggedVCPUs = append(q.state.HotpluggedVCPUs, hv.CPUDevice{ ID: cpu.ID, }) } @@ -2543,12 +2535,9 @@ func (q *qemu) Check() error { } func (q *qemu) GenerateSocket(id string) (interface{}, error) { - return generateVMSocket(id, q.store.RunVMStoragePath()) + return generateVMSocket(id, q.config.VMStorePath) } func (q *qemu) IsRateLimiterBuiltin() bool { return false } - -func (q *qemu) setSandbox(sandbox *Sandbox) { -} diff --git a/src/runtime/virtcontainers/qemu_amd64.go b/src/runtime/virtcontainers/qemu_amd64.go index 689575ab5..c1464809a 100644 --- a/src/runtime/virtcontainers/qemu_amd64.go +++ b/src/runtime/virtcontainers/qemu_amd64.go @@ -169,7 +169,7 @@ func (q *qemuAmd64) cpuModel() string { // VMX is not migratable yet. // issue: https://github.com/kata-containers/runtime/issues/1750 if q.vmFactory { - virtLog.WithField("subsystem", "qemuAmd64").Warn("VMX is not migratable yet: turning it off") + hvLogger.WithField("subsystem", "qemuAmd64").Warn("VMX is not migratable yet: turning it off") cpuModel += ",vmx=off" } @@ -200,7 +200,7 @@ func (q *qemuAmd64) enableProtection() error { if err != nil { return err } - logger := virtLog.WithFields(logrus.Fields{ + logger := hvLogger.WithFields(logrus.Fields{ "subsystem": "qemuAmd64", "machine": q.qemuMachine, "kernel-params-debug": q.kernelParamsDebug, diff --git a/src/runtime/virtcontainers/qemu_arch_base.go b/src/runtime/virtcontainers/qemu_arch_base.go index 426da6bac..97cd6eb83 100644 --- a/src/runtime/virtcontainers/qemu_arch_base.go +++ b/src/runtime/virtcontainers/qemu_arch_base.go @@ -846,6 +846,6 @@ func (q *qemuArchBase) setPFlash(p []string) { // append protection device func (q *qemuArchBase) appendProtectionDevice(devices []govmmQemu.Device, firmware string) ([]govmmQemu.Device, string, error) { - virtLog.WithField("arch", runtime.GOARCH).Warnf("Confidential Computing has not been implemented for this architecture") + hvLogger.WithField("arch", runtime.GOARCH).Warnf("Confidential Computing has not been implemented for this architecture") return devices, firmware, nil } diff --git a/src/runtime/virtcontainers/qemu_arm64.go b/src/runtime/virtcontainers/qemu_arm64.go index 2cd869a8c..452493ce1 100644 --- a/src/runtime/virtcontainers/qemu_arm64.go +++ b/src/runtime/virtcontainers/qemu_arm64.go @@ -171,6 +171,6 @@ func (q *qemuArm64) enableProtection() error { func (q *qemuArm64) appendProtectionDevice(devices []govmmQemu.Device, firmware string) ([]govmmQemu.Device, string, error) { err := q.enableProtection() - virtLog.WithField("arch", runtime.GOARCH).Warnf("%v", err) + hvLogger.WithField("arch", runtime.GOARCH).Warnf("%v", err) return devices, firmware, err } diff --git a/src/runtime/virtcontainers/qemu_ppc64le.go b/src/runtime/virtcontainers/qemu_ppc64le.go index 51c9003ba..00fec3529 100644 --- a/src/runtime/virtcontainers/qemu_ppc64le.go +++ b/src/runtime/virtcontainers/qemu_ppc64le.go @@ -51,7 +51,7 @@ var supportedQemuMachine = govmmQemu.Machine{ // Logger returns a logrus logger appropriate for logging qemu messages func (q *qemuPPC64le) Logger() *logrus.Entry { - return virtLog.WithField("subsystem", "qemuPPC64le") + return hvLogger.WithField("subsystem", "qemuPPC64le") } // MaxQemuVCPUs returns the maximum number of vCPUs supported @@ -141,7 +141,7 @@ func (q *qemuPPC64le) enableProtection() error { q.qemuMachine.Options += "," } q.qemuMachine.Options += fmt.Sprintf("confidential-guest-support=%s", pefID) - virtLog.WithFields(logrus.Fields{ + hvLogger.WithFields(logrus.Fields{ "subsystem": "qemuPPC64le", "machine": q.qemuMachine, "kernel-params": q.kernelParams, diff --git a/src/runtime/virtcontainers/qemu_s390x.go b/src/runtime/virtcontainers/qemu_s390x.go index b7611decc..d6c013156 100644 --- a/src/runtime/virtcontainers/qemu_s390x.go +++ b/src/runtime/virtcontainers/qemu_s390x.go @@ -324,7 +324,7 @@ func (q *qemuS390x) enableProtection() error { q.qemuMachine.Options += "," } q.qemuMachine.Options += fmt.Sprintf("confidential-guest-support=%s", secExecID) - virtLog.WithFields(logrus.Fields{ + hvLogger.WithFields(logrus.Fields{ "subsystem": logSubsystem, "machine": q.qemuMachine}). Info("Enabling guest protection with Secure Execution") diff --git a/src/runtime/virtcontainers/qemu_test.go b/src/runtime/virtcontainers/qemu_test.go index deeb482dc..9104a6892 100644 --- a/src/runtime/virtcontainers/qemu_test.go +++ b/src/runtime/virtcontainers/qemu_test.go @@ -78,7 +78,10 @@ func TestQemuCreateVM(t *testing.T) { store, err := persist.GetDriver() assert.NoError(err) q := &qemu{ - store: store, + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, } sandbox := &Sandbox{ ctx: context.Background(), @@ -94,7 +97,7 @@ func TestQemuCreateVM(t *testing.T) { assert.NoError(err) // Create parent dir path for hypervisor.json - parentDir := filepath.Join(q.store.RunStoragePath(), sandbox.id) + parentDir := filepath.Join(store.RunStoragePath(), sandbox.id) assert.NoError(os.MkdirAll(parentDir, DirMode)) err = q.CreateVM(context.Background(), sandbox.id, NetworkNamespace{}, &sandbox.config.HypervisorConfig) @@ -110,7 +113,10 @@ func TestQemuCreateVMMissingParentDirFail(t *testing.T) { store, err := persist.GetDriver() assert.NoError(err) q := &qemu{ - store: store, + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, } sandbox := &Sandbox{ ctx: context.Background(), @@ -126,7 +132,7 @@ func TestQemuCreateVMMissingParentDirFail(t *testing.T) { assert.NoError(err) // Ensure parent dir path for hypervisor.json does not exist. - parentDir := filepath.Join(q.store.RunStoragePath(), sandbox.id) + parentDir := filepath.Join(store.RunStoragePath(), sandbox.id) assert.NoError(os.RemoveAll(parentDir)) err = q.CreateVM(context.Background(), sandbox.id, NetworkNamespace{}, &sandbox.config.HypervisorConfig) @@ -192,7 +198,10 @@ func TestQemuKnobs(t *testing.T) { assert.NoError(err) q := &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } err = q.CreateVM(context.Background(), sandbox.id, NetworkNamespace{}, &sandbox.config.HypervisorConfig) assert.NoError(err) @@ -325,11 +334,14 @@ func TestQemuGetSandboxConsole(t *testing.T) { store, err := persist.GetDriver() assert.NoError(err) q := &qemu{ - ctx: context.Background(), - store: store, + ctx: context.Background(), + config: HypervisorConfig{ + VMStorePath: store.RunVMStoragePath(), + RunStorePath: store.RunStoragePath(), + }, } sandboxID := "testSandboxID" - expected := filepath.Join(q.store.RunVMStoragePath(), sandboxID, consoleSocket) + expected := filepath.Join(store.RunVMStoragePath(), sandboxID, consoleSocket) proto, result, err := q.GetVMConsole(q.ctx, sandboxID) assert.NoError(err) @@ -460,7 +472,10 @@ func TestQemuFileBackedMem(t *testing.T) { assert.NoError(err) q := &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } sandbox.config.HypervisorConfig.SharedFS = config.VirtioFS err = q.CreateVM(context.Background(), sandbox.id, NetworkNamespace{}, &sandbox.config.HypervisorConfig) @@ -475,7 +490,10 @@ func TestQemuFileBackedMem(t *testing.T) { assert.NoError(err) q = &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } sandbox.config.HypervisorConfig.BootToBeTemplate = true sandbox.config.HypervisorConfig.SharedFS = config.VirtioFS @@ -491,7 +509,10 @@ func TestQemuFileBackedMem(t *testing.T) { assert.NoError(err) q = &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } sandbox.config.HypervisorConfig.FileBackedMemRootDir = "/tmp/xyzabc" err = q.CreateVM(context.Background(), sandbox.id, NetworkNamespace{}, &sandbox.config.HypervisorConfig) @@ -505,7 +526,10 @@ func TestQemuFileBackedMem(t *testing.T) { assert.NoError(err) q = &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } sandbox.config.HypervisorConfig.EnableVhostUserStore = true sandbox.config.HypervisorConfig.HugePages = true @@ -518,7 +542,10 @@ func TestQemuFileBackedMem(t *testing.T) { assert.NoError(err) q = &qemu{ - store: sandbox.store, + config: HypervisorConfig{ + VMStorePath: sandbox.store.RunVMStoragePath(), + RunStorePath: sandbox.store.RunStoragePath(), + }, } sandbox.config.HypervisorConfig.EnableVhostUserStore = true sandbox.config.HypervisorConfig.HugePages = false diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index 00426f8a5..2b3e780fe 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -529,12 +529,9 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor swapDevices: []*config.BlockDrive{}, } - hypervisor.setSandbox(s) - if s.store, err = persist.GetDriver(); err != nil || s.store == nil { return nil, fmt.Errorf("failed to get fs persist driver: %v", err) } - defer func() { if retErr != nil { s.Logger().WithError(retErr).Error("Create new sandbox failed") @@ -542,6 +539,9 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor } }() + sandboxConfig.HypervisorConfig.VMStorePath = s.store.RunVMStoragePath() + sandboxConfig.HypervisorConfig.RunStorePath = s.store.RunStoragePath() + spec := s.GetPatchedOCISpec() if spec != nil && spec.Process.SelinuxLabel != "" { sandboxConfig.HypervisorConfig.SELinuxProcessLabel = spec.Process.SelinuxLabel @@ -782,40 +782,6 @@ func (s *Sandbox) Delete(ctx context.Context) error { return s.store.Destroy(s.id) } -func (s *Sandbox) startNetworkMonitor(ctx context.Context) error { - span, ctx := katatrace.Trace(ctx, s.Logger(), "startNetworkMonitor", sandboxTracingTags, map[string]string{"sandbox_id": s.id}) - defer span.End() - - binPath, err := os.Executable() - if err != nil { - return err - } - - logLevel := "info" - if s.config.NetworkConfig.NetmonConfig.Debug { - logLevel = "debug" - } - - params := netmonParams{ - netmonPath: s.config.NetworkConfig.NetmonConfig.Path, - debug: s.config.NetworkConfig.NetmonConfig.Debug, - logLevel: logLevel, - runtime: binPath, - sandboxID: s.id, - } - - return s.network.Run(ctx, s.networkNS.NetNsPath, func() error { - pid, err := startNetmon(params) - if err != nil { - return err - } - - s.networkNS.NetmonPID = pid - - return nil - }) -} - func (s *Sandbox) createNetwork(ctx context.Context) error { if s.config.NetworkConfig.DisableNewNetNs || s.config.NetworkConfig.NetNSPath == "" { @@ -842,18 +808,11 @@ func (s *Sandbox) createNetwork(ctx context.Context) error { } s.networkNS.Endpoints = endpoints - - if s.config.NetworkConfig.NetmonConfig.Enable { - if err := s.startNetworkMonitor(ctx); err != nil { - return err - } - } } return nil } func (s *Sandbox) postCreatedNetwork(ctx context.Context) error { - return s.network.PostAdd(ctx, &s.networkNS, s.factory != nil) } @@ -861,12 +820,6 @@ func (s *Sandbox) removeNetwork(ctx context.Context) error { span, ctx := katatrace.Trace(ctx, s.Logger(), "removeNetwork", sandboxTracingTags, map[string]string{"sandbox_id": s.id}) defer span.End() - if s.config.NetworkConfig.NetmonConfig.Enable { - if err := stopNetmon(s.networkNS.NetmonPID); err != nil { - return err - } - } - return s.network.Remove(ctx, &s.networkNS, s.hypervisor) } @@ -1222,12 +1175,6 @@ func (s *Sandbox) startVM(ctx context.Context) (err error) { } s.networkNS.Endpoints = endpoints - - if s.config.NetworkConfig.NetmonConfig.Enable { - if err := s.startNetworkMonitor(ctx); err != nil { - return err - } - } } s.Logger().Info("VM started") diff --git a/src/runtime/virtcontainers/sandbox_test.go b/src/runtime/virtcontainers/sandbox_test.go index 435a13da3..3d6f88e21 100644 --- a/src/runtime/virtcontainers/sandbox_test.go +++ b/src/runtime/virtcontainers/sandbox_test.go @@ -10,7 +10,6 @@ import ( "fmt" "io/ioutil" "os" - "os/exec" "path" "path/filepath" "strings" @@ -1300,33 +1299,6 @@ func TestGetNetNs(t *testing.T) { assert.Equal(t, netNs, expected) } -func TestStartNetworkMonitor(t *testing.T) { - if os.Getuid() != 0 { - t.Skip("Test disabled as requires root user") - } - trueBinPath, err := exec.LookPath("true") - assert.Nil(t, err) - assert.NotEmpty(t, trueBinPath) - - s := &Sandbox{ - id: testSandboxID, - config: &SandboxConfig{ - NetworkConfig: NetworkConfig{ - NetmonConfig: NetmonConfig{ - Path: trueBinPath, - }, - }, - }, - networkNS: NetworkNamespace{ - NetNsPath: fmt.Sprintf("/proc/%d/task/%d/ns/net", os.Getpid(), unix.Gettid()), - }, - ctx: context.Background(), - } - - err = s.startNetworkMonitor(context.Background()) - assert.Nil(t, err) -} - func TestSandboxStopStopped(t *testing.T) { s := &Sandbox{ ctx: context.Background(), diff --git a/src/runtime/virtcontainers/virtcontainers_test.go b/src/runtime/virtcontainers/virtcontainers_test.go index 56b27b1c5..5772dd60a 100644 --- a/src/runtime/virtcontainers/virtcontainers_test.go +++ b/src/runtime/virtcontainers/virtcontainers_test.go @@ -60,7 +60,7 @@ var testHyperstartTtySocket = "" func cleanUp() { os.RemoveAll(fs.MockRunStoragePath()) os.RemoveAll(fs.MockRunVMStoragePath()) - syscall.Unmount(getSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow) + syscall.Unmount(GetSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow) os.RemoveAll(testDir) os.MkdirAll(testDir, DirMode) diff --git a/src/runtime/virtcontainers/virtiofsd.go b/src/runtime/virtcontainers/virtiofsd.go index d35399bea..baaec862f 100644 --- a/src/runtime/virtcontainers/virtiofsd.go +++ b/src/runtime/virtcontainers/virtiofsd.go @@ -216,7 +216,7 @@ func (v *virtiofsd) valid() error { } func (v *virtiofsd) Logger() *log.Entry { - return virtLog.WithField("subsystem", "virtiofsd") + return hvLogger.WithField("subsystem", "virtiofsd") } func (v *virtiofsd) kill(ctx context.Context) (err error) { diff --git a/tools/agent-ctl/src/main.rs b/tools/agent-ctl/src/main.rs index 00519c932..88c12e984 100644 --- a/tools/agent-ctl/src/main.rs +++ b/tools/agent-ctl/src/main.rs @@ -134,16 +134,14 @@ fn make_examples_text(program_name: &str) -> String { fn connect(name: &str, global_args: clap::ArgMatches) -> Result<()> { let args = global_args .subcommand_matches("connect") - .ok_or("BUG: missing sub-command arguments".to_string()) - .map_err(|e| anyhow!(e))?; + .ok_or_else(|| anyhow!("BUG: missing sub-command arguments"))?; let interactive = args.is_present("interactive"); let ignore_errors = args.is_present("ignore-errors"); let server_address = args .value_of("server-address") - .ok_or("need server adddress".to_string()) - .map_err(|e| anyhow!(e))? + .ok_or_else(|| anyhow!("need server adddress"))? .to_string(); let mut commands: Vec<&str> = Vec::new(); @@ -151,13 +149,13 @@ fn connect(name: &str, global_args: clap::ArgMatches) -> Result<()> { if !interactive { commands = args .values_of("cmd") - .ok_or("need commands to send to the server".to_string()) - .map_err(|e| anyhow!(e))? + .ok_or_else(|| anyhow!("need commands to send to the server"))? .collect(); } - // Cannot fail as a default has been specified - let log_level_name = global_args.value_of("log-level").unwrap(); + let log_level_name = global_args + .value_of("log-level") + .ok_or_else(|| anyhow!("cannot get log level"))?; let log_level = logging::level_name_to_slog_level(log_level_name).map_err(|e| anyhow!(e))?; @@ -169,10 +167,10 @@ fn connect(name: &str, global_args: clap::ArgMatches) -> Result<()> { None => 0, }; - let hybrid_vsock_port: u64 = args + let hybrid_vsock_port = args .value_of("hybrid-vsock-port") - .ok_or("Need Hybrid VSOCK port number") - .map(|p| p.parse::().unwrap()) + .ok_or_else(|| anyhow!("Need Hybrid VSOCK port number"))? + .parse::() .map_err(|e| anyhow!("VSOCK port number must be an integer: {:?}", e))?; let bundle_dir = args.value_of("bundle-dir").unwrap_or("").to_string(); @@ -218,7 +216,7 @@ fn real_main() -> Result<()> { .long("log-level") .short("l") .help("specific log level") - .default_value(logging::slog_level_to_level_name(DEFAULT_LOG_LEVEL).unwrap()) + .default_value(logging::slog_level_to_level_name(DEFAULT_LOG_LEVEL).map_err(|e| anyhow!(e))?) .possible_values(&logging::get_log_levels()) .takes_value(true) .required(false), @@ -304,35 +302,29 @@ fn real_main() -> Result<()> { let subcmd = args .subcommand_name() - .ok_or("need sub-command".to_string()) - .map_err(|e| anyhow!(e))?; + .ok_or_else(|| anyhow!("need sub-command"))?; match subcmd { "generate-cid" => { println!("{}", utils::random_container_id()); - return Ok(()); + Ok(()) } "generate-sid" => { println!("{}", utils::random_sandbox_id()); - return Ok(()); + Ok(()) } "examples" => { println!("{}", make_examples_text(name)); - return Ok(()); - } - "connect" => { - return connect(name, args); + Ok(()) } + "connect" => connect(name, args), _ => return Err(anyhow!(format!("invalid sub-command: {:?}", subcmd))), } } fn main() { - match real_main() { - Err(e) => { - eprintln!("ERROR: {}", e); - exit(1); - } - _ => (), - }; + if let Err(e) = real_main() { + eprintln!("ERROR: {}", e); + exit(1); + } } diff --git a/tools/packaging/kata-deploy/README.md b/tools/packaging/kata-deploy/README.md index 9f54f0a72..b6d455adc 100644 --- a/tools/packaging/kata-deploy/README.md +++ b/tools/packaging/kata-deploy/README.md @@ -4,8 +4,8 @@ and artifacts required to run Kata Containers, as well as reference DaemonSets, which can be utilized to install Kata Containers on a running Kubernetes cluster. -Note, installation through DaemonSets successfully installs `katacontainers.io/kata-runtime` on -a node only if it uses either containerd or CRI-O CRI-shims. +> **Note**: installation through DaemonSets successfully installs `katacontainers.io/kata-runtime` +> on a node only if it uses either containerd or CRI-O CRI-shims. ## Kubernetes quick start @@ -24,8 +24,8 @@ $ kubectl apply -f https://raw.githubusercontent.com/kata-containers/kata-contai The stable image refers to the last stable releases content. -Note that if you use a tagged version of the repo, the stable image does match that version. -For instance, if you use the 2.2.1 tagged version of the kata-deploy.yaml file, then the version 2.2.1 of the kata runtime will be deployed. +> **Note:** if you use a tagged version of the repo, the stable image does match that version. +> For instance, if you use the 2.2.1 tagged version of the kata-deploy.yaml file, then the version 2.2.1 of the kata runtime will be deployed. ```sh $ kubectl apply -f https://raw.githubusercontent.com/kata-containers/kata-containers/main/tools/packaging/kata-deploy/kata-rbac/base/kata-rbac.yaml @@ -165,7 +165,7 @@ This image contains all the necessary artifacts for running Kata Containers, all from the [Kata Containers release page](https://github.com/kata-containers/kata-containers/releases). Host artifacts: -* `cloud-hypervisor`, `firecracker`, `qemu-system-x86_64`, and supporting binaries +* `cloud-hypervisor`, `firecracker`, `qemu`, and supporting binaries * `containerd-shim-kata-v2` * `kata-collect-data.sh` * `kata-runtime` diff --git a/tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup-stable.yaml b/tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup-stable.yaml new file mode 100644 index 000000000..f1d9d0a2f --- /dev/null +++ b/tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup-stable.yaml @@ -0,0 +1,46 @@ +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: kubelet-kata-cleanup + namespace: kube-system +spec: + selector: + matchLabels: + name: kubelet-kata-cleanup + template: + metadata: + labels: + name: kubelet-kata-cleanup + spec: + serviceAccountName: kata-label-node + nodeSelector: + katacontainers.io/kata-runtime: cleanup + containers: + - name: kube-kata-cleanup + image: quay.io/kata-containers/kata-deploy:stable + imagePullPolicy: Always + command: [ "bash", "-c", "/opt/kata-artifacts/scripts/kata-deploy.sh reset" ] + env: + - name: NODE_NAME + valueFrom: + fieldRef: + fieldPath: spec.nodeName + securityContext: + privileged: false + volumeMounts: + - name: dbus + mountPath: /var/run/dbus + - name: systemd + mountPath: /run/systemd + volumes: + - name: dbus + hostPath: + path: /var/run/dbus + - name: systemd + hostPath: + path: /run/systemd + updateStrategy: + rollingUpdate: + maxUnavailable: 1 + type: RollingUpdate diff --git a/tools/packaging/kata-deploy/kata-deploy/base/kata-deploy-stable.yaml b/tools/packaging/kata-deploy/kata-deploy/base/kata-deploy-stable.yaml new file mode 100644 index 000000000..346e4c0ee --- /dev/null +++ b/tools/packaging/kata-deploy/kata-deploy/base/kata-deploy-stable.yaml @@ -0,0 +1,69 @@ +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: kata-deploy + namespace: kube-system +spec: + selector: + matchLabels: + name: kata-deploy + template: + metadata: + labels: + name: kata-deploy + spec: + serviceAccountName: kata-label-node + containers: + - name: kube-kata + image: quay.io/kata-containers/kata-deploy:stable + imagePullPolicy: Always + lifecycle: + preStop: + exec: + command: ["bash", "-c", "/opt/kata-artifacts/scripts/kata-deploy.sh cleanup"] + command: [ "bash", "-c", "/opt/kata-artifacts/scripts/kata-deploy.sh install" ] + env: + - name: NODE_NAME + valueFrom: + fieldRef: + fieldPath: spec.nodeName + securityContext: + privileged: false + volumeMounts: + - name: crio-conf + mountPath: /etc/crio/ + - name: containerd-conf + mountPath: /etc/containerd/ + - name: kata-artifacts + mountPath: /opt/kata/ + - name: dbus + mountPath: /var/run/dbus + - name: systemd + mountPath: /run/systemd + - name: local-bin + mountPath: /usr/local/bin/ + volumes: + - name: crio-conf + hostPath: + path: /etc/crio/ + - name: containerd-conf + hostPath: + path: /etc/containerd/ + - name: kata-artifacts + hostPath: + path: /opt/kata/ + type: DirectoryOrCreate + - name: dbus + hostPath: + path: /var/run/dbus + - name: systemd + hostPath: + path: /run/systemd + - name: local-bin + hostPath: + path: /usr/local/bin/ + updateStrategy: + rollingUpdate: + maxUnavailable: 1 + type: RollingUpdate diff --git a/tools/packaging/release/update-repository-version.sh b/tools/packaging/release/update-repository-version.sh index cded448cc..4703032cb 100755 --- a/tools/packaging/release/update-repository-version.sh +++ b/tools/packaging/release/update-repository-version.sh @@ -34,12 +34,8 @@ handle_error() { trap 'handle_error $LINENO' ERR get_changes() { - local current_version=$1 + local current_version="$1" [ -n "${current_version}" ] || die "current version not provided" - if [ "${current_version}" == "new" ];then - echo "Starting to version this repository" - return - fi # If for some reason there is not a tag this could fail # better fail and write the error in the PR @@ -62,9 +58,41 @@ get_changes() { git log --oneline "${current_version}..HEAD" --no-merges } +generate_kata_deploy_commit() { + local new_version="$1" + [ -n "$new_version" ] || die "no new version" + + printf "release: Adapt kata-deploy for %s" "${new_version}" + + printf "\n +kata-deploy files must be adapted to a new release. The cases where it +happens are when the release goes from -> to: +* main -> stable: + * kata-deploy / kata-cleanup: change from \"latest\" to \"rc0\" + * kata-deploy-stable / kata-cleanup-stable: are removed + +* stable -> stable: + * kata-deploy / kata-cleanup: bump the release to the new one. + +There are no changes when doing an alpha release, as the files on the +\"main\" branch always point to the \"latest\" and \"stable\" tags." +} + +generate_revert_kata_deploy_commit() { + local new_version="$1" + [ -n "$new_version" ] || die "no new version" + + printf "release: Revert kata-deploy changes after %s release" "${new_version}" + + printf "\n +As %s has been released, let's switch the kata-deploy / kata-cleanup +tags back to \"latest\", and re-add the kata-deploy-stable and the +kata-cleanup-stable files." "${new_version}" +} + generate_commit() { - local new_version=$1 - local current_version=$2 + local new_version="$1" + local current_version="$2" [ -n "$new_version" ] || die "no new version" [ -n "$current_version" ] || die "no current version" @@ -90,24 +118,25 @@ bump_repo() { pushd "${repo}" >>/dev/null + local kata_deploy_dir="tools/packaging/kata-deploy" + local kata_deploy_base="${kata_deploy_dir}/kata-deploy/base" + local kata_cleanup_base="${kata_deploy_dir}/kata-cleanup/base" + local kata_deploy_yaml="${kata_deploy_base}/kata-deploy.yaml" + local kata_cleanup_yaml="${kata_cleanup_base}/kata-cleanup.yaml" + local kata_deploy_stable_yaml="${kata_deploy_base}/kata-deploy-stable.yaml" + local kata_cleanup_stable_yaml="${kata_cleanup_base}/kata-cleanup-stable.yaml" + branch="${new_version}-branch-bump" git fetch origin "${target_branch}" git checkout "origin/${target_branch}" -b "${branch}" - # All repos we build should have a VERSION file - if [ ! -f "VERSION" ]; then - current_version="new" - echo "${new_version}" >VERSION - else - current_version="$(grep -v '#' ./VERSION)" + local current_version="$(egrep -v '^(#|$)' ./VERSION)" - info "Updating VERSION file" - echo "${new_version}" >VERSION - if git diff --exit-code; then - info "${repo} already in version ${new_version}" - cat VERSION - return 0 - fi + info "Updating VERSION file" + echo "${new_version}" >VERSION + if git diff --exit-code; then + info "${repo} already in version ${new_version}" + return 0 fi if [ "${repo}" == "kata-containers" ]; then @@ -117,61 +146,70 @@ bump_repo() { # 1) [main] ------> [main] NO-OP # "alpha0" "alpha1" # - # +----------------+----------------+ - # | from | to | - # -----------------+----------------+----------------+ - # kata-deploy | "latest" | "latest" | - # -----------------+----------------+----------------+ - # kata-deploy-base | "stable | "stable" | - # -----------------+----------------+----------------+ + # +----------------+----------------+ + # | from | to | + # -------------------+----------------+----------------+ + # kata-deploy | "latest" | "latest" | + # -------------------+----------------+----------------+ + # kata-deploy-stable | "stable | "stable" | + # -------------------+----------------+----------------+ # # # 2) [main] ------> [stable] Update kata-deploy and - # "alpha2" "rc0" get rid of kata-deploy-base + # "alpha2" "rc0" get rid of kata-deploy-stable # - # +----------------+----------------+ - # | from | to | - # -----------------+----------------+----------------+ - # kata-deploy | "latest" | "rc0" | - # -----------------+----------------+----------------+ - # kata-deploy-base | "stable" | REMOVED | - # -----------------+----------------+----------------+ + # +----------------+----------------+ + # | from | to | + # -------------------+----------------+----------------+ + # kata-deploy | "latest" | "rc0" | + # -------------------+----------------+----------------+ + # kata-deploy-stable | "stable" | REMOVED | + # -------------------+----------------+----------------+ # # # 3) [stable] ------> [stable] Update kata-deploy # "x.y.z" "x.y.(z+1)" # - # +----------------+----------------+ - # | from | to | - # -----------------+----------------+----------------+ - # kata-deploy | "x.y.z" | "x.y.(z+1)" | - # -----------------+----------------+----------------+ - # kata-deploy-base | NON-EXISTENT | NON-EXISTENT | - # -----------------+----------------+----------------+ + # +----------------+----------------+ + # | from | to | + # -------------------+----------------+----------------+ + # kata-deploy | "x.y.z" | "x.y.(z+1)" | + # -------------------+----------------+----------------+ + # kata-deploy-stable | NON-EXISTENT | NON-EXISTENT | + # -------------------+----------------+----------------+ + + local registry="quay.io/kata-containers/kata-deploy" info "Updating kata-deploy / kata-cleanup image tags" - if [ "${target_branch}" == "main" ] && [[ "${new_version}" =~ "rc" ]]; then - # case 2) - ## change the "latest" tag to the "#{new_version}" one - sed -i "s#quay.io/kata-containers/kata-deploy:latest#quay.io/kata-containers/kata-deploy:${new_version}#g" tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml - sed -i "s#quay.io/kata-containers/kata-deploy:latest#quay.io/kata-containers/kata-deploy:${new_version}#g" tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup.yaml + local version_to_replace="${current_version}" + local replacement="${new_version}" + if [ "${target_branch}" == "main" ]; then + if [[ "${new_version}" =~ "rc" ]]; then + ## this is the case 2) where we remove te kata-deploy / kata-cleanup stable files + git rm "${kata_deploy_stable_yaml}" + git rm "${kata_cleanup_stable_yaml}" + + else + ## this is the case 1) where we just do nothing + replacement="latest" + fi + version_to_replace="latest" + fi + + if [ "${version_to_replace}" != "${replacement}" ]; then + ## this covers case 2) and 3), as on both of them we have changes on kata-deploy / kata-cleanup files + sed -i "s#${registry}:${version_to_replace}#${registry}:${new_version}#g" "${kata_deploy_yaml}" + sed -i "s#${registry}:${version_to_replace}#${registry}:${new_version}#g" "${kata_cleanup_yaml}" git diff - git add tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml - git add tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup.yaml + git add "${kata_deploy_yaml}" + git add "${kata_cleanup_yaml}" - ## and remove the kata-deploy & kata-cleanup stable yaml files - git rm tools/packaging/kata-deploy/kata-deploy/base/kata-deploy-stable.yaml - git rm tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup-stable.yaml - elif [[ "${target_branch}" =~ "stable" ]]; then - # case 3) - sed -i "s#quay.io/kata-containers/kata-deploy:${current_version}#quay.io/kata-containers/kata-deploy:${new_version}#g" tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml - sed -i "s#quay.io/kata-containers/kata-deploy:${current_version}#quay.io/kata-containers/kata-deploy:${new_version}#g" tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup.yaml - git diff - - git add tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml - git add tools/packaging/kata-deploy/kata-cleanup/base/kata-cleanup.yaml + info "Creating the commit with the kata-deploy changes" + local commit_msg="$(generate_kata_deploy_commit $new_version)" + git commit -s -m "${commit_msg}" + local kata_deploy_commit="$(git rev-parse HEAD)" fi fi @@ -206,8 +244,31 @@ EOT ${hub_bin} push fork -f "${branch}" info "Create PR" out="" - out=$("${hub_bin}" pull-request -b "${target_branch}" -F "${notes_file}" 2>&1) || echo "$out" | grep "A pull request already exists" + out=$(LC_ALL=C LANG=C "${hub_bin}" pull-request -b "${target_branch}" -F "${notes_file}" 2>&1) || echo "$out" | grep "A pull request already exists" fi + + if [ "${repo}" == "kata-containers" ] && [ "${target_branch}" == "main" ] && [[ "${new_version}" =~ "rc" ]]; then + reverting_kata_deploy_changes_branch="revert-kata-deploy-changes-after-${new_version}-release" + git checkout -b "${reverting_kata_deploy_changes_branch}" + + git revert --no-edit ${kata_deploy_commit} >>/dev/null + commit_msg="$(generate_revert_kata_deploy_commit $new_version)" + info "Creating the commit message reverting the kata-deploy changes" + git commit --amend -s -m "${commit_msg}" + + echo "${commit_msg}" >"${notes_file}" + echo "" >>"${notes_file}" + echo "Only merge this commit after ${new_version} release is successfully tagged!" >>"${notes_file}" + + if [[ ${PUSH} == "true" ]]; then + info "Push \"${reverting_kata_deploy_changes_branch}\" to fork" + ${hub_bin} push fork -f "${reverting_kata_deploy_changes_branch}" + info "Create \"${reverting_kata_deploy_changes_branch}\" PR" + out="" + out=$(LC_ALL=C LANG=C "${hub_bin}" pull-request -b "${target_branch}" -F "${notes_file}" 2>&1) || echo "$out" | grep "A pull request already exists" + fi + fi + popd >>/dev/null } @@ -244,8 +305,8 @@ main(){ shift $((OPTIND - 1)) - new_version=${1:-} - target_branch=${2:-} + new_version="${1:-}" + target_branch="${2:-}" [ -n "${new_version}" ] || { echo "ERROR: no new version" && usage 1; } [ -n "${target_branch}" ] || die "no target branch" for repo in "${repos[@]}"