Sync dependencies

[yhabteab]

Requires

• Sync dependencies to Redis icinga2#10290

[julianbrost]

So now I have two places things that I suggest to rename in the schema:

1. affected_children -> total_children, see Sync dependencies #889 (comment)
2. redundancy_group -> redundancygroup in all table and column names, see Sync dependencies #889 (comment)

Both affect web, so that should be coordinated. @nilmerg Are you fine with both of these changes? And if so, shall we just update this PR and Icinga/icinga2#10290 and ping you once done?

[julianbrost]

I'd rename this table to redundancygroup (and redundancygroup_state respectively) for two reasons:

• Consistency with other already existing group tables:

  icingadb/schema/mysql/schema.sql

  Line 229 in a77498d

  CREATE TABLE hostgroup (

  icingadb/schema/mysql/schema.sql

  Line 395 in a77498d

  CREATE TABLE servicegroup (

  icingadb/schema/mysql/schema.sql

  Line 1027 in a77498d

  CREATE TABLE usergroup (

• Makes the following special-casing for the table name unnecessary ¹:

  icingadb/pkg/icingadb/v1/dependency.go

  Lines 16 to 19 in 8fdf0a2

  	// TableName implements [database.TableNamer].
  	func (r *Redundancygroup) TableName() string {
  	return "redundancy_group"
  	}

Footnotes

1. I presume that's currently necessary due to the inconsistency between Redis (icinga:redundancygroup, i.e. no separator in redundancygroup) and SQL (redundancy_group, i.e. with a separator), otherwise the Go type could also be named RedundancyGroup. ↩

[yhabteab]

1. otherwise the Go type could also be named RedundancyGroup. ↩

No, it cannot! The Redis keys are generated dynamically based on the Go type name and that would result in icinga:redundancy:group which does not exist in Icinga 2.

[julianbrost]

"Otherwise" as in "if it actually was icinga:redundancy:group and redundancy_group"

[yhabteab]

2. redundancy_group -> redundancygroup in all table and column names, see Sync dependencies #889 (comment)

I initially asked Johannes about this and both agreed to keep it as it is, as the effort for renaming on the web side is not worth it compared to a one-line solution on the Go side, i.e. implementing the TableName() method.

[nilmerg]

I'm totally fine with total_children. But redundancygroup because consistency? Web would like to keep naming the respective model RedundancyGroup. I'd also liked to name the Hostgroup model HostGroup, but I was not involved in its naming. I don't see why we should continue this typo here on this new table.

[julianbrost]

Why is it a typo?

[nilmerg]

https://www.dict.cc/?s=usergroup

[julianbrost]

I would consider it a typo if it was was named "hostgruop" or something like that. In that case, naming new things like this for consistency would be truely insane. However, nothing is misspelled here, it's just a question of convention how things are mapped to SQL identifiers.

but I was not involved in its naming

Neither was I, so I don't know the exact motivation for the current names, but the general pattern seems to be that tables are named like the corresponding Icinga 2 object types converted to lowercase with sub-tables (like group members) being named with some _-separated suffix.

So for me, that looks perfectly fine as well, that is, considered in isolation, hostgroup and host_group would be equally valid for me, my preference towards naming the new table redundancygroup only exists because hostgroup was chosen over host_group in the past.

Web would like to keep naming the respective model RedundancyGroup. I'd also liked to name the Hostgroup model HostGroup

I mean that might be nice, but do you really expect that we'd go through the pain of renaming many existing tables any time soon just for cosmetic reasons? And were would you stop? "checkcommand" and "customvar" aren't English words either.

If you insist on the naming for this individual table, I predict that there will be inconsistencies in both the table names (hostgroup + redundancy_group) and the PHP class names (Hostgroup + RedundancyGroup) for the foreseeable future.

In case the PHP class names are that important to you and they are indeed directly tied to the table names, I'd consider relaxing the latter and adding a mechanism similar to that TableName() function in Go on the PHP more constructive: that would introduce no inconsistencies and you could name all your PHP classes just the way you like.

(I really didn't expect that to be such a controversial suggestion...)

[nilmerg]

the general pattern seems to be that tables are named like the corresponding Icinga 2 object types converted to lowercase

Indeed. That's where things went wrong. Icinga 2's class is also camel case. For some reason, someone decided against transforming this to snake case.

I'd consider relaxing the latter and adding a mechanism similar to that TableName() function in Go on the PHP more constructive

There is such a mechanism, but that wasn't implemented for the purpose of having a different naming syntax for class names. I'd have to verify whether this mechanism reliably works this way, where as what you have does already reliably work.

(I really didn't expect that to be such a controversial suggestion...)

I'm actually somewhat annoyed that this even comes up, half a year after I put up the schema suggestion.

[julianbrost]

the general pattern seems to be that tables are named like the corresponding Icinga 2 object types converted to lowercase

Indeed. That's where things went wrong. Icinga 2's class is also camel case. For some reason, someone decided against transforming this to snake case.

I don't see that there's anything inherently wrong with that choice.

(I really didn't expect that to be such a controversial suggestion...)

I'm actually somewhat annoyed that this even comes up, half a year after I put up the schema suggestion.

That's simply something I didn't notice when looking at the schema diff as there's no inconsistency within the diff but only with other parts of the same file. So I actually only noticed the inconsistency when seeing that TableName() implementation and looking into the naming in more detail.

I expected the suggested change to be trivial and I still believe it would be a bad idea to introduce that inconsistency, but it really doesn't feel like that discussion is worth my time. So if you believe this is actually a good idea, I won't stop you, just be prepared for some "told you so" in the future.

[nilmerg]

just be prepared for some "told you so" in the future.

I'll gladly take that risk :)

[julianbrost]

Needs a rebase due to newly introduced merge conflicts. Apart from that, the first three inline comments are probably the most important ones.

[julianbrost]

Wait, do you actually still send them?

[yhabteab]

Yes.

[julianbrost]

So what exactly is the effect of the race condition mentioned in this comment? Like if it's too broken to be tested, why should it be fine for production?

[yhabteab]

We have already talked about this in person, and I have even created a draft PR on your request #894, so there is no point in repeating all of it here again.

[julianbrost]

Yes, we had a discussion about that and I don't want to repeat the full discussion here, just write down the relevant parts why we're doing it that way now. I remember that we agreed that it doesn't make sense to solve that fundamental runtime update limitation in this PR, but did we really say that we want to send the updates nonetheless and ignore the problem? If so, I need a refresher on the reasons (hence my surprise in the initial comment).

From the PR you linked:

It seems that when the allowParallel parameter is set to true, the RuntimeUpdates#Sync() method does not honor the exact order of delete and upsert events from Redis, resulting in some rare and difficult to manually trigger race conditions where a subsequent runtime delete event from Icinga 2 might actually surpass the previously sent upsert event.

What exactly does "difficult to manually trigger" mean? If the tests can trigger it, why can't it happen to a user? I mean you don't need to trigger it manually for it to be a problem, if some user's automation script could trigger it, it wouldn't be good either.

So what makes it safe enough to ignore the problem here, when for regular state updates, it's worked around by not sending runtime deletes?

[yhabteab]

but did we really say that we want to send the updates nonetheless and ignore the problem?

Well, not exactly, but I definitely remember you saying to not send a runtime delete event for the regular tables, as you saw that I had already added that code in the Icinga 2 PR, so I only removed that part of the code, but not for the dependency states.

So what makes it safe enough to ignore the problem here, when for regular state updates, it's worked around by not sending runtime deletes?

Safe? I don't think it can be described as safe or not safe, if you delete some hosts or services at runtime, you will have dead state entries in the database until the next reload, that's all, nothing more. By the way, the discussion about sending or not sending delete events is not the right place here, as this PR only receives, but never sends 😅.

[julianbrost]

Safe? I don't think it can be described as safe or not safe, if you delete some hosts or services at runtime, you will have dead state entries in the database until the next reload, that's all, nothing more.

Well it's not perfect but from my understanding, not sending runtime state deletions is the safer option in the sense that all that can happen is that there are a few obsolete state rows in the database, which won't cause any user-facing problems (there's just a row in the database that will never be used). In contrast, if you send deletions, that problem can happen the other way around as well, so that would be state rows missing in the database which will be visible to the user in Icinga Web.

By the way, the discussion about sending or not sending delete events is not the right place here, as this PR only receives, but never sends 😅.

The fun of multi-project changes. The problem is in this project, the workaround in the other.

[julianbrost]

Fine for me now. Can't be merged immediately for two reasons:

1. This is not supposed to become part of the imminent release (https://github.com/Icinga/icingadb/milestone/9).
2. Requires coordination with Sync dependencies to Redis icinga2#10290 (see Sync dependencies to Redis icinga2#10290 (comment)), in particular, both releases of Icinga 2 and Icinga DB will be necessary due to the incompatibility in Redis.

[julianbrost]

Given yesterday's release, the schema upgrade files have to be renamed. Also found a minor problem with the issue with the PostgreSQL schema upgrade (actually forgot to do my standard "does the migration match the full schema" check before :o)