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.

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

Fix cgroup name mismatches and add missing services #4165

Merged

Conversation

OhmSpectator
Copy link
Member

This PR addresses discrepancies between cgroup names used in the 010-eve-cgroup initialization script and the service configuration files. These mismatches have led to certain memory limits not being applied correctly to specific services. The changes included in this PR ensure consistency across service names in the cgroup settings and service configurations, thereby fixing the memory limit application issue.

There were three main issues identified:

  • EdgeView Service Name Mismatch: The edgeview service was using a different name in the cgroup settings and service configuration files, leading to incorrect cgroup paths and, subsequently, memory limits not being applied.
  • Debug Service Omission: The debug service was not listed in the EVESERVICES array in the 010-eve-cgroup initialization script, which caused it to bypass the regular EVE cgroup memory limit.
  • Memory-Monitor Service Omission: Similar to the debug service, the memory-monitor service was also missing from the EVESERVICES array, leading to it not being assigned the appropriate memory limit.

@@ -9,7 +9,7 @@ hv=`cat /run/eve-hv-type`
default_cgroup_cpus_limit=1

default_cgroup_memory_limit=838860800 #800M
EVESERVICES="sshd edgeview wwan wlan lisp guacd pillar vtpm watchdog xen-tools newlogd memlogd"
EVESERVICES="sshd eve-edgeview wwan wlan lisp guacd pillar vtpm watchdog xen-tools newlogd memlogd memory-monitor debug"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we unify cgroup names and avoid the eve- prefix which no other service is using?
CC @naiming-zededa

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would be nicer, but changing only one file is simpler =)

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

Does this need to be backported i.e., should it be labeled stable?

@OhmSpectator
Copy link
Member Author

LGTM

Does this need to be backported i.e., should it be labeled stable?

Good point. I think, yes, it should. I'll create backport PRs later today.

@OhmSpectator OhmSpectator added the stable Should be backported to stable release(s) label Aug 28, 2024
@@ -9,7 +9,7 @@ hv=`cat /run/eve-hv-type`
default_cgroup_cpus_limit=1

default_cgroup_memory_limit=838860800 #800M
EVESERVICES="sshd edgeview wwan wlan lisp guacd pillar vtpm watchdog xen-tools newlogd memlogd"
EVESERVICES="sshd eve-edgeview wwan wlan lisp guacd pillar vtpm watchdog xen-tools newlogd memlogd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't forget to reflect changed directory output in your own documentation: ./pkg/memory-monitor/README.md:18

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the example of the cgroups hierarchy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

The EdgeView service is integrated into EVE OS with the name
"eve-edgeview". It is set to use the corresponding cgroup name. Hence
fix the cgroup path in the cgroup setting script.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Added `memory-monitor` to the `EVESERVICES` list in the `010-eve-cgroup`
initialization script. This way the regular eve cgroup memory limit will
be applied to it.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
Added `debug` to the `EVESERVICES` list in the `010-eve-cgroup`
initialization script. This way the regular eve cgroup memory limit will
be applied to it.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
…up limits.

Added comments to explain memory limits for containerd and services
cgroups. Simplified the list of services under the "services" cgroup.

Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
@OhmSpectator
Copy link
Member Author

The tests were green before I updated a README file. So, I should not be affected. Mergeing to crate backport PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stable Should be backported to stable release(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants