Fix deletion test (#275)

Removes the need for patch_port() at pebble-ready event. There is no reason to check if we need to patch the port at this event, as port should be toggled to ClusterIP / NodePort in response to client relations exclusively. Also, pebble-ready happens at every restart of the pod, which per-se does not mark as a change on exposing or not the service.

Simplifying the external_connectivity() to make it more readable and adding extra safeguards in the relation checking methods for node-port.

Fixes the tests to consider NodePort or ClusterIP accordingly.

src/charm.py

@@ -348,7 +348,6 @@ def _on_pgbouncer_pebble_ready(self, event: PebbleReadyEvent) -> None:
         self.unit.set_workload_version(self.version)
 
         self.peers.unit_databag["container_initialised"] = "True"
-        self.patch_port()
 
     @property
     def is_container_ready(self) -> bool:

src/relations/pgbouncer_provider.py

@@ -98,12 +98,10 @@ def external_connectivity(self, event=None) -> bool:
             # Disregard of what has been requested by the client, this relation is being removed
             relations.remove(event.relation)
 
-        # Now, we will return true if any of the relations are marked as external OR if the event
-        # itself is requesting for an external_node_connectivity.
-        # Not all events received have external-node-connectivity
-        external_conn = getattr(event, "external_node_connectivity", lambda: False)()
-        return (event and external_conn) or any(
-            relation.data[relation.app].get("external-node-connectivity", "false") == "true"
+        # Now, we will return true if any of the relations are marked as external
+        return any(
+            relation.data.get(relation.app, {}).get("external-node-connectivity", "false")
+            == "true"
             for relation in relations
         )
 
@@ -164,6 +162,8 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
         self.database_provides.set_credentials(rel_id, user, password)
         # Set the database name
         self.database_provides.set_database(rel_id, database)
+
+        self.charm.patch_port(self.external_connectivity(event))
         self.update_connection_info(event.relation)
 
     def _on_relation_departed(self, event: RelationDepartedEvent) -> None:
@@ -214,10 +214,6 @@ def update_connection_info(self, relation):
         if not self.charm.unit.is_leader():
             return
 
-        if relation.data[relation.app].get("external-node-connectivity", "false") == "true":
-            # Make sure we have a node port exposed
-            self.charm.patch_port(True)
-
         self.update_endpoints(relation)
 
         # Set the database version.
@@ -239,6 +235,10 @@ def update_endpoints(self, relation=None) -> None:
 
         # Set the endpoints for each relation.
         for relation in relations:
+            if not relation or not relation.data or not relation.data.get(relation.app):
+                # This is a relation that is going away and finds itself in a broken state
+                # proceed to the next relation
+                continue
             # Read-write endpoint
             if relation.data[relation.app].get("external-node-connectivity", "false") == "true":
                 self.database_provides.set_endpoints(relation.id, nodeports["rw"])

tests/integration/relations/pgbouncer_provider/helpers.py

@@ -125,6 +125,24 @@ async def run_sql_on_application_charm(
     return result.results
 
 
+async def is_external_connectivity_set(
+    ops_test: OpsTest,
+    application_name: str,
+    relation_name: str,
+    relation_id: str = None,
+) -> Optional[str]:
+    data = await get_application_relation_data(
+        ops_test,
+        application_name,
+        relation_name,
+        "data",
+        relation_id,
+    )
+    if not data:
+        return False
+    return json.loads(data).get("external-node-connectivity", None) == "true"
+
+
 async def build_connection_string(
     ops_test: OpsTest,
     application_name: str,
@@ -178,14 +196,24 @@ async def build_connection_string(
     )
     host = endpoints.split(",")[0].split(":")[0]
 
-    # Translate the pod hostname to an IP address.
-    model = ops_test.model.info
-    client = AsyncClient(namespace=model.name)
-    pod = await client.get(Pod, name=host.split(".")[0])
-    ip = pod.status.podIP
+    use_node_port = await is_external_connectivity_set(
+        ops_test,
+        application_name,
+        relation_name,
+        relation_id,
+    )
+
+    if use_node_port:
+        return f"dbname='{database}' user='{username}' host='{host}' port={endpoints.split(',')[0].split(':')[1]} password='{password}' connect_timeout=10"
+    else:
+        # Translate the pod hostname to an IP address.
+        model = ops_test.model.info
+        client = AsyncClient(namespace=model.name)
+        pod = await client.get(Pod, name=host.split(".")[0])
+        ip = pod.status.podIP
 
     # Build the complete connection string to connect to the database.
-    return f"dbname='{database}' user='{username}' host='{ip}' password='{password}' connect_timeout=10"
+    return f"dbname='{database}' user='{username}' host='{ip}' password='{password}' connect_timeout=10 port=6432"
 
 
 async def check_new_relation(

tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py

@@ -498,8 +498,12 @@ async def test_indico_datatabase(ops_test: OpsTest) -> None:
             application_name="indico",
             num_units=1,
         )
-        await ops_test.model.deploy("redis-k8s", channel="stable", application_name="redis-broker")
-        await ops_test.model.deploy("redis-k8s", channel="stable", application_name="redis-cache")
+        await ops_test.model.deploy(
+            "redis-k8s", channel="latest/stable", application_name="redis-broker"
+        )
+        await ops_test.model.deploy(
+            "redis-k8s", channel="latest/stable", application_name="redis-cache"
+        )
         await asyncio.gather(
             ops_test.model.relate("redis-broker", "indico:redis-broker"),
             ops_test.model.relate("redis-cache", "indico:redis-cache"),
@@ -533,11 +537,12 @@ async def test_connection_is_possible_after_pod_deletion(ops_test: OpsTest) -> N
     await delete_pod(ops_test, unit.name)
     await ops_test.model.wait_for_idle(status="active", idle_period=3)
 
+    await asyncio.sleep(20)
+
     # Test the connection.
     connection_string = await build_connection_string(
         ops_test, DATA_INTEGRATOR_APP_NAME, relation_name="postgresql", database="test-database"
     )
-    connection_string += " port=6432"
     connection = None
     try:
         connection = psycopg2.connect(connection_string)

tests/unit/relations/test_pgbouncer_provider.py

@@ -34,6 +34,7 @@ def setUp(self):
         self.client_rel_id = self.harness.add_relation(CLIENT_RELATION_NAME, "application")
         self.harness.add_relation_unit(self.client_rel_id, "application/0")
 
+    @patch("charm.lightkube")
     @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True)
     @patch(
         "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock
@@ -78,6 +79,7 @@ def test_on_database_requested(
         _pg_databag,
         _pg,
         _check_backend,
+        _,
     ):
         self.harness.set_leader()
         _gen_rel_dbs.return_value = {}