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

Service#GetSeverity(): behave as the respective IDO query of Icinga Web #9196

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Jan 27, 2022

which doesn't include host reachability.

W/o this #9154 would still miss severity updates as it doesn't take host reachability into account.

@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Jan 27, 2022
@Al2Klimov Al2Klimov requested a review from julianbrost January 27, 2022 11:21
@cla-bot cla-bot bot added the cla/signed label Jan 27, 2022
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.

In terms of adjusting behavior to the corresponding query in the IDO monitoring module: only difference left seems to be the cases where it adds 1/2/4. This is the case when the service is OK but acknowledged (Is that even possible?) or in a downtime (Makes a small difference whether OK services are sorted by downtime state or not. Neither the monitoring nor the icingadb module seem to render OK services any different based on whether they are in a downtime or not).

History lesson:

Service::GetSeverity() is only used by the Icinga DB code at the moment. However, it's also exposed through the API and in the DSL, so the change is visible to the outside world. This also somewhat touches #9130, so I think we should come to a conclusion what we want to do there before doing this change here. Currently it seems like the existing severity value changes into some kind of opaque value that should be documented something like "the internal structure of this value may change, it should only be used for comparisons/sorting".

In general, I'm fine with this change if this means that we'll use and document the severity as suggested. I think this is the only sane option for a value that's intended to be used as a sort order, as otherwise, this might prevent us from tweaking the sort order without either breaking compatibility again or adding a new value that works as described. For #9130, we can additionally discuss if we want an additional bitset combining all state flags with a defined internal structure.

@lippserd Please approve the change considering this summary.

@Al2Klimov Please add some reference to the reachability/severity PR to the description and how this is done due to that PR.

@julianbrost julianbrost requested a review from lippserd January 27, 2022 13:12
@lippserd
Copy link
Member

we'll use and document the severity as suggested. I think this is the only sane option for a value that's intended to be used as a sort order, as otherwise, this might prevent us from tweaking the sort order without either breaking compatibility again or adding a new value that works as described. For #9130, we can additionally discuss if we want an additional bitset combining all state flags with a defined internal structure.

That sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/icingadb New backend backported Fix was included in a bugfix release cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants