Added Annotations to CDI Spec #85

zvonkok · 2022-10-19T10:32:51Z

Based on the discussion in #30 some generated devices like VFs need extra meta information to be handled correctly in virtualized environments.

zvonkok · 2022-10-19T10:37:47Z

For Kata we need to pass through the VF into the container, a VF can have different types and with the BDF the runtime can deduce the right handling. An example CDI spec would look like:

# /etc/cdi/nvidia.yaml
cdiVersion: 0.4.0
kind: nvidia.com/gpu
devices:
- name: gpu0
  annotations:
    bdf: “41:00.0”
    clique-id: “0”
  containerEdits:
    deviceNodes:
    - path: “/dev/vfio/71"

elezar

Thanks @zvonkok. Could you also provide a short description on how these will be used.

Some other points:

This requires a version bump of the spec. Let us do that now.
Could we add an example to the readme etc. where this is specified?

specs-go/config.go

zvonkok · 2022-11-02T10:39:41Z

Refactored the README according to md-lint suggestions.

kad

Beside clarification on usage of annotation field, it is ok from my side.

One side note noticed during this PR review, probably good for another PR: I've noticed that in our schema doesn't specify "global" containerEdits fields in schema/schema.json

specs-go/config.go

README.md

schema/defs.json

zvonkok · 2022-11-03T13:42:45Z

@kad @klihub @elezar I added validation of annotations PTAL; I have a general question why are we allowing an empty CDI spec to be valid? What is the purpose?
For validating annotations, I am at least assuming to have Kind and some other fields to be set.

# /etc/cdi/nvidia.yaml
cdiVersion: 0.4.0
kind: nvidia.com/gpu
annotations: 
  global: "true"
devices:
- name: gpu0
  annotations:
    bdf: “41:00.0”
    badfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadfasdfasdfasdfasdfasfasdfasfsddf: “41:00.0”
    clique-id: “0”
  containerEdits:
    deviceNodes:
    - path: “/dev/vfio/71"

# bin/validate /tmp/nvidia.yaml
Building bin/validate...
Validating against JSON schema builtin...
/tmp/nvidia.yaml: validation failed:
    [[]: Invalid value: "badfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfadfasdfasdfasdfasdfasfasdfasfsddf": name part must be no more than 63 characters]

specs-go/config.go

elezar

Thanks for adding the validation @zvonkok. Some comments -- specifically about when this validation is triggered.

schema/schema.go

schema/schema_test.go

specs-go/config.go

klihub · 2022-11-17T18:37:17Z

I have a general question why are we allowing an empty CDI spec to be valid? What is the purpose?

AFAICT, it is partly for historical reasons. IIRC how it came to be, Renaud's initial code was explicitly allowing for it, so we kept that functionality. My guess is that the orignal idea was to let vendor-specific utilities run at boot-time and generate vendor-specific 'static' Spec files in /etc (maybe by scanning /dev, and/or doing udev enumeration) and allow any empty files to indicate that the utilities did run (maybe using comments to that effect in the generated Spec files) but not trigger errors in the actual cdi Spec-handling packages. But I'm just guessing here...

elezar

@zvonkok I have reworked this PR so that it can be merged into the current main branch. This includes checks for the minimum version, for example.

I have also reworked the annotation validation to reduce the imported k8s dependencies.

@klihub @bart0sh @kad since I've also updated this PR please take a look now.

I updated the branch myself.

bart0sh · 2023-02-15T12:15:25Z

It would be nice to cover new code with unit tests.
Other than that lgtm.

klihub · 2023-02-20T08:39:49Z

This needs a rebase and conflict resolution. Other than that, I think it looks fine.

Signed-off-by: Evan Lezar <[email protected]>

This change adds an internal validation package for spec annotation validation. This includes code imported from k8s.io/apimachinery/pkg/api and k8s.io/apimachinery/pkg/util to allow for basic validation of the annotations without importing additional dependencies. Signed-off-by: Evan Lezar <[email protected]>

This change allows a set of annotations to be specified at a spec level as well as per device defined in the spec. This bumps the spec version to 0.6.0. Note that validation of the contents of the annotations is also included at a schema level. Signed-off-by: Zvonko Kaiser <[email protected]> Signed-off-by: Evan Lezar <[email protected]>

Signed-off-by: Evan Lezar <[email protected]>

elezar · 2023-02-20T08:45:45Z

This needs a rebase and conflict resolution. Other than that, I think it looks fine.

Done.

elezar · 2023-02-20T10:13:29Z

internal/validation/validate.go

+)
+
+// ValidateSpecAnnotations checks whether spec annotations are valid.
+func ValidateSpecAnnotations(name string, any interface{}) error {


@klihub I believe you had questions / concerns about the validation of the annotations.

klihub · 2023-02-20T11:51:02Z

internal/validation/validate.go

+)
+
+// ValidateSpecAnnotations checks whether spec annotations are valid.
+func ValidateSpecAnnotations(name string, any interface{}) error {


What is the reason for not having the map[string]string type for annotations as in the Spec itself ?

This function is called from the schema validation, where all we have is an interface{} type. This handles the conversion of this type to a map[string]string. Since this is required for two separate parts of the schema / spec (top-level annotations and device-level annotation), I added this helper here.

Maybe I can rename this helper to ValidateSpecAnnotationSchema instead.

Antother option is to drop the schema validation entirely which would clean up this PR significantly.

Yes, it is sort of tempting... Then again, if at all possible it is better to catch Spec errors early (when seen first time by the CDI package) than late (when an injection goes wrong in the runtime). So I guess then some basic annotation verification would be a good idea.

That said, I don't know if these checks are really that helpful in that regard. AFAICT we don't plan to inject any of these annotations to a container and we are doing K8s pod/container annotations verification here. If we don't inject them, some entity somewhere along the runtime-runc/crun/kata path will do some semantic interpretation of the annotated data... which inevitable leaves the door open for delayed post-injection errors.

zvonkok · 2023-03-13T09:49:08Z

@kad @klihub @bart0sh Anything missing to merge this?

klihub · 2023-03-13T10:03:13Z

@zvonkok Not from my point of view. As I stated in my last review comment I'm fine with merging this as such, or if @elezar wants to drop ValidateSpecAnnotations, I'm also fine with that.

elezar · 2023-03-13T10:33:53Z

@zvonkok Not from my point of view. As I stated in my last review comment I'm fine with merging this as such, or if @elezar wants to drop ValidateSpecAnnotations, I'm also fine with that.

Since we don't validate by default, I think we can leave it in for the time being. If you're good with it @klihub let's get this merged and I we can bump a new version.

internal/validation/k8s/objectmeta.go

internal/validation/validate.go

bart0sh · 2023-03-13T10:53:08Z

pkg/cdi/version.go

+	// The v0.6.0 spec allows annotations to be specified at a device level
+	for _, d := range spec.Devices {
+		for range d.Annotations {
+			return true


Do we need a loop here if we unconditionally return on the first iteration?

The intent of the loop is to detect whether a v0.6.0 spec is required. This is required if ANY annotions are detected.

We could use:

for _, d := range spec.Devices { if len(d.Annotations) > 0 { return true

instead, but I thought the loop was cleaner than the conditional in this case.

Not for me, but I'm ok with it.

klihub

LGTM.

zvonkok force-pushed the annotations branch from b3af822 to ee41dd9 Compare October 19, 2022 10:33

zvonkok force-pushed the annotations branch from ee41dd9 to a00c08b Compare October 20, 2022 09:38

zvonkok requested a review from elezar October 20, 2022 09:39

elezar previously requested changes Oct 20, 2022

View reviewed changes

specs-go/config.go Show resolved Hide resolved

klihub reviewed Oct 24, 2022

View reviewed changes

specs-go/config.go Show resolved Hide resolved

zvonkok force-pushed the annotations branch from a00c08b to 815344b Compare November 2, 2022 10:38

zvonkok force-pushed the annotations branch from 815344b to 8bcb078 Compare November 2, 2022 10:42

kad reviewed Nov 2, 2022

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

elezar reviewed Nov 2, 2022

View reviewed changes

schema/defs.json Show resolved Hide resolved

zvonkok force-pushed the annotations branch from 8bcb078 to 1d1b057 Compare November 3, 2022 13:39

kad reviewed Nov 3, 2022

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

elezar reviewed Nov 9, 2022

View reviewed changes

schema/schema.go Outdated Show resolved Hide resolved

schema/schema.go Outdated Show resolved Hide resolved

schema/schema.go Outdated Show resolved Hide resolved

schema/schema_test.go Outdated Show resolved Hide resolved

specs-go/config.go Outdated Show resolved Hide resolved

zvonkok force-pushed the annotations branch 4 times, most recently from 6b78d1c to 5080b88 Compare November 17, 2022 09:49

elezar force-pushed the annotations branch from 1f23367 to 5080b88 Compare November 25, 2022 14:08

elezar force-pushed the annotations branch from 5080b88 to 16d2508 Compare February 9, 2023 15:12

elezar self-requested a review February 9, 2023 15:12

elezar force-pushed the annotations branch 2 times, most recently from 56ddae8 to 8751388 Compare February 9, 2023 15:20

elezar reviewed Feb 9, 2023

View reviewed changes

elezar requested review from elezar and removed request for elezar February 9, 2023 15:22

elezar requested a review from klihub February 10, 2023 12:54

klihub requested review from bart0sh and kad February 14, 2023 13:17

elezar and others added 4 commits February 20, 2023 10:41

Add bin to .gitignore

84d2793

Signed-off-by: Evan Lezar <[email protected]>

Include annotation validation when loading spec

4dd97f6

Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the annotations branch from 8751388 to 4dd97f6 Compare February 20, 2023 08:45

elezar reviewed Feb 20, 2023

View reviewed changes

klihub reviewed Feb 20, 2023

View reviewed changes

Merge branch 'main' into annotations

7321bf2

bart0sh reviewed Mar 13, 2023

View reviewed changes

internal/validation/k8s/objectmeta.go Show resolved Hide resolved

bart0sh reviewed Mar 13, 2023

View reviewed changes

internal/validation/validate.go Show resolved Hide resolved

bart0sh reviewed Mar 13, 2023

View reviewed changes

klihub approved these changes Mar 13, 2023

View reviewed changes

bart0sh merged commit 9c1a1bd into cncf-tags:main Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Annotations to CDI Spec #85

Added Annotations to CDI Spec #85

zvonkok commented Oct 19, 2022 •

edited

Loading

zvonkok commented Oct 19, 2022 •

edited by elezar

Loading

elezar left a comment

zvonkok commented Nov 2, 2022

kad left a comment

zvonkok commented Nov 3, 2022 •

edited

Loading

elezar left a comment

klihub commented Nov 17, 2022

elezar left a comment

bart0sh commented Feb 15, 2023

klihub commented Feb 20, 2023

elezar commented Feb 20, 2023

elezar Feb 20, 2023

klihub Feb 20, 2023

elezar Feb 20, 2023

elezar Feb 20, 2023

klihub Feb 20, 2023 •

edited

Loading

zvonkok commented Mar 13, 2023

klihub commented Mar 13, 2023

elezar commented Mar 13, 2023

bart0sh Mar 13, 2023

elezar Mar 14, 2023

bart0sh Mar 14, 2023

klihub left a comment

Added Annotations to CDI Spec #85

Added Annotations to CDI Spec #85

Conversation

zvonkok commented Oct 19, 2022 • edited Loading

zvonkok commented Oct 19, 2022 • edited by elezar Loading

elezar left a comment

Choose a reason for hiding this comment

zvonkok commented Nov 2, 2022

kad left a comment

Choose a reason for hiding this comment

zvonkok commented Nov 3, 2022 • edited Loading

elezar left a comment

Choose a reason for hiding this comment

klihub commented Nov 17, 2022

elezar left a comment

Choose a reason for hiding this comment

bart0sh commented Feb 15, 2023

klihub commented Feb 20, 2023

elezar commented Feb 20, 2023

elezar Feb 20, 2023

Choose a reason for hiding this comment

klihub Feb 20, 2023

Choose a reason for hiding this comment

elezar Feb 20, 2023

Choose a reason for hiding this comment

elezar Feb 20, 2023

Choose a reason for hiding this comment

klihub Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

zvonkok commented Mar 13, 2023

klihub commented Mar 13, 2023

elezar commented Mar 13, 2023

bart0sh Mar 13, 2023

Choose a reason for hiding this comment

elezar Mar 14, 2023

Choose a reason for hiding this comment

bart0sh Mar 14, 2023

Choose a reason for hiding this comment

klihub left a comment

Choose a reason for hiding this comment

zvonkok commented Oct 19, 2022 •

edited

Loading

zvonkok commented Oct 19, 2022 •

edited by elezar

Loading

zvonkok commented Nov 3, 2022 •

edited

Loading

klihub Feb 20, 2023 •

edited

Loading