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.
• Known runtime implementations are runc (upcoming v1.4) and crun (v1.24+)
  • closID: supported by runc and crun
  • schemata: supported by runc (upcoming v1.4) and crun (v1.22+)
    • runc PR: libcontainer/intelrdt: add support for Schemata field opencontainers/runc#4830
    • crun PR: Support generic Intel RDT schemata containers/crun#1748
  • enableMonitoring supported by runc (upcoming v1.4) but not by crun (v1.24+)
    • runc PR: libcontainer/intelrdt: add support for EnableMonitoring field opencontainers/runc#4832)
    • crun PR: intelrdt: add support for EnableMonitoring containers/crun#1866

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]

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]

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

[marquiz]

Rebased, marked ready-for-review. Stacked on top of #212
@klihub @mikebrow @chrishenzie

[klihub]

nit: typo/missing 'be' in commit message of 'api: add rdt': "Also note that the Rdt object cannot modified on container"

[klihub]

As a purely theoretical question related to your notes in the commit log of 'api: add rdt'.

In the (I am adraid highly unlikely) situation where runc/crun/OCI compatible runtimes would gain the ability to update RDT for a running container, what would be the best way for us to model/handle that. Would we add a corresponding &LinuxRdt{} to LinuxContainerUpdate{} ?

[marquiz]

nit: typo/missing 'be' in commit message of 'api: add rdt': "Also note that the Rdt object cannot modified on container"

Thx for spotting this. Fixed

[marquiz]

In the (I am adraid highly unlikely) situation where runc/crun/OCI compatible runtimes would gain the ability to update RDT for a running container, what would be the best way for us to model/handle that. Would we add a corresponding &LinuxRdt{} to LinuxContainerUpdate{}

Yes, I think that would likely be the place (with the usual reservations of predicting the future, how this would be implemented in runc).

[marquiz]

@klihub @mikebrow @chrishenzie it would be nice to get #118 in before this. I'll then rebase

[klihub]

@klihub @mikebrow @chrishenzie it would be nice to get #118 in before this. I'll then rebase

@marquiz We could also do this the other way around. I think we're more eager to get this in. And I wouldn't mind at all that merge order forcing me to do another rebase of #118, and then once more check and make sure at that time that we really do handle gracefully all the new additions merged after #118 was originally filed, before we merge it.

[marquiz]

@marquiz We could also do this the other way around. I think we're more eager to get this in. And I wouldn't mind at all that merge order forcing me to do another rebase of #118,

OK, thanks @klihub. I now rebased this PR.

I added you as Co-author in the pkg/adaptation: support new RDT fields patch where I borrowed some snippets from #118.

[klihub]

@mikebrow @chrishenzie Could we get this reviewed if there are enough cycles to spare ?

[marquiz]

Rebased. Updated: review comments addressed, added new test case to pkg/runtime-tools/generate

[klihub]

@mikebrow Mike, how about this one ? I labelled this v.next originally, before it got reviewed. But this is pretty self-contained and should be safe to merge, so I'd be okay with pushing this in already for v0.11.0.

[mikebrow]

LTGM
Well done!