From efc7c8fe5a98f571ca8aeeaa5ad97b522b760184 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 19 Oct 2023 18:47:37 -0300 Subject: [PATCH] PR feedback (including the feedback from https://github.com/canonical/postgresql-k8s-operator/pull/281) --- config.yaml | 119 ++++++++++++++++++------- src/charm.py | 9 +- src/config.py | 4 +- tests/integration/test_charm.py | 12 +-- tests/integration/test_tls.py | 23 ----- tests/unit/test_charm.py | 7 +- tests/unit/test_db.py | 2 + tests/unit/test_postgresql_provider.py | 3 +- 8 files changed, 105 insertions(+), 74 deletions(-) diff --git a/config.yaml b/config.yaml index 59883d1955..781c213e62 100644 --- a/config.yaml +++ b/config.yaml @@ -2,75 +2,103 @@ # See LICENSE file for licensing details. options: - connection_ssl: - description: | - Enables SSL connections - type: boolean durability_synchronous_commit: description: | - Sets the current transactions synchronization level + Sets the current transactions synchronization level. This charm allows only the + “on”, “remote_apply” and “remote_write” values to avoid data loss if the primary + crashes and there are replicas. type: string + default: "on" instance_default_text_search_config: description: | Selects the text search configuration that is used by those variants of the text - search functions that do not have an explicit argument specifying it + search functions that do not have an explicit argument specifying it. + Allowed values start with “pg_catalog.” followed by a language name, like + “pg_catalog.english”. type: string + default: "pg_catalog.simple" instance_password_encryption: description: | - Determines the algorithm to use to encrypt the password + Determines the algorithm to use to encrypt the password. + Allowed values are: “md5” and “scram-sha-256”. type: string + default: "scram-sha-256" logging_log_connections: description: | - Logs each successful connection + Logs each successful connection. type: boolean + default: false logging_log_disconnections: description: | - Logs end of a session, including duration + Logs end of a session, including duration. type: boolean + default: false logging_log_lock_waits: description: | - Logs long lock waits + Logs long lock waits. type: boolean + default: false logging_log_min_duration_statement: description: | - Sets the minimum running time above which statements will be logged + Sets the minimum running time (milliseconds) above which statements will be logged. + Allowed values are: from -1 to 2147483647 (-1 disables logging + statement durations). type: int + default: -1 memory_maintenance_work_mem: description: | - Sets the maximum memory to be used for maintenance operations + Sets the maximum memory (KB) to be used for maintenance operations. + Allowed values are: from 1024 to 2147483647. type: int + default: 65536 memory_max_prepared_transactions: description: | - Sets the maximum number of simultaneously prepared transactions + Sets the maximum number of simultaneously prepared transactions. + Allowed values are: from 0 to 262143. type: int + default: 0 memory_shared_buffers: description: | - Sets the number of shared memory buffers used by the server + Sets the number of shared memory buffers (8 kB) used by the server. This charm allows + to set this value up to 40% of the available memory from the unit, as it is unlikely + that an allocation of more than that will work better than a smaller amount. + Allowed values are: from 16 to 1073741823. type: int memory_temp_buffers: description: | - Sets the maximum number of temporary buffers used by each session + Sets the maximum number of temporary buffers (8 kB) used by each session. + Allowed values are: from 100 to 1073741823. type: int + default: 1024 memory_work_mem: description: | - Sets the maximum memory to be used for query workspaces + Sets the maximum memory (KB) to be used for query workspaces. + Allowed values are: from 64 to 2147483647. type: int + default: 4096 optimizer_constraint_exclusion: description: | - Enables the planner to use constraints to optimize queries + Enables the planner to use constraints to optimize queries. + Allowed values are: “on”, “off” and “partition”. type: string + default: "partition" optimizer_default_statistics_target: description: | - Sets the default statistics target + Sets the default statistics target. Allowed values are: from 1 to 10000. type: int + default: 100 optimizer_from_collapse_limit: description: | - Sets the FROM-list size beyond which subqueries are not collapsed + Sets the FROM-list size beyond which subqueries are not collapsed. + Allowed values are: from 1 to 2147483647. type: int + default: 8 optimizer_join_collapse_limit: description: | - Sets the FROM-list size beyond which JOIN constructs are not flattened + Sets the FROM-list size beyond which JOIN constructs are not flattened. + Allowed values are: from 1 to 2147483647. type: int + default: 8 plugin_citext_enable: default: false type: boolean @@ -111,57 +139,82 @@ options: Only comes into effect when the `production` profile is selected. request_date_style: description: | - Sets the display format for date and time values + Sets the display format for date and time values. Allowed formats are explained + in https://www.postgresql.org/docs/14/runtime-config-client.html#GUC-DATESTYLE. type: string + default: "ISO, MDY" request_standard_conforming_strings: description: | - Causes ... strings to treat backslashes literally + Causes ... strings to treat backslashes literally. type: boolean + default: true request_time_zone: description: | - Sets the time zone for displaying and interpreting time stamps + Sets the time zone for displaying and interpreting time stamps. + Allowed values are the ones from IANA time zone data, a time zone abbreviation + like PST and POSIX-style time zone specifications. type: string + default: "UTC" response_bytea_output: description: | - Sets the output format for bytes + Sets the output format for bytes. + Allowed values are: “escape” and “hex”. type: string + default: "hex" response_lc_monetary: description: | - Sets the locale for formatting monetary amounts + Sets the locale for formatting monetary amounts. + Allowed values are the locales available in the unit. type: string + default: "C" response_lc_numeric: description: | - Sets the locale for formatting numbers + Sets the locale for formatting numbers. + Allowed values are the locales available in the unit. type: string + default: "C" response_lc_time: description: | - Sets the locale for formatting date and time values + Sets the locale for formatting date and time values. + Allowed values are the locales available in the unit. type: string + default: "C" vacuum_autovacuum_analyze_scale_factor: description: | Specifies a fraction of the table size to add to autovacuum_vacuum_threshold when - deciding whether to trigger a VACUUM + deciding whether to trigger a VACUUM. The default, 0.1, means 10% of table size. + Allowed values are: from 0 to 100. type: float + default: 0.1 vacuum_autovacuum_analyze_threshold: description: | Sets the minimum number of inserted, updated or deleted tuples needed to trigger - an ANALYZE in any one table. + an ANALYZE in any one table. Allowed values are: from 0 to 2147483647. type: int + default: 50 vacuum_autovacuum_freeze_max_age: description: | Maximum age (in transactions) before triggering autovacuum on a table to prevent - transaction ID wraparound + transaction ID wraparound. Allowed values are: from 100000 to 2000000000. type: int + default: 200000000 vacuum_autovacuum_vacuum_cost_delay: description: | - Sets cost delay value (milliseconds) that will be used in automatic VACUUM operations + Sets cost delay value (milliseconds) that will be used in automatic VACUUM operations. + Allowed values are: from -1 to 100 (-1 tells PostgreSQL to use the regular + vacuum_cost_delay value). type: float + default: 2.0 vacuum_autovacuum_vacuum_scale_factor: description: | Specifies a fraction of the table size to add to autovacuum_vacuum_threshold when - deciding whether to trigger a VACUUM + deciding whether to trigger a VACUUM. The default, 0.2, means 20% of table size. + Allowed values are: from 0 to 100. type: float + default: 0.2 vacuum_vacuum_freeze_table_age: description: | - Age at which VACUUM should scan whole table to freeze tuples + Age (in transactions) at which VACUUM should scan whole table to freeze tuples. + Allowed values are: from 0 to 2000000000. type: int + default: 150000000 diff --git a/src/charm.py b/src/charm.py index 773fa0ba91..87a4ea40e7 100755 --- a/src/charm.py +++ b/src/charm.py @@ -724,9 +724,7 @@ def is_primary(self) -> bool: @property def is_tls_enabled(self) -> bool: """Return whether TLS is enabled.""" - return all(self.tls.get_tls_files()) and ( - self.config.connection_ssl or self.config.connection_ssl is None - ) + return all(self.tls.get_tls_files()) @property def _peer_members_ips(self) -> Set[str]: @@ -1490,11 +1488,6 @@ def update_config(self, is_creating_backup: bool = False) -> bool: def _validate_config_options(self) -> None: """Validates specific config options that need access to the database or to the TLS status.""" - if self.config.connection_ssl and not self.is_tls_enabled: - raise Exception( - "connection_ssl config option should not be set to true when there is no configured TLS relation" - ) - if ( self.config.instance_default_text_search_config is not None and self.config.instance_default_text_search_config diff --git a/src/config.py b/src/config.py index a6382410b4..795357815d 100644 --- a/src/config.py +++ b/src/config.py @@ -16,7 +16,6 @@ class CharmConfig(BaseConfigModel): """Manager for the structured configuration.""" - connection_ssl: Optional[bool] durability_synchronous_commit: Optional[str] instance_default_text_search_config: Optional[str] instance_password_encryption: Optional[str] @@ -114,7 +113,7 @@ def memory_max_prepared_transactions_values(cls, value: int) -> Optional[int]: @classmethod def memory_shared_buffers_values(cls, value: int) -> Optional[int]: """Check memory_shared_buffers config option is greater or equal than 16.""" - if value < 16: + if value < 16 or value > 1073741823: raise ValueError("Shared buffers config option should be at least 16") return value @@ -202,6 +201,7 @@ def response_lc_values(cls, value: str) -> Optional[str]: """Check if the requested locale is available in the system.""" output = subprocess.check_output(["ls", "/snap/charmed-postgresql/current/usr/lib/locale"]) locales = [locale.decode() for locale in output.splitlines()] + locales.append("C") if value not in locales: raise ValueError("Value not one of the locales available in the system") diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 356fe16e4e..935051c306 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -126,12 +126,12 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): # Validate each configuration set by Patroni on PostgreSQL. assert settings["archive_command"] == "/bin/true" assert settings["archive_mode"] == "on" - assert settings["autovacuum"] + assert settings["autovacuum"] == "on" assert settings["cluster_name"] == DATABASE_APP_NAME assert settings["data_directory"] == f"{STORAGE_PATH}/var/lib/postgresql" assert settings["data_checksums"] == "on" - assert settings["fsync"] - assert settings["full_page_writes"] + assert settings["fsync"] == "on" + assert settings["full_page_writes"] == "on" assert settings["lc_messages"] == "en_US.UTF8" assert settings["listen_addresses"] == host assert settings["log_autovacuum_min_duration"] == "60000" @@ -147,9 +147,9 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): settings = result.json() # Validate each configuration related to Patroni - assert settings["postgresql"]["use_pg_rewind"] - assert settings["postgresql"]["remove_data_directory_on_rewind_failure"] - assert settings["postgresql"]["remove_data_directory_on_diverged_timelines"] + assert settings["postgresql"]["use_pg_rewind"] is True + assert settings["postgresql"]["remove_data_directory_on_rewind_failure"] is True + assert settings["postgresql"]["remove_data_directory_on_diverged_timelines"] is True assert settings["loop_wait"] == 10 assert settings["retry_timeout"] == 10 assert settings["maximum_lag_on_failover"] == 1048576 diff --git a/tests/integration/test_tls.py b/tests/integration/test_tls.py index 2da3f5b3e2..7146d8a43a 100644 --- a/tests/integration/test_tls.py +++ b/tests/integration/test_tls.py @@ -140,30 +140,7 @@ async def test_tls_enabled(ops_test: OpsTest) -> None: "grep 'connection authorized: user=rewind database=postgres SSL enabled' /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log", ) - # Disable TLS. - logger.info("disabling TLS through connection_ssl config option") - await ops_test.model.applications[DATABASE_APP_NAME].set_config( - {"connection_ssl": "False"} - ) - await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") - for unit in ops_test.model.applications[DATABASE_APP_NAME].units: - assert await check_tls(ops_test, unit.name, enabled=False) - assert await check_tls_patroni_api(ops_test, unit.name, enabled=False) - - # Re-enable TLS. - logger.info("re-enabling TLS through connection_ssl config option") - await ops_test.model.applications[DATABASE_APP_NAME].set_config({"connection_ssl": "True"}) - await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") - for unit in ops_test.model.applications[DATABASE_APP_NAME].units: - assert await check_tls(ops_test, unit.name, enabled=True) - assert await check_tls_patroni_api(ops_test, unit.name, enabled=True) - # Remove the relation. - logger.info("removing the TLS relation") - await ops_test.model.applications[DATABASE_APP_NAME].set_config( - {"connection_ssl": "False"} - ) - await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( f"{DATABASE_APP_NAME}:certificates", f"{TLS_CERTIFICATES_APP_NAME}:certificates" ) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index c83223103a..5344dd9ce4 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -250,7 +250,8 @@ def test_on_config_changed( _enable_disable_extensions.assert_called_once() _set_up_relation.assert_called_once() - def test_enable_disable_extensions(self): + @patch("subprocess.check_output", return_value=b"C") + def test_enable_disable_extensions(self, _): with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: # Test when all extensions install/uninstall succeed. postgresql_mock.enable_disable_extension.side_effect = None @@ -457,6 +458,7 @@ def test_on_start_replica( self.assertTrue(isinstance(self.harness.model.unit.status, WaitingStatus)) @patch_network_get(private_address="1.1.1.1") + @patch("subprocess.check_output", return_value=b"C") @patch("charm.snap.SnapCache") @patch("charm.PostgresqlOperatorCharm.postgresql") @patch("charm.Patroni") @@ -471,6 +473,7 @@ def test_on_start_no_patroni_member( patroni, _postgresql, _snap_cache, + _, ): # Mock the passwords. patroni.return_value.member_started = False @@ -1055,6 +1058,7 @@ 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("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") @@ -1075,6 +1079,7 @@ def test_update_config( _reload_patroni_configuration, _restart, ___, + ____, ): with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: # Mock some properties. diff --git a/tests/unit/test_db.py b/tests/unit/test_db.py index 5bcef37e95..6fe44a9b88 100644 --- a/tests/unit/test_db.py +++ b/tests/unit/test_db.py @@ -182,6 +182,7 @@ def test_get_extensions(self): ([extensions[1], extensions[2]], {extensions[2]}), ) + @patch("subprocess.check_output", return_value=b"C") @patch("relations.db.DbProvides._update_unit_status") @patch("relations.db.DbProvides.update_endpoints") @patch("charm.PostgresqlOperatorCharm.enable_disable_extensions") @@ -194,6 +195,7 @@ def test_set_up_relation( _enable_disable_extensions, _update_endpoints, _update_unit_status, + _, ): with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: # Define some mocks' side effects. diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index 28d20093c3..ea9dd0d796 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -73,6 +73,7 @@ def request_database(self): {"database": DATABASE, "extra-user-roles": EXTRA_USER_ROLES}, ) + @patch("subprocess.check_output", return_value=b"C") @patch("charm.PostgreSQLProvider.update_endpoints") @patch("relations.postgresql_provider.new_password", return_value="test-password") @patch.object(EventBase, "defer") @@ -82,7 +83,7 @@ def request_database(self): ) @patch("charm.Patroni.member_started", new_callable=PropertyMock) def test_on_database_requested( - self, _member_started, _primary_endpoint, _defer, _new_password, _update_endpoints + self, _member_started, _primary_endpoint, _defer, _new_password, _update_endpoints, _ ): with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: # Set some side effects to test multiple situations.