Initialize Configuration#Concurrency from /sys/fs/cgroup/... - #7845
Initialize Configuration#Concurrency from /sys/fs/cgroup/...#7845Al2Klimov wants to merge 3 commits into
Conversation
|
TODO
|
|
... e.g. |
5793b4e to
66b84be
Compare
MacOSCentOSDocker |
|
@cla-bot check |
julianbrost
left a comment
There was a problem hiding this comment.
Doesn't work with cgroup2, there the information is in /sys/fs/cgroup/cpu* files (i.e. not in extra subdirectories).
In general, an overview of how the Docker options map to cgroup attributes (and therefore which of these options this PR takes into account) would be nice. Looking at the corresponding systemd options is probably also interesting and they do properly document the cgroup options affected.
| int Configuration::Concurrency{static_cast<int>(std::thread::hardware_concurrency())}; | ||
|
|
||
| #ifdef __linux__ | ||
| static std::string ReadSysLine(const char *file) |
There was a problem hiding this comment.
This function looks overly complex for what it does. Shouldn't this basically be the same as std::ifstream + std::getline? The only thing I see that wouldn't be possible with that is the check for ENOENT. If this is just for kernel version compatibility, using an extra check would be fine. If this file can (dis)appear at runtime (changing the cgroup options, moving the process between cgroups), have a look at boost::iostreams like it's already used in AtomicFile.
| return ""; | ||
| } | ||
|
|
||
| throw system_error(error_code(errno, system_category())); |
There was a problem hiding this comment.
Do we really want to fail starting in this case? I think logging a warning and continuing with the value from std::thread::hardware_concurrency() would be a better option.
There was a problem hiding this comment.
- In "this case" a kernel subsystem(!) is out of service. The best reason to panic.
- Logging? We're in a static constructor.
|
|
TODO @Al2Klimov
|
66b84be to
1d6ab00
Compare
|
Doesn’t whoever controls the container spec also control the command it runs? I.e. can't they just set -DConfiguration.Concurrency= in addition to --cpu-whatever? I.e. do we need this PR? |
Most likely yes.
Not strictly needed as it can be worked around. However, the user has to be aware of this and it would be nice if Icinga would just do the correct thing by itself. |
1d6ab00 to
cda810d
Compare
cda810d to
26c8c91
Compare
26c8c91 to
dde5e35
Compare
dde5e35 to
7e9bbc7
Compare
... not to try to consume more hardware resources than the admin assigned.
fixes #7842