Icinga2: Duplicate/useless local command execution for remote command?

[Thomas-Gelf]

Please note that I neither faced a problem nor did I test this, but this line seems to be superfluous:

https://github.com/Icinga/icinga2/blob/43f7d35/lib/icinga/checkable-check.cpp#L533

IMHO this is correct for local, but not for a remote check.

[Al2Klimov]

Hello @Thomas-Gelf and thank you for the good catch!

However you didn't face any problem for a reason – there is actually no problem at all.

The effective call stack looks like this:

• CheckCommand#Execute(_, _, {}, false)
• PluginCheckTask#ScriptFunc(_, _, {}, false)
• PluginUtility#ExecuteCommand(_, _, _, _, {}, false, _, _)
• if ({} /* true */ && !false) return;

This does everything up to and not including the actual process spawn for having everything resolved to give it to the agent.

Best,
AK

[Thomas-Gelf]

@Al2Klimov: thanks a lot! As I created the issue for you, and at first sight it seemed strange to you too: what do you think, would it be a good idea to shift this "having everything resolved" to a dedicated method with a more explicit name?

[dnsmichi]

This line is used to pre-calculate all required macro values with later sending them inside the cluster message. Rewriting the code parts for the command_endpoint is very fragile, currently I would opt against such attempts to avoid breaking things we did not expect to break.

[Al2Klimov]

I absolutely agree with you, @Thomas-Gelf, but IMO not touching a running system (as @dnsmichi said) is more important.

[Thomas-Gelf]

Completely understandable, that's up to you. And there are enough other tasks for now :-)

Eventually you can tackle this one far day when cleaning up/refactoring parts of the code. I personally consider methods triggering unexpected side-effect dangerous, sooner or later they'll bite you. Such constructs lead to a code base where you fear touching things because you never know what's gonna break next. This is unnecessarily slowing you down.

So, I'm a strong advocate of splitting logic into smaller, predictable components and more explicit related method calls. When I call Execute(), I expect something to be executed. If I want a dictionary to be filled with resolved Macros, I'd prefer to call ResolveMacros(). Calling Execute() just to get a macro dictionary filled for later use will lead to unexpected side-effects once someone places new autonomous logic into Execute.

Just my two cents.