[DPE-3591] Fix shared buffers validation

lib/charms/postgresql_k8s/v0/postgresql.py

@@ -35,7 +35,7 @@
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 22
+LIBPATCH = 23
 
 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles"
 
@@ -572,15 +572,20 @@ def build_postgresql_parameters(
             if parameter in ["date_style", "time_zone"]:
                 parameter = "".join(x.capitalize() for x in parameter.split("_"))
             parameters[parameter] = value
-        shared_buffers_max_value = int(int(available_memory * 0.4) / 10**6)
+        shared_buffers_max_value_in_mb = int(available_memory * 0.4 / 10**6)
+        shared_buffers_max_value = int(shared_buffers_max_value_in_mb * 10**3 / 8)
         if parameters.get("shared_buffers", 0) > shared_buffers_max_value:
             raise Exception(
-                f"Shared buffers config option should be at most 40% of the available memory, which is {shared_buffers_max_value}MB"
+                f"Shared buffers config option should be at most 40% of the available memory, which is {shared_buffers_max_value_in_mb}MB"
             )
         if profile == "production":
-            # Use 25% of the available memory for shared_buffers.
-            # and the remaining as cache memory.
-            shared_buffers = int(available_memory * 0.25)
+            if "shared_buffers" in parameters:
+                # Convert to bytes to use in the calculation.
+                shared_buffers = parameters["shared_buffers"] * 8 * 10**3
+            else:
+                # Use 25% of the available memory for shared_buffers.
+                # and the remaining as cache memory.
+                shared_buffers = int(available_memory * 0.25)
             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"})

src/charm.py

@@ -8,7 +8,6 @@
 import os
 import platform
 import subprocess
-import time
 from typing import Dict, List, Literal, Optional, Set, get_args
 
 from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit
@@ -44,7 +43,7 @@
     Unit,
     WaitingStatus,
 )
-from tenacity import RetryError, Retrying, retry, stop_after_delay, wait_fixed
+from tenacity import RetryError, Retrying, retry, stop_after_attempt, stop_after_delay, wait_fixed
 
 from backups import PostgreSQLBackups
 from cluster import (
@@ -1436,20 +1435,7 @@ def update_config(self, is_creating_backup: bool = False) -> bool:
             }
         )
 
-        restart_postgresql = self.is_tls_enabled != self.postgresql.is_tls_enabled()
-        self._patroni.reload_patroni_configuration()
-        # Sleep the same time as Patroni's loop_wait default value, which tells how much time
-        # Patroni will wait before checking the configuration file again to reload it.
-        time.sleep(10)
-        restart_postgresql = restart_postgresql or self.postgresql.is_restart_pending()
-        self.unit_peer_data.update({"tls": "enabled" if enable_tls else ""})
-
-        # Restart PostgreSQL if TLS configuration has changed
-        # (so the both old and new connections use the configuration).
-        if restart_postgresql:
-            logger.info("PostgreSQL restart required")
-            self._peers.data[self.unit].pop("postgresql_restarted", None)
-            self.on[self.restart_manager.name].acquire_lock.emit()
+        self._handle_postgresql_restart_need(enable_tls)
 
         # Restart the monitoring service if the password was rotated
         cache = snap.SnapCache()
@@ -1489,6 +1475,31 @@ def _validate_config_options(self) -> None:
         ):
             raise Exception("request_time_zone config option has an invalid value")
 
+    def _handle_postgresql_restart_need(self, enable_tls: bool) -> None:
+        """Handle PostgreSQL restart need based on the TLS configuration and configuration changes."""
+        restart_postgresql = self.is_tls_enabled != self.postgresql.is_tls_enabled()
+        self._patroni.reload_patroni_configuration()
+        # Wait for some more time than the Patroni's loop_wait default value (10 seconds),
+        # which tells how much time Patroni will wait before checking the configuration
+        # file again to reload it.
+        try:
+            for attempt in Retrying(stop=stop_after_attempt(5), wait=wait_fixed(3)):
+                with attempt:
+                    restart_postgresql = restart_postgresql or self.postgresql.is_restart_pending()
+                    if not restart_postgresql:
+                        raise Exception
+        except RetryError:
+            # Ignore the error, as it happens only to indicate that the configuration has not changed.
+            pass
+        self.unit_peer_data.update({"tls": "enabled" if enable_tls else ""})
+
+        # Restart PostgreSQL if TLS configuration has changed
+        # (so the both old and new connections use the configuration).
+        if restart_postgresql:
+            logger.info("PostgreSQL restart required")
+            self._peers.data[self.unit].pop("postgresql_restarted", None)
+            self.on[self.restart_manager.name].acquire_lock.emit()
+
     def _update_relation_endpoints(self) -> None:
         """Updates endpoints and read-only endpoint in all relations."""
         self.postgresql_client_relation.update_endpoints()

tests/unit/test_charm.py

@@ -24,7 +24,7 @@
 )
 from ops.testing import Harness
 from parameterized import parameterized
-from tenacity import RetryError
+from tenacity import RetryError, wait_fixed
 
 from charm import EXTENSIONS_DEPENDENCY_MESSAGE, NO_PRIMARY_MESSAGE, PostgresqlOperatorCharm
 from cluster import RemoveRaftMemberFailedError
@@ -1130,30 +1130,26 @@ def test_restart(self, _are_all_members_ready, _restart_postgresql):
         mock_event.defer.assert_not_called()
 
     @patch_network_get(private_address="1.1.1.1")
-    @patch("charm.time.sleep", return_value=None)
     @patch("subprocess.check_output", return_value=b"C")
     @patch("charm.snap.SnapCache")
-    @patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock")
-    @patch("charm.Patroni.reload_patroni_configuration")
+    @patch("charm.PostgresqlOperatorCharm._handle_postgresql_restart_need")
     @patch("charm.Patroni.bulk_update_parameters_controller_by_patroni")
     @patch("charm.PostgresqlOperatorCharm._validate_config_options")
     @patch("charm.Patroni.member_started", new_callable=PropertyMock)
     @patch("charm.PostgresqlOperatorCharm._is_workload_running", new_callable=PropertyMock)
     @patch("charm.Patroni.render_patroni_yml_file")
-    @patch("charms.postgresql_k8s.v0.postgresql_tls.PostgreSQLTLS.get_tls_files")
+    @patch("charm.PostgresqlOperatorCharm.is_tls_enabled", new_callable=PropertyMock)
     def test_update_config(
         self,
-        _get_tls_files,
+        _is_tls_enabled,
         _render_patroni_yml_file,
         _is_workload_running,
         _member_started,
         _,
         __,
-        _reload_patroni_configuration,
-        _restart,
+        _handle_postgresql_restart_need,
         ___,
         ____,
-        _____,
     ):
         with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
             # Mock some properties.
@@ -1180,10 +1176,9 @@ def test_update_config(
 
             # Test without TLS files available.
             self.harness.update_config(unset={"profile-limit-memory", "profile_limit_memory"})
-            self.harness.update_relation_data(
-                self.rel_id, self.charm.unit.name, {"tls": "enabled"}
-            )  # Mock some data in the relation to test that it change.
-            _get_tls_files.return_value = [None]
+            with self.harness.hooks_disabled():
+                self.harness.update_relation_data(self.rel_id, self.charm.unit.name, {"tls": ""})
+            _is_tls_enabled.return_value = False
             self.charm.update_config()
             _render_patroni_yml_file.assert_called_once_with(
                 connectivity=True,
@@ -1194,20 +1189,18 @@ def test_update_config(
                 restore_stanza=None,
                 parameters={"test": "test"},
             )
-            _reload_patroni_configuration.assert_called_once()
-            _restart.assert_called_once()
+            _handle_postgresql_restart_need.assert_called_once_with(False)
             self.assertNotIn(
                 "tls", self.harness.get_relation_data(self.rel_id, self.charm.unit.name)
             )
 
             # Test with TLS files available.
-            _restart.reset_mock()
+            _handle_postgresql_restart_need.reset_mock()
             self.harness.update_relation_data(
                 self.rel_id, self.charm.unit.name, {"tls": ""}
             )  # Mock some data in the relation to test that it change.
-            _get_tls_files.return_value = ["something"]
+            _is_tls_enabled.return_value = True
             _render_patroni_yml_file.reset_mock()
-            _reload_patroni_configuration.reset_mock()
             self.charm.update_config()
             _render_patroni_yml_file.assert_called_once_with(
                 connectivity=True,
@@ -1218,20 +1211,21 @@ def test_update_config(
                 restore_stanza=None,
                 parameters={"test": "test"},
             )
-            _reload_patroni_configuration.assert_called_once()
-            _restart.assert_called_once()
-            self.assertEqual(
-                self.harness.get_relation_data(self.rel_id, self.charm.unit.name)["tls"], "enabled"
+            _handle_postgresql_restart_need.assert_called_once()
+            self.assertNotIn(
+                "tls",
+                self.harness.get_relation_data(
+                    self.rel_id, self.charm.unit.name
+                ),  # The "tls" flag is set in handle_postgresql_restart_need.
             )
 
             # Test with workload not running yet.
             self.harness.update_relation_data(
                 self.rel_id, self.charm.unit.name, {"tls": ""}
             )  # Mock some data in the relation to test that it change.
-            _reload_patroni_configuration.reset_mock()
+            _handle_postgresql_restart_need.reset_mock()
             self.charm.update_config()
-            _reload_patroni_configuration.assert_not_called()
-            _restart.assert_called_once()
+            _handle_postgresql_restart_need.assert_not_called()
             self.assertEqual(
                 self.harness.get_relation_data(self.rel_id, self.charm.unit.name)["tls"], "enabled"
             )
@@ -1241,8 +1235,7 @@ def test_update_config(
                 self.rel_id, self.charm.unit.name, {"tls": ""}
             )  # Mock some data in the relation to test that it doesn't change.
             self.charm.update_config()
-            _reload_patroni_configuration.assert_not_called()
-            _restart.assert_called_once()
+            _handle_postgresql_restart_need.assert_not_called()
             self.assertNotIn(
                 "tls", self.harness.get_relation_data(self.rel_id, self.charm.unit.name)
             )
@@ -1936,3 +1929,54 @@ def test_migration_from_single_secret(self, scope, is_leader, _, __):
         assert SECRET_INTERNAL_LABEL not in self.harness.get_relation_data(
             self.rel_id, getattr(self.charm, scope).name
         )
+
+    @patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_acquire_lock")
+    @patch("charm.wait_fixed", return_value=wait_fixed(0))
+    @patch("charm.Patroni.reload_patroni_configuration")
+    @patch("charm.PostgresqlOperatorCharm._unit_ip")
+    @patch("charm.PostgresqlOperatorCharm.is_tls_enabled", new_callable=PropertyMock)
+    def test_handle_postgresql_restart_need(
+        self, _is_tls_enabled, _, _reload_patroni_configuration, __, _restart
+    ):
+        with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock:
+            for values in itertools.product(
+                [True, False], [True, False], [True, False], [True, False], [True, False]
+            ):
+                _reload_patroni_configuration.reset_mock()
+                _restart.reset_mock()
+                with self.harness.hooks_disabled():
+                    self.harness.update_relation_data(
+                        self.rel_id, self.charm.unit.name, {"tls": ""}
+                    )
+                    self.harness.update_relation_data(
+                        self.rel_id,
+                        self.charm.unit.name,
+                        {"postgresql_restarted": ("True" if values[4] else "")},
+                    )
+
+                _is_tls_enabled.return_value = values[1]
+                postgresql_mock.is_tls_enabled = PropertyMock(return_value=values[2])
+                postgresql_mock.is_restart_pending = PropertyMock(return_value=values[3])
+
+                self.charm._handle_postgresql_restart_need(values[0])
+                _reload_patroni_configuration.assert_called_once()
+                self.assertIn(
+                    "tls", self.harness.get_relation_data(self.rel_id, self.charm.unit)
+                ) if values[0] else self.assertNotIn(
+                    "tls", self.harness.get_relation_data(self.rel_id, self.charm.unit)
+                )
+                if (values[1] != values[2]) or values[3]:
+                    self.assertNotIn(
+                        "postgresql_restarted",
+                        self.harness.get_relation_data(self.rel_id, self.charm.unit),
+                    )
+                    _restart.assert_called_once()
+                else:
+                    self.assertIn(
+                        "postgresql_restarted",
+                        self.harness.get_relation_data(self.rel_id, self.charm.unit),
+                    ) if values[4] else self.assertNotIn(
+                        "postgresql_restarted",
+                        self.harness.get_relation_data(self.rel_id, self.charm.unit),
+                    )
+                    _restart.assert_not_called()