search

lf-edge / eve

EVE is Edge Virtualization Engine
https://www.lfedge.org/projects/eve/
Apache License 2.0
468 stars 158 forks source link

Integrate Memory Monitor #3984

Closed OhmSpectator closed 1 week ago

OhmSpectator commented 3 weeks ago

This PR integrates the previously standalone memory-monitor tool into the EVE root filesystem, along with several enhancements and refinements to ensure seamless operation within the EVE environment.

The memory-monitor tool, which monitors the memory usage of the zedbox process, EVE, and pillar cgroups, has been adapted and integrated into the EVE root filesystem. This integration includes creating necessary Dockerfiles, adding services, implementing scripts, and updating build configurations to ensure the tool operates smoothly within the EVE environment. The tool's configuration management has been improved to prioritize user-provided configuration files, allowing for easier customization and management.

Logging has been enhanced to include configuration values at startup, improving visibility and debugging capabilities. Error handling has been refactored to ensure proper operation and avoid undefined behavior. Additionally, operational improvements have been made to ensure necessary application directories are created for writable operation spaces and to introduce a wait loop that ensures the monitoring thread starts only after the zedbox process is fully operational.

Documentation has been updated to reflect the integration, detailing the startup process, configuration management, and deployment instructions.

Scripts and binaries have been renamed for improved clarity and consistency.

OhmSpectator commented 3 weeks ago

Fixed the EOL errors in the README file

OhmSpectator commented 3 weeks ago

Fixed the shellcheck errors

eriknordmark commented 3 weeks ago

Note that yetus only provides the first 10 errors the way we run it on a PR; seems like there are 10 markdownlint errors which means that there are likely to be more than 10.

OhmSpectator commented 3 weeks ago

Note that yetus only provides the first 10 errors the way we run it on a PR; seems like there are 10 markdownlint errors which means that there are likely to be more than 10.

@eriknordmark, I have not touched the markdown linter error yet. I see more than 10 and will fix )

OhmSpectator commented 3 weeks ago

I hope I've fixed the markdownlint warnings...

OhmSpectator commented 3 weeks ago

I think I've taken into account all the comments and issues.

OhmSpectator commented 2 weeks ago

Not sure why the PR builds are failed... =\

OhmSpectator commented 2 weeks ago

Not sure why the PR builds are failed... =\

Actually, I know, I will fix it now.

OhmSpectator commented 2 weeks ago

I'm also

OhmSpectator commented 2 weeks ago
OhmSpectator commented 2 weeks ago

@uncleDecart , @christoph-zededa is it a "regular" test failure: https://github.com/lf-edge/eve/actions/runs/9551860219/job/26328227599#step:3:2779 ?..

uncleDecart commented 2 weeks ago

@OhmSpectator see https://github.com/lf-edge/eden/pull/969, there was a problem with upgrading tests

milan-zededa commented 2 weeks ago

@uncleDecart , @christoph-zededa is it a "regular" test failure: https://github.com/lf-edge/eve/actions/runs/9551860219/job/26328227599#step:3:2779 ?..

Yes, this one is known. Soon I will have patches for eden ready. For now we have to ignore these failures.

OhmSpectator commented 2 weeks ago

Hey @christoph-zededa and @shjala, do you notice any important issues in the PR? I'm working on addressing all your comments, but there's a request to merge it sooner rather than later. If there's something we can address in future PRs, it would be great. I'm currently adding the security flags and will attempt to remove the privileged property from the container image. The AppArmor integration task I would postpone for an upcoming PR. Your opinion?

OhmSpectator commented 2 weeks ago

I've also removed the privileged option from the container.

shjala commented 2 weeks ago

Hey @christoph-zededa and @shjala, do you notice any important issues in the PR? I'm working on addressing all your comments, but there's a request to merge it sooner rather than later. If there's something we can address in future PRs, it would be great. I'm currently adding the security flags and will attempt to remove the privileged property from the container image. The AppArmor integration task I would postpone for an upcoming PR. Your opinion?

Can you wait and merge it at start of next week?

OhmSpectator commented 2 weeks ago

Can you wait and merge it at start of next week?

@shjala, I think so. Do you need extra time to general review it? Or would you like me to look at something specific? I would rather not postpone me working on the code while I remember it =D

OhmSpectator commented 1 week ago

@shjala will we continue with the review today?

shjala commented 1 week ago

Lets do the UID/Apparmor adjustment in another PR, except the two comments, LGTM.

OhmSpectator commented 1 week ago

Lets do the UID/Apparmor adjustment in another PR

I've created a ticket for that.