icinga2 daemon: write icinga2.debug only if --dump-objects given #9586

Al2Klimov · 2022-11-21T11:39:17Z

to save config (re)load time.

Al2Klimov · 2022-11-21T11:40:16Z

➜  icinga2 git:(9363) ✗ prefix/sbin/icinga2 daemon -C
[2022-11-21 12:29:24 +0100] information/cli: Icinga application loader (version: v2.13.0-480-ga6b05059a; debug)
[2022-11-21 12:29:24 +0100] information/cli: Loading configuration file(s).
[2022-11-21 12:29:24 +0100] information/ConfigItem: Committing config item(s).
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 244 CheckCommands.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 FileLogger.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 Endpoint.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 CheckerComponent.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 Host.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 2 HostGroups.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 IcingaApplication.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 User.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 2 NotificationCommands.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 3 Zones.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 3 ServiceGroups.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 3 TimePeriods.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 UserGroup.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 NotificationComponent.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 12 Notifications.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 1 ScheduledDowntime.
[2022-11-21 12:29:24 +0100] information/ConfigItem: Instantiated 11 Services.
[2022-11-21 12:29:24 +0100] information/ScriptGlobal: Dumping variables to file '/Users/aklimov/NET/WS/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2022-11-21 12:29:24 +0100] information/cli: Finished validating the configuration file(s).
➜  icinga2 git:(9363) ✗ ls -l prefix/var/cache/icinga2
total 8
-rw-------  1 aklimov  staff  855 21 Nov 12:29 icinga2.vars
➜  icinga2 git:(9363) ✗

Al2Klimov · 2022-11-21T11:40:47Z

➜  icinga2 git:(9363) ✗ prefix/sbin/icinga2 daemon -C --dump-objects
[2022-11-21 12:33:44 +0100] information/cli: Icinga application loader (version: v2.13.0-480-ga6b05059a; debug)
[2022-11-21 12:33:44 +0100] information/cli: Loading configuration file(s).
[2022-11-21 12:33:45 +0100] information/ConfigItem: Committing config item(s).
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 244 CheckCommands.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 FileLogger.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 Endpoint.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 CheckerComponent.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 Host.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 2 HostGroups.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 IcingaApplication.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 User.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 2 NotificationCommands.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 3 Zones.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 3 ServiceGroups.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 3 TimePeriods.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 UserGroup.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 NotificationComponent.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 12 Notifications.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 1 ScheduledDowntime.
[2022-11-21 12:33:45 +0100] information/ConfigItem: Instantiated 11 Services.
[2022-11-21 12:33:45 +0100] information/ScriptGlobal: Dumping variables to file '/Users/aklimov/NET/WS/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2022-11-21 12:33:45 +0100] information/cli: Finished validating the configuration file(s).
➜  icinga2 git:(9363) ✗ ls -l prefix/var/cache/icinga2
total 2056
-rw-------  1 aklimov  staff  1045698 21 Nov 12:33 icinga2.debug
-rw-------  1 aklimov  staff      855 21 Nov 12:33 icinga2.vars
➜  icinga2 git:(9363) ✗

julianbrost · 2022-11-21T12:11:07Z

I doubt that there's really no other place in the documentation that would benefit from mentioning the new command explicitly. So if the documentation suggests to use icinga2 object list, it should now also suggest to do a icinga2 daemon -C --dump-objects first.

Additionally, it would be great if icinga2 object list could warn if it's using outdated information.

julianbrost

Now the following data structure is generated for nothing unless started with --dump-objects, you might want to optimize this:

icinga2/lib/config/configitem.cpp

Lines 288 to 300 in 0c67abe

    
           Dictionary::Ptr persistentItem = new Dictionary({ 
        
           	{ "type", type->GetName() }, 
        
           	{ "name", GetName() }, 
        
           	{ "properties", serializedObject }, 
        
           	{ "debug_hints", dhint }, 
        
           	{ "debug_info", new Array({ 
        
           		m_DebugInfo.Path, 
        
           		m_DebugInfo.FirstLine, 
        
           		m_DebugInfo.FirstColumn, 
        
           		m_DebugInfo.LastLine, 
        
           		m_DebugInfo.LastColumn, 
        
           	}) } 
        
           });

