Add --cgroups parameter to jailer #2012

christian7007 · 2020-07-06T16:43:37Z

Reason for This PR

Increase cgroups integration flexibility for the Jailer.
Fixes #1937
Fixes #1617

Description of Changes

New --cgroups argument have been added to the Jailer. This argument takes care of setting the passed cgroups generically and check that the expected value is correctly written.

The Cgroup type have been modified in order to represent each of the cgroups passed by --cgroups command line argument.

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

lauralt · 2020-07-10T13:09:51Z

Hi, @christian7007! Thanks for the PR, we will take a closer look at it soon.

ioanachirca

1st pass, mostly nits :D will review the functionality in depth a bit later

CHANGELOG.md

docs/jailer.md

ioanachirca · 2020-07-13T08:58:20Z

src/jailer/src/cgroup.rs

+        })
+    }
+
+    // This writes the cgroup value into the cgroup property file and attach the pid to the cgroup


Suggested change

// This writes the cgroup value into the cgroup property file and attach the pid to the cgroup

// This writes the cgroup value into the cgroup property file and attaches the pid to the cgroup.

But does it attach the pid? It seems there is a different method that does this.

Yes, you're right. I forgot to change that comment, I'll update that.

src/jailer/src/cgroup.rs

src/jailer/src/env.rs

christian7007 · 2020-07-13T15:36:08Z

Thanks for the feedback @ioanachirca. I've added all the changes.

acatangiu · 2020-07-20T16:30:15Z

Hi @christian7007 we're not sure if we should downright remove the --node support or follow some deprecation mechanism.

I am currently reaching out to some of our users to get some customer feedback and we will come back with a decision.

christian7007 · 2020-07-20T16:39:18Z

Hello @acatangiu,

No problem, if you need to proceed in a different way with the --node argument deprecation I can upgrade the PR and make it fits with the new Cgroups interface until it can be removed.

iulianbarbu

The PR looks good overall. I have some concerns tho. How can we be sure that controllers separator, at the moment being comma, can be safely used? I noticed that using comma for separating multiple controllers does not fit when we have comma separated values. Have you looked on existing controllers properties values to see what can be safely used to separate controllers, but not interfere with values separators? We need to make sure that what is of interest for controlling the Firecracker process is fully supported.

iulianbarbu · 2020-07-20T19:57:36Z

CHANGELOG.md

@@ -68,6 +70,9 @@
  `403 BadRequest`.
 - Segregated MMDS documentation in MMDS design documentation and MMDS user
  guide documentation.
+- Jailer `--node` attribute has been replaced by `--cgroups`. In order to achieve the same
+ functionality, `--cgroups` can be used like this:
+  `--cgroups cpuset.mems=<node>,cpuset.cpus=$(cat /sys/devices/system/node/node<node>/cpulist)`


Looking at the implementation, I see that controller.property=value are separated by comma. I think we want to support comma separated values as well, for a property of a controller. An example is the case for cpuset.cpus, where we can have multiple comma separated values. E.g: 0,2,8-15.

Yes @iulianbarbu, you are totally right. I didn't take that case into account. Probably it's better to use a different separator like a semicolon. I'll take a closer look on the supported values and I'll update the PR changing the separator.

Thanks for the feedback!

acatangiu · 2020-07-24T09:48:38Z

Hello @acatangiu,

No problem, if you need to proceed in a different way with the --node argument deprecation I can upgrade the PR and make it fits with the new Cgroups interface until it can be removed.

We decided to go with a deprecation mechanism here. For the purpose of this PR it is enough to keep both --node and --cgroup parameters functional, and we will add a deprecation mechanism for --node on top of that.

christian7007 · 2020-08-10T08:46:08Z

Hello @iulianbarbu and @acatangiu, sorry for the late reply I've been on vacation last days. I'll take care of submitting commits addressing both changes. The support for --node attribute and the separator change.

iulianbarbu · 2020-08-11T09:28:08Z

Hello @iulianbarbu and @acatangiu, sorry for the late reply I've been on vacation last days. I'll take care of submitting commits addressing both changes. The support for --node attribute and the separator change.

Cool! Let us know if you need any help or feedback.

christian7007 · 2020-08-12T09:11:33Z

Hello @iulianbarbu, I've been thinking and making some test regarding changing the separator. I think that probably using ; as separator could be a bad idea too, as it might be interpreted by bash. I think that maybe we could use a different approach, something like:

jailer ... --cgroup cpuset.mems=0 --cgroup cpuset.cpus=1-3,4 ...

This way we use different arguments as separator, it seems cleaner to me. I've prepare a commit with a prototype for supporting this into the arg parser: christian7007@979873f

What do you think about it? If you think this solution is good enough I can open a new PR with the parser changes and work over it.

lauralt · 2020-08-12T10:24:08Z

Hello @iulianbarbu, I've been thinking and making some test regarding changing the separator. I think that probably using ; as separator could be a bad idea too, as it might be interpreted by bash. I think that maybe we could use a different approach, something like:
jailer ... --cgroup cpuset.mems=0 --cgroup cpuset.cpus=1-3,4 ... 
This way we use different arguments as separator, it seems cleaner to me. I've prepare a commit with a prototype for supporting this into the arg parser: christian7007@979873f

What do you think about it? If you think this solution is good enough I can open a new PR with the parser changes and work over it.

Hi, @christian7007. I just took a look at this thread, but I might still be missing some context sorry :D. Anyway, regarding the parser changes, I do not quite like being able to set multiple values for an argument by repeating that argument (but it is indeed a pretty subjective opinion). It would become confusing to allow both arguments with just a value (we do not overwrite, but return a DuplicateArgument error if we repeat such argument, e.g. --secomp-level 1 --secomp-level 2) and arguments with more values, in this form: --cgroup pair1 --cgroup pair2.

Basically, you want to pass a list as value to the --cgroups parameter, I think it would look better like this --cgroups pair1 pair2or --cgroups pair1<some_delimiter>pair2 (I prefer the first option).
Also, since it seems the cgroups argument can take many key=value pairs, another idea that would simplify the parsing would be to store these pairs in a file (--cgroups-file some_file).

iulianbarbu · 2020-08-12T12:01:27Z

Hello @iulianbarbu, I've been thinking and making some test regarding changing the separator. I think that probably using ; as separator could be a bad idea too, as it might be interpreted by bash.

Yes, sure. I think a workaround for this would be to require escaping: --cgroups "cpuset.mems=0;cpuset.cpus=1,2,8-10".

I think that maybe we could use a different approach, something like:
jailer ... --cgroup cpuset.mems=0 --cgroup cpuset.cpus=1-3,4 ... 
This way we use different arguments as separator, it seems cleaner to me. I've prepare a commit with a prototype for supporting this into the arg parser: christian7007@979873f

What do you think about it? If you think this solution is good enough I can open a new PR with the parser changes and work over it.

This might work. I do not have a strong preference for one or the other. As @lauralt pointed, we must make sure we handle properly cases which map to DuplicateArgumentError. Such cases can happen for both available alternatives. Other than that, I would gladly appreciate any input from the others reviewers. I am not sure if there is a best way to choose between both alternatives, but I think there are some guidelines which we can use, like:

comparing the final user experience for both alternatives in terms of clarity and simplicity.
arguments combination corner cases handled properly.
significant performance turnoffs, when we compare the two alternatives, since their on the hot path for starting a microVM.
overall implementation details.

I guess we want to give it some thought, and then align on it. Let me have it discussed inside the team. I'll come back with an update soon.

christian7007 · 2020-08-12T13:13:06Z

Hello @lauralt, I agree that the best solution would be to use a separator, the problem here is that common used separator might interfere with the functionality (e.g it can be used by cgroups for something else or be interpreted by bash). That's why I'm trying to find a different approach.

