Add IntelRdt to CDI spec

[marquiz]

Add support for specifying the OCI Linux.IntelRdt configuration that is the control point for cache and memory bandwidth allocation technologies sucn as Intel RDT (Resource Director Technology). There can only be one IntelRdt configuration per container so in case multiple specs happened to specify it the last one applied prevails. Also, the IntelRdt configuration is always applied as a whole - editing specific sub-fields is not supported.

[marquiz]

Updated PR:

• Rebased
• Aligned schema fields, now they correspond one-to-one with the OCI spec
• Removed WIP status

[zvonkok]

@marquiz You may want to enclose CLOS as CLOS so that spell check succeeds.

[marquiz]

You may want to enclose CLOS as CLOS so that spell check succeeds.

Thx, did that. Lovely smart spell checker...

[elezar]

Thanks for the work here @marquiz.

I have some questions / comments below.

One thing that is also missing is automatically determining spec version 0.7.0 when calling MinimumRequiredVersion in version.go.

[elezar]

Question: Should we explicitly check for an empty IntelRdt object here, or is checking for a nil sufficient?

[marquiz]

I think checking nil is the right thing to do. From the OCI runtime-spec perspective (or the crun implementation, on it's behalf, IIRC) it's valid to have a non-nil IntelRdt in which all fields are empty. (In this case the runtime should create a container-specific CLOS with default settings).

[elezar]

What is the expected behaviour is both are non-nil? Here we overwrite the previous value if the input value is non-nil.

[kad]

intelRDT fiels in OCI spec can't have multiple values, so overwriting already set values theoretically expected behaviour. One thing which is questionable, is to go over individual values, instead of overwriting as a whole struct.

[marquiz]

Yes, it's like @kad explained. There can only be single RDT configuration per container. Also, the config must be applied as a whole (merging multiple RDT configs together is not something that we want).

[elezar]

As discussed in the COD working group meeting, I'm happy as long as this is consistent with what is done for other edits (Env, Mounts). It seems to be the case in that the last value applied takes precedence.

We also mentioned that if we do want to expose errors due to conflicts, we can do so in a follow-up.

One thing that would be good would be to add a unit test to capture this behaviour to prevent regressions.

[marquiz]

TODO:

• support version validation
• add unit test(s) for overrides

[elezar]

@marquiz here is the code to determine the automatic version:

container-device-interface/pkg/cdi/version.go

Line 122 in 7b12d28

func requiresV060(spec *cdi.Spec) bool {

Something like:

diff --git a/pkg/cdi/version.go b/pkg/cdi/version.go
index a617812..d2f024b 100644
--- a/pkg/cdi/version.go
+++ b/pkg/cdi/version.go
@@ -39,6 +39,7 @@ const (
 	v040 version = "v0.4.0"
 	v050 version = "v0.5.0"
 	v060 version = "v0.6.0"
+	v070 version = "v0.7.0"
 
 	// vEarliest is the earliest supported version of the CDI specification
 	vEarliest version = v030
@@ -54,6 +55,7 @@ var validSpecVersions = requiredVersionMap{
 	v040: requiresV040,
 	v050: requiresV050,
 	v060: requiresV060,
+	v070: requiresV070,
 }
 
 // MinimumRequiredVersion determines the minimum spec version for the input spec.
@@ -118,6 +120,20 @@ func (r requiredVersionMap) requiredVersion(spec *cdi.Spec) version {
 	return minVersion
 }
 
+func requiresV070(spec *cdi.Spec) bool {
+	if spec.ContainerEdits.IntelRdt != nil {
+		return true
+	}
+
+	for _, d := range spec.Devices {
+		if d.ContainerEdits.IntelRdt != nil {
+			return true
+		}
+	}
+
+	return false
+}
+
 // requiresV060 returns true if the spec uses v0.6.0 features
 func requiresV060(spec *cdi.Spec) bool {
 	// The v0.6.0 spec allows annotations to be specified at a spec level

[elezar]

@marquiz I have added the AdditionalGIDs extension as discussed yesterday in #179. I would expect some conflicts with your changes, so I'm happy to wait for your PR to land first.

[marquiz]

Thanks @elezar. I now updated the PR with the TODO items resolved:

1. Support for v0.7.0 version check was added (with unit tests)
2. Unit tests (TestApply) extended to cover overriding previous data

In addition I updated the TestAppend unit test to cover intelRdt field.

[elezar]

One last question / comment.

Otherwise LGTM.

[elezar]

Thanks @marquiz.

LGTM!