-
Notifications
You must be signed in to change notification settings - Fork 377
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
Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
2f8fbc7
cgroup rewrite
nagworld9 b863a09
new test
nagworld9 083a531
mock cleanup
nagworld9 3bd0904
logging
nagworld9 cee9d2f
address comments
nagworld9 58709df
Merge remote-tracking branch 'origin/develop' into cgroup-v2
nagworld9 274def2
removed script
nagworld9 39ec1ea
new comment changes
nagworld9 8a31f42
pylint
nagworld9 4533f3a
Merge branch 'develop' into cgroup-v2
nagworld9 3b6d0d3
e2e test changes
nagworld9 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,7 +59,7 @@ class CGroupUtil(object): | |
Cgroup utility methods which are independent of systemd cgroup api. | ||
""" | ||
@staticmethod | ||
def cgroups_supported(): | ||
def distro_supported(): | ||
distro_info = get_distro() | ||
distro_name = distro_info[0] | ||
try: | ||
|
@@ -149,7 +149,7 @@ def __init__(self, msg=None): | |
super(InvalidCgroupMountpointException, self).__init__(msg) | ||
|
||
|
||
def get_cgroup_api(): | ||
def create_cgroup_api(): | ||
""" | ||
Determines which version of Cgroup should be used for resource enforcement and monitoring by the Agent and returns | ||
the corresponding Api. | ||
|
@@ -172,7 +172,6 @@ def get_cgroup_api(): | |
root_hierarchy_mode = shellutil.run_command(["stat", "-f", "--format=%T", CGROUP_FILE_SYSTEM_ROOT]).rstrip() | ||
|
||
if root_hierarchy_mode == "cgroup2fs": | ||
log_cgroup_info("Using cgroup v2 for resource enforcement and monitoring") | ||
return SystemdCgroupApiv2() | ||
|
||
elif root_hierarchy_mode == "tmpfs": | ||
|
@@ -192,7 +191,6 @@ def get_cgroup_api(): | |
# mounted in a location other than the systemd default, raise Exception. | ||
if not cgroup_api_v1.are_mountpoints_systemd_created(): | ||
raise InvalidCgroupMountpointException("Expected cgroup controllers to be mounted at '{0}', but at least one is not. v1 mount points: \n{1}".format(CGROUP_FILE_SYSTEM_ROOT, json.dumps(cgroup_api_v1.get_controller_mountpoints()))) | ||
log_cgroup_info("Using cgroup v1 for resource enforcement and monitoring") | ||
return cgroup_api_v1 | ||
|
||
raise CGroupsException("{0} has an unexpected file type: {1}".format(CGROUP_FILE_SYSTEM_ROOT, root_hierarchy_mode)) | ||
|
@@ -650,8 +648,6 @@ def get_controllers(self, expected_relative_path=None): | |
controller = MemoryControllerV1(self._cgroup_name, controller_path) | ||
|
||
if controller is not None: | ||
msg = "{0} controller for cgroup: {1}".format(supported_controller_name, controller) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging here is a side-effect of calling get_controllers which being called everywhere. As a result, we are simply logging at random places where it's not needed. So, removing it here |
||
log_cgroup_info(msg) | ||
controllers.append(controller) | ||
|
||
return controllers | ||
|
@@ -729,8 +725,6 @@ def get_controllers(self, expected_relative_path=None): | |
controller = MemoryControllerV2(self._cgroup_name, self._cgroup_path) | ||
|
||
if controller is not None: | ||
msg = "{0} controller for cgroup: {1}".format(supported_controller_name, controller) | ||
log_cgroup_info(msg) | ||
controllers.append(controller) | ||
|
||
return controllers | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What does it mean for a unit to be loaded
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.
Unit being loaded means unit is parsed and available to the systemd process. We check this for extension services when we set quotas. If systemd is unaware of extension services and not loaded in the system yet, we get error while setting quotas. Hence, I added loaded check.
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.
If systemd is unaware of extension services and not loaded in the system yet, we get error while setting quotas. Hence, I added loaded check.
can you add this as a comment where you call this function?