Support direct editing of the intelRdt config

[marquiz]

This PR adds support for directly adjusting the intelRdt object of the container config.

Previously, we have the concept of RDT class, which provides indirect manipulation of the intelRdt.closID field. There the higher level runtime (containerd, cri-o) registers a resolver function which the NRI adaptation uses to translate the provided "RDT class" to actual closID (which is basically the resctrl group in the Linux resctrl filesystem).

The API extensions in this PR provides direct control of the relevant fields of the intelRdt object. The PR also includes a sample plugin to test/demonstrate the functionality (including deployment files and CI integration to build/publish images)

REFS:

• config-linux: add schemata field to IntelRdt opencontainers/runtime-spec#1230
• config-linux: add intelRdt.enableMonitoring opencontainers/runtime-spec#1287

NOTES:

• Depends/stacks on pkg/api: add OptionalRepeatedString type #212. That could be merged separately
• The "pkg/adaptation: support new RDT fields" commit borrows some snippets from adaptation: return nil as no-op adjustment. #118. Let's get that one in first.
• There is currently no runtime implementation (runc or crun) that would support all the functionality
  • closID: supported by runc and crun
  • schemata
    • runc: not supported (PR: libcontainer/intelrdt: add support for Schemata field opencontainers/runc#4830)
    • crun: supported (Support generic Intel RDT schemata containers/crun#1748)
  • enableMonitoring:
    • runc: not supported (PR: libcontainer/intelrdt: add support for EnableMonitoring field opencontainers/runc#4832)
    • crun: not supported, no PR open (afaik)

OPEN QUESTION/TODO:

In this PR there is no mechanism for removing the intelRdt object from the config. That is probably a desirable feature.

[marquiz]

@klihub @chrishenzie @mikebrow @giuseppe @saschagrunert

[klihub]

@marquiz Do we know what is the expected timeframe for cutting the first such runtime-spec release which would put 383cadbf08c0 commit behind a tag ?

I am asking because we have been reluctant lately to pull in untagged versions of the runtime spec, because that would cause extra hassle/work if we wanted to or were forced to cut a new release and bump containerd and cri-o to pull in that new version. For instance #157 and #166 are current drafts exactly for this reason: the runtime-spec they require is not available yet in a tagged release.

[marquiz]

Yeah, sure. I don't think we need to hurry with this.

[klihub]

@marquiz @mikebrow @chrishenzie In principle there is one way how we could get this partially in already now, to avoid having to keep rebasing this with a guaranteed conflict whenever we add something to LinuxContainer or LinuxContainerAdjustment.

It would be to split this up to 2 stacked PRs:

• PR#1: The bulk of this PR, without the OCI Spec bump + a single change to Generator.Adjust(): return an error if adjust.GetLinux().GetRdt() != nil
• draft PR#2: All the commits in PR#1 + the OCI Spec bump (eventually to a tagged version) + a Generator.Adjust() updated to do RDT adjustment if requested

Then we could merge #PR1 (and rebase PR#2 when that happens), and keep the OCI Spec-dependent bits pending until the necessary bits get tagged. We could apply the same to #157 and #166...

Maybe not worth the effort though, and we can just mark this a draft until we get a tagged OCI Spec.

[klihub]

nit: typo in the body of commit message for 'api: add rdt': repeated 'the' in "... the the IntelRdt message is sepate from LinuxResources. This is to ..."

[klihub]

Just a note: #118 has a bunch of queued similar changes, updating the result-handling code so that there is no need to always a priory create a fully constructed but empty adjustment. It would be nice to get that finally in, so I'll rebase it that on the latest main/HEAD then update with any missing init*() bits necessary.

[marquiz]

👍 I'll review when that's updated. And then rebase when that's merged

[klihub]

@marquiz @mikebrow @chrishenzie This overall LGTM. My biggest question is the timeline for tagging a new opencontainers/runtime-spec so that bits we rely on here become available via a tagged release.

[marquiz]

My biggest question is the timeline for tagging a new opencontainers/runtime-spec so that bits we rely on here become available via a tagged release.

I'm on the same side. I think there's no immediate hurry with this, especially because even the runc/crun implementation(s) are not there, yet. I've been working on this area so I wanted to get this one off my shoulders. That said, I think this is ready for review. I could've submitted this as a draft, but decided not to because from my perspective this is complete.

[marquiz]

I just recalled that there's actually one aspect/feature missing from the new extensions: ability to remove the linux.intelRdt from the container config (set it to nil/null). Suggestions for how to implement this are warmly welcome.

I couldn't come up with any nice solutions, I played with two alternatives but didn't commit either of them:

1. Have a separate field (like drop) in the NRI API
2. Have a well documented, otherwise invalid value for the closID field in the NRI API (e.g. -/) that causes the config to be dropped

[giuseppe]

LGTM

[klihub]

I just recalled that there's actually one aspect/feature missing from the new extensions: ability to remove the linux.intelRdt from the container config (set it to nil/null). Suggestions for how to implement this are warmly welcome.

I couldn't come up with any nice solutions, I played with two alternatives but didn't commit either of them:

1. Have a separate field (like drop) in the NRI API
2. Have a well documented, otherwise invalid value for the closID field in the NRI API (e.g. -/) that causes the config to be dropped

@marquiz I'd vote with a mild bias for 2 above (because it's how we handle other similar cases where removal is supported). But with the addition that on the wire-protocol this is the convention we use, but in the user-facing API we provide explicit functions for clearing any LinuxRdt, with something along the lines of this:

func (a *ContainerAdjustment) RemoveLinuxRDT() {
    a.initLinux()
    a.Linux.Rdt = &LinuxRdt{
        ClosId = MarkForRemoval("") // or MarkForRemoval("/") if you think that's better
    }
}

And then have the corresponding handling for this in intermediate result-calculation and final Spec generation.

[marquiz]

@klihub thanks for the feedback. The MarkForRemoval thingie was a good hint, I didn't have that in my early prototype. I'll cook up something along the lines you suggested and then we can mull over that.

[marquiz]

Updated:

• rebased (on top of the containerized-protobuild commit)
• added unit test for pkg/runtime-tools
• small change in pkg/adaptation/result.go (rely on initAdjustRdt() in initializing Container.Linux.Rdt)
• fixed commit message
• added support for removing RDT config (as a separate commit)

nit: typo in the body of commit message for 'api: add rdt': repeated 'the' in "... the the IntelRdt message is sepate from LinuxResources. This is to ..."

Thx for spotting this. Fixed

[marquiz]

I'd vote with a mild bias for 2 above (because it's how we handle other similar cases where removal is supported). But with the addition that on the wire-protocol this is the convention we use, but in the user-facing API we provide explicit functions for clearing any LinuxRdt,

I ended up with adding a separate field. I implemented both variants and the "prefix closid with -/" variant was an ugly fragile mess tbh. So much cleaner to just have a separate field for this purpose. For envs and other guys it's a bit different when we're updating a map or slice and you selectively remove some items. But here we're managing the whole object. (if interested, I have the still lying around at https://github.com/marquiz/nri/commits/devel/rdt-remove-2/)

[klihub]

I'd vote with a mild bias for 2 above (because it's how we handle other similar cases where removal is supported). But with the addition that on the wire-protocol this is the convention we use, but in the user-facing API we provide explicit functions for clearing any LinuxRdt,

I ended up with adding a separate field. I implemented both variants and the "prefix closid with -/" variant was an ugly fragile mess tbh. So much cleaner to just have a separate field for this purpose. For envs and other guys it's a bit different when we're updating a map or slice and you selectively remove some items. But here we're managing the whole object.

@marquiz Yes, and sorry about the long sidetrack in our offline discussion earlier. I was in the totally wrong mental model, associated with most of the existing bits (slices and maps). You were absolutely right that we can't reasonably handle removal with an inline notation, because then it would not be possible to indicate a removal prior to an addition to avoiding conflict with an earlier plugin. I agree a separate dedicated field in the wire-protocol is the way to go here.