Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update log collector unit file to remove memorylimit #2757

Merged
merged 10 commits into from
Feb 10, 2023
Merged

Update log collector unit file to remove memorylimit #2757

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
da72c37
Update version to dummy 1.0.0.0'
maddieford Nov 8, 2022
59dbd22
Revert version change
maddieford Nov 8, 2022
633a826
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Nov 21, 2022
14a743f
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Dec 8, 2022
54ea0f3
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Jan 10, 2023
e79c4c5
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Feb 8, 2023
162ebbc
Update log collector slice unit file without memory limit
maddieford Feb 9, 2023
c8dda53
Update comment
maddieford Feb 9, 2023
cc879ac
Update setup errors as exceptions
maddieford Feb 10, 2023
18f2c24
Update comment for accuracy
maddieford Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions azurelinuxagent/common/cgroupconfigurator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,9 @@ def __setup_azure_slice():
if not os.path.exists(vmextensions_slice):
files_to_create.append((vmextensions_slice, _VMEXTENSIONS_SLICE_CONTENTS))

if not os.path.exists(logcollector_slice):
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
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment

slice_contents = _LOGCOLLECTOR_SLICE_CONTENTS_FMT.format(cpu_quota=_LOGCOLLECTOR_CPU_QUOTA)
files_to_create.append((logcollector_slice, slice_contents))

if fileutil.findre_in_file(agent_unit_file, r"Slice=") is not None:
CGroupConfigurator._Impl.__cleanup_unit_file(agent_drop_in_file_slice)
Expand Down
2 changes: 1 addition & 1 deletion azurelinuxagent/common/logcollector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

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

# Initialize the common parameters for telemetry events
initialize_event_logger_vminfo_common_parameters(protocol)

Expand Down
1 change: 1 addition & 0 deletions tests/common/mock_cgroup_environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@

class UnitFilePaths:
walinuxagent = "/lib/systemd/system/walinuxagent.service"
logcollector = "/lib/systemd/system/azure-walinuxagent-logcollector.slice"
azure = "/lib/systemd/system/azure.slice"
vmextensions = "/lib/systemd/system/azure-vmextensions.slice"
extensionslice = "/lib/systemd/system/azure-vmextensions-Microsoft.CPlat.Extension.slice"
Expand Down
21 changes: 21 additions & 0 deletions tests/common/test_cgroupconfigurator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,27 @@ def test_initialize_should_create_unit_files_when_the_agent_service_file_is_not_
self.assertTrue(os.path.exists(agent_drop_in_file_cpu_accounting), "{0} was not created".format(agent_drop_in_file_cpu_accounting))
self.assertTrue(os.path.exists(agent_drop_in_file_memory_accounting), "{0} was not created".format(agent_drop_in_file_memory_accounting))

def test_initialize_should_update_logcollector_memorylimit(self):
with self._get_cgroup_configurator(initialize=False) as configurator:
log_collector_unit_file = configurator.mocks.get_mapped_path(UnitFilePaths.logcollector)
original_memory_limit = "MemoryLimit=30M"

# 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),
Copy link
Member

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)

Copy link
Contributor Author

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

"{0} was not created".format(log_collector_unit_file))
self.assertTrue(fileutil.findre_in_file(log_collector_unit_file, original_memory_limit),
"MemoryLimit was not set correctly. Expected: {0}. Got:\n{1}".format(
original_memory_limit, fileutil.read_file(log_collector_unit_file)))

configurator.initialize()

# initialize() should update the unit file to remove the memory limit
self.assertFalse(fileutil.findre_in_file(log_collector_unit_file, original_memory_limit),
"Log collector slice unit file was not updated correctly. Expected no memory limit. Got:\n{0}".format(
fileutil.read_file(log_collector_unit_file)))

def test_setup_extension_slice_should_create_unit_files(self):
with self._get_cgroup_configurator() as configurator:
# get the paths to the mocked files
Expand Down
9 changes: 9 additions & 0 deletions tests/data/init/azure-walinuxagent-logcollector.slice
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[Unit]
Description=Slice for Azure VM Agent Periodic Log Collector
DefaultDependencies=no
Before=slices.target
[Slice]
CPUAccounting=yes
CPUQuota=5%
MemoryAccounting=yes
MemoryLimit=30M