Fix database relation state handling #75

carlcsaposs-canonical · 2023-05-23T18:50:34Z

Issue

Fixes #73, fixes #78

Database relations can be in many states:

Relation missing
Relation created but no data in databag or some required keys missing from databag (KeyError "database" in self._remote_databag #78)—we need to wait for related application
Relation data valid from database_provides remote application but we haven't created user yet—we need to create database/user & populate databag
Relation data valid from database_provides remote application and we've created a user—do nothing
Relation breaking—remote databag not accessible (https://bugs.launchpad.net/juju/+bug/1960934, KeyError: 'Cannot index relation data with "None". Are you trying to access remote app data during a relation-broken event? This is not allowed.' #73); we need to clean up user if we created one

Before this PR, the _Relation class could be in multiple states and some properties could be accessed but some properties would cause an exception if accessed depending on the state. In other words, which properties were available was implicit.

For example, these properties

mysql-router-k8s-operator/src/relations/database_requires.py

Lines 44 to 62 in 92b029e

    
           @property 
        
           def host(self) -> str: 
        
               """MySQL cluster primary host""" 
        
               return self._endpoint.split(":")[0] 
        
           @property 
        
           def port(self) -> str: 
        
               """MySQL cluster primary port""" 
        
               return self._endpoint.split(":")[1] 
        
           @property 
        
           def username(self) -> str: 
        
               """Admin username""" 
        
               return self._remote_databag["username"] 
        
           @property 
        
           def password(self) -> str: 
        
               """Admin password""" 
        
               return self._remote_databag["password"]

can only be accessed if

mysql-router-k8s-operator/src/relations/database_requires.py

Lines 64 to 66 in 92b029e

    
           def is_breaking(self, event): 
        
               """Whether relation will be broken after the current event is handled""" 
        
               return isinstance(event, ops.RelationBrokenEvent) and event.relation.id == self._id

is False and these conditions are met

mysql-router-k8s-operator/src/relations/database_requires.py

Lines 102 to 108 in 92b029e

    
           if not self._interface.is_resource_created(): 
        
               return 
        
           relation = Relation(self._interface) 
        
           # TODO: Refactor `Relation` so that we don't need to access private class member 
        
           if relation._remote_databag.get("endpoints") is None: 
        
               return 
        
           return relation

Solution

Create different _Relation classes for different states so that all properties/methods on the class can be called
e.g.

mysql-router-k8s-operator/src/relations/database_provides.py

Line 57 in 5a1757b

class _RequestedUser(_Relation):

mysql-router-k8s-operator/src/relations/database_provides.py

Line 103 in 5a1757b

class _CreatedUser(_Relation):

Also raise exceptions during the __init__ of those objects so that we don't have to check for exceptional cases (e.g. required key missing from relation databag, relation is breaking) when using the object—if the object initializes successfully, all of its properties/methods are safe to use

carlcsaposs-canonical · 2023-05-23T19:01:04Z

I believe there's a similar issue with database_provides—if a relation is created via database_provides but no database is requested, the relation will be included in _requested_users when it should not be:

mysql-router-k8s-operator/src/relations/database_provides.py

Lines 135 to 143 in 68d723c

    
           def _requested_users(self, *, event) -> list[_Relation]: 
        
               """Related application charms that have requested a database & user""" 
        
               requested_users = [] 
        
               for relation in self._relations: 
        
                   if isinstance(event, ops.RelationBrokenEvent) and event.relation.id == relation.id: 
        
                       # Relation is being removed; delete user 
        
                       continue 
        
                   requested_users.append(relation) 
        
               return requested_users

carlcsaposs-canonical · 2023-05-24T20:01:50Z

PR is partially completed—I believe database_requires is fixed but database_provides is still broken

(Requested review early in the hopes of speeding up final review later since this issue currently affects openstack charmers' testing)

src/workload.py

shayancanonical · 2023-05-26T12:04:01Z

src/relations/database_provides.py

+    """Database & user has not been provided to related application charm"""
+
+
+class _CreatedUser(_Relation):


This feels a little counterintuitive: a _CreatedUser is actually a subclass of a _Relation

same thing with _RequestedUser

what about it feels counterintuitive—the name, that it's a subclass of _Relation, or something else?
Also here's the docstring for context
"""Related application charm that has been provided with a database & user"""

the name is definitely a big part of it. when i saw it in reconcile_users, i was surprised that it was being compared to a Relation. i expected it to be a class that encapsulates Users, but it's not quite storing any user info (except a method delete_user)

I agree that the name is confusing

do you have ideas for better names? I struggled to come up with names for them

would something like _RelationThatRequestedUser and _RelationWithCreatedUser be better?

renamed in c7d24df—if anyone thinks of better names happy to rename again

shayancanonical · 2023-05-26T12:04:56Z

src/relations/database_provides.py

@@ -180,3 +183,28 @@ def reconcile_users(
            if relation not in requested_users:
                relation.delete_user(shell=shell)


it may be a good idea to log out users created and users deleted to the debug-log

They're already logged in lower-level methods (e.g. shell.create_application_database_and_user and self._set_databag)

shayancanonical · 2023-05-26T12:08:46Z

src/relations/database_provides.py

+            # At least one relation is active—do not report about inactive relations
+            return
+        for exception in exceptions:
+            if isinstance(exception, remote_databag.IncompleteDatabag):


it might be a good idea to add a suffix Exception so that we know that remote_databag.IncompleteDatabagException is an exception (without having to trace back to the definition of the class to see that it's a subclass of an exception)

same applies to all subclasses of StatusException

That seems a bit verbose to me. I think that's something that should be handled by the IDE instead of the naming (e.g. hovering over IncompleteDatabag shows it's a StatusException). Somewhat analogous, we don't generally name something like requested_users as requested_users_list

Also, I think it's fairly clear from the context—exceptions has type annotation list[status_exception.StatusException], the variable is called exception, and the only other usage of these classes (besides isinstance(exception, )) is in an except statement

shayancanonical · 2023-05-26T12:12:54Z

src/relations/remote_databag.py

+        interface: data_interfaces.DatabaseRequires | data_interfaces.DatabaseProvides,
+        relation: ops.Relation,
+    ) -> None:
+        super().__init__(interface.fetch_relation_data()[relation.id])


taurus-forever

Nice!

…te databag

paulomach

lgtm, some generators adoption!

paulomach · 2023-05-26T13:31:20Z

src/relations/database_provides.py

+        self._local_databag = relation.data[interface.local_app]
+        for key in ("database", "username", "password", "endpoints"):
+            if key not in self._local_databag:
+                raise _UserNotCreated


Would be useful to extent _UserNotCreated to include the missing key in the exception message, e.g.:

Suggested change

raise _UserNotCreated

raise _UserNotCreated(f"missing {key}")

I chose not to include it since it's not being used (_UserNotCreated is always caught & it's not parsing the message) and since it would be incomplete—the exception would only include the first missing key

paulomach · 2023-05-26T13:56:20Z

src/charm.py

@@ -201,35 +198,31 @@ def _patch_service(self, *, name: str, ro_port: int, rw_port: int) -> None:

    def reconcile_database_relations(self, event=None) -> None:
        """Handle database requires/provides events."""
+        workload_ = self.get_workload(event=event)


why use of post _ ? afaik, that's commonly usud to avoid clashes with python keywords

Conflict with workload.py (import workload)

carlcsaposs-canonical force-pushed the fix-scale-down branch from 003ab21 to a093aa2 Compare May 24, 2023 12:37

carlcsaposs-canonical requested review from paulomach, taurus-forever and shayancanonical May 24, 2023 19:58

delgod reviewed May 25, 2023

View reviewed changes

src/workload.py Outdated Show resolved Hide resolved

carlcsaposs-canonical force-pushed the fix-scale-down branch from c31bbc4 to f5e644b Compare May 25, 2023 12:20

carlcsaposs-canonical changed the title ~~Fix scale down~~ Fix database relation state handling May 25, 2023

carlcsaposs-canonical marked this pull request as ready for review May 25, 2023 18:26

carlcsaposs-canonical force-pushed the fix-scale-down branch from 4a3e003 to 5a1757b Compare May 25, 2023 19:05

shayancanonical reviewed May 26, 2023

View reviewed changes

shayancanonical approved these changes May 26, 2023

View reviewed changes

taurus-forever approved these changes May 26, 2023

View reviewed changes

carlcsaposs-canonical added 14 commits May 26, 2023 13:55

use property

6876469

Check if database requires relation is breaking before accessing remo…

c1c9ffb

…te databag

update comment

0704c66

fixup

cf02944

Connection info

fd5da5e

Update docstrings

81df5d6

missing docstring

4dcb921

remove relation wrapper

e5ef742

subclass _Relation

b97c26f

Move shared code into modules

0b9d285

isort

02f9c01

Rename _Relation subclasses

3d84a83

Use tuples/generators instead of lists

3580623

use generic iterable type annotation

3ebbc46

carlcsaposs-canonical force-pushed the fix-scale-down branch from 6ef7f4e to 3ebbc46 Compare May 26, 2023 13:56

carlcsaposs-canonical merged commit 608c094 into main May 26, 2023

carlcsaposs-canonical deleted the fix-scale-down branch May 26, 2023 13:58

paulomach reviewed May 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix database relation state handling #75

Fix database relation state handling #75

carlcsaposs-canonical commented May 23, 2023 •

edited

Loading

carlcsaposs-canonical commented May 23, 2023

carlcsaposs-canonical commented May 24, 2023

shayancanonical May 26, 2023

carlcsaposs-canonical May 26, 2023

shayancanonical May 26, 2023

carlcsaposs-canonical May 26, 2023

carlcsaposs-canonical May 26, 2023

shayancanonical May 26, 2023

carlcsaposs-canonical May 26, 2023

shayancanonical May 26, 2023

carlcsaposs-canonical May 26, 2023

shayancanonical May 26, 2023

taurus-forever left a comment

paulomach left a comment

paulomach May 26, 2023

carlcsaposs-canonical May 26, 2023

paulomach May 26, 2023

carlcsaposs-canonical May 26, 2023

	@property
	def host(self) -> str:
	"""MySQL cluster primary host"""
	return self._endpoint.split(":")[0]

	@property
	def port(self) -> str:
	"""MySQL cluster primary port"""
	return self._endpoint.split(":")[1]

	@property
	def username(self) -> str:
	"""Admin username"""
	return self._remote_databag["username"]

	@property
	def password(self) -> str:
	"""Admin password"""
	return self._remote_databag["password"]

	def is_breaking(self, event):
	"""Whether relation will be broken after the current event is handled"""
	return isinstance(event, ops.RelationBrokenEvent) and event.relation.id == self._id

	if not self._interface.is_resource_created():
	return
	relation = Relation(self._interface)
	# TODO: Refactor `Relation` so that we don't need to access private class member
	if relation._remote_databag.get("endpoints") is None:
	return
	return relation

		"""Database & user has not been provided to related application charm"""


		class _CreatedUser(_Relation):

		@@ -180,3 +183,28 @@ def reconcile_users(
		if relation not in requested_users:
		relation.delete_user(shell=shell)

	raise _UserNotCreated
	raise _UserNotCreated(f"missing {key}")

Fix database relation state handling #75

Fix database relation state handling #75

Conversation

carlcsaposs-canonical commented May 23, 2023 • edited Loading

Issue

Solution

carlcsaposs-canonical commented May 23, 2023

carlcsaposs-canonical commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taurus-forever left a comment

Choose a reason for hiding this comment

paulomach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlcsaposs-canonical commented May 23, 2023 •

edited

Loading