-
Notifications
You must be signed in to change notification settings - Fork 371
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
Update log collector unit file to remove memorylimit #2757
Conversation
@@ -119,7 +119,7 @@ def _set_resource_usage_cgroups(cpu_cgroup_path, memory_cgroup_path): | |||
@staticmethod | |||
def _initialize_telemetry(): | |||
protocol = get_protocol_util().get_protocol(init_goal_state=False) | |||
protocol.client.reset_goal_state(goalstate_properties=GoalStateProperties.RoleConfig | GoalStateProperties.HostingEnv) | |||
protocol.client.reset_goal_state(goal_state_properties=GoalStateProperties.RoleConfig | GoalStateProperties.HostingEnv) |
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.
I found this issue from a previous PR because collect-logs was failing to run
Codecov Report
@@ Coverage Diff @@
## develop #2757 +/- ##
===========================================
- Coverage 72.04% 72.03% -0.01%
===========================================
Files 104 104
Lines 15832 15831 -1
Branches 2265 2264 -1
===========================================
- Hits 11406 11404 -2
+ Misses 3912 3909 -3
- Partials 514 518 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
slice_contents = _LOGCOLLECTOR_SLICE_CONTENTS_FMT.format(cpu_quota=_LOGCOLLECTOR_CPU_QUOTA) | ||
|
||
files_to_create.append((logcollector_slice, slice_contents)) | ||
## Log collector slice should be updated to remove MemoryLimit quota from previous GA versions |
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.
I would rephrase this comment a little. In this specific instance we want to remove the limit set by 2.8, but in general what we want is to update the settings with the values used by the current version (which is what this code is doing)
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.
Updated the comment
# The mock creates the slice unit file with memory limit | ||
configurator.mocks.add_data_file(os.path.join(data_dir, 'init', "azure-walinuxagent-logcollector.slice"), | ||
UnitFilePaths.logcollector) | ||
self.assertTrue(os.path.exists(log_collector_unit_file), |
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.
setup errors should be reported with exceptions, rather than asserts, to distinguish them from actual test verifications (such as line 208 below)
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.
Updated this check & the next check that memory limit exists to raise exceptions instead
Description
This previous PR (#2637) removed memory limit from the log collector slice unit file, but cgroupconfigurator currently only updates the unit file if it does not already exist. This memory limit change is included in 2.9.0.4. For VMs which upgrade to 2.9.0.4 from earlier versions, the unit file is created with a memorylimit and never updated.
This PR updates cgroupconfigurator to always update the log collector slice unit file to remove memorylimit.
Issue #
PR information
Quality of Code and Contribution Guidelines