Introduce a new wsl.conf config value to allow distributions to opt-in to cgroupv1 mounts #13546
Conversation
OneBlue
requested review from
benhillis,
Copilot,
craigloewen-msft and
justus-camp-microsoft
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new configuration option in /etc/wsl.conf to allow distributions to opt-in to cgroupv1 mounts instead of the default cgroupv2. The change adds an automount.cgroups setting that accepts values "v1" or "v2", with fallback logic to cgroupv2 when the kernel has cgroup_no_v1=all set.
Key changes:
- Adds CGroupVersion enum with v1 and v2 options to WslDistributionConfig
- Implements cgroupv1 mounting logic with kernel command line validation
- Adds comprehensive unit tests for the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/linux/init/WslDistributionConfig.h | Defines CGroupVersion enum and adds CGroup field to config struct |
| src/linux/init/WslDistributionConfig.cpp | Adds automount.cgroups config key parsing with v1/v2 enum mapping |
| src/linux/init/config.h | Updates ConfigInitializeCgroups function signature to accept config parameter |
| src/linux/init/config.cpp | Implements cgroupv1 mounting logic with kernel validation and fallback behavior |
| test/windows/UnitTests.cpp | Adds comprehensive test coverage for cgroupv1 functionality and fallback scenarios |
| if ((UtilParseCgroupsLine(Line, &Subsystem, &Enabled) < 0) || (Enabled == false) || | ||
| std::find(DisabledControllers.begin(), DisabledControllers.end(), Subsystem) != DisabledControllers.end()) | ||
|
|
There was a problem hiding this comment.
The condition check spans multiple lines with poor formatting. The empty line 1874 creates confusion about where the condition ends. Consider reformatting this condition for better readability, such as breaking it into multiple if statements or aligning the conditions properly.
| if ((UtilParseCgroupsLine(Line, &Subsystem, &Enabled) < 0) || (Enabled == false) || | |
| std::find(DisabledControllers.begin(), DisabledControllers.end(), Subsystem) != DisabledControllers.end()) | |
| if ((UtilParseCgroupsLine(Line, &Subsystem, &Enabled) < 0) || | |
| (Enabled == false) || | |
| std::find(DisabledControllers.begin(), DisabledControllers.end(), Subsystem) != DisabledControllers.end()) |
Copilot uses AI. Check for mistakes.
| auto position = commandLine.find(CGROUPS_NO_V1); | ||
| if (position != std::string::npos) | ||
| { | ||
| auto list = commandLine.substr(position + sizeof(CGROUPS_NO_V1) - 1); | ||
| auto end = list.find_first_of(" \n"); | ||
| if (end != std::string::npos) | ||
| { | ||
| list = list.substr(0, end); | ||
| } | ||
|
|
||
| if (list == "all") | ||
| { | ||
| LOG_WARNING("Distribution has cgroupv1 enabled, but kernel command line has {}all. Falling back to cgroupv2", CGROUPS_NO_V1); | ||
| Config.CGroup = WslDistributionConfig::CGroupVersion::v2; | ||
| } | ||
| else | ||
| { | ||
| DisabledControllers = wsl::shared::string::Split(list, ','); | ||
| } |
There was a problem hiding this comment.
The code doesn't handle potential errors from UtilReadFileContent. If reading /proc/cmdline fails, the subsequent string operations could operate on invalid data. Consider adding error handling or validation that the file was read successfully.
| auto position = commandLine.find(CGROUPS_NO_V1); | |
| if (position != std::string::npos) | |
| { | |
| auto list = commandLine.substr(position + sizeof(CGROUPS_NO_V1) - 1); | |
| auto end = list.find_first_of(" \n"); | |
| if (end != std::string::npos) | |
| { | |
| list = list.substr(0, end); | |
| } | |
| if (list == "all") | |
| { | |
| LOG_WARNING("Distribution has cgroupv1 enabled, but kernel command line has {}all. Falling back to cgroupv2", CGROUPS_NO_V1); | |
| Config.CGroup = WslDistributionConfig::CGroupVersion::v2; | |
| } | |
| else | |
| { | |
| DisabledControllers = wsl::shared::string::Split(list, ','); | |
| } | |
| if (!commandLine.empty()) { | |
| auto position = commandLine.find(CGROUPS_NO_V1); | |
| if (position != std::string::npos) | |
| { | |
| auto list = commandLine.substr(position + sizeof(CGROUPS_NO_V1) - 1); | |
| auto end = list.find_first_of(" \n"); | |
| if (end != std::string::npos) | |
| { | |
| list = list.substr(0, end); | |
| } | |
| if (list == "all") | |
| { | |
| LOG_WARNING("Distribution has cgroupv1 enabled, but kernel command line has {}all. Falling back to cgroupv2", CGROUPS_NO_V1); | |
| Config.CGroup = WslDistributionConfig::CGroupVersion::v2; | |
| } | |
| else | |
| { | |
| DisabledControllers = wsl::shared::string::Split(list, ','); | |
| } | |
| } | |
| } else { | |
| LOG_WARNING("Failed to read /proc/cmdline; skipping cgroup_no_v1 check."); |
Copilot uses AI. Check for mistakes.
|
What are the real life purposes here. Running old Linux distributions? Just curious. |
4 participants
…
Summary of the Pull Request
This change introduces a new
/etc/wsl.confconfig value to allow a specific distribution to opt-in to unified cgroup mount instead of just cgroupv2:While this config value only has two options, it's designed as an enum to allow for other values in the future.
This will help solve issues like #13328, where systemd worked on earlier versions of WSL (before unified cgroup support was dropped).
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed