annotations: Improve asset annotation handling #3031

jodh-intel · 2020-10-28T15:40:31Z

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: #3030.

Signed-off-by: James O. D. Hunt [email protected]

jodh-intel · 2020-10-28T15:40:39Z

/test

jodh-intel · 2020-10-28T15:54:55Z

Minimal manual test (based on https://github.com/kata-containers/documentation/blob/master/Developer-Guide.md#running-standalone):

$ bundle="/tmp/bundle"
$ rootfs="$bundle/rootfs"
$ mkdir -p "$rootfs" && (cd "$bundle" && kata-runtime spec)
$ sudo docker export $(sudo docker create busybox) | tar -C "$rootfs" -xvf -
$ (cd $bundle && jq '. |= . + {"annotations": {"io.katacontainers.config.hypervisor.path": "/does/not/exist"}}' < config.json > tmp && mv tmp config.json)
$ sudo kata-runtime --log=/dev/stdout run --bundle "$bundle" foo

The above will work (incorrectly) with current releases, but will fail with this PR (assuming /does/not/exist really doesn't exist ;)

/cc @gkunz

gkunz · 2020-10-28T16:02:50Z

@jodh-intel thanks for the quick fix! I'll test this PR as well in my lab.

codecov · 2020-10-28T16:15:44Z

Codecov Report

Merging #3031 into master will increase coverage by 0.05%.
The diff coverage is 60.60%.

@@            Coverage Diff             @@
##           master    #3031      +/-   ##
==========================================
+ Coverage   50.25%   50.30%   +0.05%     
==========================================
  Files         120      120              
  Lines       15844    15840       -4     
==========================================
+ Hits         7963     7969       +6     
+ Misses       6799     6786      -13     
- Partials     1082     1085       +3

jodh-intel · 2020-10-28T16:24:01Z

@GabyCT - jenkins-ci-podman-clh-fedora-31 is failing with:

16:08:18 ./scripts/dev_cli.sh: line 122: docker: command not found

GabyCT · 2020-10-28T16:41:54Z

@jodh-intel kata-containers/ci#340 /cc @likebreath @jcvenegas

gkunz · 2020-10-28T17:05:48Z

virtcontainers/pkg/oci/utils.go

-		vcAnnotations.KernelPath,
-		vcAnnotations.ImagePath,
-		vcAnnotations.InitrdPath,
+		vcAnnotations.AssetHashType,


are you not missing vcAnnotations.HypervisorPath here? In my local test of this PR, the hypervisor annotations still gets ignored. Upon including vcAnnotations.HypervisorPath here, it works.

@gkunz - apologies - git stash fail :) I've actually found a way to make the code more robust, so marking as do-not-merge for now. I'll push an update tomorrow...

@jodh-intel no problem. Thanks again for addressing the issue so quickly!

devimc

thanks @jodh-intel

Improve formatting, grammar and whitespace. Signed-off-by: James O. D. Hunt <[email protected]>

Add missing annotation definitions for a hypervisor control binary: - `io.katacontainers.config.hypervisor.ctlpath` - `io.katacontainers.config.hypervisor.hypervisorctl_hash` Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2020-10-29T10:12:25Z

/test

jodh-intel · 2020-10-29T10:18:09Z

/test

jodh-intel · 2020-10-29T12:08:52Z

virtcontainers/hypervisor.go

-	// We could not find a custom asset for the given type, let's
-	// fall back to the configured ones.
-	switch t {
-	case types.KernelAsset:
-		return conf.KernelPath, nil
-	case types.ImageAsset:
-		return conf.ImagePath, nil
-	case types.InitrdAsset:
-		return conf.InitrdPath, nil
-	case types.HypervisorAsset:
-		return conf.HypervisorPath, nil
-	case types.HypervisorCtlAsset:
-		return conf.HypervisorCtlPath, nil
-	case types.JailerAsset:
-		return conf.JailerPath, nil
-	case types.FirmwareAsset:
-		return conf.FirmwarePath, nil
-	default:
-		return "", fmt.Errorf("Unknown asset type %v", t)
+	assetPath, _, err := t.Annotations()
+	if err != nil {
+		return "", err
 	}
+
+	return assetPath, nil


This is incorrect as it changes the function to return the asset annotation name, not the config value. I'm about to push a fix and a new TestAssetPath() test to assert the expected behaviour. However, I'm going to have to revert this change meaning we will still have a "list of asset annotations" outside of asset.go. This is annoying as I'd like to totally encapsulate that list in asset.go.

One possible improvement would be to annotate HypervisorConfig with custom struct tags like this:

type HypervisorConfig struct { FirmwarePath string `annotation:"io.katacontainers.config.hypervisor.firmware"` HypervisorCtlPath string `annotation:"io.katacontainers.config.hypervisor.ctlpath"` HypervisorPath string `annotation:"io.katacontainers.config.hypervisor.path"` ImagePath string `annotation:"io.katacontainers.config.hypervisor.image"` InitrdPath string `annotation:"io.katacontainers.config.hypervisor.initrd"` JailerPath string `annotation:"io.katacontainers.config.hypervisor.jailer_path"` KernelPath string `annotation:"io.katacontainers.config.hypervisor.kernel"` }

That may make the code clearer and less error prone. However, there would be a runtime impact as we'd need to use reflect to map the struct tag (annotation) back to the config value.

@jodh-intel I like the idea of encapsulating all the assets constants and logic in assets.go, feel that it is all over the place right now. Maybe using reflection is not such a bad idea if we can cleanup the code and make sure that we do this once in the CreateSandbox code path.
We can address that in a separate PR.

What I did see is that the hypervisor.go has a lot of functions CustomFirmwareAsset, CustomJailerAsset etc which are not being called at all.
We need to assess if these functions should be called from getHypervisorDetails or we can just get rid of them.

jodh-intel · 2020-10-29T12:09:47Z

/test

jodh-intel · 2020-10-29T12:40:34Z

HI @gkunz - I've updated the branch now if you'd like to give the latest a try?

gkunz · 2020-10-29T13:17:56Z

@jodh-intel works nicely for me!

jodh-intel · 2020-10-29T13:40:02Z

Thanks for confirming @gkunz !

jodh-intel · 2020-10-29T17:04:44Z

Added dnm label to allow for a few more eyes to review before this lands.

jodh-intel · 2020-10-30T18:02:11Z

@amshinde - any further thoughts on this? I've updated since you approved I think.

amshinde · 2020-10-30T20:40:17Z

virtcontainers/types/asset.go

+	}
+
+	// Add the odd one out; it isn't part of a (path+hash) pair.
+	result = append(result, annotations.AssetHashType)


We dont need to address this in this PR.
But raising my concern about value of supporting this. Maybe we should just support sha512 hash and document it so, rather than providing this.

amshinde · 2020-10-30T20:57:33Z

virtcontainers/types/asset.go

+// AssetTypes returns a list of all known asset types.
+//
+// XXX: New asset types *MUST* be added here.
+func AssetTypes() []AssetType {


Nit: Suggestion to move all the consts for FirmwareAsset, HypervisorAsset etc to the top of the file for better readability.

amshinde · 2020-10-30T21:00:01Z

virtcontainers/types/asset.go

+		JailerAsset,
+		KernelAsset,
+	}
+}


