Support relation aliases and multiple relations

[arturo-seijas]

Right now the redis client library does not support adding aliases to the relations and hence, multiple relations with the same consumer charm. The relation event name is hardcoded:

  class RedisRequires(Object):    
      def __init__(self, charm, _stored):

          """A class implementing the redis requires relation."""
          super().__init__(charm, "redis")
          self.framework.observe(charm.on.redis_relation_changed, self._on_relation_changed)
          self.framework.observe(charm.on.redis_relation_broken, self._on_relation_broken)
          self._stored = _stored
          self.charm = charm

We request this change because we've identified use cases in which two different redis clusters need to be consumed.

[zmraul]

Several connections can be created using alias for the application itself:

juju deploy redis-k8s redis-cache
juju deploy redis-k8s redis-broker

juju deploy my-app

juju relate redis-cache my-app
juju relate redis-broker my-app

In this case, information about the relation can be accessed on my-app charm with:

for relation in self.model.relations["<metadata-relation-name>"]:
    print(relation.app.name)

This would print: 'redis-cache' and 'redis-broker'. I'm trying to figure out if there is a better way to obtain this information and I'll update accordingly.

I've been trying this out with the current redis-k8s charm and a small charm I use for development and testing.

Would this cover the use case? @mthaddon @arturo-seijas

[jnsgruk]

There is some flexibility too, you can handle different relation names relatively easily redis side if consuming charms want to have multiple relations on the same interface - see here and here

[mthaddon]

Hi Folks,

We think this will be possible, and will confirm the code changes needed to make it happen (from our side). However, we also think it'll be a little sub-optimal as it'll mean we need to manually keep track of which one is the cache and which one is the broker. If you're working on a new library, we just wanted to confirm at what point you'd be committed to one approach or another? This will help us prioritise the work to confirm exactly whether the approach above will work and how important it might be to have dedicated relations for these.

Thanks, Tom

[arturo-seijas]

Hi Raul,

I've managed to cover our use case following your advice. Thank you for that.

Unfortunately, I don't find the solution very intuitive from a library consumer's perspective. An example of the resulting code to retrieve the relation data is as follows:

cache_rel = next(
    rel for rel in self.model.relations["redis"] if rel.app.name == "redis-cache"
)
cache_host = self._stored.redis_relation[cache_rel.id]["hostname"]

This doesn't look as intuitive as having a unique listener for each relation defined in the metadata. Also, I needed to discriminate the relations based on the alias, which isn't visible for the outside. Moreover, there's added complexity when unit testing this chunk of code. The following snippet illustrates how to define one relation in a unit test:

self.harness.add_relation("indico-peers", "indico")
broker_relation_id = self.harness.add_relation("redis", self.harness.charm.app.name)
self.harness.add_relation_unit(broker_relation_id, "redis-broker/0")
self.harness.charm._stored.redis_relation = {
    broker_relation_id: {"hostname": "broker-host", "port": 1010},
}

For the reasons above I think that it'd be worth to align this charm library with the expected behaviour, that is, supporting the multiple relations defined in the metadata file.

Thanks

[zmraul]

An alternative to iterating over all relations could be to use the application peer databag to store mapping information:

def _on_database_created(self, event: RelationCreatedEvent):
    # As juju leader:
    # Get the remote app name
    remote_redis_name = event.relation.app.name
    # Create an entry on the application databag (app_databag = self.model.get_relation(PEER_RELATION).data[self.app])
    self.app_databag[remote_redis_name] = {}

I think that for older relations this is the correct approach. The main issue is that the current redis relation library uses _stored and the leader unit databag to share information (the library might come from a time where the application databag didn't exist). These two patterns are no longer preferred on relations.

Once the new relation is in place, there will be the possibility of declaring event handlers for each relation. See here

As per the unit test, I'm afraid that's how it's going to look like regardless of relation implementation. This is a current peer relation test:

# At test setUp():
self._peer_relation = "redis-peers"
self.harness.add_relation(self._peer_relation, self.harness.charm.app.name)

# Relation test:
rel = self.harness.charm.model.get_relation(self._peer_relation)
# Trigger peer_relation_joined/changed
self.harness.add_relation_unit(rel.id, "redis-k8s/1")
# Simulate an update to the application databag made by the leader unit
self.harness.update_relation_data(rel.id, "redis-k8s", {key: value, key:value})

The only significant difference is that instead of calling _stored, now we call update_relation_data().

[zmraul]

Just to reiterate on the current status of this issue:

• The concern is valid, and we are aware that the current relation is lacking that feature.
• New relation lib (not yet implemented for redis) has a way to implement aliases. On the module docs for the lib there is an example on how to use it. @arturo-seijas Please take a look at that solution. Would that approach be reasonable for the future?

[arturo-seijas]

Hi @zmraul. From what I see in that example, aliases would cover our needs but still wouldn't reflect the specific required relations in the metadata file. What I had in mind is supporting multiple relations for the same interface:

requires:
  redis-broker:
    interface: redis
  redis-cache:
    interface: redis

Note that in our scenario these redis relations correspond to completely different components

[marceloneppel]

Hi @zmraul. From what I see in that example, aliases would cover our needs but still wouldn't reflect the specific required relations in the metadata file. What I had in mind is supporting multiple relations for the same interface:

requires:
  redis-broker:
    interface: redis
  redis-cache:
    interface: redis

Note that in our scenario these redis relations correspond to completely different components

Thanks for the details @arturo-seijas! I guess I confused the concept of alias for this specific scenario.

For this specific scenario, you can use the new library that Raúl mentioned and use something like the code below in charm.py:

self.broker = DatabaseRequires(
    self, "redis-broker", database_name="0"
)
self.framework.observe(
    self.broker.on.database_created, self._on_broker_database_created
)

self.cache = DatabaseRequires(
    self, "redis-cache", database_name="0"
)
self.framework.observe(
    self.cache.on.database_created, self._on_cache_database_created
)        

The database name field may not be so useful for a scenario with Redis, as it uses the predefined 0 to 15 indexes as the database identifiers.

One detail is that the new relations using that library are not yet implemented in the Redis charm.