libcontainer/intelrdt: support explicit assignment to root CLOS #4854

marquiz · 2025-08-15T12:13:21Z

Makes it possible e.g. to enable monitoring
(linux.intelRdt.enableMonitoring) without creating a CLOS (resctrl group) for the container.

Implements opencontainers/runtime-spec#1289.

kolyshkin · 2025-08-17T22:43:12Z

libcontainer/configs/validate/validator.go

@@ -287,7 +287,7 @@ func intelrdtCheck(config *configs.Config) error {
 			return fmt.Errorf("intelRdt is specified in config, but Intel RDT is not enabled")
 		}

-		if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || strings.Contains(config.IntelRdt.ClosID, "/") {
+		if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || (config.IntelRdt.ClosID != "/" && strings.Contains(config.IntelRdt.ClosID, "/")) {


Can be simplified to len > 1?

Suggested change

if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || (config.IntelRdt.ClosID != "/" && strings.Contains(config.IntelRdt.ClosID, "/")) {

if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || (len(config.IntelRdt.ClosID) > 1 && strings.Contains(config.IntelRdt.ClosID, "/")) {

(but maybe it's less readable, IDK)

Personally, I find that less obviouls, kinda obfuscating that we're looking for / as a special value. But let others comment on this, too...

Reading the spec, I think what @kolyshkin suggested is slightly more readable for me. / is a special value, so something that is more than one char and contains a / is clearly not just a / in my mind.

I think I'd split it in two different error cases, one for "." and "..", the other for this. But this is really a personal preference.

Fair enough, I changed the code along these lines. However, I changed it to switch-case to make it shorter and more readable (imo)

marquiz · 2025-08-27T07:21:02Z

Ready for review, opencontainers/runtime-spec#1289 was merged.

@AkihiroSuda @rata

AkihiroSuda

Can we have a unit test?

marquiz · 2025-08-27T12:39:36Z

Can we have a unit test?

Check. I added comprehensive unit tests for the IntelRdt validation as a separate commit.

@rata @AkihiroSuda

rata

This almost lgtm, left a few suggestions to see if it can be slightly simpler

rata · 2025-08-28T09:25:33Z

libcontainer/configs/validate/validator.go

+type intelRdtStatus struct {
+	sync.Once
+	rdtEnabled bool
+	catEnabled bool
+	mbaEnabled bool
+}
+
+var intelRdt = &intelRdtStatus{}


I think the methods and the struct might be better on a different file in this package now, it's growing.

However, do we need so much plumbing? Can't we have package-level bool variables just assigned with the result of intelrdt.IsEnabled() (and friends)? I'm not sure if this can change at runtime (it seems it doesn't) nor what the isEnabled() check does, but if that works, it seems simpler.

What do you think?

Thanks @rata. This was exactly my initial implementation. However, I changed it in order to not cause ANY functional changes (other than the if logic). IOW, reason for the extensive plumbing here is to only do the discovery if the container config specifies linux.intelRdt (that involves parsing mounts and reading files from sysfs, for example)

I don't know how critical this really is for runc so I'm fully ok to do the simple thing you suggested. Thoughts @rata @kolyshkin @AkihiroSuda ?

Oh, right, no, let's keep this then. intelrdt is quite slow, let's do it conditional. But maybe move the methods and that to another file :)

Check. They're now in a separate file (libcontainer/configs/validate/intelrdt.go)

libcontainer/configs/validate/validator_test.go

rata · 2025-08-28T09:33:41Z

libcontainer/configs/validate/validator.go

@@ -287,7 +287,7 @@ func intelrdtCheck(config *configs.Config) error {
 			return fmt.Errorf("intelRdt is specified in config, but Intel RDT is not enabled")
 		}

-		if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || strings.Contains(config.IntelRdt.ClosID, "/") {
+		if config.IntelRdt.ClosID == "." || config.IntelRdt.ClosID == ".." || (config.IntelRdt.ClosID != "/" && strings.Contains(config.IntelRdt.ClosID, "/")) {


Reading the spec, I think what @kolyshkin suggested is slightly more readable for me. / is a special value, so something that is more than one char and contains a / is clearly not just a / in my mind.

I think I'd split it in two different error cases, one for "." and "..", the other for this. But this is really a personal preference.

Makes it possible e.g. to enable monitoring (linux.intelRdt.enableMonitoring) without creating a CLOS (resctrl group) for the container. Implements opencontainers/runtime-spec#1289. Signed-off-by: Markus Lehtonen <[email protected]>

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz · 2025-08-28T11:13:49Z

Updated:

change logic similar to what @kolyshkin suggested
put intelrdt plumbing into a separate file
put new unit tests into a separate file

marquiz · 2025-08-28T11:53:03Z

Why do I get a linter error about missing package comment (in the new file)? None of the existing files have package comments 🤔

rata · 2025-08-28T12:45:54Z

@marquiz not sure, but let's ignore them for now :)

rata

LGTM, thanks!

rata · 2025-08-29T08:49:49Z

@marquiz the linter is failing due to this PR now. Can you fix it?

Linter is failing due to this PR (I thought it wasn't related, as before it was failing due to files not changed here)

Add package comment to make revive pass muster. Signed-off-by: Markus Lehtonen <[email protected]>

marquiz · 2025-08-29T09:39:21Z

Linter is failing due to this PR (I thought it wasn't related, as before it was failing due to files not changed here)

The linter failure had nothing to do with this PR, except that it added a new file and the linter started to complain about package comments because of this. Most of the packages in runc don't have package comments but because we lint with --new-from-rev=HEAD~ those are not picked up when golangci-lint version is bumbed and new warnings/errors get (silently) enabled.

However, I now added doc.go, even if outside the scope of this PR, to fix the failure

rata

LGTM. Thanks for the doc one-liner to fix the linter :)

kolyshkin reviewed Aug 17, 2025

View reviewed changes

marquiz changed the title ~~[RFC] libcontainer/intelrdt: support explicit assignment to root CLOS~~ libcontainer/intelrdt: support explicit assignment to root CLOS Aug 27, 2025

marquiz marked this pull request as ready for review August 27, 2025 07:17

AkihiroSuda approved these changes Aug 27, 2025

View reviewed changes

rata added the impact/changelog label Aug 27, 2025

marquiz force-pushed the devel/rdt-root-clos branch from d0b9477 to 748abd3 Compare August 27, 2025 12:28

rata reviewed Aug 28, 2025

View reviewed changes

marquiz added 2 commits August 28, 2025 14:08

libcontainer/configs: add validator unit tests for intelRdt

ba68a17

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz force-pushed the devel/rdt-root-clos branch from 748abd3 to ba68a17 Compare August 28, 2025 11:11

rata previously approved these changes Aug 29, 2025

View reviewed changes

libcontainer/configs/validate: add doc.go

7628194

Add package comment to make revive pass muster. Signed-off-by: Markus Lehtonen <[email protected]>

marquiz force-pushed the devel/rdt-root-clos branch from 0ae05e6 to 7628194 Compare August 29, 2025 09:36

rata approved these changes Aug 29, 2025

View reviewed changes

rata merged commit 7a982f4 into opencontainers:main Aug 29, 2025
36 checks passed

marquiz deleted the devel/rdt-root-clos branch August 29, 2025 10:26

	if config.IntelRdt.ClosID == "." \|\| config.IntelRdt.ClosID == ".." \|\| (config.IntelRdt.ClosID != "/" && strings.Contains(config.IntelRdt.ClosID, "/")) {
	if config.IntelRdt.ClosID == "." \|\| config.IntelRdt.ClosID == ".." \|\| (len(config.IntelRdt.ClosID) > 1 && strings.Contains(config.IntelRdt.ClosID, "/")) {

libcontainer/intelrdt: support explicit assignment to root CLOS #4854

libcontainer/intelrdt: support explicit assignment to root CLOS #4854

Uh oh!

Conversation

marquiz commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 27, 2025

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 27, 2025

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 28, 2025

Uh oh!

marquiz commented Aug 28, 2025

Uh oh!

rata commented Aug 28, 2025

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

rata commented Aug 29, 2025

Uh oh!

marquiz commented Aug 29, 2025

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!