From e4a76cdb079c1787d4b16bf3c8a5168c0a27f3e7 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 21 Aug 2024 16:01:24 +0300 Subject: [PATCH 01/14] Provide socket in URI --- src/charm.py | 8 +++++++- src/constants.py | 3 ++- src/relations/pgbouncer_provider.py | 16 +++++++--------- tests/unit/test_charm.py | 5 +++-- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/charm.py b/src/charm.py index 00585197d..6f72276c2 100755 --- a/src/charm.py +++ b/src/charm.py @@ -50,6 +50,7 @@ PGB, PGB_CONF_DIR, PGB_LOG_DIR, + PGB_RUN_DIR, PGBOUNCER_EXECUTABLE, PGBOUNCER_SNAP_NAME, SECRET_DELETED_LABEL, @@ -165,11 +166,14 @@ def create_instance_directories(self): """Create configuration directories for pgbouncer instances.""" pg_user = pwd.getpwnam(PG_USER) app_conf_dir = f"{PGB_CONF_DIR}/{self.app.name}" + app_run_dir = f"{PGB_RUN_DIR}/{self.app.name}" # Make a directory for each service to store configs. for service_id in self.service_ids: os.makedirs(f"{app_conf_dir}/{INSTANCE_DIR}{service_id}", 0o700, exist_ok=True) os.chown(f"{app_conf_dir}/{INSTANCE_DIR}{service_id}", pg_user.pw_uid, pg_user.pw_gid) + os.makedirs(f"{app_run_dir}/{INSTANCE_DIR}{service_id}", 0o777, exist_ok=True) + os.chown(f"{app_run_dir}/{INSTANCE_DIR}{service_id}", pg_user.pw_uid, pg_user.pw_gid) @property def tracing_endpoint(self) -> Optional[str]: @@ -259,6 +263,7 @@ def _on_remove(self, _) -> None: shutil.rmtree(f"{PGB_CONF_DIR}/{self.app.name}") shutil.rmtree(f"{PGB_LOG_DIR}/{self.app.name}") + shutil.rmtree(f"{PGB_RUN_DIR}/{self.app.name}") shutil.rmtree(f"{SNAP_TMP_DIR}/{self.app.name}") systemd.daemon_reload() @@ -766,6 +771,7 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): # PgbConfig objects, and is modified below to implement individual services. app_conf_dir = f"{PGB_CONF_DIR}/{self.app.name}" app_log_dir = f"{PGB_LOG_DIR}/{self.app.name}" + app_run_dir = f"{PGB_RUN_DIR}/{self.app.name}" app_temp_dir = f"/tmp/{self.app.name}" max_db_connections = self.config["max_db_connections"] @@ -797,7 +803,7 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): databases=databases, readonly_databases=readonly_dbs, peer_id=service_id, - base_socket_dir=f"{app_temp_dir}/{INSTANCE_DIR}", + base_socket_dir=f"{app_run_dir}/{INSTANCE_DIR}", peers=self.service_ids, log_file=f"{app_log_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.log", pid_file=f"{app_temp_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.pid", diff --git a/src/constants.py b/src/constants.py index aeb82b6b6..96d0c4aaa 100644 --- a/src/constants.py +++ b/src/constants.py @@ -15,7 +15,7 @@ SNAP_PACKAGES = [ ( PGBOUNCER_SNAP_NAME, - {"revision": {"aarch64": "4", "x86_64": "3"}, "channel": "1/stable"}, + {"revision": {"aarch64": "13", "x86_64": "14"}, "channel": "1/stable"}, ) ] @@ -23,6 +23,7 @@ SNAP_CURRENT_PATH = f"/var/snap/{PGBOUNCER_SNAP_NAME}/current" PGB_CONF_DIR = f"{SNAP_CURRENT_PATH}/etc/pgbouncer" +PGB_RUN_DIR = f"{SNAP_CURRENT_PATH}/run/pgbouncer" PGB_LOG_DIR = f"{SNAP_COMMON_PATH}/var/log/pgbouncer" SNAP_TMP_DIR = f"/tmp/snap-private-tmp/snap.{PGBOUNCER_SNAP_NAME}/tmp" diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index d7f5874da..67f009e32 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -50,19 +50,17 @@ PostgreSQLDeleteUserError, PostgreSQLGetPostgreSQLVersionError, ) -from ops.charm import ( - CharmBase, - RelationBrokenEvent, - RelationDepartedEvent, -) -from ops.framework import Object -from ops.model import ( +from ops import ( Application, BlockedStatus, + CharmBase, MaintenanceStatus, + Object, + RelationBrokenEvent, + RelationDepartedEvent, ) -from constants import CLIENT_RELATION_NAME +from constants import CLIENT_RELATION_NAME, PGB_RUN_DIR logger = logging.getLogger(__name__) @@ -242,7 +240,7 @@ def update_connection_info(self, relation): password = self.database_provides.fetch_my_relation_field(relation.id, "password") host = "localhost" - uri_host = host + uri_host = f"{PGB_RUN_DIR}/{self.charm.app.name}/instance_0" if exposed: if vip := self.charm.config.get("vip"): host = vip diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 022f8241f..66b3c7e29 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -30,6 +30,7 @@ PEER_RELATION_NAME, PGB_CONF_DIR, PGB_LOG_DIR, + PGB_RUN_DIR, SECRET_INTERNAL_LABEL, SNAP_PACKAGES, ) @@ -396,7 +397,7 @@ def test_render_pgb_config( readonly_databases={}, peer_id=0, peers=range(1), - base_socket_dir="/tmp/pgbouncer/instance_", + base_socket_dir=f"{PGB_RUN_DIR}/pgbouncer/instance_", log_file=f"{PGB_LOG_DIR}/pgbouncer/instance_0/pgbouncer.log", pid_file="/tmp/pgbouncer/instance_0/pgbouncer.pid", listen_addr="127.0.0.1", @@ -442,7 +443,7 @@ def test_render_pgb_config( readonly_databases={}, peer_id=0, peers=range(1), - base_socket_dir="/tmp/pgbouncer/instance_", + base_socket_dir=f"{PGB_RUN_DIR}/pgbouncer/instance_", log_file=f"{PGB_LOG_DIR}/pgbouncer/instance_0/pgbouncer.log", pid_file="/tmp/pgbouncer/instance_0/pgbouncer.pid", listen_addr="127.0.0.1", From c7467514f0895c86754a07554265f76aefc77bea Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 13:31:04 +0300 Subject: [PATCH 02/14] Add config switch for socket connectivity --- config.yaml | 6 ++++++ src/charm.py | 3 +-- src/relations/pgbouncer_provider.py | 8 ++++++-- .../pgbouncer_provider/test_pgbouncer_provider.py | 1 + 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/config.yaml b/config.yaml index 1eb6ba8a9..9d4ebaf10 100644 --- a/config.yaml +++ b/config.yaml @@ -21,6 +21,12 @@ options: Virtual IP to use to front pgbouncer units. Used only in case of external node connection. type: string + local_connection_type: + default: localhost + description: | + Type of local connectivity to provide. Must be either 'localhost' or 'socket'. Only applies to the database interface. + type: string + pool_mode: default: session description: | diff --git a/src/charm.py b/src/charm.py index 6f72276c2..11284b4ce 100755 --- a/src/charm.py +++ b/src/charm.py @@ -579,8 +579,7 @@ def _on_config_changed(self, event) -> None: if self.backend.postgres: self.render_prometheus_service() - if port_changed or vip_changed: - self.update_client_connection_info() + self.update_client_connection_info() def check_pgb_running(self): """Checks that pgbouncer service is running, and updates status accordingly.""" diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index 880608eed..aa8de26b7 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -239,8 +239,6 @@ def update_connection_info(self, relation): user = f"relation_id_{relation.id}" password = self.database_provides.fetch_my_relation_field(relation.id, "password") - host = "localhost" - uri_host = f"{PGB_RUN_DIR}/{self.charm.app.name}/instance_0" if exposed: if vip := self.charm.config.get("vip"): host = vip @@ -251,6 +249,12 @@ def update_connection_info(self, relation): self.charm.leader_ip, *[ip for ip in self.charm.peers.units_ips if ip != self.charm.leader_ip], ]) + elif self.charm.config["local_connection_type"] == "socket": + host = f"{PGB_RUN_DIR}/{self.charm.app.name}/instance_0" + uri_host = host + else: + host = "localhost" + uri_host = host port = self.charm.config["listen_port"] initial_status = self.charm.unit.status diff --git a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py index 9ec13e67e..e998b53b3 100644 --- a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py +++ b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py @@ -61,6 +61,7 @@ async def test_database_relation_with_charm_libraries(ops_test: OpsTest, pgb_cha pgb_charm_jammy, application_name=PGB, num_units=None, + config={"local_connection_type": "socket"}, ), ops_test.model.deploy( PG, From 1d34d88843c13666f1da92081e3222e4b5748f1c Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 15:24:58 +0300 Subject: [PATCH 03/14] Switch to charm config lib --- .../data_platform_libs/v0/data_models.py | 354 ++++++++++++++++++ src/charm.py | 47 +-- src/config.py | 24 ++ src/relations/db.py | 4 +- src/relations/hacluster.py | 2 +- src/relations/pgbouncer_provider.py | 14 +- tests/unit/relations/test_hacluster.py | 5 +- 7 files changed, 415 insertions(+), 35 deletions(-) create mode 100644 lib/charms/data_platform_libs/v0/data_models.py create mode 100644 src/config.py diff --git a/lib/charms/data_platform_libs/v0/data_models.py b/lib/charms/data_platform_libs/v0/data_models.py new file mode 100644 index 000000000..a1dbb8299 --- /dev/null +++ b/lib/charms/data_platform_libs/v0/data_models.py @@ -0,0 +1,354 @@ +# Copyright 2023 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +r"""Library to provide simple API for promoting typed, validated and structured dataclass in charms. + +Dict-like data structure are often used in charms. They are used for config, action parameters +and databag. This library aims at providing simple API for using pydantic BaseModel-derived class +in charms, in order to enhance: +* Validation, by embedding custom business logic to validate single parameters or even have + validators that acts across different fields +* Parsing, by loading data into pydantic object we can both allow for other types (e.g. float) to + be used in configuration/parameters as well as specify even nested complex objects for databags +* Static typing checks, by moving from dict-like object to classes with typed-annotated properties, + that can be statically checked using mypy to ensure that the code is correct. + +Pydantic models can be used on: + +* Charm Configuration (as defined in config.yaml) +* Actions parameters (as defined in actions.yaml) +* Application/Unit Databag Information (thus making it more structured and encoded) + + +## Creating models + +Any data-structure can be modeled using dataclasses instead of dict-like objects (e.g. storing +config, action parameters and databags). Within pydantic, we can define dataclasses that provides +also parsing and validation on standard dataclass implementation: + +```python + +from charms.data_platform_libs.v0.data_models import BaseConfigModel + +class MyConfig(BaseConfigModel): + + my_key: int + + @validator("my_key") + def is_lower_than_100(cls, v: int): + if v > 100: + raise ValueError("Too high") + +``` + +This should allow to collapse both parsing and validation as the dataclass object is parsed and +created: + +```python +dataclass = MyConfig(my_key="1") + +dataclass.my_key # this returns 1 (int) +dataclass["my_key"] # this returns 1 (int) + +dataclass = MyConfig(my_key="102") # this returns a ValueError("Too High") +``` + +## Charm Configuration Model + +Using the class above, we can implement parsing and validation of configuration by simply +extending our charms using the `TypedCharmBase` class, as shown below. + +```python +class MyCharm(TypedCharmBase[MyConfig]): + config_type = MyConfig + + # everywhere in the code you will have config property already parsed and validate + def my_method(self): + self.config: MyConfig +``` + +## Action parameters + +In order to parse action parameters, we can use a decorator to be applied to action event +callbacks, as shown below. + +```python +@validate_params(PullActionModel) +def _pull_site_action( + self, event: ActionEvent, + params: Optional[Union[PullActionModel, ValidationError]] = None +): + if isinstance(params, ValidationError): + # handle errors + else: + # do stuff +``` + +Note that this changes the signature of the callbacks by adding an extra parameter with the parsed +counterpart of the `event.params` dict-like field. If validation fails, we return (not throw!) the +exception, to be handled (or raised) in the callback. + +## Databag + +In order to parse databag fields, we define a decorator to be applied to base relation event +callbacks. + +```python +@parse_relation_data(app_model=AppDataModel, unit_model=UnitDataModel) +def _on_cluster_relation_joined( + self, event: RelationEvent, + app_data: Optional[Union[AppDataModel, ValidationError]] = None, + unit_data: Optional[Union[UnitDataModel, ValidationError]] = None +) -> None: + ... +``` + +The parameters `app_data` and `unit_data` refers to the databag of the entity which fired the +RelationEvent. + +When we want to access to a relation databag outsides of an action, it can be useful also to +compact multiple databags into a single object (if there are no conflicting fields), e.g. + +```python + +class ProviderDataBag(BaseClass): + provider_key: str + +class RequirerDataBag(BaseClass): + requirer_key: str + +class MergedDataBag(ProviderDataBag, RequirerDataBag): + pass + +merged_data = get_relation_data_as( + MergedDataBag, relation.data[self.app], relation.data[relation.app] +) + +merged_data.requirer_key +merged_data.provider_key + +``` + +The above code can be generalized to other kinds of merged objects, e.g. application and unit, and +it can be extended to multiple sources beyond 2: + +```python +merged_data = get_relation_data_as( + MergedDataBag, relation.data[self.app], relation.data[relation.app], ... +) +``` + +""" + +import json +from functools import reduce, wraps +from typing import Callable, Generic, MutableMapping, Optional, Type, TypeVar, Union + +import pydantic +from ops.charm import ActionEvent, CharmBase, RelationEvent +from ops.model import RelationDataContent +from pydantic import BaseModel, ValidationError + +# The unique Charmhub library identifier, never change it +LIBID = "cb2094c5b07d47e1bf346aaee0fcfcfe" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 4 + +PYDEPS = ["ops>=2.0.0", "pydantic>=1.10,<2"] + +G = TypeVar("G") +T = TypeVar("T", bound=BaseModel) +AppModel = TypeVar("AppModel", bound=BaseModel) +UnitModel = TypeVar("UnitModel", bound=BaseModel) + +DataBagNativeTypes = (int, str, float) + + +class BaseConfigModel(BaseModel): + """Class to be used for defining the structured configuration options.""" + + def __getitem__(self, x): + """Return the item using the notation instance[key].""" + return getattr(self, x.replace("-", "_")) + + +class TypedCharmBase(CharmBase, Generic[T]): + """Class to be used for extending config-typed charms.""" + + config_type: Type[T] + + @property + def config(self) -> T: + """Return a config instance validated and parsed using the provided pydantic class.""" + translated_keys = {k.replace("-", "_"): v for k, v in self.model.config.items()} + return self.config_type(**translated_keys) + + +def validate_params(cls: Type[T]): + """Return a decorator to allow pydantic parsing of action parameters. + + Args: + cls: Pydantic class representing the model to be used for parsing the content of the + action parameter + """ + + def decorator( + f: Callable[[CharmBase, ActionEvent, Union[T, ValidationError]], G] + ) -> Callable[[CharmBase, ActionEvent], G]: + @wraps(f) + def event_wrapper(self: CharmBase, event: ActionEvent): + try: + params = cls( + **{key.replace("-", "_"): value for key, value in event.params.items()} + ) + except ValidationError as e: + params = e + return f(self, event, params) + + return event_wrapper + + return decorator + + +def write(relation_data: RelationDataContent, model: BaseModel): + """Write the data contained in a domain object to the relation databag. + + Args: + relation_data: pointer to the relation databag + model: instance of pydantic model to be written + """ + for key, value in model.dict(exclude_none=False).items(): + if value: + relation_data[key.replace("_", "-")] = ( + str(value) + if any(isinstance(value, _type) for _type in DataBagNativeTypes) + else json.dumps(value) + ) + else: + relation_data[key.replace("_", "-")] = "" + + +def read(relation_data: MutableMapping[str, str], obj: Type[T]) -> T: + """Read data from a relation databag and parse it into a domain object. + + Args: + relation_data: pointer to the relation databag + obj: pydantic class representing the model to be used for parsing + """ + return obj( + **{ + field_name: ( + relation_data[parsed_key] + if field.outer_type_ in DataBagNativeTypes + else json.loads(relation_data[parsed_key]) + ) + for field_name, field in obj.__fields__.items() + # pyright: ignore[reportGeneralTypeIssues] + if (parsed_key := field_name.replace("_", "-")) in relation_data + if relation_data[parsed_key] + } + ) + + +def parse_relation_data( + app_model: Optional[Type[AppModel]] = None, unit_model: Optional[Type[UnitModel]] = None +): + """Return a decorator to allow pydantic parsing of the app and unit databags. + + Args: + app_model: Pydantic class representing the model to be used for parsing the content of the + app databag. None if no parsing ought to be done. + unit_model: Pydantic class representing the model to be used for parsing the content of the + unit databag. None if no parsing ought to be done. + """ + + def decorator( + f: Callable[ + [ + CharmBase, + RelationEvent, + Optional[Union[AppModel, ValidationError]], + Optional[Union[UnitModel, ValidationError]], + ], + G, + ] + ) -> Callable[[CharmBase, RelationEvent], G]: + @wraps(f) + def event_wrapper(self: CharmBase, event: RelationEvent): + try: + app_data = ( + read(event.relation.data[event.app], app_model) + if app_model is not None and event.app + else None + ) + except pydantic.ValidationError as e: + app_data = e + + try: + unit_data = ( + read(event.relation.data[event.unit], unit_model) + if unit_model is not None and event.unit + else None + ) + except pydantic.ValidationError as e: + unit_data = e + + return f(self, event, app_data, unit_data) + + return event_wrapper + + return decorator + + +class RelationDataModel(BaseModel): + """Base class to be used for creating data models to be used for relation databags.""" + + def write(self, relation_data: RelationDataContent): + """Write data to a relation databag. + + Args: + relation_data: pointer to the relation databag + """ + return write(relation_data, self) + + @classmethod + def read(cls, relation_data: RelationDataContent) -> "RelationDataModel": + """Read data from a relation databag and parse it as an instance of the pydantic class. + + Args: + relation_data: pointer to the relation databag + """ + return read(relation_data, cls) + + +def get_relation_data_as( + model_type: Type[AppModel], + *relation_data: RelationDataContent, +) -> Union[AppModel, ValidationError]: + """Return a merged representation of the provider and requirer databag into a single object. + + Args: + model_type: pydantic class representing the merged databag + relation_data: list of RelationDataContent of provider/requirer/unit sides + """ + try: + app_data = read(reduce(lambda x, y: dict(x) | dict(y), relation_data, {}), model_type) + except pydantic.ValidationError as e: + app_data = e + return app_data diff --git a/src/charm.py b/src/charm.py index 11284b4ce..1082147f7 100755 --- a/src/charm.py +++ b/src/charm.py @@ -17,6 +17,7 @@ import psycopg2 from charms.data_platform_libs.v0.data_interfaces import DataPeerData, DataPeerUnitData +from charms.data_platform_libs.v0.data_models import TypedCharmBase from charms.grafana_agent.v0.cos_agent import COSAgentProvider from charms.operator_libs_linux.v1 import systemd from charms.operator_libs_linux.v2 import snap @@ -27,7 +28,6 @@ from ops import ( ActiveStatus, BlockedStatus, - CharmBase, JujuVersion, MaintenanceStatus, ModelError, @@ -37,6 +37,7 @@ ) from ops.main import main +from config import CharmConfig from constants import ( APP_SCOPE, AUTH_FILE_DATABAG_KEY, @@ -91,9 +92,11 @@ PostgreSQLTLS, ), ) -class PgBouncerCharm(CharmBase): +class PgBouncerCharm(TypedCharmBase): """A class implementing charmed PgBouncer.""" + config_type = CharmConfig + def __init__(self, *args): super().__init__(*args) @@ -133,7 +136,7 @@ def __init__(self, *args): self._grafana_agent = COSAgentProvider( self, metrics_endpoints=[ - {"path": "/metrics", "port": self.config["metrics_port"]}, + {"path": "/metrics", "port": self.config.metrics_port}, ], log_slots=[f"{PGBOUNCER_SNAP_NAME}:logs"], refresh_events=[self.on.config_changed], @@ -519,18 +522,17 @@ def update_status(self): self.unit.status = BlockedStatus("ha integration used without data-intgrator") return - vip = self.config.get("vip") - if self.hacluster.relation and not vip: + if self.hacluster.relation and not self.config.vip: self.unit.status = BlockedStatus("ha integration used without vip configuration") return - if vip and not self._is_exposed: + if self.config.vip and not self._is_exposed: self.unit.status = BlockedStatus("vip configuration without data-intgrator") return if self.check_pgb_running(): - if self.unit.is_leader() and vip: - self.unit.status = ActiveStatus(f"VIP: {vip}") + if self.unit.is_leader() and self.config.vip: + self.unit.status = ActiveStatus(f"VIP: {self.config.vip}") else: self.unit.status = ActiveStatus() @@ -546,22 +548,22 @@ def _on_config_changed(self, event) -> None: return old_vip = self.peers.app_databag.get("current_vip", "") - vip = self.config.get("vip", "") + vip = self.config.vip if self.config.vip else "" vip_changed = old_vip != vip if vip_changed and self._is_exposed: - self.hacluster.set_vip(self.config.get("vip")) + self.hacluster.set_vip(self.config.vip) old_port = self.peers.app_databag.get("current_port") - port_changed = old_port != str(self.config["listen_port"]) + port_changed = old_port != str(self.config.listen_port) if port_changed and self._is_exposed: # Open port try: - self.unit.set_ports(self.config["listen_port"]) + self.unit.set_ports(self.config.listen_port) except ModelError: logger.exception("failed to open port") if self.unit.is_leader(): - self.peers.app_databag["current_port"] = str(self.config["listen_port"]) + self.peers.app_databag["current_port"] = str(self.config.listen_port) if vip: self.peers.app_databag["current_vip"] = str(vip) else: @@ -759,7 +761,7 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): if self._is_exposed: # Open port try: - self.unit.open_port("tcp", self.config["listen_port"]) + self.unit.open_port("tcp", self.configlisten_port) except ModelError: logger.exception("failed to open port") @@ -773,13 +775,12 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): app_run_dir = f"{PGB_RUN_DIR}/{self.app.name}" app_temp_dir = f"/tmp/{self.app.name}" - max_db_connections = self.config["max_db_connections"] - if max_db_connections == 0: + if self.config.max_db_connections == 0: default_pool_size = 20 min_pool_size = 10 reserve_pool_size = 10 else: - effective_db_connections = max_db_connections / self.instances_count + effective_db_connections = self.config.max_db_connections / self.instances_count default_pool_size = math.ceil(effective_db_connections / 2) min_pool_size = math.ceil(effective_db_connections / 4) reserve_pool_size = math.ceil(effective_db_connections / 4) @@ -807,9 +808,9 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): log_file=f"{app_log_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.log", pid_file=f"{app_temp_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.pid", listen_addr=addr, - listen_port=self.config["listen_port"], - pool_mode=self.config["pool_mode"], - max_db_connections=max_db_connections, + listen_port=self.config.listen_port, + pool_mode=self.config.pool_mode, + max_db_connections=self.config.max_db_connections, default_pool_size=default_pool_size, min_pool_size=min_pool_size, reserve_pool_size=reserve_pool_size, @@ -840,8 +841,8 @@ def render_prometheus_service(self): stats_user=self.backend.stats_user, pgb_service=f"{PGB}-{self.app.name}", stats_password=self.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), - listen_port=self.config["listen_port"], - metrics_port=self.config["metrics_port"], + listen_port=self.config.listen_port, + metrics_port=self.config.metrics_port, ) service = f"{PGB}-{self.app.name}-prometheus" @@ -952,7 +953,7 @@ def update_client_connection_info(self): if not self.backend.postgres or not self.unit.is_leader(): return - port = self.config["listen_port"] + port = self.config.listen_port for relation in self.model.relations.get("db", []): self.legacy_db_relation.update_connection_info(relation, port) diff --git a/src/config.py b/src/config.py new file mode 100644 index 000000000..c7cd8ba36 --- /dev/null +++ b/src/config.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Structured configuration for the PostgreSQL charm.""" + +import logging +from typing import Literal, Optional + +from charms.data_platform_libs.v0.data_models import BaseConfigModel +from pydantic import IPvAnyAddress, PositiveInt + +logger = logging.getLogger(__name__) + + +class CharmConfig(BaseConfigModel): + """Manager for the structured configuration.""" + + listen_port: PositiveInt + metrics_port: PositiveInt + vip: Optional[IPvAnyAddress] + local_connection_type: Literal["localhost", "socket"] + pool_mode: Literal["session", "transaction", "statement"] + max_db_connections: int diff --git a/src/relations/db.py b/src/relations/db.py index a1229af29..4b9a6010b 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -324,12 +324,12 @@ def _on_relation_changed(self, change_event: RelationChangedEvent): "state": "master", }, ) - self.update_connection_info(change_event.relation, self.charm.config["listen_port"]) + self.update_connection_info(change_event.relation, self.charm.config.listen_port) def update_connection_info(self, relation: Relation, port: str = None): """Updates databag connection information.""" if not port: - port = self.charm.config["listen_port"] + port = self.charm.config.listen_port databag = self.get_databags(relation)[0] database = databag.get("database") diff --git a/src/relations/hacluster.py b/src/relations/hacluster.py index 914cb0461..4cdbe4ae2 100644 --- a/src/relations/hacluster.py +++ b/src/relations/hacluster.py @@ -41,7 +41,7 @@ def _is_clustered(self) -> bool: return False def _on_changed(self, event: RelationChangedEvent) -> None: - self.set_vip(self.charm.config.get("vip")) + self.set_vip(self.charm.config.vip) def set_vip(self, vip: Optional[str]) -> None: """Adds the requested virtual IP to the integration.""" diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index aa8de26b7..d2bc3a03a 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -240,22 +240,22 @@ def update_connection_info(self, relation): password = self.database_provides.fetch_my_relation_field(relation.id, "password") if exposed: - if vip := self.charm.config.get("vip"): - host = vip - uri_host = vip + if self.charm.config.vip: + host = self.charm.config.vip + uri_host = self.charm.config.vip else: host = self.charm.leader_ip uri_host = ",".join([ self.charm.leader_ip, *[ip for ip in self.charm.peers.units_ips if ip != self.charm.leader_ip], ]) - elif self.charm.config["local_connection_type"] == "socket": + elif self.charm.config.local_connection_type == "socket": host = f"{PGB_RUN_DIR}/{self.charm.app.name}/instance_0" uri_host = host else: host = "localhost" uri_host = host - port = self.charm.config["listen_port"] + port = self.charm.config.listen_port initial_status = self.charm.unit.status self.charm.unit.status = MaintenanceStatus( @@ -290,14 +290,14 @@ def set_ready(self) -> None: def update_read_only_endpoints(self, event: DatabaseRequestedEvent = None) -> None: """Set the read-only endpoint only if there are replicas.""" - if not self.charm.unit.is_leader() or self.charm.config.get("vip"): + if not self.charm.unit.is_leader() or self.charm.config.vip: return # Get the current relation or all the relations if this is triggered by another type of # event. relations = [event.relation] if event else self.model.relations[self.relation_name] - port = self.charm.config["listen_port"] + port = self.charm.config.listen_port ips = self.charm.peers.units_ips ips.discard(self.charm.peers.leader_ip) ips = list(ips) diff --git a/tests/unit/relations/test_hacluster.py b/tests/unit/relations/test_hacluster.py index ff90c2025..29289faf3 100644 --- a/tests/unit/relations/test_hacluster.py +++ b/tests/unit/relations/test_hacluster.py @@ -1,6 +1,7 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. +from ipaddress import IPv4Address from unittest import TestCase from unittest.mock import Mock, PropertyMock, patch @@ -46,11 +47,11 @@ def test_is_clustered(self): @patch("charm.HaCluster.set_vip", return_value=True) def test_on_changed(self, _set_vip): with self.harness.hooks_disabled(): - self.harness.update_config({"vip": "test_vip"}) + self.harness.update_config({"vip": "1.2.3.4"}) self.charm.hacluster._on_changed(Mock()) - _set_vip.assert_called_once_with("test_vip") + _set_vip.assert_called_once_with(IPv4Address('1.2.3.4')) @patch("charm.HaCluster._is_clustered", return_value=False) @patch("charm.HaCluster.relation", new_callable=PropertyMock, return_value=False) From bea0f13e0c539a5981f7a2ca8164a0df62f0626d Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 15:33:07 +0300 Subject: [PATCH 04/14] Fix lint --- tests/unit/relations/test_hacluster.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/relations/test_hacluster.py b/tests/unit/relations/test_hacluster.py index 29289faf3..f729215a1 100644 --- a/tests/unit/relations/test_hacluster.py +++ b/tests/unit/relations/test_hacluster.py @@ -51,7 +51,7 @@ def test_on_changed(self, _set_vip): self.charm.hacluster._on_changed(Mock()) - _set_vip.assert_called_once_with(IPv4Address('1.2.3.4')) + _set_vip.assert_called_once_with(IPv4Address("1.2.3.4")) @patch("charm.HaCluster._is_clustered", return_value=False) @patch("charm.HaCluster.relation", new_callable=PropertyMock, return_value=False) From 5c4c2ca9df87cb31777b1d31f2132d273dec85b5 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 16:37:50 +0300 Subject: [PATCH 05/14] Typo --- src/charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 1082147f7..bd7faa2fd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -761,7 +761,7 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): if self._is_exposed: # Open port try: - self.unit.open_port("tcp", self.configlisten_port) + self.unit.open_port("tcp", self.config.listen_port) except ModelError: logger.exception("failed to open port") From 76cfc54f546b8a322fe6b6f643d630c3dcdc3b99 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 18:21:47 +0300 Subject: [PATCH 06/14] Ip addr to str --- src/relations/hacluster.py | 11 +++++------ src/relations/pgbouncer_provider.py | 4 ++-- tests/unit/relations/test_hacluster.py | 10 +++++----- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/relations/hacluster.py b/src/relations/hacluster.py index 4cdbe4ae2..9b7e18f12 100644 --- a/src/relations/hacluster.py +++ b/src/relations/hacluster.py @@ -5,8 +5,8 @@ import json import logging from hashlib import shake_128 -from ipaddress import IPv4Address, ip_address -from typing import Optional +from ipaddress import IPv4Address, IPv6Address +from typing import Optional, Union from ops import CharmBase, Object, Relation, RelationChangedEvent, Unit @@ -43,7 +43,7 @@ def _is_clustered(self) -> bool: def _on_changed(self, event: RelationChangedEvent) -> None: self.set_vip(self.charm.config.vip) - def set_vip(self, vip: Optional[str]) -> None: + def set_vip(self, vip: Optional[Union[IPv4Address, IPv6Address]]) -> None: """Adds the requested virtual IP to the integration.""" if not self.relation: return @@ -54,10 +54,9 @@ def set_vip(self, vip: Optional[str]) -> None: if vip: # TODO Add nic support - ipaddr = ip_address(vip) - vip_key = f"res_{self.charm.app.name}_{shake_128(vip.encode()).hexdigest(7)}_vip" + vip_key = f"res_{self.charm.app.name}_{shake_128(str(vip).encode()).hexdigest(7)}_vip" vip_params = " params" - if isinstance(ipaddr, IPv4Address): + if isinstance(vip, IPv4Address): vip_resources = "ocf:heartbeat:IPaddr2" vip_params += f' ip="{vip}"' else: diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index d2bc3a03a..b1f99bb2e 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -241,8 +241,8 @@ def update_connection_info(self, relation): if exposed: if self.charm.config.vip: - host = self.charm.config.vip - uri_host = self.charm.config.vip + host = str(self.charm.config.vip) + uri_host = host else: host = self.charm.leader_ip uri_host = ",".join([ diff --git a/tests/unit/relations/test_hacluster.py b/tests/unit/relations/test_hacluster.py index f729215a1..57bac0076 100644 --- a/tests/unit/relations/test_hacluster.py +++ b/tests/unit/relations/test_hacluster.py @@ -1,7 +1,7 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. -from ipaddress import IPv4Address +from ipaddress import IPv4Address, IPv6Address from unittest import TestCase from unittest.mock import Mock, PropertyMock, patch @@ -57,20 +57,20 @@ def test_on_changed(self, _set_vip): @patch("charm.HaCluster.relation", new_callable=PropertyMock, return_value=False) def test_set_vip_no_relation(self, _relation, _is_clustered): # Not rel - self.charm.hacluster.set_vip("1.2.3.4") + self.charm.hacluster.set_vip(IPv4Address("1.2.3.4")) assert not _is_clustered.called @patch("charm.HaCluster._is_clustered", return_value=False) def test_set_vip(self, _is_clustered): # Not clustered - self.charm.hacluster.set_vip("1.2.3.4") + self.charm.hacluster.set_vip(IPv4Address("1.2.3.4")) assert self.harness.get_relation_data(self.rel_id, self.charm.unit) == {} # ipv4 address _is_clustered.return_value = True - self.charm.hacluster.set_vip("1.2.3.4") + self.charm.hacluster.set_vip(IPv4Address("1.2.3.4")) assert self.harness.get_relation_data(self.rel_id, self.charm.unit) == { "json_resource_params": '{"res_pgbouncer_d716ce1885885a_vip": " params ' @@ -82,7 +82,7 @@ def test_set_vip(self, _is_clustered): } # ipv6 address - self.charm.hacluster.set_vip("::1") + self.charm.hacluster.set_vip(IPv6Address("::1")) assert self.harness.get_relation_data(self.rel_id, self.charm.unit) == { "json_resource_params": '{"res_pgbouncer_61b6532057c944_vip": " params ' From 11d201570fb83327b5b45e67a2bc61f60f26c5e5 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 18:39:12 +0300 Subject: [PATCH 07/14] Add config test --- src/config.py | 4 +- tests/integration/test_config.py | 82 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 tests/integration/test_config.py diff --git a/src/config.py b/src/config.py index c7cd8ba36..d3fd9a6a2 100644 --- a/src/config.py +++ b/src/config.py @@ -8,7 +8,7 @@ from typing import Literal, Optional from charms.data_platform_libs.v0.data_models import BaseConfigModel -from pydantic import IPvAnyAddress, PositiveInt +from pydantic import IPvAnyAddress, PositiveInt, conint logger = logging.getLogger(__name__) @@ -21,4 +21,4 @@ class CharmConfig(BaseConfigModel): vip: Optional[IPvAnyAddress] local_connection_type: Literal["localhost", "socket"] pool_mode: Literal["session", "transaction", "statement"] - max_db_connections: int + max_db_connections: conint(ge=0) diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py new file mode 100644 index 000000000..fcecc3dab --- /dev/null +++ b/tests/integration/test_config.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python3 +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +import logging +from asyncio import gather + +import pytest as pytest +from pytest_operator.plugin import OpsTest + +from .helpers.helpers import ( + BACKEND_RELATION_NAME, + CLIENT_APP_NAME, + FIRST_DATABASE_RELATION_NAME, + PG, + PGB, +) + +logger = logging.getLogger(__name__) + + +@pytest.mark.group(1) +@pytest.mark.abort_on_fail +async def test_config_parameters(ops_test: OpsTest, pgb_charm_jammy) -> None: + """Build and deploy one unit of PostgreSQL and then test config with wrong parameters.""" + # Build and deploy the PostgreSQL charm. + async with ops_test.fast_forward(): + await gather( + ops_test.model.deploy( + CLIENT_APP_NAME, + application_name=CLIENT_APP_NAME, + channel="edge", + ), + ops_test.model.deploy( + pgb_charm_jammy, + application_name=PGB, + num_units=None, + ), + ops_test.model.deploy( + PG, + application_name=PG, + num_units=1, + channel="14/edge", + config={"profile": "testing"}, + ), + ) + await ops_test.model.add_relation(f"{PGB}:{BACKEND_RELATION_NAME}", f"{PG}:database") + await ops_test.model.wait_for_idle(apps=[PG, CLIENT_APP_NAME], timeout=1200) + # Relate the charms and wait for them exchanging some connection data. + await ops_test.model.add_relation(f"{CLIENT_APP_NAME}:{FIRST_DATABASE_RELATION_NAME}", PGB) + + await ops_test.model.wait_for_idle(status="active", timeout=600) + + unit = ops_test.model.applications[PGB].units[0] + test_string = "abcXYZ123" + + configs = [ + {"listen_port": ["0", "6432"]}, + {"metrics_port": ["0", "9127"]}, + {"vip": [test_string, ""]}, + {"local_connection_type": [test_string, "localhost"]}, + {"pool_mode": [test_string, "session"]}, + {"max_db_connections": ["-1", "100"]}, + ] + + charm_config = {} + for config in configs: + for k, v in config.items(): + logger.info(k) + charm_config[k] = v[0] + await ops_test.model.applications[PGB].set_config(charm_config) + await ops_test.model.block_until( + lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "blocked", + timeout=100, + ) + assert "Configuration Error" in unit.workload_status_message + charm_config[k] = v[1] + + await ops_test.model.applications[PGB].set_config(charm_config) + await ops_test.model.block_until( + lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "active", + timeout=100, + ) From a6642d18a450d308e9322b48a209c47c6b03c550 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 19:50:40 +0300 Subject: [PATCH 08/14] Fix import --- tests/integration/test_config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index fcecc3dab..f26c54ae6 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -7,8 +7,9 @@ import pytest as pytest from pytest_operator.plugin import OpsTest +from constants import BACKEND_RELATION_NAME + from .helpers.helpers import ( - BACKEND_RELATION_NAME, CLIENT_APP_NAME, FIRST_DATABASE_RELATION_NAME, PG, From 0fc21be06c85542a51c7d0a22f04a8963fc96f8d Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 21:33:51 +0300 Subject: [PATCH 09/14] Block on wrong config --- lib/charms/grafana_agent/v0/cos_agent.py | 8 +++++-- src/charm.py | 28 +++++++++++++++++------- src/constants.py | 2 +- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/lib/charms/grafana_agent/v0/cos_agent.py b/lib/charms/grafana_agent/v0/cos_agent.py index 582b70c07..cc4da25a8 100644 --- a/lib/charms/grafana_agent/v0/cos_agent.py +++ b/lib/charms/grafana_agent/v0/cos_agent.py @@ -22,7 +22,7 @@ Using the `COSAgentProvider` object only requires instantiating it, typically in the `__init__` method of your charm (the one which sends telemetry). -The constructor of `COSAgentProvider` has only one required and nine optional parameters: +The constructor of `COSAgentProvider` has only one required and ten optional parameters: ```python def __init__( @@ -36,6 +36,7 @@ def __init__( log_slots: Optional[List[str]] = None, dashboard_dirs: Optional[List[str]] = None, refresh_events: Optional[List] = None, + tracing_protocols: Optional[List[str]] = None, scrape_configs: Optional[Union[List[Dict], Callable]] = None, ): ``` @@ -65,6 +66,8 @@ def __init__( - `refresh_events`: List of events on which to refresh relation data. +- `tracing_protocols`: List of requested tracing protocols that the charm requires to send traces. + - `scrape_configs`: List of standard scrape_configs dicts or a callable that returns the list in case the configs need to be generated dynamically. The contents of this list will be merged with the configs from `metrics_endpoints`. @@ -108,6 +111,7 @@ def __init__(self, *args): log_slots=["my-app:slot"], dashboard_dirs=["./src/dashboards_1", "./src/dashboards_2"], refresh_events=["update-status", "upgrade-charm"], + tracing_protocols=["otlp_http", "otlp_grpc"], scrape_configs=[ { "job_name": "custom_job", @@ -249,7 +253,7 @@ class _MetricsEndpointDict(TypedDict): LIBID = "dc15fa84cef84ce58155fb84f6c6213a" LIBAPI = 0 -LIBPATCH = 10 +LIBPATCH = 11 PYDEPS = ["cosl", "pydantic"] diff --git a/src/charm.py b/src/charm.py index bd7faa2fd..7dc302622 100755 --- a/src/charm.py +++ b/src/charm.py @@ -133,14 +133,17 @@ def __init__(self, *args): f"{PGB}-{self.app.name}@{service_id}" for service_id in self.service_ids ] - self._grafana_agent = COSAgentProvider( - self, - metrics_endpoints=[ - {"path": "/metrics", "port": self.config.metrics_port}, - ], - log_slots=[f"{PGBOUNCER_SNAP_NAME}:logs"], - refresh_events=[self.on.config_changed], - ) + try: + self._grafana_agent = COSAgentProvider( + self, + metrics_endpoints=[ + {"path": "/metrics", "port": self.config.metrics_port}, + ], + log_slots=[f"{PGBOUNCER_SNAP_NAME}:logs"], + refresh_events=[self.on.config_changed], + ) + except ValueError: + logger.warning("Unable to set COS agent, invalid config") self.upgrade = PgbouncerUpgrade( self, @@ -507,6 +510,13 @@ def _on_update_status(self, _) -> None: def update_status(self): """Health check to update pgbouncer status based on charm state.""" + try: + self.config + except ValueError: + self.unit.status = BlockedStatus("Configuration Error. Please check the logs") + logger.exception("Invalid configuration") + return + if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: return @@ -547,6 +557,8 @@ def _on_config_changed(self, event) -> None: event.defer() return + self.update_status() + old_vip = self.peers.app_databag.get("current_vip", "") vip = self.config.vip if self.config.vip else "" vip_changed = old_vip != vip diff --git a/src/constants.py b/src/constants.py index 96d0c4aaa..c74a9a7a0 100644 --- a/src/constants.py +++ b/src/constants.py @@ -15,7 +15,7 @@ SNAP_PACKAGES = [ ( PGBOUNCER_SNAP_NAME, - {"revision": {"aarch64": "13", "x86_64": "14"}, "channel": "1/stable"}, + {"revision": {"aarch64": "15", "x86_64": "16"}, "channel": "1/stable"}, ) ] From 4a0d25165b95570e4f38968ff144bc8a37348fa5 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 21:45:50 +0300 Subject: [PATCH 10/14] Split boilerplate --- config.yaml | 6 ------ src/relations/pgbouncer_provider.py | 8 ++------ .../pgbouncer_provider/test_pgbouncer_provider.py | 1 - tests/integration/test_config.py | 1 - 4 files changed, 2 insertions(+), 14 deletions(-) diff --git a/config.yaml b/config.yaml index 9d4ebaf10..1eb6ba8a9 100644 --- a/config.yaml +++ b/config.yaml @@ -21,12 +21,6 @@ options: Virtual IP to use to front pgbouncer units. Used only in case of external node connection. type: string - local_connection_type: - default: localhost - description: | - Type of local connectivity to provide. Must be either 'localhost' or 'socket'. Only applies to the database interface. - type: string - pool_mode: default: session description: | diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index b1f99bb2e..c884b1ade 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -239,6 +239,8 @@ def update_connection_info(self, relation): user = f"relation_id_{relation.id}" password = self.database_provides.fetch_my_relation_field(relation.id, "password") + host = "localhost" + uri_host = host if exposed: if self.charm.config.vip: host = str(self.charm.config.vip) @@ -249,12 +251,6 @@ def update_connection_info(self, relation): self.charm.leader_ip, *[ip for ip in self.charm.peers.units_ips if ip != self.charm.leader_ip], ]) - elif self.charm.config.local_connection_type == "socket": - host = f"{PGB_RUN_DIR}/{self.charm.app.name}/instance_0" - uri_host = host - else: - host = "localhost" - uri_host = host port = self.charm.config.listen_port initial_status = self.charm.unit.status diff --git a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py index e998b53b3..9ec13e67e 100644 --- a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py +++ b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py @@ -61,7 +61,6 @@ async def test_database_relation_with_charm_libraries(ops_test: OpsTest, pgb_cha pgb_charm_jammy, application_name=PGB, num_units=None, - config={"local_connection_type": "socket"}, ), ops_test.model.deploy( PG, diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index f26c54ae6..692d0b96c 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -58,7 +58,6 @@ async def test_config_parameters(ops_test: OpsTest, pgb_charm_jammy) -> None: {"listen_port": ["0", "6432"]}, {"metrics_port": ["0", "9127"]}, {"vip": [test_string, ""]}, - {"local_connection_type": [test_string, "localhost"]}, {"pool_mode": [test_string, "session"]}, {"max_db_connections": ["-1", "100"]}, ] From 0a710a2eb3f88752c6d92938f917cb52c7ec420d Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 21:48:51 +0300 Subject: [PATCH 11/14] Split boilerplate --- src/config.py | 1 - src/relations/pgbouncer_provider.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/config.py b/src/config.py index d3fd9a6a2..7883de226 100644 --- a/src/config.py +++ b/src/config.py @@ -19,6 +19,5 @@ class CharmConfig(BaseConfigModel): listen_port: PositiveInt metrics_port: PositiveInt vip: Optional[IPvAnyAddress] - local_connection_type: Literal["localhost", "socket"] pool_mode: Literal["session", "transaction", "statement"] max_db_connections: conint(ge=0) diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index c884b1ade..a0d6d5117 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -60,7 +60,7 @@ RelationDepartedEvent, ) -from constants import CLIENT_RELATION_NAME, PGB_RUN_DIR +from constants import CLIENT_RELATION_NAME logger = logging.getLogger(__name__) From c2199856e584367f22eb3af260a54c1c20bb7f57 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 22 Aug 2024 22:03:03 +0300 Subject: [PATCH 12/14] Early exit on wrong config --- src/charm.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 7dc302622..42fa44c81 100755 --- a/src/charm.py +++ b/src/charm.py @@ -557,7 +557,12 @@ def _on_config_changed(self, event) -> None: event.defer() return - self.update_status() + try: + self.config + except ValueError: + self.unit.status = BlockedStatus("Configuration Error. Please check the logs") + logger.exception("Invalid configuration") + return old_vip = self.peers.app_databag.get("current_vip", "") vip = self.config.vip if self.config.vip else "" @@ -594,6 +599,7 @@ def _on_config_changed(self, event) -> None: self.render_prometheus_service() self.update_client_connection_info() + self.update_status() def check_pgb_running(self): """Checks that pgbouncer service is running, and updates status accordingly.""" From c14999c72619e0e9e0594272acc2b6c5da4e159e Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Fri, 23 Aug 2024 01:47:43 +0300 Subject: [PATCH 13/14] Invalid config guards --- src/charm.py | 27 +++++++------- src/relations/db.py | 10 +++--- src/relations/hacluster.py | 4 +++ src/relations/peers.py | 2 +- src/relations/pgbouncer_provider.py | 2 +- .../test_data_integrator_ha.py | 2 +- tests/integration/test_config.py | 36 +++++++++---------- tests/unit/relations/test_db.py | 8 ++--- 8 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/charm.py b/src/charm.py index 42fa44c81..aea7d6fc0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -508,13 +508,19 @@ def _on_update_status(self, _) -> None: self.peers.update_leader() self._collect_readonly_dbs() - def update_status(self): - """Health check to update pgbouncer status based on charm state.""" + def configuration_check(self) -> bool: + """Check that configuration is valid.""" try: self.config + return True except ValueError: self.unit.status = BlockedStatus("Configuration Error. Please check the logs") logger.exception("Invalid configuration") + return False + + def update_status(self): + """Health check to update pgbouncer status based on charm state.""" + if not self.configuration_check(): return if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: @@ -557,11 +563,7 @@ def _on_config_changed(self, event) -> None: event.defer() return - try: - self.config - except ValueError: - self.unit.status = BlockedStatus("Configuration Error. Please check the logs") - logger.exception("Invalid configuration") + if not self.configuration_check(): return old_vip = self.peers.app_databag.get("current_vip", "") @@ -766,7 +768,7 @@ def _get_relation_config(self) -> [Dict[str, Dict[str, Union[str, bool]]]]: } return pgb_dbs - def render_pgb_config(self, reload_pgbouncer=False, restart=False): + def render_pgb_config(self, reload_pgbouncer=False, restart=False) -> None: """Derives config files for the number of required services from given config. This method takes a primary config and generates one unique config for each intended @@ -775,6 +777,9 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False): initial_status = self.unit.status self.unit.status = MaintenanceStatus("updating PgBouncer config") + if not self.configuration_check(): + return + # If exposed relation, open the listen port for all units if self._is_exposed: # Open port @@ -971,13 +976,11 @@ def update_client_connection_info(self): if not self.backend.postgres or not self.unit.is_leader(): return - port = self.config.listen_port - for relation in self.model.relations.get("db", []): - self.legacy_db_relation.update_connection_info(relation, port) + self.legacy_db_relation.update_connection_info(relation) for relation in self.model.relations.get("db-admin", []): - self.legacy_db_admin_relation.update_connection_info(relation, port) + self.legacy_db_admin_relation.update_connection_info(relation) for relation in self.model.relations.get(CLIENT_RELATION_NAME, []): self.client_relation.update_connection_info(relation) diff --git a/src/relations/db.py b/src/relations/db.py index 4b9a6010b..019579f7c 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -324,12 +324,14 @@ def _on_relation_changed(self, change_event: RelationChangedEvent): "state": "master", }, ) - self.update_connection_info(change_event.relation, self.charm.config.listen_port) + self.update_connection_info(change_event.relation) - def update_connection_info(self, relation: Relation, port: str = None): + def update_connection_info(self, relation: Relation): """Updates databag connection information.""" - if not port: - port = self.charm.config.listen_port + if not self.charm.configuration_check(): + return + + port = self.charm.config.listen_port databag = self.get_databags(relation)[0] database = databag.get("database") diff --git a/src/relations/hacluster.py b/src/relations/hacluster.py index 9b7e18f12..5c6b3f535 100644 --- a/src/relations/hacluster.py +++ b/src/relations/hacluster.py @@ -41,6 +41,10 @@ def _is_clustered(self) -> bool: return False def _on_changed(self, event: RelationChangedEvent) -> None: + if not self.charm.configuration_check(): + event.defer() + return + self.set_vip(self.charm.config.vip) def set_vip(self, vip: Optional[Union[IPv4Address, IPv6Address]]) -> None: diff --git a/src/relations/peers.py b/src/relations/peers.py index 86a42c474..749faae7d 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -119,7 +119,7 @@ def _get_unit_ip(self, unit: Unit) -> Optional[str]: def _on_joined(self, event: HookEvent): self._on_changed(event) - if self.charm.unit.is_leader(): + if self.charm.unit.is_leader() and self.charm.configuration_check(): self.charm.client_relation.update_read_only_endpoints() def _on_changed(self, event: HookEvent): diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index a0d6d5117..1b68f12bc 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -227,7 +227,7 @@ def _on_relation_broken(self, event: RelationBrokenEvent) -> None: def update_connection_info(self, relation): """Updates client-facing relation information.""" - if not self.charm.unit.is_leader(): + if not self.charm.unit.is_leader() or not self.charm.configuration_check(): return # Set the read/write endpoint. diff --git a/tests/integration/relations/pgbouncer_provider/test_data_integrator_ha.py b/tests/integration/relations/pgbouncer_provider/test_data_integrator_ha.py index 460dd8741..f1a832a06 100644 --- a/tests/integration/relations/pgbouncer_provider/test_data_integrator_ha.py +++ b/tests/integration/relations/pgbouncer_provider/test_data_integrator_ha.py @@ -143,7 +143,7 @@ async def test_remove_tls(ops_test: OpsTest, pgb_charm_jammy): @pytest.mark.group(1) async def test_remove_vip(ops_test: OpsTest): async with ops_test.fast_forward(): - await ops_test.model.applications[PGB].set_config({"vip": ""}) + await ops_test.model.applications[PGB].reset_config(["vip"]) await ops_test.model.wait_for_idle(apps=[PGB], status="blocked", timeout=300) assert ( ops_test.model.applications[PGB].units[0].workload_status_message diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 692d0b96c..9839b14dc 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -54,28 +54,24 @@ async def test_config_parameters(ops_test: OpsTest, pgb_charm_jammy) -> None: unit = ops_test.model.applications[PGB].units[0] test_string = "abcXYZ123" - configs = [ - {"listen_port": ["0", "6432"]}, - {"metrics_port": ["0", "9127"]}, - {"vip": [test_string, ""]}, - {"pool_mode": [test_string, "session"]}, - {"max_db_connections": ["-1", "100"]}, - ] + configs = { + "listen_port": "0", + "metrics_port": "0", + "vip": test_string, + "pool_mode": test_string, + "max_db_connections": "-1", + } - charm_config = {} - for config in configs: - for k, v in config.items(): - logger.info(k) - charm_config[k] = v[0] - await ops_test.model.applications[PGB].set_config(charm_config) - await ops_test.model.block_until( - lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "blocked", - timeout=100, - ) - assert "Configuration Error" in unit.workload_status_message - charm_config[k] = v[1] + for key, val in configs.items(): + logger.info(key) + await ops_test.model.applications[PGB].set_config({key: val}) + await ops_test.model.block_until( + lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "blocked", + timeout=100, + ) + assert "Configuration Error" in unit.workload_status_message - await ops_test.model.applications[PGB].set_config(charm_config) + await ops_test.model.applications[PGB].reset_config([key]) await ops_test.model.block_until( lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "active", timeout=100, diff --git a/tests/unit/relations/test_db.py b/tests/unit/relations/test_db.py index 7e429b738..ca809ce47 100644 --- a/tests/unit/relations/test_db.py +++ b/tests/unit/relations/test_db.py @@ -179,9 +179,7 @@ def test_on_relation_changed( # Call the function self.db_relation._on_relation_changed(event) - _update_connection_info.assert_called_with( - event.relation, self.charm.config["listen_port"] - ) + _update_connection_info.assert_called_with(event.relation) _update_databags.assert_called_with( event.relation, { @@ -206,6 +204,8 @@ def test_update_connection_info(self, _update_databags, _get_external_app, _get_ user = "test_user" password = "test_pw" port = "5555" + with self.harness.hooks_disabled(): + self.harness.update_config({"listen_port": int(port)}) _get_databags.return_value[0] = { "database": database, @@ -228,7 +228,7 @@ def test_update_connection_info(self, _update_databags, _get_external_app, _get_ standby_dbconnstr = dict(master_dbconnstr) standby_dbconnstr.update({"host": standby_hostname, "dbname": f"{database}_standby"}) - self.db_relation.update_connection_info(relation, port) + self.db_relation.update_connection_info(relation) _update_databags.assert_called_with( relation, { From a0adef91a73e9a4e701067f43b2f182c5579b23b Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Fri, 23 Aug 2024 03:09:16 +0300 Subject: [PATCH 14/14] Unit tests --- src/charm.py | 4 ++-- tests/integration/test_config.py | 10 +++++----- tests/unit/relations/test_hacluster.py | 11 ++++++++++- tests/unit/relations/test_peers.py | 22 +++++++++++++++++++++- tests/unit/test_charm.py | 9 +++++++++ 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/charm.py b/src/charm.py index aea7d6fc0..7c9fd3a15 100755 --- a/src/charm.py +++ b/src/charm.py @@ -520,10 +520,10 @@ def configuration_check(self) -> bool: def update_status(self): """Health check to update pgbouncer status based on charm state.""" - if not self.configuration_check(): + if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: return - if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: + if not self.configuration_check(): return if self.backend.postgres is None: diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 9839b14dc..8488f23e1 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -71,8 +71,8 @@ async def test_config_parameters(ops_test: OpsTest, pgb_charm_jammy) -> None: ) assert "Configuration Error" in unit.workload_status_message - await ops_test.model.applications[PGB].reset_config([key]) - await ops_test.model.block_until( - lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "active", - timeout=100, - ) + await ops_test.model.applications[PGB].reset_config([key]) + await ops_test.model.block_until( + lambda: ops_test.model.units[f"{PGB}/0"].workload_status == "active", + timeout=100, + ) diff --git a/tests/unit/relations/test_hacluster.py b/tests/unit/relations/test_hacluster.py index 57bac0076..ca3b0c824 100644 --- a/tests/unit/relations/test_hacluster.py +++ b/tests/unit/relations/test_hacluster.py @@ -44,8 +44,17 @@ def test_is_clustered(self): assert self.charm.hacluster._is_clustered() + @patch("charm.PgBouncerCharm.configuration_check", return_value=False) @patch("charm.HaCluster.set_vip", return_value=True) - def test_on_changed(self, _set_vip): + def test_on_changed(self, _set_vip, _configuration_check): + # Defer on invalid configuration + event = Mock() + self.charm.hacluster._on_changed(event) + + event.defer.assert_called_once_with() + assert not _set_vip.called + + _configuration_check.return_value = True with self.harness.hooks_disabled(): self.harness.update_config({"vip": "1.2.3.4"}) diff --git a/tests/unit/relations/test_peers.py b/tests/unit/relations/test_peers.py index 8ceecfd3e..0ce4f2e57 100644 --- a/tests/unit/relations/test_peers.py +++ b/tests/unit/relations/test_peers.py @@ -2,7 +2,7 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, Mock, patch from ops.testing import Harness @@ -29,6 +29,26 @@ def setUp(self): self.rel_id = self.harness.add_relation(PEER_RELATION_NAME, self.charm.app.name) + @patch("charm.PgBouncerCharm.configuration_check", return_value=True) + @patch("charm.PgBouncerProvider.update_read_only_endpoints") + @patch("charm.Peers._on_changed") + def test_on_peers_joined(self, _on_changed, _update_read_only_endpoints, _configuration_check): + with self.harness.hooks_disabled(): + self.harness.set_leader() + + event = Mock() + self.charm.peers._on_joined(event) + _on_changed.assert_called_once_with(event) + _update_read_only_endpoints.assert_called_once_with() + _update_read_only_endpoints.reset_mock() + _on_changed.reset_mock() + + # Misconfiguration guard + _configuration_check.return_value = False + self.charm.peers._on_joined(event) + _on_changed.assert_called_once_with(event) + assert not _update_read_only_endpoints.called + @patch("charm.PgBouncerCharm.render_pgb_config") @patch("charm.PgBouncerCharm.render_auth_file") @patch("charm.PgBouncerCharm.reload_pgbouncer") diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 66b3c7e29..9c68fdb4a 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -679,6 +679,15 @@ def test_update_status(self, _check_pgb_running, _postgres, _ready, _is_exposed, assert self.charm.unit.status.message == "VIP: 1.2.3.4" + @patch("charm.PgBouncerCharm.config", new_callable=PropertyMock, return_value={}) + def test_configuration_check(self, _config): + assert self.charm.configuration_check() + + _config.side_effect = ValueError + assert not self.charm.configuration_check() + assert isinstance(self.charm.unit.status, BlockedStatus) + assert self.charm.unit.status.message == "Configuration Error. Please check the logs" + # # Secrets #