master before #9627 (a0286e9c6):
<1> => namespace n { x = 42; x = 42 }
^^^^^^
Constant must not be modified.
<2> =>
HEAD of #9627 (24b57f0d3):
<1> => namespace n { x = 42; x = 42 }
null
<2> =>
if caused by dependency or check period.
Now as long as any of the above causes check skips
next check and next update will be up-to-date in Icinga DB,
so the checkable won't slide into false positive overdue.
This was accidentally broken by #9627 because during config sync, a config
validation happens that uses `--define System.ZonesStageVarDir=...` which fails
on the now frozen namespace.
This commit changes this to use `Internal.ZonesStageVarDir` instead. After all,
this is used for internal functionality, users should not directly interact
with this flag.
Additionally, it no longer freezes the `Internal` namespace which actually
allows using `Internal.ZonesStageVarDir` in the first place. This also fixes
`--define Internal.Debug*` which was also broken by said PR. Freezing of the
`Internal` namespace is not necessary for performance reasons as it's not
searched implicitly (for example when accessing `globals.x`) and should users
actually interact with it, they should know by that name that they are on their
own.
This commit moves the initialization of the globals.Types namespace to type.cpp
in order to keep a pointer to the Namespace object in Type::m_Namespace and
simplify Type::GetByName() using it.
The dynamic type check is moved into an assertion after freezing the namespace.
This makes freezing a namespace an irrevocable operation but in return allows
omitting further lock operations. This results in a performance improvement as
reading an atomic bool is faster than acquiring and releasing a shared lock.
ObjectLocks on namespaces remain untouched as these mostly affect write
operations which there should be none of after freezing (if there are some,
they will throw exceptions anyways).
This commit removes EmbeddedNamespaceValue and ConstEmbeddedNamespaceValue and
reduces NamespaceValue down to a simple struct without inheritance or member
functions. The code from these clases is inlined into the Namespace class. The
class hierarchy determining whether a value is const is moved to an attribute
of NamespaceValue.
This is done in preparation for changes to the locking in the Namespace class.
Currently, it relies on a recursive mutex. In the future, a shared mutex
(read/write lock) should be used instead, which cannot allow recursive locking
(without failing or risk deadlocking on lock upgrades). With this change, all
operations requiring a lock for one operation are within one function, no
recursive locking is not needed any more.
This commit adds a new initialization priority `FreezeNamespaces` that is run
last and moves all calls to `Namespace::Freeze()` there. This allows all other
initialization functions to still update namespaces without the use of the
`overrideFrozen` flag.
It also moves the initialization of `System.Platform*` and `System.Build*` to
an initialize function so that these can also be set without setting
`overrideFrozen`.
This is preparation for a following commit that will make the frozen flag in
namespaces finial, no longer allowing it to be overriden (freezing the
namespace will disable locking, so performing further updates would be unsafe).
Now that all values are in one place, there is no reason for this numbering
with gaps anymore. If you need to insert a new value in between, you can just
do so in the enum.
This reverses the sort order of the enum, thereby requiring a change to the
sort order of the std::priority_queue containing the elements.
Change the type of the priority values from int to a new enum. By replacing the
magic int values throughout the code base with named values, there is now a
single place where all priority values are defined and you get an overview over
the initialization order.
InitializeOnceHelper calls Loader::AddDeferredInitializer which takes a
std::function, so it's eventually converted to that anyways. This commit just
does this a bit earlier, and by saving the step of the intermediate C function
pointer, this would now also work for capturing lambdas (which there are none
of at the moment).
to restore the behavior before the previous commit. Otherwise we'd delete all
API object's child objects' files including applied child object rules in /etc.
In essence, namespace behaviors acted as hooks for update operations on
namespaces. Two behaviors were implemented:
- `NamespaceBehavior`: allows the update operation unless it acts on a value
that itself was explicitly marked as constant.
- `ConstNamespaceBehavior`: initially allows insert operations but marks the
individual values as const. Additionally provides a `Freeze()` member
function. After this was called, updates are rejected unless a special
`overrideFrozen` flag is set explicitly.
This marvel of object-oriented programming can be replaced with a simple bool.
This commit basically replaces `Namespace::m_Behavior` with
`Namespace::m_ConstValues` and inlines the behavior functions where they were
called. While doing so, the code was slightly simplified by assuming that
`m_ConstValues` is true if `m_Frozen` is true. This is similar to what the API
allowed in the old code as you could only freeze a `ConstNamespaceBehavior`.
However, this PR moves the `Freeze()` member function and the related
`m_Freeze` member variable to the `Namespace` class. So now the API allows any
namespace to be frozen. The new code also makes sense with the previously
mentioned simplification: a `Namespace` with `m_ConstValues = false` can be
modified without restrictions until `Freeze()` is called. When this is done, it
becomes read-only.
The changes outside of `namespace.*` just adapt the code to the slightly
changed API.
Copying an ObjectLock results in the underlying mutex being unlocked too often.
There's also no good reason for copying a scoped locking class (if at all, it
should be moved).
instead of computing from scratch if they're in the _api package.
For now this changes literally nothing as paths of existing objects still follow
the scheme of paths of new objects which didn't change. Now Icinga only doesn't expect
existing objects at particular paths. However, with the latter in v2.14+ (agent,
satellite) we can just change the path scheme of new objects in v2.16+ (master)
as we wish. The child nodes will just follow the new scheme of paths of new objects.
This prevents the `m_HasMatches` property from being altered simultaneously.
This might seem harmless (since this property can only be set to true by any calling thread),
however, from a technical (C++) point of view, this constitutes a data race.
1. The lookup of apply rules per source type now implies
no String(const char*) (no malloc()) and just pointer (uint64) comparisions
2. Apply rules are now also grouped by target type via a nested map, that obsoletes
checking the target type while iterating over all rules per source type
When committing an item with `ignore_on_error` flag set fails, the `Commit()` method only returns `nullptr`
and the current item is not being dropped from `m_Items`. `CommittNewItems()` also doesn't check the return
value of `Commit()` but just continues and tries to commit all items from `m_Items` in recursive call. Since
this corrupt item is never removed from `m_Items`, it ends up in an endless recursion till it finally crashes.
by caching the total minimum log severity of all loggers in a
"global variable" and whether a message's severity is large enough for any of
the loggers in a per-message no-op flag.
1. Don't set a custom handler for SIGCHLD (in the umbrella process)
as that handler doesn't actually handle SIGCHLD anymore
2. Don't reset the SIGCHLD handler (in the worker process)
as there's nothing to reset anymore due to the above change
3. Don't block SIGCHLD across fork(2) as its handler doesn't change anymore
due to the above changes
Before:
On SIGCHLD from the forked worker the umbrella process sets a failure flag.
StartUnixWorker() recognises that and does waitpid(), failure message, etc..
On OpenBSD we can't tell the signal source, so we always set the failure flag.
That's not how our IPC shall work, that breaks the IPC sooner or later.
After:
No SIGCHLD handling and no failure flag setting.
Instead StartUnixWorker()'s wait loop uses waitpid(x,y,WNOHANG)
to avoid false positives while watching the forked worker.
This ensures that `frame.Depth` is only decreased when preceding `frame.IncreaseStackDepth()` callee was successful.
This way, `frame.Depth` will have the same depth prior to and after evaluating a frame.
Before (time: vertical, stack: horizontal):
* Checkable::ExecuteCheck
* Checkable::UpdateNextCheck
* IcingaDB::NextCheckChangedHandler
* HSET icinga:host:state
* HSET icinga:checksum:host:state
* ZADD icinga:nextupdate:host
* RandomCheckTask::ScriptFunc
* Checkable::ProcessCheckResult
* Checkable::UpdateNextCheck
* IcingaDB::NextCheckChangedHandler
* HSET icinga:host:state
* HSET icinga:checksum:host:state
* ZADD icinga:nextupdate:host
* IcingaDB::NewCheckResultHandler
* HSET icinga:host:state
* HSET icinga:checksum:host:state
* ZADD icinga:nextupdate:host
* IcingaDB::StateChangeHandler
* XADD icinga:runtime:state
* IcingaDB::ForwardHistoryEntries
* XADD icinga:history:stream:state
After:
* Checkable::ExecuteCheck
* Checkable::UpdateNextCheck
* RandomCheckTask::ScriptFunc
* Checkable::ProcessCheckResult
* Checkable::UpdateNextCheck
* IcingaDB::NewCheckResultHandler
* HSET icinga:host:state
* HSET icinga:checksum:host:state
* ZADD icinga:nextupdate:host
* IcingaDB::StateChangeHandler
* XADD icinga:runtime:state
* IcingaDB::ForwardHistoryEntries
* XADD icinga:history:stream:state
The first state + nextupdate (for overdue) update comes from next_check being
set to now + interval immediately before doing the actual check (not to trigger
it twice). This update is not only not important for the end user, but even
inappropriate. The end user SHALL see next_check being e.g. in -4s, not 5m, as
the check is running at the moment.
The second one is just redundant as IcingaDB::NewCheckResultHandler (the third
one) is called anyway and will update state + nextupdate as well.
Case:
1. icinga2 api setup
2. icinga2 daemon -C -x debug
Before: Second commands crashes at exit.
After: No crash.
As the comment between the removed lines clearly says:
Our destructors haven't been built for static data.
This is build type independent.
The point of logging to the Windows Event Log was to catch errors that happen
before the full logging configuration has been loaded and enabled. Messages
like the number of loaded objects per type just cause noise in the log and
provide little benefit. Therefore raise the required log level at this stage.
Note that this commit removes the (never documented) ability to use the -x flag
to change the level. But doing so would require patching the command line of
the service in the registry anyways.
If some kind of query is not supposed to be processed at the moment, there is
little point in checking it. During a full dump, state updates are suppressed
(i.e. delayed), so when a dump takes very long, this would have resulted in a
false Redis backlog warning.
The check makes no attempt to explicitly connect to Redis, it uses the
connection of the IcingaDB feature, so this message better describes the state
in this situation.
IcingaDB::GetConnection() uses IcingaDB::m_Rcon which is only initialized in
IcingaDB::Start(), therefore add a nullptr check to the check command.
Additionally, as m_Rcon is potentially accessed concurrently, add a copy of the
value that is safe for concurrent use.
icingadb-web shows multiple lines from the check output collapsed into a single
line. The lines containing just minuses make this look cluttered and making
making it a heading provides little to no benefit. Even when rendering markdown
in the check output at some point, having the lists labeled using normal
paragraphs would look just fine.
- Add icinga2_ and icingadb_ prefixes to make clear which component is
responsible for the value.
- Rename heartbeat_lag to heartbeat_age, describes it better in my opinion and
sound a bit less like something that should be as close to zero as possible.
- Rename redis_dump/database_sync into full_dump/full_sync as this is how these
operations are refered to in log messages as well.
- Rename Redis backlog into Redis query backlog, makes it a bit clearer in my
opinion.
- Rename runtime_backlog into runtime_update_backlog, as the component in
Icinga DB is called that way and this naming is also exposed in log messages.
- Rename dump_config/state/history into config/state/history_dump, makes it
sound more natural.
Alexander A. Klimov
Julian Brost
Yonas Habteab
Michael Friedrich