Share "Last reload attempt failed" time across Icinga process tree on *nix

[Al2Klimov]

... as only the umbrella process knows that time,
but the icinga check running in the main process also needs to know it.

fixes #8428

TODO

• GHA: use a CMake version which can set(CMAKE_CXX_STANDARD 17) where possible #9754

[Al2Klimov]

@cla-bot check

[Al2Klimov]

@julianbrost Would you prefer Boost shared memory?

[julianbrost]

The deal with shared memory across processes is that if you use it, you start relying on implementation-defined behavior. This answer on Stack Overflow provides a pretty good analysis for std::atomic<> in shared memory. Summary being that if it's lock-free (which in itself isn't guaranteed), then it should probably be fine (but also, not guaranteed).

So in general, I prefer to avoid shared memory if feasible but this boils down to whether we have a suitable alternative. In this sense, it's closely related to #9445: the timestamp here should be the same as the last time an existing worker was allowed to process config updates in #9445. As we consider both PRs for 2.14, I'll take a closer look at both soon and come back to this once I know more.

If the answer is shared memory, it should probably be an abstraction like in 58c15bc. And whatever is communicated over that should be as safe as possible, that should mean at least testing or asserting that std::atomic<double> is lock free or having a look if Boost provides something suitable (i.e. they did the testing for us).

[Al2Klimov]

Don't worry. Nobody's gonna support Icinga on your toaster. It runs NetBSD doesn’t mean it runs Icinga :)

[Al2Klimov]

GHA is wrong. Works for me.

[Al2Klimov]

Compiles even on my favourite marginal OS. 🎉

[Al2Klimov]

Apropos.

@sthen Consider disabling unity builds by default (net/icinga/core2). I did this locally in Makefile and was able to compile Icinga + deps (actually deps + Icinga ;) ) with only 2G RAM.

[sthen]

The port is setup to use non-unity builds on 32-bit architectures for memory reasons already - and I did see build failures with that after updating to some newer release before - which makes me want to stick with the upstream default where possible really because we're then closer to the normal build environment.

…

On 12 May 2023 09:18:23 Alexander Aleksandrovič Klimov ***@***.***> wrote: Apropos. @sthen Consider disabling unity builds by default (net/icinga/core2). I did this locally in Makefile and was able to compile Icinga + deps (actually deps + Icinga ;) ) with only 2G RAM. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[Al2Klimov]

Even this isn’t working.

@yhabteab Any idea why?

[yhabteab]

Even this isn’t working.

@yhabteab Any idea why?

Boost warns about the following problem:

When constructing a class with static members, each process has its own copy of the static member, so updating a static member in one process does not change the value of the static member the another process. So be careful with these classes. Static members are not dangerous if they are just constant variables initialized when the process starts, but they don't change at all (for example, when used like enums) and their value is the same for all processes.

It seems that you have encountered exactly the same problem here. When the worker process is initially launched, it obtains a copy of this member variable and it won't notice that it's meanwhile updated by other (the umbrella) processes.

[Al2Klimov]

Even this isn’t working.

Just because I've reloaded not the node the check was scheduled on. 🐘

A simple icinga service, a config syntax error, a reload – and it complains.

[julianbrost]

Just because I've reloaded not the node the check was scheduled on.

Not only for the intermediate versions you pushed but also with the previous version? So every "this doesn't work" you said this week was a false alarm?

[Al2Klimov]

Especially with the previous version! I.e. 5c330e9

[yhabteab]

This actually works well on my end, and the code is also (to me) fine compared to the previous version.