-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Linux Control Group version 2 API support cgroup v2 #1329
base: main
Are you sure you want to change the base?
Conversation
Binary incompatibility detected for commit 1a35f5d. com.aws.greengrass.deployment.DeploymentConfigMerger is binary incompatible and is source incompatible because of CONSTRUCTOR_REMOVED, METHOD_REMOVED Produced by binaryCompatability.py |
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 1a35f5d |
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 1a35f5d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much duplicate code both between the existing Cgroup enum and the new CgroupV2 class, and also between the existing linux system resource controller and the new v2 class you're adding. The only difference I see is the cgroup file names to write to and the content to write which needs to be derived from the system resource config passed. Please refactor and move everything that's common into base classes. Might not even need a new v2 resource controller class if you simply refactor the cgroup enum/classes to appropriately override methods that provide file names, and also add new methods that will format the cgroup file contents for each cgroup/resource limit
src/main/java/com/aws/greengrass/util/platforms/unix/linux/Cgroup.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/Cgroup.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CgroupSubSystemV2.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CgroupSubSystemV2.java
Outdated
Show resolved
Hide resolved
...va/com/aws/greengrass/integrationtests/lifecyclemanager/GenericExternalServiceIntegTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/LinuxSystemResourceControllerV2.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CGroupSubSystemPath.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/Cgroup.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/Cgroup.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CgroupSubSystem.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CgroupSubSystemV2.java
Outdated
Show resolved
Hide resolved
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CgroupSubSystemV2.java
Outdated
Show resolved
Hide resolved
...t/java/com/aws/greengrass/util/platforms/unix/linux/LinuxSystemResourceControllerV2Test.java
Fixed
Show fixed
Hide fixed
src/main/java/com/aws/greengrass/util/platforms/unix/linux/CGroupSubSystemPaths.java
Fixed
Show fixed
Hide fixed
...t/java/com/aws/greengrass/util/platforms/unix/linux/LinuxSystemResourceControllerV2Test.java
Fixed
Show fixed
Hide fixed
...t/java/com/aws/greengrass/util/platforms/unix/linux/LinuxSystemResourceControllerV2Test.java
Fixed
Show fixed
Hide fixed
cpuPeriodStr = cpuMaxContentArr[1]; | ||
|
||
if (!StringUtils.isEmpty(cpuPeriodStr)) { | ||
int period = Integer.parseInt(cpuPeriodStr.trim()); |
Check notice
Code scanning / CodeQL
Missing catch of NumberFormatException Note
public void handleCpuLimits(GreengrassService component, double cpu) throws IOException { | ||
byte[] content = Files.readAllBytes( | ||
getComponentCpuPeriodPath(component.getServiceName())); | ||
int cpuPeriodUs = Integer.parseInt(new String(content, StandardCharsets.UTF_8).trim()); |
Check notice
Code scanning / CodeQL
Missing catch of NumberFormatException Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment on this PR with test results for the cgroup tests from a system using cgroup v1 and a system using cgroup v2. Show that the unit and integration tests for this feature are passing on both types of cgroup.
Also post a different comment that explains the manual testing performed to ensure that the feature is working properly for v1 and v2.
Issue #, if available:
Description of changes:
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.