lib/cli/objectlistcommand.cpp

Al2Klimov · 2022-11-22T11:35:49Z

Shall I also optimise Serialize()?

julianbrost · 2022-11-22T11:45:07Z

Shall I also optimise Serialize()?

That will be the next step in this area, so ahead, will just be fun keeping the side effect of checking for circular references without making the code messy.

Al2Klimov · 2022-11-22T11:48:33Z

So we shall (at least try to) preserve circular ref checks?

IMAO useless as you can make ones at runtime.

julianbrost · 2022-11-22T11:57:38Z

You have to do something about it so that it behaves consistently.

Having configs that are valid if and only if you don't specify --dump-objects would be bad. This would reduce object list in usefulness for debugging as there would be valid configs you can't debug with it.

But I would be open to gracefully handling cyclic references in the debug file. Python for example does this:

>>> x = [23, 42]
>>> x.append(x)
>>> x
[23, 42, [...]]

Al2Klimov · 2022-11-22T12:02:53Z

This would reduce object list in usefulness for debugging as there would be valid configs you can't debug with it.

Weeeell... you'd "just" have to remove the circular refs to start with debugging. You probably won't even need an actual Icinga 2 reload.

julianbrost · 2022-11-22T12:21:47Z

Well, you already put "just" into quotation marks yourself. This means that it could happen that for a support ticket where you ask for the output of icinga2 object list, you could get the response that it shows an error and would then have to instruct them to change something completely unrelated in the config so that this can continue. I don't think that's a very good option.

Al2Klimov · 2022-11-22T12:24:15Z

OK. Shall I allow Serialize() to optionally handle circular refs like this?

But I would be open to gracefully handling cyclic references in the debug file. Python for example does this:

julianbrost · 2022-11-22T13:29:27Z

Please first share how you want to do this. Serialize() generates Array/Dictionary objects that are then serialized into JSON, so you can't have a magic value for "this was a circular reference".

Also, I feel like this might be a good cutoff point for this PR: put the initialization of persistentItem into an if in this PR and keep Serialize() as-is. Then this PR will do exactly as advertised: changes when icinga2.debug is written without other changes in behavior. This should be done rather quickly. And then later (after having agreed on a concept, see below), a second PR for potential changes to Serialize() to handle this.

IMAO useless as you can make ones at runtime.

Do you have an example for creating a circular reference in a place where it would be rejected if you write it into the config?

On second thought, allowing this (circular references inside of attributes of an object) might be problematic: API responses also use Serialize(), so you might run into this issue there (that's why I'm asking if you know an example for creating this as runtime).

On third thought: At the moment, Deserialize(Serialize(x)) == x doesn't hold (at least if function objects are involved) abd allowing this would put us even further apart from this (IMO desirable) property.

So after thinking about this in more detail, I'm not sure about this. If we'd want to do this, there should be a good concept that takes all of this into account.

Al2Klimov · 2022-11-22T14:03:59Z

Would you prefer a no-op flag, so that it effectively just checks circular refs on demand?

julianbrost · 2022-11-22T15:11:24Z

You mean for the serialization? Do you have more alternatives in mind than rewriting a separate function? I don't think I'm perfectly happy with either, but everything else that comes to my mind feels like it would end in total over-engineering.

But please only make it a flag for SerializeInternal() and not Serialize() and add a second function for that so that the interface at least looks nice on the outside.

lib/config/configitem.cpp

lib/cli/objectlistcommand.cpp

to save config (re)load time.

julianbrost

Just that small grammar error left: #9586 (comment)

Apart from that, should be good to go.

cla-bot bot added the cla/signed label Nov 21, 2022

icinga-probot bot added area/configuration DSL, parser, compiler, error handling enhancement New feature or request labels Nov 21, 2022

