Provide cancel_time in Icinga DB downtime history where has_been_cancelled may be 1

[Al2Klimov]

The table sla_history_downtime requires a downtime_end. The Go daemon takes the cancel_time if has_been_cancelled is 1. So we must supply a cancel_time whereever has_been_cancelled is 1. Otherwise the Go daemon can't process some entries.

fixes #9942

Btw SendRemovedDowntime() does the same.

TODO

• Concept ok?
• Transfer issue here
• Check "fixes" in OP
• Merge

[julianbrost]

This code change might prevent the crash described in #9942. However, the observed behavior suggests there is synchronization missing between triggering and cancelling a downtime: a downtime shouldn't be cancelled before it finished triggering. If IcingaDB::SendRemovedDowntime() can observe downtime->GetWasCancelled() == true, is there anything that would prevent the downtime removed history event overtaking the downtime added event? (And if that's the case, who knows in which ways the resulting database entries could be incorrect.)

So please have another look at this. This should not only fix inconsistencies that are so obvious that they are caught by database constraints.

[drapiti]

Hi guys, I think we have run into this issue. We are getting this constraint violation on our secondarey master but luckily not on the primary for the moment.

[yhabteab]

Acquiring the object lock at the binging of TriggerDowntime() is supposed to block the RemoveDowntime() cleanup timer thread or external ones trying to acquire that very same lock in:

icinga2/lib/base/configobject.cpp

Lines 393 to 402 in d05be80

	ObjectLock olock(this);
	if (!IsActive())
	return;
	SetActive(false, true);
	SetAuthority(false);
	Stop(runtimeRemoved);

So, apart from the two comments/questions below LGTM!

[yhabteab]

It shouldn't harm to include it, but it's rather strange to send cancel_time for a downtime_start event that would contain 0 anyway.

[Al2Klimov]

"would" is the most important word here. If for some reason this goes wrong, I want

• to know it via history
• no Icinga DB crash

[julianbrost]

no Icinga DB crash

That's the primary objective of this PR.

• to know it via history

Know what? I mean a possible alternative could be to just remove has_been_cancelled from the start event altogether as that doesn't really make sense here (note: I didn't check/test how Icinga DB will behave in that case) and should be set in the end event. The downtime start and end events affect the same row in downtime_history anyways, so you can't tell which of both even set cancel_time.

[Al2Klimov]

Nope. 2023-12-19 15:09:14 2023-12-19T14:09:14.260Z FATAL icingadb Error 1048 (23000): Column 'has_been_cancelled' cannot be null

[yhabteab]

That's because the DB column is not nullable. How about just sending a hardcoded 0 for has_been_cancelled then? I mean, this is a downtime start event, we don't need to set this to a meaningful value as it is guaranteed to be synchronised with the downtime end event.

icinga2/lib/icingadb/icingadb-objects.cpp

Lines 1954 to 1956 in 949d983

	"has_been_cancelled", Convert::ToString((unsigned short)downtime->GetWasCancelled()),
	"trigger_time", Convert::ToString(TimestampToMilliseconds(downtime->GetTriggerTime())),
	"cancel_time", Convert::ToString(TimestampToMilliseconds(downtime->GetRemoveTime())),

[Al2Klimov]

This sounds so cheaty, that's literally fake info.

[julianbrost]

For completeness: this PR was split into "things to directly prevent the crash of Icinga DB" (this PR) and "things to create less strange events in the first place" (#9935) where the latter has the problem that it has to keep locks while calling boost signals, which could easily introduce locking issues and is quite complex to review, so that won't make it into the next bugfix release due to time constraints.