In terms of cleanup, there is another function called Valid() which could make use of this list instead of comparing against individual values.

amshinde · 2020-10-30T22:26:50Z

virtcontainers/hypervisor.go

-	// We could not find a custom asset for the given type, let's
-	// fall back to the configured ones.
-	switch t {
-	case types.KernelAsset:
-		return conf.KernelPath, nil
-	case types.ImageAsset:
-		return conf.ImagePath, nil
-	case types.InitrdAsset:
-		return conf.InitrdPath, nil
-	case types.HypervisorAsset:
-		return conf.HypervisorPath, nil
-	case types.HypervisorCtlAsset:
-		return conf.HypervisorCtlPath, nil
-	case types.JailerAsset:
-		return conf.JailerPath, nil
-	case types.FirmwareAsset:
-		return conf.FirmwarePath, nil
-	default:
-		return "", fmt.Errorf("Unknown asset type %v", t)
+	assetPath, _, err := t.Annotations()
+	if err != nil {
+		return "", err
 	}
+
+	return assetPath, nil


@jodh-intel I like the idea of encapsulating all the assets constants and logic in assets.go, feel that it is all over the place right now. Maybe using reflection is not such a bad idea if we can cleanup the code and make sure that we do this once in the CreateSandbox code path.
We can address that in a separate PR.

What I did see is that the hypervisor.go has a lot of functions CustomFirmwareAsset, CustomJailerAsset etc which are not being called at all.
We need to assess if these functions should be called from getHypervisorDetails or we can just get rid of them.

jodh-intel · 2020-11-04T12:26:51Z

@amshinde - I've removed the unused functions. On reflection (:smile:), I'm not totally convinced about my reflection idea: there
is another problem of using that technique - we can't use variables in the struct tags so we'd end up with another maintenance issue as it would be easy to rename an annotation variable value, but forget to update the corresponding static struct tag annotation string. Sigh.

Let's ponder this some more and make further improvements if possible on a follow-up PR.

For now, I'd like to get this landed so it can be ported to 2.x.

jodh-intel · 2020-11-04T12:28:03Z

/test

amshinde · 2020-11-04T17:55:11Z

Let's ponder this some more and make further improvements if possible on a follow-up PR.

For now, I'd like to get this landed so it can be ported to 2.x.

Agree. SGTM.

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]>

Deleted `HypervisorConfig`'s unused `CustomFirmwareAsset()` and `JailerAssetPath()` methods. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2020-11-04T17:59:59Z

/test

jodh-intel · 2020-11-05T12:18:04Z

forward port PR: kata-containers/kata-containers#1086

The changes introduced in PR kata-containers#3031 require additional changes. Code lifted from kata-containers/kata-containers#1086. Fixes: kata-containers#3005 Suggested-by: James O.D. Hunt <[email protected]> Signed-off-by: Christophe de Dinechin <[email protected]>

jodh-intel added needs-forward-port Changes need to be applied to a newer branch / repository needs-backport Changes need to be applied to an older branch / repository labels Oct 28, 2020

amshinde approved these changes Oct 28, 2020

View reviewed changes

gkunz suggested changes Oct 28, 2020

View reviewed changes

jodh-intel added the do-not-merge PR has problems or depends on another label Oct 28, 2020

devimc approved these changes Oct 28, 2020

View reviewed changes

jodh-intel added 2 commits October 29, 2020 08:51

asset: Formatting, grammar and whitespace

f53406f

Improve formatting, grammar and whitespace. Signed-off-by: James O. D. Hunt <[email protected]>

annotations: Add missing hypervisor control annotation

7d9860d

Add missing annotation definitions for a hypervisor control binary: - `io.katacontainers.config.hypervisor.ctlpath` - `io.katacontainers.config.hypervisor.hypervisorctl_hash` Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the fix-annotations branch from a8df626 to 935e607 Compare October 29, 2020 10:12

jodh-intel removed the do-not-merge PR has problems or depends on another label Oct 29, 2020

jodh-intel changed the title ~~annotations: Add missing asset annotations~~ annotations: Improve asset annotation handling Oct 29, 2020

jodh-intel force-pushed the fix-annotations branch from 935e607 to d1db6aa Compare October 29, 2020 10:17

jodh-intel commented Oct 29, 2020

View reviewed changes

jodh-intel force-pushed the fix-annotations branch from d1db6aa to 6a31139 Compare October 29, 2020 12:08

gkunz approved these changes Oct 29, 2020

View reviewed changes

jodh-intel added the do-not-merge PR has problems or depends on another label Oct 29, 2020

amshinde reviewed Oct 30, 2020

View reviewed changes

jodh-intel force-pushed the fix-annotations branch from 9f1c7e9 to 0f8b94b Compare November 4, 2020 12:22

jodh-intel removed the do-not-merge PR has problems or depends on another label Nov 4, 2020

jodh-intel added 2 commits November 4, 2020 17:59

hypervisor: Remove unused methods

4ce09fb

Deleted `HypervisorConfig`'s unused `CustomFirmwareAsset()` and `JailerAssetPath()` methods. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the fix-annotations branch from 0f8b94b to 4ce09fb Compare November 4, 2020 17:59

jodh-intel merged commit 3108536 into kata-containers:master Nov 5, 2020

jodh-intel mentioned this pull request Nov 5, 2020

Validate annotations that refer to binaries #3005

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

annotations: Improve asset annotation handling #3031

annotations: Improve asset annotation handling #3031

jodh-intel commented Oct 28, 2020 •

edited

Loading

jodh-intel commented Oct 28, 2020

jodh-intel commented Oct 28, 2020

gkunz commented Oct 28, 2020

codecov bot commented Oct 28, 2020 •

edited

Loading

jodh-intel commented Oct 28, 2020

GabyCT commented Oct 28, 2020

gkunz Oct 28, 2020

jodh-intel Oct 28, 2020

gkunz Oct 28, 2020

devimc left a comment

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

jodh-intel Oct 29, 2020

amshinde Oct 30, 2020

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

gkunz commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 30, 2020

amshinde Oct 30, 2020

jodh-intel Nov 4, 2020

amshinde Oct 30, 2020

jodh-intel Nov 4, 2020

amshinde Oct 30, 2020

jodh-intel Nov 4, 2020

amshinde Oct 30, 2020

jodh-intel commented Nov 4, 2020

jodh-intel commented Nov 4, 2020

amshinde commented Nov 4, 2020

jodh-intel commented Nov 4, 2020

jodh-intel commented Nov 5, 2020

annotations: Improve asset annotation handling #3031

annotations: Improve asset annotation handling #3031

Conversation

jodh-intel commented Oct 28, 2020 • edited Loading

jodh-intel commented Oct 28, 2020

jodh-intel commented Oct 28, 2020

gkunz commented Oct 28, 2020

codecov bot commented Oct 28, 2020 • edited Loading

Codecov Report

jodh-intel commented Oct 28, 2020

GabyCT commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc left a comment

Choose a reason for hiding this comment

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

gkunz commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 29, 2020

jodh-intel commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Nov 4, 2020

jodh-intel commented Nov 4, 2020

amshinde commented Nov 4, 2020

jodh-intel commented Nov 4, 2020

jodh-intel commented Nov 5, 2020

jodh-intel commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 28, 2020 •

edited

Loading