libcontainer/intelrdt: add support for EnableMonitoring field by marquiz · Pull Request #4832 · opencontainers/runc

marquiz · 2025-08-01T13:54:52Z

The linux.intelRdt.enableMonitoring field enables the creation of a per-container monitoring group. The monitoring group is removed when the container is destroyed.

Refs: opencontainers/runtime-spec#1287

rata · 2025-08-05T19:22:03Z


+	// Create MON group
+	if monPath := m.GetMonPath(); monPath != "" {
+		if err := os.MkdirAll(monPath, 0o755); err != nil {


Where is this path? Inside the container? On the host?

This is on the host (now changed to Mkdir)

marquiz · 2025-08-06T06:57:00Z

Review comments from @rata addressed. Rebased. Submitted the first commit as a separate PR (#4840), too

marquiz · 2025-08-07T19:07:39Z

Rebased

marquiz · 2025-08-18T11:33:01Z

Updated:

rebased
go.mod: bumped runtime-spec to latest tip of the main branch
features.go: set linux.intelRdt.monitoring to true

marquiz · 2025-08-26T17:27:48Z

@rata @kolyshkin @AkihiroSuda @cyphar

rata

Please remove the fmt.Println() lines

rata · 2025-08-28T10:28:23Z


+	// Create MON group
+	if monPath := m.GetMonPath(); monPath != "" {
+		if err := os.Mkdir(monPath, 0o755); err != nil && !os.IsExist(err) {


Who has control of this path? In runc this is trusted, okay, but is it exposed in k8s or containerd or some other to the user?

Not sure with the https://github.com/intel/k8s-rdt-controller what is exposed to an end user

It's runc, it's in this very same file (and patch):

func (m *Manager) GetMonPath() string { if closPath := m.GetPath(); closPath != "" && m.config.IntelRdt.EnableMonitoring { path, err := securejoin.SecureJoin(filepath.Join(closPath, "mon_groups"), m.id)

So it's basically <clos-dir>/mon_groups/<container-id>

Thanks, and that comes from intelRdtRoot. But where does that come from? Is there any way an unprivileged user (or just anyone that is not the sysadmin or so) can control any path component (or the intelRDT root)?

That, in turn, comes from parsing the output of statfs syscall (unix.Statfs()). Note that in the case of runc update the closPath is taken from the config.json of the container.

In any case I cannot see any way that an unprivileged user can control the path

So statfs will have some string value and we will mkdir it on the host, outside of the container rootfs?

This conversation was broken out of this thread, it continued in the main PR discussion. The final answer I found for this is posted here: #4832 (comment)

Basically, the fs will be mounted somewhere on the host by the admin and we might create/delete some dirs inside that. None of that is accessible to the container process.

@marquiz correct me if I'm getting something wrong.

@kolyshkin PTAL, I'd like someone else to look at this part too, just in case I'm missing something :)

marquiz · 2025-08-29T10:32:02Z

Rebased (see if the unrelated linter errors go away...)

rata · 2025-08-29T15:14:47Z


+	// Create MON group
+	if monPath := m.GetMonPath(); monPath != "" {
+		if err := os.Mkdir(monPath, 0o755); err != nil && !os.IsExist(err) {


So statfs will have some string value and we will mkdir it on the host, outside of the container rootfs?

marquiz · 2025-08-29T18:49:09Z

So statfs will have some string value and we will mkdir it on the host, outside of the container rootfs?

Yes the data comes from the Linux kernel. See statfs syscall, e.g. https://man7.org/linux/man-pages/man2/statfs.2.html

rata · 2025-09-02T09:32:36Z

@marquiz that manpage doesn't tell anything about intelrdt :-). But ok, I guess you need to mount the restctrl fs like shown here and therefore the host admin decides where it is mounted, no container will have write access to any file/dir there, I guess.

rata

This mostly LGTM. But let's use the well-known table tests (or are you avoiding them for a reason?)

marquiz · 2025-09-04T15:00:47Z

Updated: test refactored

@rata @kolyshkin

rata

Thanks, this is getting closer IMHO :)

rata · 2025-09-12T10:41:14Z

+			if tt.precreateClos {
+				if err := os.MkdirAll(filepath.Join(closPath, "mon_groups"), 0o755); err != nil {
+					t.Fatal(err)
+				}
+			}
+			m := newManager(&configs.Config{IntelRdt: &tt.config}, id, closPath)
+			err := m.Apply(pid)


This seems weird. Apply will create the "mon_groups" directory, so maybe you need to create some parent paths but why "mon_groups"?

This is for the mocked filesystem layout. When you have the real resctrl filesystem mounted all the files and directories are there (created/exposed by the linux kernel). Specifically, the Apply() does NOT create the mon_groups directory but the mon_groups/<mon-group-name> dir

EDIT: @rata I probably misread your comment a bit. Apply() creates the CLOS in some cases if it does not already exist. In the precreateClos case we want to test the case(s) where the CLOS already exists and Apply does not create the CLOS. Nevertheless, Apply() never creates the mon_groups directory (under the CLOS directory)

Ohh, I see. But we end up doing this here and in the other tests: https://github.com/opencontainers/runc/pull/4832/files#diff-10a177e788e7be5a6826c74946ebbb7390ead3a61f1342a4776dba4d57143942R264-R267

Any reason to not put this also in the constructor for the "fake" intelrdt used in tests?

It can be a param, saying create it, and we just take that from the test if it's an issue to create it when we don't need it. (it seems cleaner to do it that way, maybe)

rata

One more comment and I think we are there :)

rata · 2025-09-12T13:25:22Z

+			if tt.precreateClos {
+				if err := os.MkdirAll(filepath.Join(closPath, "mon_groups"), 0o755); err != nil {
+					t.Fatal(err)
+				}
+			}
+			m := newManager(&configs.Config{IntelRdt: &tt.config}, id, closPath)
+			err := m.Apply(pid)


Ohh, I see. But we end up doing this here and in the other tests: https://github.com/opencontainers/runc/pull/4832/files#diff-10a177e788e7be5a6826c74946ebbb7390ead3a61f1342a4776dba4d57143942R264-R267

Any reason to not put this also in the constructor for the "fake" intelrdt used in tests?

It can be a param, saying create it, and we just take that from the test if it's an issue to create it when we don't need it. (it seems cleaner to do it that way, maybe)

marquiz · 2025-09-12T16:35:30Z

Any reason to not put this also in the constructor for the "fake" intelrdt used in tests?

It can be a param, saying create it, and we just take that from the test if it's an issue to create it when we don't need it. (it > seems cleaner to do it that way, maybe)

I wouldn't put it there. The constructor is used also in other tests that do not need/want Apply(). We want to setup a mock fs, not exercise the Apply() function which has a very refined logic. Also, in the case of the tests modified/added here. For apply test we want to TEST the Apply() function, thus calling it. For the Destroy test we use it for creation of the per-container-id-CLOS. We could create it "manually", not using Apply() if you think that's better.

I know the unit tests are a mess. I would suggest to do bigger refactoring/sanitizing in a separate PR.

rata

@marquiz I guess some github email reply is not keeping the thread on the proper line of code for you. I'm not sure what you mean with: "we can create it manually, not using Apply()". My suggestion was to move os.mkdirAll() into the fake constructor, maybe under a param if you don't want to create the dirs on evey instantiation.

Or maybe doing it unconditionally as part of the current tests (given that is really just a mkdir, it's not a heavy thing to do) if that sounds better for you.

Marking it as LGTM, but I'd really like for @kolyshkin to take a look, specially at: #4832 (comment) in case I'm missing something

marquiz · 2025-09-15T13:23:18Z

I'm not sure what you mean with: "we can create it manually, not using Apply()". My suggestion was to move os.mkdirAll() into the fake constructor, maybe under a param if you don't want to create the dirs on evey instantiation.

Ah OK, now that makes sense to me. There really was some misalignment. I'll check the details and update the PR

marquiz · 2025-09-15T15:53:58Z

Or maybe doing it unconditionally as part of the current tests (given that is really just a mkdir, it's not a heavy thing to do) if that sounds better for you.

We cannot do that unconditionally as some tests require the CLOS not to be pre-created. I added a new commit that refactors the tests to give the pre-existing clos (if any) as an arg to the helper constructor.

@rata @kolyshkin

The linux.intelRdt.enableMonitoring field enables the creation of a per-container monitoring group. The monitoring group is removed when the container is destroyed. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz · 2025-09-17T05:57:55Z

Rebased

marquiz · 2025-09-23T09:03:01Z

friendly ping @rata @kolyshkin

rata · 2025-09-24T08:20:22Z

I have already LGTM, but we need two LGTM to merge :)

marquiz · 2025-09-24T09:51:36Z

I have already LGTM, but we need two LGTM to merge :)

Thanks :) I had added this one commit (refactoring unit tests) since your last comment.

rata · 2025-09-24T14:12:47Z

@marquiz thanks, but can we remove that commit?

It's not what I had in mind and it's not making the code simpler to read. Sorry.

marquiz · 2025-09-25T06:47:29Z

thanks, but can we remove that commit?

Done

marquiz · 2025-10-08T06:08:10Z

ping @kolyshkin

kolyshkin

lgtm

kolyshkin · 2025-10-08T07:18:26Z

@marquiz do you want it to be in 1.4, or 1.5 (in ~6 months) is fine? We're very close to releasing 1.4.0-rc.2 and if you want it, a backport PR is needed right now.

marquiz · 2025-10-08T07:44:49Z

@kolyshkin if possible, I'd very much like to see this in 1.4. It would complete the intelRdt features wrt the runtime-spec.

I can do the backport

EDIT: #4921

marquiz force-pushed the devel/rdt-enablemonitoring branch 4 times, most recently from 520aadd to d141fb0 Compare August 4, 2025 14:18

rata reviewed Aug 5, 2025

View reviewed changes

marquiz force-pushed the devel/rdt-enablemonitoring branch 3 times, most recently from 169609d to 4024488 Compare August 6, 2025 06:55

marquiz force-pushed the devel/rdt-enablemonitoring branch 2 times, most recently from 86329fe to 2380794 Compare August 7, 2025 19:06

marquiz force-pushed the devel/rdt-enablemonitoring branch 2 times, most recently from e31d98c to 4af20b5 Compare August 18, 2025 11:32

marquiz mentioned this pull request Aug 26, 2025

Support direct editing of the intelRdt config containerd/nri#215

Merged

marquiz force-pushed the devel/rdt-enablemonitoring branch 2 times, most recently from fd132e2 to 4e43d65 Compare August 26, 2025 17:23

rata requested changes Aug 27, 2025

View reviewed changes

Comment thread libcontainer/intelrdt/intelrdt.go Outdated

rata added the impact/changelog label Aug 27, 2025

marquiz force-pushed the devel/rdt-enablemonitoring branch from 4e43d65 to 2fbcde5 Compare August 27, 2025 12:44

rata reviewed Aug 28, 2025

View reviewed changes

marquiz force-pushed the devel/rdt-enablemonitoring branch from 2fbcde5 to abafdf8 Compare August 29, 2025 10:31

rata reviewed Aug 29, 2025

View reviewed changes

rata requested changes Sep 2, 2025

View reviewed changes

Comment thread libcontainer/intelrdt/intelrdt_test.go Outdated

kolyshkin reviewed Sep 3, 2025

View reviewed changes

Comment thread libcontainer/intelrdt/intelrdt.go Outdated

rata requested changes Sep 12, 2025

View reviewed changes

marquiz force-pushed the devel/rdt-enablemonitoring branch 2 times, most recently from eb6f5ec to 04b55ac Compare September 12, 2025 12:35

rata reviewed Sep 12, 2025

View reviewed changes

rata approved these changes Sep 15, 2025

View reviewed changes

rata requested a review from kolyshkin September 15, 2025 12:28

libcontainer/intelrdt: add support for EnableMonitoring field

7aa4e1a

The linux.intelRdt.enableMonitoring field enables the creation of a per-container monitoring group. The monitoring group is removed when the container is destroyed. Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>

marquiz force-pushed the devel/rdt-enablemonitoring branch from bb35f6c to f352280 Compare September 17, 2025 05:57

marquiz force-pushed the devel/rdt-enablemonitoring branch from f352280 to 7aa4e1a Compare September 25, 2025 06:47

kolyshkin approved these changes Oct 8, 2025

View reviewed changes

kolyshkin merged commit 28daf53 into opencontainers:main Oct 8, 2025
36 checks passed

marquiz deleted the devel/rdt-enablemonitoring branch October 8, 2025 07:44

marquiz mentioned this pull request Oct 8, 2025

[1.4] libcontainer/intelrdt: add support for EnableMonitoring field #4921

Merged

cyphar added this to the 1.4.0-rc.2 milestone Oct 8, 2025

cyphar added the backport/1.4-todo A PR in main branch which needs to backported to release-1.4 label Oct 8, 2025

lifubang added backport/1.4-done A PR in main branch which has been backported to release-1.4 and removed backport/1.4-todo A PR in main branch which needs to backported to release-1.4 labels Oct 8, 2025

Uh oh!

Conversation

marquiz commented Aug 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 6, 2025

Uh oh!

marquiz commented Aug 7, 2025

Uh oh!

marquiz commented Aug 18, 2025

Uh oh!

marquiz commented Aug 26, 2025

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rata Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 29, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Aug 29, 2025

Uh oh!

rata commented Sep 2, 2025

Uh oh!

rata left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marquiz commented Sep 4, 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!

marquiz Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rata Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

rata Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Sep 12, 2025

Uh oh!

rata left a comment

rata Aug 28, 2025 •

edited

Loading

rata left a comment •

edited

Loading

marquiz Sep 12, 2025 •

edited

Loading

rata Sep 12, 2025 •

edited

Loading

rata Sep 12, 2025 •

edited

Loading

kolyshkin commented Oct 8, 2025 •

edited

Loading

marquiz commented Oct 8, 2025 •

edited

Loading