Yes, sure. I think a workaround for this would be to require escaping: --cgroups "cpuset.mems=0;cpuset.cpus=1,2,8-10".

You're right @iulianbarbu, but IMHO it doesn't look too clean, also these escapes could be a bit annoying when the command generation/execution is automated.

I agree with the guidelines and with giving a thought to make sure that the interface we are defining is good enough to avoid future breaking changes. So, just let me know when you and the team have an update and I'll keep working on this.

lauralt · 2020-08-12T13:30:50Z

Hello @lauralt, I agree that the best solution would be to use a separator, the problem here is that common used separator might interfere with the functionality (e.g it can be used by cgroups for something else or be interpreted by bash). That's why I'm trying to find a different approach.

Hmm, could a " " separator be used by cgroups for something else? I would expect to not have spaces inside a key, value pair.
Also, in case we accept that the value, for example, can contain spaces, wouldn't this be a problem in the --cgroup pair1 --cgroup pair2 solution that you proposed too, i.e. you would keep only the first part of the value?

christian7007 · 2020-08-12T15:21:51Z

I'm not sure if the " " is used for something else by cgroups (probably not, but I can try to find out). Anyway, in case we need to support that specific case we can try to find a workaround like:

--cgroups "cpuset.cpus=1 2<delimiter>cpuset.mems=0"

or like:

--cgroup "cpuset.cpus=1, 2" --cgroup cpuset.mems=0

And I guess that once we have read the entire string that shouldn't be a problem any more, as it will fit to the format <cgroup_controller>.<cgroup_property>=<value> being <value> every character after the = including white spaces.

christian7007 · 2020-08-14T11:49:00Z

Hello @acatangiu, I've just pushed the commit restoring --node functionality (47ea8c1). It takes care of creating the corresponding cgroups to isolate the process in the specified node. I'd like to mention that previously, the jailer, apart from isolating the proces in the NUMA node, it also register the PID for some cgroups controllers (cpu, cpuset and pids) so people can modified the cgroups after the jailer was executed, From the jailer docs:

The jailer then writes the current pid to /sys/fs/cgroup/cpu/firecracker/551e7604-e35c-42b3-b825-416853441234/tasks, /sys/fs/cgroup/cpuset/firecracker/551e7604-e35c-42b3-b825-416853441234/tasks, and /sys/fs/cgroup/pids/firecracker/551e7604-e35c-42b3-b825-416853441234/tasks

With my current changes this is only done when a cgroup is set for a specific controller, so if only --node is used the PID will be only registered for cpuset controller (which is the one used for the NUMA node isolation).

I don't know if we should take care of it too, and follow some deprecation mode for this specific behavior too.

Regarding the CI tests, I'm not sure if the Internal CI error of aarch64 is related with my changes (it doesn't show any error related information). For the x86_64 error, it's related with using , as separator, so it should be fixed once we decide how to proceed on this issue.

iulianbarbu · 2020-08-19T06:13:00Z

Hi, @christian7007 !
We've decided to move forward with the alternative where you have multiple --cgroup flags:
E.g --cgroup "cpuset.cpus=1, 2" --cgroup cpuset.mems=0,1 etc.

Let us know if we can help with anything.

christian7007 · 2020-08-27T11:25:51Z

Hello @iulianbarbu, I've pushed a commit (5c5809c) with the requested changes. Please take a look at it when you have some time, if the changes looks good to you I'll squash all the commits in two.

iulianbarbu

Few nits and a suggestion for changing a comment.

Feel free to create the final two commits. I'll review each of them once more and then approve it if there is no other issue. Good job! 👍

docs/jailer.md

src/jailer/src/cgroup.rs

src/utils/src/arg_parser.rs

tests/framework/jailer.py

iulianbarbu

Looks good. Thanks. Feel free to compact the changes in those two commits.

christian7007 · 2020-08-28T10:35:24Z

