Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Pod annotations for selecting a custom hypervisor binary not working #3030

Closed
gkunz opened this issue Oct 27, 2020 · 2 comments · Fixed by #3031
Closed

Pod annotations for selecting a custom hypervisor binary not working #3030

gkunz opened this issue Oct 27, 2020 · 2 comments · Fixed by #3031
Assignees
Labels
bug Incorrect behaviour needs-backport Changes need to be applied to an older branch / repository needs-forward-port Changes need to be applied to a newer branch / repository

Comments

@gkunz
Copy link

gkunz commented Oct 27, 2020

Problem

We have a use case in which we'd like to select custom components of the the Kata sandbox (kernel, image, hypervisor) using pod annotations. Following the corresponding documentation [1], setting the kernel and image via pod annotations works fine, but the custom hypervisor binary gets ignored and instead the default hypervisor as configured in the config file is used.

Example workload definition:

  apiVersion: v1
  kind: Pod
  metadata:
    name: ubuntu-untrusted-custom-kernel
    annotations:
      # works
      io.katacontainers.config.hypervisor.kernel: "/tmp/kata_custom_sandbox/kernel/vmlinuz.container"
      # works
      io.katacontainers.config.hypervisor.image: "/tmp/kata_custom_sandbox/image/kata-containers.img"
      # gets ignored
      io.katacontainers.config.hypervisor.path: /tmp/kata_custom_sandbox/hypervisor/qemu-virtiofs-system-x86_64
  spec:
    runtimeClassName: kata-qemu
    containers:
      - name: ubuntu
        image: ubuntu
        command: ['bash', '-c', 'sleep infinity']

I am running Kata 1.12-alpha as deployed (some time ago) by kata-deploy.

Expected result

The sandbox should be run by the custom hypervisor specified by the pod annotation.

Actual result

The sandbox is being run by the default hypervisor according to the runtime class definition.

Further information

Looking at the code, it seems that the custom "hypervisor asset" is not considered here [2] and here [3] and so the pod annotations are not passed down to sandbox. Those links point to the Kata 1.x repos, but it seems the same situation is true in the 2.0 repo.

[1] https://github.com/kata-containers/documentation/blob/master/how-to/how-to-set-sandbox-config-kata.md
[2] https://github.com/kata-containers/runtime/blob/master/virtcontainers/pkg/oci/utils.go#L353
[3] https://github.com/kata-containers/runtime/blob/master/virtcontainers/sandbox.go#L441

@gkunz gkunz added bug Incorrect behaviour needs-review Needs to be assessed by the team. labels Oct 27, 2020
@jodh-intel jodh-intel added bug Incorrect behaviour needs-forward-port Changes need to be applied to a newer branch / repository and removed bug Incorrect behaviour needs-review Needs to be assessed by the team. labels Oct 28, 2020
@jodh-intel
Copy link
Contributor

Thanks for raising @gkunz. I can confirm this behaviour.

@jodh-intel jodh-intel added the needs-backport Changes need to be applied to an older branch / repository label Oct 28, 2020
@jodh-intel jodh-intel self-assigned this Oct 28, 2020
@c3d
Copy link
Member

c3d commented Oct 28, 2020

I had noticed the discrepancy while working on #3005. I should have asked, I assumed there was something intentional in the list given in addAssetAnnotations.

jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 28, 2020
Fix `createAssets()` by adding in the missing annotations for:

- firmware:
  - `io.katacontainers.config.hypervisor.firmware`
  - `io.katacontainers.config.hypervisor.firmware_hash`

- hypervisor:
  - `io.katacontainers.config.hypervisor.path`
  - `io.katacontainers.config.hypervisor.hypervisor_hash`

- hypervisor control binary:
  - `io.katacontainers.config.hypervisor.ctlpath`
  - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

- jailer:
  - `io.katacontainers.config.hypervisor.jailer_path`
  - `io.katacontainers.config.hypervisor.jailer_hash`

This fixes the issue where a custom hypervisor binary annotation was
being ignored.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 29, 2020
Make the `assets` package the arbiter of the asset annotations by
removing all asset annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 29, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Oct 29, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Nov 4, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/runtimes that referenced this issue Nov 4, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
amshinde pushed a commit to amshinde/kata-runtime that referenced this issue Nov 10, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
(cherry picked from commit 6a5eb0d)
amshinde pushed a commit to amshinde/kata-runtime that referenced this issue Nov 10, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
(cherry picked from commit 6a5eb0d)
amshinde pushed a commit to amshinde/kata-runtime that referenced this issue Nov 11, 2020
Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: kata-containers#3030.

Signed-off-by: James O. D. Hunt <[email protected]>
(cherry picked from commit 6a5eb0d)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Incorrect behaviour needs-backport Changes need to be applied to an older branch / repository needs-forward-port Changes need to be applied to a newer branch / repository
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants