Icinga DB `state_type` and `is_acknowledged` keys in redis don't match with what's inserted into the DB

[nilmerg]

Describe the bug

state_type in redis is a integer instead of 'soft' or 'hard'
is_acknowledged is a integer instead of 'y', 'n' or 'sticky'

There may be more cases, didn't check further.

Your Environment

Include as many relevant details about the environment you experienced the problem in

• Version used (icinga2 --version): v2.13.0-369-g73d3323b4

[lippserd]

There is a workaround in place at the moment so it’s NOT a production issue.

[julianbrost]

This would be an incompatible change to the Redis schema, so what are the options here?

1. Wait until we want to do another incompatible change and then include it as well.
2. Include patches in the next releases of icingadb and icingadb-web so that they can handle both integer and string values for these keys but keep icinga2 as-is for now and only change it after it's pretty safe to assume that most users will be on versions that support reading the strings from Redis.

[nilmerg]

in_downtime is affected as well, it's a boolean, instead of 'y' or 'n'

[julianbrost]

Is that unexpected? That enum('n', 'y') thing in the database is supposed to represent a boolean after all. You shouldn't find "n" or "y" anywhere in Redis (well, expect if some string attribute is set to that value of course).

[nilmerg]

The problem with this is, if we apply redis results to database results, it performs an unexpected transformation. In Web's view, the types returned by either source should be identical.

[yhabteab]

1. Wait until we want to do another incompatible change and then include it as well.

Now might be the time to introduce such an incompatible change (the Redis schema version is going to change with #10290 anyway).

[julianbrost]

For the actual enum types (i.e. those that are not fake booleans) mentioned in the issue, so far, the underlying raw enum values are used in the JSON:

icinga2/lib/icinga/checkresult.ti

Lines 40 to 44 in a53792d

	enum StateType
	{
	StateTypeSoft = 0,
	StateTypeHard = 1
	};

icinga2/lib/icinga/checkable.ti

Lines 20 to 25 in a53792d

	enum AcknowledgementType
	{
	AcknowledgementNone = 0,
	AcknowledgementNormal = 1,
	AcknowledgementSticky = 2
	};

There's quite a bunch more of enums in the schema (search for enum and ignore enum('n', 'y')). So more than the ones mentioned in the issue might be affected and would need adjustments.

@nilmerg Regarding fake booleans, I need some help understanding:

The problem with this is, if we apply redis results to database results, it performs an unexpected transformation.

This refers to the operation that after a row from (SQL) host_state was queried, the corresponding entry for the same ID is queried from (Redis) icinga:host:state and then both are merged?

In Web's view, the types returned by either source should be identical.

So let's take host_state.in_downtime as an example. How is that represented in PHP? I'd expect a boolean but all of this sounds a bit like this boolean is treated as a string internally. Even if that's the case, couldn't you just normalize the boolean(true)/boolean(false) to string("y")/string("n") then?

[nilmerg]

This refers to the operation that after a row from (SQL) host_state was queried, the corresponding entry for the same ID is queried from (Redis) icinga:host:state and then both are merged?

Yes.

So let's take host_state.in_downtime as an example. How is that represented in PHP? I'd expect a boolean but all of this sounds a bit like this boolean is treated as a string internally. Even if that's the case, couldn't you just normalize the boolean(true)/boolean(false) to string("y")/string("n") then?

It is a boolean, yes. But redis results are merged way before any transformation to boolean happens. The actual transformation then must be able to assume a single type. Varying types are bad as they lead to more complexity on our part. (i.e. non-reusable code) We have a single part that's responsible for the transformation of "y" and "n" and I don't want to teach this to also accept an integer in case of a specific column.

[julianbrost]

I don't want to teach this to also accept an integer in case of a specific column.

Integer? I'm talking about columns of type enum('n','y') in the database that are represented as false/true in JSON. So transforming bools from JSON/Redis to "n"/"y" should just work. If it wasn't the case, that would mean simply replacing all bools in JSON/Redis with "n"/"y"¹ would break Icinga DB Web.

Actual enums are a different question. is_acknowledged is a bit of a nasty example with two of three values acting like boolean values. On a green field, I'd do this differently: currently a filter for is_acknowledged = 'y' is somewhat misleading as it excludes those with a sticky acknowledgement, hence I'd do one of two things:

1. Make it to boolean columns, one for acknowledged and one for whether that's sticky (with the latter being ignored unless acknowledged).
2. Make it so that the column can hold the values 'normal' (that's how the enum value is named in Icinga 2), 'sticky' or NULL, that would then allow using acknowledgement IS (NOT) NULL to query if there is (no) acknowledgement.

The question is: could we do any of this realistically? I think the more promising option would be (1) as this would keep is_acknowledged = 'n' and is_acknowledged = 'y' filters working (however, changing the behavior of the second to also include sticky acknowledgements), it would only break is_acknowledged = 'sticky', that would have to be rewritten to is_acknowledged = 'y' AND is_sticky_acknowledgement = 'y'.

For enums in general, I'd be fine with using string representations. That would then also be the fallback plan for is_acknowledged, with n/y/sticky just being somewhat poorly named (none/normal/sticky from Icinga 2 aren't too bad names actually).

Footnotes

1. Something that I'd really hate to do, but having a mix of both would be even worse. ↩

[nilmerg]

I'm actually not sure what you're up to. I'd like the same data representation in Redis as in the database for the aforementioned columns. What ever is changed, if that goal is met, Icinga DB Web can cope with it fine.

For filtering none of the changes are relevant. Redis results do not influence that.

[julianbrost]

The more invasive changes to is_acknowledged would involve adapting the database schema accordingly.

That would then also be the fallback plan for is_acknowledged, with n/y/sticky

That would only affect Redis and would use the the current enum values (I'd treat n and y as enum values then, not as boolean values).

It's just that if we were to do more invasive changes here, I want to bring to bring that weirdness of is_acknowledged to our attention with the question, if others agree that it would be a good idea to improve this.

[nilmerg]

that would then allow using acknowledgement IS (NOT) NULL to query if there is (no) acknowledgement.

Nah. Please not. We should not start give meaning to nothing (i.e. NULL) 😅

I see no problem with that enum being what it currently is in the database.