Perfect! Thanks for the feedback. I'm running the tests locally after changing this: #2012 (comment) to make sure I haven't break anything (nice catch btw). I'll update the PR as soon as tests are OK.

iulianbarbu

Please explicitly sign each commit:
git commit -s --amend.

christian7007 · 2020-08-28T11:29:39Z

Please explicitly sign each commit:
git commit -s --amend.

Done!

acatangiu · 2020-09-03T09:42:53Z

@christian7007 looks like the PR is ready for merging. There's only a problem of solving a test_coverage.py conflict.
We could do that ourselves if that's fine with you.

christian7007 · 2020-09-03T09:49:20Z

@christian7007 looks like the PR is ready for merging. There's only a problem of solving a test_coverage.py conflict.
We could do that ourselves if that's fine with you.

Sure, feel free to fix it.

gbionescu · 2020-09-07T08:29:00Z

Reworded the commit since it mentioned ARM instead of AMD.

It adds support for arguments which can appear multiple times (e.g jailer --feature f1 --feature f2). This is useful when multiple features of the same type needs to be set via CLI arguments. Signed-off-by: Christian González <cgonzalez@opennebula.io>

New --cgroup argument has been added to the jailer. It adds more flexibility on how cgroups are set by the jailer. - The Cgroup type has been redefined to wrap a cgroup to be set. - The test and documentation have been updated to reflect these changes Signed-off-by: Christian González <cgonzalez@opennebula.io>

christian7007 force-pushed the f1937 branch 2 times, most recently from 8a5d979 to 69a7ebc Compare July 6, 2020 17:36

ioanachirca assigned christian7007 Jul 7, 2020

lauralt added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jul 10, 2020

ioanachirca reviewed Jul 13, 2020

View reviewed changes

iulianbarbu reviewed Jul 20, 2020

View reviewed changes

iulianbarbu added Status: Author and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Jul 22, 2020

iulianbarbu mentioned this pull request Jul 30, 2020

[RFC] tools: add support for environment shielding #2041

Closed

5 tasks

christian7007 force-pushed the f1937 branch 3 times, most recently from 4722a3d to 47ea8c1 Compare August 14, 2020 11:13

iulianbarbu added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Author labels Aug 17, 2020

iulianbarbu removed the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Aug 19, 2020

iulianbarbu reviewed Aug 28, 2020

View reviewed changes

christian7007 force-pushed the f1937 branch from 7773869 to a83d221 Compare August 28, 2020 11:01

iulianbarbu reviewed Aug 28, 2020

View reviewed changes

christian7007 force-pushed the f1937 branch from c66b8d8 to ff80f8a Compare August 28, 2020 11:28

iulianbarbu previously approved these changes Aug 28, 2020

View reviewed changes

gbionescu previously approved these changes Aug 31, 2020

View reviewed changes

gbionescu mentioned this pull request Sep 1, 2020

[Bug] Jailer fails to set cpuset #2110

Closed

3 tasks

gbionescu dismissed stale reviews from iulianbarbu and themself via 048bc3b September 4, 2020 14:22

iulianbarbu previously approved these changes Sep 7, 2020

View reviewed changes

gbionescu dismissed iulianbarbu’s stale review via b9817db September 7, 2020 08:28

gbionescu force-pushed the f1937 branch from ccf1ecb to b9817db Compare September 7, 2020 08:28

iulianbarbu previously approved these changes Sep 7, 2020

View reviewed changes

gbionescu previously approved these changes Sep 7, 2020

View reviewed changes

Christian González added 2 commits September 7, 2020 15:12

gbionescu dismissed stale reviews from iulianbarbu and themself via c1d5d38 September 7, 2020 12:16

gbionescu force-pushed the f1937 branch from 6f065c5 to c1d5d38 Compare September 7, 2020 12:16

gbionescu approved these changes Sep 7, 2020

View reviewed changes

iulianbarbu approved these changes Sep 7, 2020

View reviewed changes

