[DPE-4594][DPE-4620] Address test_charm and test_self_healing instabi…

…lities (#510)

* test modified logic for shared_buffers

* fix shared_buffers logic and unit test

* revert profile setting in test

* add shared_buffers to bulk update

* multiple fixes

* fix unit test

* revert backup test + increase check verbosity

* fix health check

* revert tls change in charm

* fix unit test

* re-write connection check for self-healing

* add timeout to calls

* fix position of tasks

* retry get patroni cluster

* refactor is_connection_possible check

* brush up changes

* revert member_started changes

lib/charms/postgresql_k8s/v0/postgresql.py

@@ -36,7 +36,7 @@
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 28
+LIBPATCH = 29
 
 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles"
 
@@ -604,12 +604,11 @@ def build_postgresql_parameters(
                 # Use 25% of the available memory for shared_buffers.
                 # and the remaining as cache memory.
                 shared_buffers = int(available_memory * 0.25)
+                parameters["shared_buffers"] = f"{int(shared_buffers * 128 / 10**6)}"
             effective_cache_size = int(available_memory - shared_buffers)
-            parameters.setdefault("shared_buffers", f"{int(shared_buffers / 10**6)}MB")
-            parameters.update({"effective_cache_size": f"{int(effective_cache_size / 10**6)}MB"})
-        else:
-            # Return default
-            parameters.setdefault("shared_buffers", "128MB")
+            parameters.update({
+                "effective_cache_size": f"{int(effective_cache_size / 10**6) * 128}"
+            })
         return parameters
 
     def validate_date_style(self, date_style: str) -> bool:

src/charm.py

@@ -1618,6 +1618,7 @@ def update_config(self, is_creating_backup: bool = False) -> bool:
         self._patroni.bulk_update_parameters_controller_by_patroni({
             "max_connections": max_connections,
             "max_prepared_transactions": self.config.memory_max_prepared_transactions,
+            "shared_buffers": self.config.memory_shared_buffers,
         })
 
         self._handle_postgresql_restart_need()

src/relations/postgresql_provider.py

@@ -138,7 +138,7 @@ def _on_relation_departed(self, event: RelationDepartedEvent) -> None:
         # Set a flag to avoid deleting database users when this unit
         # is removed and receives relation broken events from related applications.
         # This is needed because of https://bugs.launchpad.net/juju/+bug/1979811.
-        if event.departing_unit == self.charm.unit:
+        if event.departing_unit == self.charm.unit and self.charm._peers:
             self.charm._peers.data[self.charm.unit].update({"departing": "True"})
 
     def _on_relation_broken(self, event: RelationBrokenEvent) -> None:

tests/integration/ha_tests/helpers.py

@@ -91,7 +91,9 @@ async def are_all_db_processes_down(ops_test: OpsTest, process: str) -> bool:
 
 
 def get_patroni_cluster(unit_ip: str) -> Dict[str, str]:
-    resp = requests.get(f"http://{unit_ip}:8008/cluster")
+    for attempt in Retrying(stop=stop_after_delay(30), wait=wait_fixed(3)):
+        with attempt:
+            resp = requests.get(f"http://{unit_ip}:8008/cluster")
     return resp.json()
 
 
@@ -481,23 +483,28 @@ async def get_sync_standby(model: Model, application_name: str) -> str:
             return member["name"]
 
 
-@retry(stop=stop_after_attempt(8), wait=wait_fixed(15), reraise=True)
 async def is_connection_possible(ops_test: OpsTest, unit_name: str) -> bool:
     """Test a connection to a PostgreSQL server."""
-    app = unit_name.split("/")[0]
-    password = await get_password(ops_test, database_app_name=app, unit_name=unit_name)
-    address = await get_unit_address(ops_test, unit_name)
     try:
+        app = unit_name.split("/")[0]
         for attempt in Retrying(stop=stop_after_delay(60), wait=wait_fixed(3)):
             with attempt:
+                password = await asyncio.wait_for(
+                    get_password(ops_test, database_app_name=app), 15
+                )
+                address = await asyncio.wait_for(get_unit_address(ops_test, unit_name), 15)
+
                 with db_connect(
                     host=address, password=password
                 ) as connection, connection.cursor() as cursor:
                     cursor.execute("SELECT 1;")
                     success = cursor.fetchone()[0] == 1
                 connection.close()
-                return success
-    except (psycopg2.Error, RetryError):
+
+                if not success:
+                    raise Exception
+                return True
+    except RetryError:
         # Error raised when the connection is not possible.
         return False
 

tests/integration/ha_tests/test_self_healing.py

@@ -330,11 +330,15 @@ async def test_forceful_restart_without_data_and_transaction_logs(
 
         # Start the systemd service in the old primary.
         logger.info(f"starting database on {primary_name}")
-        await run_command_on_unit(ops_test, primary_name, "/charm/bin/pebble start postgresql")
+        for attempt in Retrying(stop=stop_after_delay(30), wait=wait_fixed(3), reraise=True):
+            with attempt:
+                await run_command_on_unit(
+                    ops_test, primary_name, "/charm/bin/pebble start postgresql"
+                )
 
-        # Verify that the database service got restarted and is ready in the old primary.
-        logger.info(f"waiting for the database service to restart on {primary_name}")
-        assert await is_postgresql_ready(ops_test, primary_name)
+                # Verify that the database service got restarted and is ready in the old primary.
+                logger.info(f"waiting for the database service to restart on {primary_name}")
+                assert await is_postgresql_ready(ops_test, primary_name)
 
     await is_cluster_updated(ops_test, primary_name)
 

tests/integration/helpers.py

@@ -672,10 +672,12 @@ async def run_command_on_unit(ops_test: OpsTest, unit_name: str, command: str) -
     return_code, stdout, stderr = await ops_test.juju(*complete_command.split())
     if return_code != 0:
         raise Exception(
-            "Expected command %s to succeed instead it failed: %s. Code: %s",
-            command,
-            stderr,
-            return_code,
+            "Expected command %s to succeed instead it failed: %s. Code: %s"
+            % (
+                command,
+                stderr,
+                return_code,
+            )
         )
     return stdout
 

tests/unit/test_postgresql.py

@@ -259,16 +259,16 @@ def test_build_postgresql_parameters(harness):
         "TimeZone": "UTC",
         "test_config_option_8": "partial",
         "test_config_option_9": 10.5,
-        "shared_buffers": "250MB",
-        "effective_cache_size": "750MB",
+        "shared_buffers": f"{250 * 128}",
+        "effective_cache_size": f"{750 * 128}",
     }
 
     # Test with a limited imposed to the available memory.
     parameters = harness.charm.postgresql.build_postgresql_parameters(
         config_options, 1000000000, 600000000
     )
-    assert parameters["shared_buffers"] == "150MB"
-    assert parameters["effective_cache_size"] == "450MB"
+    assert parameters["shared_buffers"] == f"{150 * 128}"
+    assert parameters["effective_cache_size"] == f"{450 * 128}"
 
     # Test when the requested shared buffers are greater than 40% of the available memory.
     config_options["memory_shared_buffers"] = 50001
@@ -285,7 +285,7 @@ def test_build_postgresql_parameters(harness):
     config_options["memory_shared_buffers"] = 50000
     parameters = harness.charm.postgresql.build_postgresql_parameters(config_options, 1000000000)
     assert parameters["shared_buffers"] == 50000
-    assert parameters["effective_cache_size"] == "600MB"
+    assert parameters["effective_cache_size"] == f"{600 * 128}"
 
     # Test when the profile is set to "testing".
     config_options["profile"] = "testing"
@@ -296,5 +296,5 @@ def test_build_postgresql_parameters(harness):
     # Test when there is no shared_buffers value set in the config option.
     del config_options["memory_shared_buffers"]
     parameters = harness.charm.postgresql.build_postgresql_parameters(config_options, 1000000000)
-    assert parameters["shared_buffers"] == "128MB"
+    assert "shared_buffers" not in parameters
     assert "effective_cache_size" not in parameters