Repository navigation
Added functionality to cope with hybrid cgroups set ups - #7493
ChrisPaulBennett wants to merge 11 commits into
Conversation
jfrost-mo
left a comment
jfrost-mo
left a comment
There was a problem hiding this comment.
The approach looks sensible, and it looks like the cgroup interaction should work. I've added several comments on the implementation specifics, and I still need to run it myself to test.
| return Process( | ||
| cgroup_memory_path=memory_dir / "memory.stat", | ||
| cgroup_cpu_path=( | ||
| cpu_dir / ("cpu.stat" if cpu_version == 2 else "cpuacct.usage") |
There was a problem hiding this comment.
When using the same string literals more than once, please extract into variables to protect against typos and future changes only affecting one instance etc.
2dd3c84 to
87bd90c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ddee313 to
5c7ce5f
Compare
jfrost-mo
left a comment
jfrost-mo
left a comment
There was a problem hiding this comment.
Actually, the tests are failing (looks like a dodgy import) so going to leave on request changes until fixed.
b62336f to
37455d1
Compare
jfrost-mo
left a comment
jfrost-mo
left a comment
There was a problem hiding this comment.
Looking good, and tests are now passing.
MetRonnie
left a comment
MetRonnie
left a comment
There was a problem hiding this comment.
Got some suggested tidying at ChrisPaulBennett#6
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Tidy profiler code
|
|
||
|
|
||
| def test_get_cgroup_names_test_env_var(monkeypatch): | ||
| """It should allow the functional tests to inject a cgroup name.""" |
There was a problem hiding this comment.
Eh? Testing functionality only intended for the tests?
Can profiler_test_env_var be removed?
There was a problem hiding this comment.
If you can find another way to do it (I believe this was olivers suggestion, because I was stuck).
As described in comments this is a fugly hack, testing it makes less fugly, no?
Like make up on a bulldog?
There was a problem hiding this comment.
So the context I was missing is that profiler_test_env_var is used in 04-profiler.t.
We do not need to test this in the unit tests, if it's broken then 04-profiler.t will fail.
The HPC cgroups in set up in a unified state. I'm assuming something has changed with the HPC set up, because this was working.
This unified set up has caused a refactor and added some complexity. CPU is controlled via v2 but memory is controlled by v1.
Due to both v1 and v2 being used together, there are instances of v2 files being present but having no data due to v1 being used. So the checking for cgroup versions has been made more robust.
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.