Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IntelRdt to CDI spec #164

Merged
merged 1 commit into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .codespellrc
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
[codespell]
skip = .git,*.pdf,*.svg,*.sum,*.mod
#
# ignore-words-list =
ignore-words-list = clos
22 changes: 19 additions & 3 deletions SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

## Version

This is CDI **spec** version **0.6.0**.
This is CDI **spec** version **0.7.0**.

### Update policy

Expand All @@ -27,7 +27,8 @@ Released versions of the spec are available as Git tags.
| v0.4.0 | | Added `type` field to Mount specification |
| v0.5.0 | | Add `HostPath` to `DeviceNodes` |
| v0.6.0 | | Add `Annotations` field to `Spec` and `Device` specifications |
| | | Allow dots (`.`) in name segment of `Kind` field |
| | | Allow dots (`.`) in name segment of `Kind` field. |
| v0.7.0 | | Add `IntelRdt`field. |

*Note*: The initial release of a **spec** with version `v0.x.0` will be tagged as
`v0.x.0` with subsequent changes to the API applicable to this version tagged as `v0.x.y`.
Expand Down Expand Up @@ -82,7 +83,7 @@ The keywords "must", "must not", "required", "shall", "shall not", "should", "sh

```
{
"cdiVersion": "0.6.0",
"cdiVersion": "0.7.0",
"kind": "<name>",

// This field contains a set of key-value pairs that may be used to provide
Expand Down Expand Up @@ -150,6 +151,13 @@ The keywords "must", "must not", "required", "shall", "shall not", "should", "sh
"timeout": <int> (optional)
}
]
"intelRdt": { (optional)
"closID": "<name>", (optional)
"l3CacheSchema": "string" (optional)
"memBwSchema": "string" (optional)
"enableCMT": "<boolean>" (optional)
"enableMBM": "<boolean>" (optional)
}
}
]
}
Expand Down Expand Up @@ -220,6 +228,12 @@ The `containerEdits` field has the following definition:
* `args` (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008 execv's argv.
* `env` (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008's environ.
* `timeout` (int, OPTIONAL) is the number of seconds before aborting the hook. If set, timeout MUST be greater than zero. If not set container runtime will wait for the hook to return.
* `intelRdt` (object, OPTIONAL) describes the Linux [resctrl][resctrl] settings for the container (object, OPTIONAL)
* `closID` (string, OPTIONAL) name of the `CLOS` (Class of Service).
* `l3CacheSchema` (string, OPTIONAL) L3 cache allocation schema for the `CLOS`.
* `memBwSchema` (string, OPTIONAL) memory bandwidth allocation schema for the `CLOS`.
* `enableCMT` (boolean, OPTIONAL) whether to enable cache monitoring
* `enableMBM` (boolean, OPTIONAL) whether to enable memory bandwidth monitoring

## Error Handling
* Kind requested is not present in any CDI file.
Expand All @@ -231,3 +245,5 @@ The `containerEdits` field has the following definition:
This is because a resource does not need to exist when the spec is written, but it needs to exist when the container is created.
* Hook fails to execute.
Container runtimes should surface an error when hooks fails to execute.

[resctrl]: https://docs.kernel.org/arch/x86/resctrl.html
26 changes: 25 additions & 1 deletion pkg/cdi/container-edits.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error {
}
}

if e.IntelRdt != nil {
// The specgen is missing functionality to set all parameters so we
// just piggy-back on it to initialize all structs and the copy over.
specgen.SetLinuxIntelRdtClosID(e.IntelRdt.ClosID)
spec.Linux.IntelRdt = e.IntelRdt.ToOCI()
}

return nil
}

Expand Down Expand Up @@ -171,6 +178,11 @@ func (e *ContainerEdits) Validate() error {
return err
}
}
if e.IntelRdt != nil {
if err := ValidateIntelRdt(e.IntelRdt); err != nil {
return err
}
}

return nil
}
Expand All @@ -192,6 +204,9 @@ func (e *ContainerEdits) Append(o *ContainerEdits) *ContainerEdits {
e.DeviceNodes = append(e.DeviceNodes, o.DeviceNodes...)
e.Hooks = append(e.Hooks, o.Hooks...)
e.Mounts = append(e.Mounts, o.Mounts...)
if o.IntelRdt != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

e.IntelRdt = o.IntelRdt
}

return e
}
Expand All @@ -202,7 +217,7 @@ func (e *ContainerEdits) isEmpty() bool {
if e == nil {
return false
}
return len(e.Env)+len(e.DeviceNodes)+len(e.Hooks)+len(e.Mounts) == 0
return len(e.Env)+len(e.DeviceNodes)+len(e.Hooks)+len(e.Mounts) == 0 && e.IntelRdt == nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

// ValidateEnv validates the given environment variables.
Expand Down Expand Up @@ -280,6 +295,15 @@ func (m *Mount) Validate() error {
return nil
}

// ValidateIntelRdt validates the IntelRdt configuration.
func ValidateIntelRdt(i *specs.IntelRdt) error {
// ClosID must be a valid Linux filename
if len(i.ClosID) >= 4096 || i.ClosID == "." || i.ClosID == ".." || strings.ContainsAny(i.ClosID, "/\n") {
elezar marked this conversation as resolved.
Show resolved Hide resolved
return errors.New("invalid ClosID")
}
return nil
}

// Ensure OCI Spec hooks are not nil so we can add hooks.
func ensureOCIHooks(spec *oci.Spec) {
if spec.Hooks == nil {
Expand Down
106 changes: 106 additions & 0 deletions pkg/cdi/container-edits_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,50 @@ func TestValidateContainerEdits(t *testing.T) {
},
invalid: true,
},
{
name: "valid rdt config",
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "foo.bar",
},
},
},
{
name: "invalid rdt config, invalid closID (slash)",
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "foo/bar",
elezar marked this conversation as resolved.
Show resolved Hide resolved
},
},
invalid: true,
},
{
name: "invalid rdt config, invalid closID (dot)",
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: ".",
},
},
invalid: true,
},
{
name: "invalid rdt config, invalid closID (double dot)",
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "..",
},
},
invalid: true,
},
{
name: "invalid rdt config, invalid closID (newline)",
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "foo\nbar",
},
},
invalid: true,
},
} {
t.Run(tc.name, func(t *testing.T) {
edits := ContainerEdits{tc.edits}
Expand Down Expand Up @@ -467,6 +511,58 @@ func TestApplyContainerEdits(t *testing.T) {
},
},
},
{
name: "empty spec, rdt",
spec: &oci.Spec{},
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "clos-1",
L3CacheSchema: "L3:0=ff;1=ff",
MemBwSchema: "MB:0=50;1=50",
EnableCMT: true,
EnableMBM: true,
},
},
result: &oci.Spec{
Linux: &oci.Linux{
IntelRdt: &oci.LinuxIntelRdt{
ClosID: "clos-1",
L3CacheSchema: "L3:0=ff;1=ff",
MemBwSchema: "MB:0=50;1=50",
EnableCMT: true,
EnableMBM: true,
},
},
},
},
{
name: "non-empty spec, overriding rdt",
spec: &oci.Spec{
Linux: &oci.Linux{
IntelRdt: &oci.LinuxIntelRdt{
ClosID: "clos-1",
L3CacheSchema: "L3:0=ff",
MemBwSchema: "MB:0=100",
EnableCMT: true,
EnableMBM: true,
},
},
},
edits: &cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "clos-2",
L3CacheSchema: "L3:0=f",
},
},
result: &oci.Spec{
Linux: &oci.Linux{
IntelRdt: &oci.LinuxIntelRdt{
ClosID: "clos-2",
L3CacheSchema: "L3:0=f",
},
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
edits := ContainerEdits{tc.edits}
Expand Down Expand Up @@ -555,6 +651,10 @@ func TestAppend(t *testing.T) {
Path: "/dev/dev1",
},
},
IntelRdt: &cdi.IntelRdt{
ClosID: "clos-1",
L3CacheSchema: "L3:0=ff",
},
},
},
{
Expand Down Expand Up @@ -583,6 +683,9 @@ func TestAppend(t *testing.T) {
Path: "/dev/dev4",
},
},
IntelRdt: &cdi.IntelRdt{
ClosID: "clos-2",
},
},
},
},
Expand All @@ -609,6 +712,9 @@ func TestAppend(t *testing.T) {
Path: "/dev/dev4",
},
},
IntelRdt: &cdi.IntelRdt{
ClosID: "clos-2",
},
},
},
},
Expand Down
27 changes: 27 additions & 0 deletions pkg/cdi/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,33 @@ func TestRequiredVersion(t *testing.T) {
},
expectedVersion: "0.6.0",
},
{
description: "IntelRdt requires v0.7.0",
spec: &cdi.Spec{
ContainerEdits: cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "foo",
},
},
},
expectedVersion: "0.7.0",
},
{
description: "IntelRdt (on devices) requires v0.7.0",
spec: &cdi.Spec{
Devices: []cdi.Device{
{
Name: "device0",
ContainerEdits: cdi.ContainerEdits{
IntelRdt: &cdi.IntelRdt{
ClosID: "foo",
},
},
},
},
},
expectedVersion: "0.7.0",
},
}

for _, tc := range testCases {
Expand Down
17 changes: 17 additions & 0 deletions pkg/cdi/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -118,6 +120,21 @@ func (r requiredVersionMap) requiredVersion(spec *cdi.Spec) version {
return minVersion
}

// requiresV070 returns true if the spec uses v0.7.0 features
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
Expand Down
23 changes: 23 additions & 0 deletions schema/defs.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
"type": "string"
}
},
"FileName": {
"type": "string"
},
"FilePath": {
"type": "string"
},
Expand Down Expand Up @@ -134,6 +137,26 @@
"items": {
"$ref": "#/definitions/Hook"
}
},
"intelRdt": {
"type": "object",
"properties": {
"closID": {
"$ref": "#/definitions/FileName"
},
"l3CacheSchema": {
"type": "string"
},
"memBwSchema": {
"type": "string"
},
"enableCMT": {
"type": "boolean"
},
"enableMBM": {
"type": "boolean"
}
}
}
}
},
Expand Down
10 changes: 10 additions & 0 deletions specs-go/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type ContainerEdits struct {
DeviceNodes []*DeviceNode `json:"deviceNodes,omitempty"`
Hooks []*Hook `json:"hooks,omitempty"`
Mounts []*Mount `json:"mounts,omitempty"`
IntelRdt *IntelRdt `json:"intelRdt,omitempty"`
}

// DeviceNode represents a device node that needs to be added to the OCI spec.
Expand Down Expand Up @@ -60,3 +61,12 @@ type Hook struct {
Env []string `json:"env,omitempty"`
Timeout *int `json:"timeout,omitempty"`
}

// IntelRdt describes the Linux IntelRdt parameters to set in the OCI spec.
type IntelRdt struct {
ClosID string `json:"closID,omitempty"`
L3CacheSchema string `json:"l3CacheSchema,omitempty"`
MemBwSchema string `json:"memBwSchema,omitempty"`
EnableCMT bool `json:"enableCMT,omitempty"`
EnableMBM bool `json:"enableMBM,omitempty"`
}
Loading