Al2Klimov force-pushed the 9363 branch from 59f4961 to 0c67abe Compare November 21, 2022 13:57

Al2Klimov requested a review from julianbrost November 21, 2022 13:57

julianbrost reviewed Nov 22, 2022

View reviewed changes

lib/cli/objectlistcommand.cpp Outdated Show resolved Hide resolved

lib/cli/objectlistcommand.cpp Outdated Show resolved Hide resolved

lib/cli/objectlistcommand.cpp Show resolved Hide resolved

Al2Klimov force-pushed the 9363 branch from 0c67abe to 7dd8561 Compare November 22, 2022 14:02

Al2Klimov requested a review from julianbrost November 22, 2022 15:14

Al2Klimov mentioned this pull request Nov 22, 2022

icinga2 daemon: w/o --dump-objects just check for circular refs #9591

Merged

julianbrost reviewed Nov 23, 2022

View reviewed changes

lib/config/configitem.cpp Outdated Show resolved Hide resolved

lib/cli/objectlistcommand.cpp Show resolved Hide resolved

lib/cli/objectlistcommand.cpp Outdated Show resolved Hide resolved

Al2Klimov force-pushed the 9363 branch from 7dd8561 to 7df8736 Compare November 23, 2022 10:12

julianbrost reviewed Nov 23, 2022

View reviewed changes

lib/cli/objectlistcommand.cpp Outdated Show resolved Hide resolved

icinga2 daemon -C: write icinga2.debug only if --dump-objects given

0767c6e

to save config (re)load time.

Al2Klimov force-pushed the 9363 branch from 7df8736 to d9aca48 Compare November 23, 2022 11:55

julianbrost requested changes Nov 24, 2022

View reviewed changes

Al2Klimov force-pushed the 9363 branch from d9aca48 to ca8c5e6 Compare November 24, 2022 09:48

icinga2 object list: warn on possibly outdated config

f71612d

Al2Klimov force-pushed the 9363 branch from ca8c5e6 to f71612d Compare November 24, 2022 09:50

julianbrost approved these changes Nov 24, 2022

View reviewed changes

julianbrost enabled auto-merge November 24, 2022 10:23

julianbrost merged commit ae32b3c into master Nov 24, 2022

icinga-probot bot deleted the 9363 branch November 24, 2022 15:04

julianbrost added this to the 2.14.0 milestone Nov 25, 2022

tbauriedel mentioned this pull request Jul 13, 2023

Build icinga2.debug before collecting object list NETWAYS/support-collector#93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

icinga2 daemon: write icinga2.debug only if --dump-objects given #9586

icinga2 daemon: write icinga2.debug only if --dump-objects given #9586

Al2Klimov commented Nov 21, 2022

Al2Klimov commented Nov 21, 2022

Al2Klimov commented Nov 21, 2022

julianbrost commented Nov 21, 2022

julianbrost left a comment

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

julianbrost left a comment

	Dictionary::Ptr persistentItem = new Dictionary({
	{ "type", type->GetName() },
	{ "name", GetName() },
	{ "properties", serializedObject },
	{ "debug_hints", dhint },
	{ "debug_info", new Array({
	m_DebugInfo.Path,
	m_DebugInfo.FirstLine,
	m_DebugInfo.FirstColumn,
	m_DebugInfo.LastLine,
	m_DebugInfo.LastColumn,
	}) }
	});

icinga2 daemon: write icinga2.debug only if --dump-objects given #9586

icinga2 daemon: write icinga2.debug only if --dump-objects given #9586

Conversation

Al2Klimov commented Nov 21, 2022

Al2Klimov commented Nov 21, 2022

Al2Klimov commented Nov 21, 2022

julianbrost commented Nov 21, 2022

julianbrost left a comment

Choose a reason for hiding this comment

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

Al2Klimov commented Nov 22, 2022

julianbrost commented Nov 22, 2022

julianbrost left a comment

Choose a reason for hiding this comment