iulianbarbu merged commit 79d9c3d into firecracker-microvm:master Sep 7, 2020

alindima mentioned this pull request Jun 7, 2021

Deprecate jailer's --node parameter #2613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --cgroups parameter to jailer #2012

Add --cgroups parameter to jailer #2012

christian7007 commented Jul 6, 2020 •

edited by dianpopa

lauralt commented Jul 10, 2020

ioanachirca left a comment

ioanachirca Jul 13, 2020

ioanachirca Jul 13, 2020

christian7007 Jul 13, 2020

christian7007 commented Jul 13, 2020

acatangiu commented Jul 20, 2020

christian7007 commented Jul 20, 2020

iulianbarbu left a comment •

edited

iulianbarbu Jul 20, 2020 •

edited

christian7007 Jul 21, 2020

acatangiu commented Jul 24, 2020

christian7007 commented Aug 10, 2020

iulianbarbu commented Aug 11, 2020

christian7007 commented Aug 12, 2020

lauralt commented Aug 12, 2020

iulianbarbu commented Aug 12, 2020 •

edited

christian7007 commented Aug 12, 2020

lauralt commented Aug 12, 2020

christian7007 commented Aug 12, 2020

christian7007 commented Aug 14, 2020 •

edited

iulianbarbu commented Aug 19, 2020

christian7007 commented Aug 27, 2020

iulianbarbu left a comment

iulianbarbu left a comment

christian7007 commented Aug 28, 2020

iulianbarbu left a comment

christian7007 commented Aug 28, 2020

acatangiu commented Sep 3, 2020

christian7007 commented Sep 3, 2020

gbionescu commented Sep 7, 2020

	// This writes the cgroup value into the cgroup property file and attach the pid to the cgroup
	// This writes the cgroup value into the cgroup property file and attaches the pid to the cgroup.

Add --cgroups parameter to jailer #2012

Add --cgroups parameter to jailer #2012

Conversation

christian7007 commented Jul 6, 2020 • edited by dianpopa

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

lauralt commented Jul 10, 2020

ioanachirca left a comment

Choose a reason for hiding this comment

ioanachirca Jul 13, 2020

Choose a reason for hiding this comment

ioanachirca Jul 13, 2020

Choose a reason for hiding this comment

christian7007 Jul 13, 2020

Choose a reason for hiding this comment

christian7007 commented Jul 13, 2020

acatangiu commented Jul 20, 2020

christian7007 commented Jul 20, 2020

iulianbarbu left a comment • edited

Choose a reason for hiding this comment

iulianbarbu Jul 20, 2020 • edited

Choose a reason for hiding this comment

christian7007 Jul 21, 2020

Choose a reason for hiding this comment

acatangiu commented Jul 24, 2020

christian7007 commented Aug 10, 2020

iulianbarbu commented Aug 11, 2020

christian7007 commented Aug 12, 2020

lauralt commented Aug 12, 2020

iulianbarbu commented Aug 12, 2020 • edited

christian7007 commented Aug 12, 2020

lauralt commented Aug 12, 2020

christian7007 commented Aug 12, 2020

christian7007 commented Aug 14, 2020 • edited

iulianbarbu commented Aug 19, 2020

christian7007 commented Aug 27, 2020

iulianbarbu left a comment

Choose a reason for hiding this comment

iulianbarbu left a comment

Choose a reason for hiding this comment

christian7007 commented Aug 28, 2020

iulianbarbu left a comment

Choose a reason for hiding this comment

christian7007 commented Aug 28, 2020

acatangiu commented Sep 3, 2020

christian7007 commented Sep 3, 2020

gbionescu commented Sep 7, 2020

christian7007 commented Jul 6, 2020 •

edited by dianpopa

iulianbarbu left a comment •

edited

iulianbarbu Jul 20, 2020 •

edited

iulianbarbu commented Aug 12, 2020 •

edited

christian7007 commented Aug 14, 2020 •

edited