Redis DB Index should be configurable

[silverwind]

Currently this application is hardcoded to use Redis DB index 0 which can lead to conflicts in redis when other applications also use DB 0.

It should be made configurable with a db option with default value 0 and possible values 0-15, e.g. 16 databases in total, which is what Redis offers by default.

[oxzi]

Could you please elaborate on your use case?

AFAIK, it is recommended to use a separate Redis instance per application. This is also backed by the Redis documentation for the SELECT command, that introduces databases as "a form of namespacing" and states that "Redis databases should be used to separate different keys belonging to the same application (if needed), and not to use a single Redis instance for multiple unrelated applications". Furthermore, Redis Cluster does not support databases.
In an older mailing list post, Salvatore Sanfilippo, the author of Redis, calls the database functionality his "worst decision in Redis design at all".

As a possible use case for Redis database support in Icinga DB, I would see the use of an existing Redis server. However, when you are already in a hosting scenario where you can install Icinga DB, I don't see a reason why you couldn't install another Redis. This might even improve performance due to Redis "mostly single-threaded" architecture, which suggests "launch[ing] several Redis instances to scale out on several cores" in its Redis benchmark, Pitfalls and misconceptions documentation.

[silverwind]

We run Icinga (without Redis currently) along with another application (which interfaces with Icinga) and in this application already uses Redis, currently on DB 0.

We are looking to upgrade Icinga and potentially enable Redis for it and I thought instead of going through the trouble of setting up a another Redis (with replication, backup and all that fuss), we could just use another Redis DB to avoid key conflicts.

Another option would be if Icingadb allows to configure a key prefix, e.g. icingadb: on all Redis keys it sets, it'd be the same outcome.

A single redis thread can easily handle upwards of 20k queries/sec, so I don't think performance is a problem, unless icingadb expects such query amounts, but at this point, one would better invested by using a multithreaded Redis fork like Dragonfly.

[oxzi]

We are looking to upgrade Icinga and potentially enable Redis for it and I thought instead of going through the trouble of setting up a another Redis (with replication, backup and all that fuss), we could just use another Redis DB to avoid key conflicts.

Depending on your setup, a Redis replication might not be necessary. Please take a look at the Distributed Setup manual of Icinga DB, if this fits your use case. In an High Availability Setup with two Icinga DB instances, each one needs its own Redis instance¹.

Another option would be if Icingadb allows to configure a key prefix, e.g. icingadb: on all Redis keys it sets, it'd be the same outcome.

Creating namespaces by prefixing keys seems like an ugly hack, which might result in nasty issues on the long run.

If one really wants to reuse an existing Redis setup, databases might be the way to go². However, as I wrote earlier, multiple Redis instances might be a better separation and might even perform better, as others stated before me.

Due to the fact that using multiple databases would exclude Redis Cluster and I cannot see why Icinga DB would require multiple databases, in my opinion nothing stands against allowing configuring other databases next to database zero. I would create a patch, as this change should be trivial. Unless @lippserd or someone else familiar with the code objects.

Edit: BTW, there is no hard limit of sixteen databases. This is just the default in the configuration.

Footnotes

1. Or its own database within the same Redis.. Please don't, I was joking. ↩

2. With the exception of a Redis Cluster, which does not support databases, Redis cluster specification, Implementation subset. ↩

[silverwind]

Ah, so as per https://icinga.com/docs/icinga-db/latest/doc/05-Distributed-Setups/, Redis is local to each Icinga master node, so in essence Redis is used as a local in-memory database similar to badger.

If above is correct then HA will likely not work correctly with a shared Redis DB, because the application expects Redis to be a dedicated resource?

I'll leave it up to you if you want to add this db option. I think it's good practice to expose all connection-related parameters for each database.

BTW, there is no hard limit of sixteen databases

Good to know, so the option should be made a uint64 likely.

[oxzi]

Ah, so as per https://icinga.com/docs/icinga-db/latest/doc/05-Distributed-Setups/, Redis is local to each Icinga master node, so in essence Redis is used as a local in-memory database similar to badger.

Yes. The Redis is used for communication between an Icinga 2 and the Icinga DB¹. Think about it as both a message queue and storage for "volatile data like check results"².

If above is correct then HA will likely not work correctly with a shared Redis DB, because the application expects Redis to be a dedicated resource?

In my understanding of Icinga DB, this is correct.

I'll leave it up to you if you want to add this db option. I think it's good practice to expose all connection-related parameters for each database.

To make the connection work, this must be configured both for Icinga 2 as well as for Icinga DB. Fortunately, Icinga 2 already supports a DB selection. There's the undocumented db_index attribute for an IcingaDB object.

Getting away from this theoretical level, I have put together a trivial testing scenario.

The shortest possible patch against Icinga DB might look like this:

diff --git a/pkg/config/redis.go b/pkg/config/redis.go
index 38571e3..14d1b78 100644
--- a/pkg/config/redis.go
+++ b/pkg/config/redis.go
@@ -22,6 +22,7 @@ type Redis struct {
        Host       string              `yaml:"host"`
        Port       int                 `yaml:"port" default:"6380"`
        Password   string              `yaml:"password"`
+       Database   int                 `yaml:"database" default:"0"`
        TlsOptions TLS                 `yaml:",inline"`
        Options    icingaredis.Options `yaml:"options"`
 }
@@ -48,7 +49,7 @@ func (r *Redis) NewClient(logger *logging.Logger) (*icingaredis.Client, error) {
        options := &redis.Options{
                Dialer:      dialWithLogging(dialer, logger),
                Password:    r.Password,
-               DB:          0, // Use default DB,
+               DB:          r.Database,
                ReadTimeout: r.Options.Timeout,
                TLSConfig:   tlsConfig,
        }

Afterwards, the Icinga 2 configuration should contain an IcingaDB object like the following:

object IcingaDB "icingadb" {
  host = "redis.example.com"
  db_index = 6
}

…and Icinga DB's configuration should contain the following redis configuration:

redis:
  host: redis.example.com
  port: 6380
  database: 6

This modifications were enough to make my test environment use the sixth Redis database, as I verified through redis-cli.

However, as I wrote before and the documentation states, reusing a Redis might only make sense for some scenarios. For a single node setup with a low load, this might make sense. However, in an HA setup, this would only lead to problems, IMHO.

Thus, I would love to hear the opinion of the contributors if we should proceed with this.

BTW, there is no hard limit of sixteen databases

Good to know, so the option should be made a uint64 likely.

Currently, DB is an int in the Redis Go library³.

Footnotes

1. https://icinga.com/docs/icinga-2/latest/doc/14-features/#icinga-db ↩

2. https://icinga.com/blog/2022/06/30/finally-accomplished-icinga-db-released/ ↩

3. https://pkg.go.dev/github.com/go-redis/redis/v8#Options ↩