Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix config sync after freezing namespaces #9648

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

julianbrost
Copy link
Contributor

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.

The PR also includes an additional commit that improves the error handling for --define so that a proper error message is shown when a user tries to update something in a read-only namespace rather than resulting in a crash with a crash log.

Test

Updating the config on master-1, result of the incoming config sync on master-2.

Before (a24375b)

Config sync fails with an error, regardless of whether the config is valid:

[2023-02-01 12:48:11 +0100] information/ApiListener: Received configuration updates (5) from endpoint 'master-1' are different to production, triggering validation and reload.
[2023-02-01 12:48:11 +0100] warning/Process: PID 410 was terminated by signal 6 (Aborted)
[2023-02-01 12:48:11 +0100] critical/ApiListener: Config validation failed for staged cluster config sync in '/var/lib/icinga2/api/zones-stage/'. Aborting. Logs: '/var/lib/icinga2/api//zones-stage-startup-last-failed.log'

Contents of that log file:

[2023-02-01 12:48:11 +0100] critical/Application: Error: Namespace is read-only and must not be modified.


Additional information is available in '/var/log/icinga2/crash/report.1675252091.567308'
<Terminated by signal 6 (Aborted).>

After (fd1aa73)

Same setting, config sync works again:

[2023-02-01 12:58:51 +0100] information/ApiListener: Received configuration updates (5) from endpoint 'master-1' are different to production, triggering validation and reload.
[...]
[2023-02-01 12:58:58 +0100] information/ApiListener: Config validation for stage '/var/lib/icinga2/api/zones-stage/' was OK, replacing into '/var/lib/icinga2/api/zones/' and triggering reload.
[...]
[2023-02-01 12:59:00 +0100] information/Application: Got reload command: Starting new instance.
[2023-02-01 12:59:00 +0100] information/cli: Loading configuration file(s).

fixes #9639

This can be observed when running something like `icinga2 daemon -C -DTypes.Host=42`.

Without this commit:

    [2023-02-01 09:29:00 +0100] critical/Application: Error: Namespace is read-only and must not be modified.

    Additional information is available in '/var/log/icinga2/crash/report.1675240140.425155'

With this commit:

    [2023-02-01 09:30:37 +0100] critical/icinga-app: cannot set 'Types.Host': Namespace is read-only and must not be modified.
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.
@icinga-probot icinga-probot bot added area/configuration DSL, parser, compiler, error handling blocker Blocks a release or needs immediate attention labels Feb 1, 2023
@cla-bot cla-bot bot added the cla/signed label Feb 1, 2023
@julianbrost julianbrost added the area/distributed Distributed monitoring (master, satellites, clients) label Feb 1, 2023
@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Feb 1, 2023
@Al2Klimov Al2Klimov merged commit 4e021e0 into master Feb 1, 2023
@icinga-probot icinga-probot bot deleted the frozen-namespace-config-validation branch February 1, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration DSL, parser, compiler, error handling area/distributed Distributed monitoring (master, satellites, clients) blocker Blocks a release or needs immediate attention cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cluster config file sync broken in master
2 participants