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

Separately handle apply rules targetting only specific parent objects #9545

Merged
merged 9 commits into from
Nov 4, 2022

Conversation

Al2Klimov
Copy link
Member

not to unnecessarily run e.g. the filter assign where host.name=="example.com" for all hosts being not example.com.

ref/IP/42584
(PR 3/3)

Blocked by

@Al2Klimov Al2Klimov self-assigned this Oct 18, 2022
@cla-bot cla-bot bot added the cla/signed label Oct 18, 2022
@icinga-probot icinga-probot bot added area/configuration DSL, parser, compiler, error handling core/quality Improve code, libraries, algorithms, inline docs labels Oct 18, 2022
@Al2Klimov
Copy link
Member Author

Before/After time icinga2 daemon -C

real 51m52.482s
user 231m26.251s
sys 114m36.421s

real 33m59.397s
user 182m42.875s
sys 53m9.410s

😎

@Al2Klimov Al2Klimov marked this pull request as ready for review October 19, 2022 12:38
@Al2Klimov Al2Klimov requested a review from julianbrost October 19, 2022 12:38
@Al2Klimov
Copy link
Member Author

Julian, shall I

  1. don't evaluate the filter for the targeted ones as they're already known or
  2. extend the filter tree scan algo a bit so that it also outsmarts host.name == "x" && vars.but_only_with_y

?

@Al2Klimov Al2Klimov removed their assignment Oct 19, 2022
@Al2Klimov
Copy link
Member Author

I prefer 1.

@Al2Klimov Al2Klimov force-pushed the targeted-apply-rules branch 2 times, most recently from 31b38e5 to 0a97f65 Compare October 21, 2022 09:00
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a list of all kinds of expressions this PR can optimize.

lib/config/applyrule.cpp Outdated Show resolved Hide resolved
@Al2Klimov
Copy link
Member Author

PING #9545 (comment)

@julianbrost
Copy link
Contributor

Julian, shall I

  1. don't evaluate the filter for the targeted ones as they're already known or
  2. extend the filter tree scan algo a bit so that it also outsmarts host.name == "x" && vars.but_only_with_y
    ?

Well if evaluating the filter expression is not needed, why do it.

Something like host.name == "x" && vars.but_only_with_y does not sound like a very common example, if you already know the host, why restrict it further. However, something like apply Notification "..." to Service { assign where service.name == "..." && host.vars.env == "production" } sounds more useful to me.

But to discuss this in detail, this would be helpful:

Please provide a list of all kinds of expressions this PR can optimize.

@Al2Klimov Al2Klimov force-pushed the targeted-apply-rules branch from 0a97f65 to 5076698 Compare October 25, 2022 10:22
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add documentation comments to the other functions in applyrule-targeted.cpp, this should make it way easier to follow that file.

lib/config/applyrule.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the use of inline: I'm not sure why you're adding that keyword explicitly. As the functions are defined inside class definitions, they are inline implicitly (https://en.cppreference.com/w/cpp/language/inline).

lib/config/applyrule.hpp Outdated Show resolved Hide resolved
lib/config/applyrule.cpp Outdated Show resolved Hide resolved
lib/config/applyrule.hpp Outdated Show resolved Hide resolved
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
@Al2Klimov
Copy link
Member Author

IMAO explicit is more expressive than implicit.

lib/config/applyrule.hpp Outdated Show resolved Hide resolved
lib/config/applyrule.hpp Outdated Show resolved Hide resolved
lib/config/applyrule.hpp Outdated Show resolved Hide resolved
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from julianbrost November 2, 2022 11:24
@yhabteab yhabteab force-pushed the targeted-apply-rules branch 2 times, most recently from ddfcf84 to 9557694 Compare November 2, 2022 11:50
@yhabteab yhabteab requested review from julianbrost and removed request for julianbrost November 2, 2022 11:51
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the targeted-apply-rules branch from 9557694 to 9250d88 Compare November 2, 2022 13:47
@yhabteab yhabteab requested a review from julianbrost November 2, 2022 13:49
lib/config/applyrule-targeted.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the targeted-apply-rules branch from 9250d88 to 9066849 Compare November 2, 2022 14:53
@yhabteab yhabteab requested a review from julianbrost November 2, 2022 14:57
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GH Actions want your attention.

@yhabteab yhabteab force-pushed the targeted-apply-rules branch from 9066849 to 74552cc Compare November 2, 2022 15:32
@yhabteab
Copy link
Member

yhabteab commented Nov 2, 2022

The GH Actions want your attention.

Forgot to also commit the changes in the header file 🙈

@yhabteab yhabteab requested a review from julianbrost November 2, 2022 15:35
@yhabteab yhabteab force-pushed the targeted-apply-rules branch 2 times, most recently from 7b956ed to c57c6a9 Compare November 4, 2022 08:32
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the point of your last push, is there anything new that this fixes?

lib/icinga/dependency-apply.cpp Outdated Show resolved Hide resolved
lib/config/applyrule.hpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the targeted-apply-rules branch from c57c6a9 to a8d46e6 Compare November 4, 2022 09:20
@yhabteab yhabteab requested a review from julianbrost November 4, 2022 09:21
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test config that hopefully covers most edge cases
object CheckCommand "dummy" { command = ["true"] }
object NotificationCommand "dummy" { command = ["true"] }
object Host "h" { check_command = "dummy" }
object User "u" {}

// ApplyRule::RegisterType("Service", { "Host" });

apply Service "s" {
    log("applying Service " + name + " to Host " + host.name)
    check_command = "dummy"
    assign where host.name == "h"
    assign where host.name == "h"
}

apply Service "s-warn" {
    log("applying Service " + name + " to Host " + host.name)
    check_command = "dummy"
    assign where host.name == "404"
    assign where host.name == "404"
}

apply Service "s-true" to Host {
    log("applying Service " + name + " to Host " + host.name)
    check_command = "dummy"
    assign where true
}

apply Service "s-false" to Host {
    log("applying Service " + name + " to Host " + host.name)
    check_command = "dummy"
    assign where false
}

// ApplyRule::RegisterType("Dependency", { "Host", "Service" });

apply Dependency "dep-host" to Host {
    log("applying Dependency " + name + " to Host " + host.name)
    parent_host_name = "h"
    assign where host.name == "h"
    assign where host.name == "h"
}

apply Dependency "dep-host-warn" to Host {
    log("applying Dependency " + name + " to Host " + host.name)
    parent_host_name = "h"
    assign where host.name == "404"
    assign where host.name == "404"
}

apply Dependency "dep-host-true" to Host {
    log("applying Dependency " + name + " to Host " + host.name)
    parent_host_name = "h"
    assign where true
}

apply Dependency "dep-host-false" to Host {
    log("applying Dependency " + name + " to Host " + host.name)
    parent_host_name = "h"
    assign where false
}

apply Dependency "dep-service" to Service {
    log("applying Dependency " + name + " to Service " + host.name + "!" + service.name)
    parent_host_name = "h"
    assign where host.name == "h" && service.name == "s"
    assign where host.name == "h" && service.name == "s"
}

apply Dependency "dep-service-warn" to Service {
    log("applying Dependency " + name + " to Service " + host.name + "!" + service.name)
    parent_host_name = "h"
    assign where host.name == "404" && service.name == "s"
    assign where host.name == "404" && service.name == "s"
}

apply Dependency "dep-service-true" to Service {
    log("applying Dependency " + name + " to Service " + host.name + "!" + service.name)
    parent_host_name = "h"
    assign where true
}

apply Dependency "dep-service-false" to Service {
    log("applying Dependency " + name + " to Service " + host.name + "!" + service.name)
    parent_host_name = "h"
    assign where false
}

// ApplyRule::RegisterType("Notification", { "Host", "Service" });

apply Notification "notify-host" to Host {
    log("applying Notification " + name + " to Host " + host.name)
    command = "dummy"
    users = ["u"]
    assign where host.name == "h"
    assign where host.name == "h"
}

apply Notification "notify-host-warn" to Host {
    log("applying Notification " + name + " to Host " + host.name)
    command = "dummy"
    users = ["u"]
    assign where host.name == "404"
    assign where host.name == "404"
}

apply Notification "notify-host-true" to Host {
    log("applying Notification " + name + " to Host " + host.name)
    command = "dummy"
    users = ["u"]
    assign where true
}

apply Notification "notify-host-false" to Host {
    log("applying Notification " + name + " to Host " + host.name)
    command = "dummy"
    users = ["u"]
    assign where false
}

apply Notification "notify-service" to Service {
    log("applying Notification " + name + " to Service " + host.name + "!" + service.name)
    command = "dummy"
    users = ["u"]
    assign where host.name == "h" && service.name == "s"
    assign where host.name == "h" && service.name == "s"
}

apply Notification "notify-service-warn" to Service {
    log("applying Notification " + name + " to Service " + host.name + "!" + service.name)
    command = "dummy"
    users = ["u"]
    assign where host.name == "404" && service.name == "s"
    assign where host.name == "404" && service.name == "s"
}

apply Notification "notify-service-true" to Service {
    log("applying Notification " + name + " to Service " + host.name + "!" + service.name)
    command = "dummy"
    users = ["u"]
    assign where true
}

apply Notification "notify-service-false" to Service {
    log("applying Notification " + name + " to Service " + host.name + "!" + service.name)
    command = "dummy"
    users = ["u"]
    assign where false
}

// ApplyRule::RegisterType("ScheduledDowntime", { "Host", "Service" });

apply ScheduledDowntime "dt-host" to Host {
    log("applying ScheduledDowntime " + name + " to Host " + host.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where host.name == "h"
    assign where host.name == "h"
}

apply ScheduledDowntime "dt-host-warn" to Host {
    log("applying ScheduledDowntime " + name + " to Host " + host.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where host.name == "404"
    assign where host.name == "404"
}

apply ScheduledDowntime "dt-host-true" to Host {
    log("applying ScheduledDowntime " + name + " to Host " + host.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where true
}

apply ScheduledDowntime "dt-host-false" to Host {
    log("applying ScheduledDowntime " + name + " to Host " + host.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where false
}

apply ScheduledDowntime "dt-service" to Service {
    log("applying ScheduledDowntime " + name + " to Service " + host.name + "!" + service.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where host.name == "h" && service.name == "s"
    assign where host.name == "h" && service.name == "s"
}

apply ScheduledDowntime "dt-service-warn" to Service {
    log("applying ScheduledDowntime " + name + " to Service " + host.name + "!" + service.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where host.name == "404" && service.name == "s"
    assign where host.name == "404" && service.name == "s"
}

apply ScheduledDowntime "dt-service-true" to Service {
    log("applying ScheduledDowntime " + name + " to Service " + host.name + "!" + service.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where true
}

apply ScheduledDowntime "dt-service-false" to Service {
    log("applying ScheduledDowntime " + name + " to Service " + host.name + "!" + service.name)
    author = "dummy"
    comment = "dummy"
    ranges = {}
    assign where false
}

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 backported Fix was included in a bugfix release cla/signed core/quality Improve code, libraries, algorithms, inline docs ref/IP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants