Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Update lxc config option and fix cgroup2 volume mount #38

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

h0tw1r3
Copy link
Contributor

@h0tw1r3 h0tw1r3 commented Jun 13, 2022

replace default_config task option with config_file

Only support config_file on the task to behave exactly as the lxc command line tools. The default_config file location is provided automatically by the lxc library and can already by overridden per-system (see man lxc.system.conf).

Also removed various stats no longer supported with cgroups v2 and some were removed in kernel 5.4+.

h0tw1r3 added 2 commits June 12, 2022 10:37
Only support config_file on the task to behave exactly as the lxc
command line tools.

The default_config file location is provided by the lxc library and
can already by overridden per-system (see man lxc.system.conf).
no equiv stats available, also removed from cgroups v1 in kernel 5.4+
@h0tw1r3 h0tw1r3 changed the title cleanup lxc config loading Update lxc config option and fix cgroup2 volume mount Jun 13, 2022
@DerekStrickland DerekStrickland removed their assignment Jun 21, 2022
@tgross tgross self-requested a review July 5, 2022 20:32
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @h0tw1r3! Thanks for this PR, but I'm not sure it can be accepted in its current state. I've left some comments for further discussion.

Comment on lines -50 to -53
"default_config": hclspec.NewDefault(
hclspec.NewAttr("default_config", "string", false),
hclspec.NewLiteral("\""+lxc.GlobalConfigItem("lxc.default_config")+"\""),
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this set in the plugin configuration and not the job spec because under the Nomad security model personas the LXC config file would be controlled by the System Administrator or Nomad Administrator, whereas the jobspec is controlled by the Nomad Operator (the person who runs nomad job run). Generally speaking it doesn't make sense to give the Nomad Operator the ability to pick and choose among files that are owned by the client.

Additionally, the appropriate file would be a property of the client, not all clients, so that belongs in the plugin configuration. Imagine the case where someone submits a job with a particular LXC config path that doesn't exist on the client it happens to get placed on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will rework.

LXCMeasuredMemStats = []string{"RSS", "Cache", "Swap", "Max Usage", "Kernel Usage", "Kernel Max Usage"}
LXCMeasuredMemStats = []string{"RSS", "Cache", "Swap"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said in the PR description:

Also removed various stats no longer supported with cgroups v2 and some were removed in kernel 5.4+.

Those stats will still be supported on cgroups v1 machines. Unless all supported versions of LXC stopped supporting cgroups v2, we shouldn't remove these stats. I can't find any notes to this in their docs, but I'll admit their docs are also not very searchable 😀

Copy link
Contributor Author

@h0tw1r3 h0tw1r3 Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed them due to provide a consistent view regardless of the system cgroup version. My goal was to support jobs running in a mixed cgroup environment. Provides a seamless path to cgroup v2 only systems.

If you would prefer to keep the stats (with no values for v2), I could ignore errors when fetching measured stats from LXC.
Or, define two different LXCMeasuredMemStats values depending on the cgroup version.

Open to any suggestions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants