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

Assign where filter rendered as match("a*",x)||match("b*",x)||match("c*",x)||... #2661

Closed
Al2Klimov opened this issue Nov 16, 2022 · 14 comments · Fixed by Icinga/icinga2#9585

Comments

@Al2Klimov
Copy link
Member

Expected Behavior

AssignRenderer, which btw. already renders different from the user input (x = a* becomes match("a*",x), but x = a becomes x == "a") for the sake of performance, should also recognise an OR tree of =s targeting the same property and render it as, IIRC, regex("(?i)^(?:a|b|c|...)",x).

Current Behavior

If you input a "monster", Director outputs a monster: match("a*",x)||match("b*",x)||match("c*",x)||... (MIMO)

Possible Solution

Maybe only for a tree with size 10+ and/or maybe with a UI swich.

If you allow me, I can implement it.

Steps to Reproduce (for bugs)

Bildschirmfoto 2022-11-16 um 11 29 00

Bildschirmfoto 2022-11-16 um 11 30 29

Your Environment

https://github.com/lippserd/docker-compose-icinga/releases/tag/v1.2.0

@Al2Klimov
Copy link
Member Author

@Thomas-Gelf Would you consider it less less readable if the renderer adds a comment with the full match() OR tree to the rendered regex()?

// assign where match("a*",x)||match("b*",x)||match("c*",x)||...
assign where regex("(?i)^(?:a|b|c|...)",x)

@Thomas-Gelf
Copy link
Contributor

We talked about this, I have no plans to implement it without numbers proving that it is worth doing so. And even then, this is confusing and renders the generated configuration less readable. Also, in the real world you'll rarely ever match rules structured exactly like this. Who builds inefficient rules, mostly combines them with a couple of other conditions, flags and filters.

As far as I can recall our discussion, my final conclusion was that if this is really worth the effort, such an optimization can perfectly be implemented as an internal magic feature in the Icinga Config parser - it does not belong into the Director.

As an alternative, we can always opt for no longer rendering Apply Rules at all. We could create cached/materialized flat objects in the Director database and deploy completely flat configurations. In case you consider this being necessary, please let me know: time for the required work needs to be blocked.

@Thomas-Gelf Thomas-Gelf closed this as not planned Won't fix, can't repro, duplicate, stale Feb 1, 2023
@Al2Klimov
Copy link
Member Author

"necessary" is relative. With only flat objects the config load would be faster than light compared to the status quo. However apply rules may be designed to also target API created parent objects at runtime. Flat-only would break this silently.

@Thomas-Gelf
Copy link
Contributor

Honestly I didn't realize, that Icinga is now able to deal with Apply Rules combined with objects created/modified at runtime. This would be great news, since which version does this work as expected?

Some notes related to the "flat config" feature

When switching to flat configuration in the Director, you can push your API changes there. We already started to also support tricky tasks, like automagic variable override handling for (virtual) single services via API and CLI, even if the Service is based on an Apply Rule.

In that case, I'd strongly suggest to re-evaluate #2342. The PR is old, and must be renewed to fit current realities (e.g. UUIDs). But as far as I have been told, related fixes (restore modified attributes, various attempts like #8739) and feature requests (allow to deploy configuration packages w/o restart, package parameter for API requests #8640) have been rejected.

Practically, for Director-only-setups, this would mean not using the Apply Rule feature in the Core at all, pre-calculating every object in the DB, and shipping every (main config branch) change immediately to the core. A full config reload would be scheduled only once a day or so, just to get rid of all the single config modification files, that accumulated in _api during the day.

No time wasted for deployments throughout the day, this would IMHO be a fantastic feature.

@Al2Klimov
Copy link
Member Author

since which version does this work as expected?

Not sure about the exact version or work as expected (older version = more bugs). What I can say for sure:

root@aklimov-appily:~# curl -k -s -u  root:123456 -H 'Accept: application/json' -X PUT -d '{ "attrs": { "address": "192.168.1.1", "check_command" : "ping", "enable_active_checks": false }, "pretty": true }' https://localhost:5665/v1/objects/hosts/a
{"results":[{"code":200.0,"status":"Object was created"}]}
root@aklimov-appily:~# curl -k -s -u root:123456 https://localhost:5665/v1/objects/services\?attrs=__name
{"results":[{"attrs":{"__name":"a!lolcat"},"joins":{},"meta":{},"name":"a!lolcat","type":"Service"}]}
root@aklimov-appily:~# curl -k -s -u root:123456 https://localhost:5665/v1
<html><head><title>Icinga 2</title></head><h1>Hello from Icinga 2 (Version: r2.6.3-1)!</h1><p>You are authenticated as <b>root</b>. Your user has the following permissions:</p> <ul><li>*</li></ul><p>More information about API requests is available in the <a href="http://docs.icinga.com/icinga2/latest" target="_blank">documentation</a>.</p></html>

@Al2Klimov
Copy link
Member Author

With only flat objects the config load would be faster than light compared to the status quo.

To be precise:

Look at these numbers and then read on: Icinga/icinga2#9627 (review)

The same config takes only the following if processed with this tool i.e. flatted: https://github.com/Al2Klimov/icinga2.debug-to-config

real 5m30,482s
user 31m15,310s
sys 4m12,789s

For me enough reason to write flat-only, but there are still apply rules targeting API objects 🤷‍♂️

@Thomas-Gelf
Copy link
Contributor

Not sure about the exact version or work as expected ....

Expected -> Apply rule reflects current reality. Service based on host.vars.environment, Notification based on host.vars.send_notifictions, host properties changed via API at runtime.

@Al2Klimov
Copy link
Member Author

I'd say v2.6.3, maybe even since older ones.

@Thomas-Gelf
Copy link
Contributor

I'd say v2.6.3, maybe even since older ones.

So 2.6.3 removes generated Services, if the Apply Rule no longer matches a Host object modified via API, and re-creates a Service when the Host reverts?

@Thomas-Gelf
Copy link
Contributor

...faster than light...

VS

real 5m30,482s user 31m15,310s sys 4m12,789s

Light used to be faster in the good old times 🤣

Look at these numbers...

I had a quick look at the sample config. If I got it right, this produces 1000 Hosts, and a bunch of Service and Notification Apply Rules. How many real Objects have been generated in those benchmark with your "rng" seed?

For me enough reason to write flat-only, but there are still apply rules targeting API objects man_shrugging

I was asking about the Apply Rule / API functionality for this reason: I expected it to be not so feature-rich, therefore such a change could be sold as an improvement / a new feature - combined with a flag allowing one, to keep the former behavior.

@Al2Klimov
Copy link
Member Author

Light used to be faster in the good old times 🤣

Nowadays it slowed down not to escalate the situation. 😉

I'd say v2.6.3, maybe even since older ones.

So 2.6.3 removes generated Services, if the Apply Rule no longer matches a Host object modified via API, and re-creates a Service when the Host reverts?

No. And v2.13.6 neither does.

I had a quick look at the sample config. If I got it right, this produces 1000 Hosts, and a bunch of Service and Notification Apply Rules. How many real Objects have been generated in those benchmark with your "rng" seed?

Which sample config? Which "rng" seed? What are you talking about? Lookup who's the customer No. 22470 in our ERP system. This should give you an idea of the size of this real-world config. Or – if you prefer numbers – read Icinga/icinga2#9627 (review) . And now if I flatten this prod. config via https://github.com/Al2Klimov/icinga2.debug-to-config I get 5.5m. Admittedly with v2.13.6.

such a change could be sold as an improvement / a new feature - combined with a flag allowing one, to keep the former behavior.

Good idea! At best overridable at rule level.

@Al2Klimov
Copy link
Member Author

⚠️ Trigger alert! Insane idea. ⚠️

💡 You could equip all Director objects w/ a customvar created_via_director=true and render all assign conditions X as !host.vars.created_via_director && (X) and the flat child objects. Icinga DSL && is short-circuit.

@Al2Klimov
Copy link
Member Author

And now if I flatten this prod. config via https://github.com/Al2Klimov/icinga2.debug-to-config I get 5.5m. Admittedly with v2.13.6.

With master:

real 5m22,748s
user 47m21,239s
sys 4m45,347s

@Thomas-Gelf
Copy link
Contributor

No. And v2.13.6 neither does.

Then I wouldn't worry too much about users using apply rules for objects created via API. I know there are such, but if the feature they're using right now works with such restrictions, they wouldn't bother if they get something better I guess.

Which sample config?

The one linked in the issue you posted, in the initial benchmark description.

...if you prefer numbers...

I do, but the linked comment seems to contain neither details about the deployed configuration (if it wasn't the one linked in the issue), nor regarding the amount of deployed objects. All I see are (good looking) before/after numbers, but w/o context.

Good idea! At best overridable at rule level.

All or nothing would be my preferred approach, just to keep things simple.

...equip all Director objects w/ a customvar created_via_director=true and render all assign conditions X as !host.vars.created_via_director && (X) and the flat child objects...

And Icinga then still needs to iterate every object (and it's ancestors?) for every apply rule, with 99% unmatched objects? Doesn't convince me ;-)

We should stop here I guess, we're far off-topic. You could ask your product lead, whether he would welcome such a feature. And please also consider what I wrote in the first related comment: I'd suggest going this way only combined with a deployment mechanism not involving a restart at every change. 5 Minute sounds great when compared to 50 minutes, but is still insanely long for "I changed the IP address for a specific Host".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants