From 06e754dc86f6dd821afbc4ba6bd3747ae5ff09ae Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Mon, 25 Aug 2025 15:49:43 +0300 Subject: [PATCH 01/10] Handle empty region --- src/backups.py | 5 ++--- tests/integration/test_backups_ceph.py | 21 --------------------- tests/unit/test_backups.py | 2 +- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/src/backups.py b/src/backups.py index f25a1e71c4..6dac6d96dd 100644 --- a/src/backups.py +++ b/src/backups.py @@ -265,7 +265,7 @@ def _create_bucket_if_not_exists(self) -> None: return bucket_name = s3_parameters["bucket"] - region = s3_parameters.get("region") + region = s3_parameters.get("region", "") try: s3 = self._get_s3_session_resource(s3_parameters) @@ -1329,8 +1329,7 @@ def _retrieve_s3_parameters(self) -> tuple[dict, list[str]]: # Add some sensible defaults (as expected by the code) for missing optional parameters s3_parameters.setdefault("endpoint", "https://s3.amazonaws.com") - # Existing behaviour is none not a str - s3_parameters.setdefault("region", None) # type: ignore + s3_parameters.setdefault("region", "") s3_parameters.setdefault("path", "") s3_parameters.setdefault("s3-uri-style", "host") s3_parameters.setdefault("delete-older-than-days", "9999999") diff --git a/tests/integration/test_backups_ceph.py b/tests/integration/test_backups_ceph.py index 4e007dc6c6..797ce0c40b 100644 --- a/tests/integration/test_backups_ceph.py +++ b/tests/integration/test_backups_ceph.py @@ -7,10 +7,7 @@ import os import socket import subprocess -import time -import boto3 -import botocore.exceptions import pytest from pytest_operator.plugin import OpsTest @@ -132,24 +129,6 @@ def microceph(): key = json.loads(output)["keys"][0] key_id = key["access_key"] secret_key = key["secret_key"] - logger.info("Creating microceph bucket") - for attempt in range(3): - try: - boto3.client( - "s3", - endpoint_url=f"https://{host_ip}", - aws_access_key_id=key_id, - aws_secret_access_key=secret_key, - verify="./ca.crt", - ).create_bucket(Bucket=_BUCKET) - except botocore.exceptions.EndpointConnectionError: - if attempt == 2: - raise - # microceph is not ready yet - logger.info("Unable to connect to microceph via S3. Retrying") - time.sleep(1) - else: - break logger.info("Set up microceph") return ConnectionInformation(key_id, secret_key, _BUCKET) diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index b0a6f6daa2..7cf03ace9c 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1803,7 +1803,7 @@ def test_retrieve_s3_parameters(harness): "delete-older-than-days": "9999999", "endpoint": "https://s3.amazonaws.com", "path": "/", - "region": None, + "region": "", "s3-uri-style": "host", "secret-key": "test-secret-key", }, From 0170bebb1ef0057e8d1a93271d107f43e89d2b9a Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Tue, 2 Sep 2025 17:30:04 +0300 Subject: [PATCH 02/10] Skip region def in pgbackrest conf --- templates/pgbackrest.conf.j2 | 2 ++ tests/integration/test_backups_ceph.py | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index ce4dbbe2ca..2eff36a0c3 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -8,7 +8,9 @@ repo1-retention-full={{ retention_full }} repo1-retention-history=365 repo1-type=s3 repo1-path={{ path }} +{%- if enable_tls %} repo1-s3-region={{ region }} +{%- endif %} repo1-s3-endpoint={{ endpoint }} repo1-s3-bucket={{ bucket }} repo1-s3-uri-style={{ s3_uri_style }} diff --git a/tests/integration/test_backups_ceph.py b/tests/integration/test_backups_ceph.py index 797ce0c40b..4efbbe0fba 100644 --- a/tests/integration/test_backups_ceph.py +++ b/tests/integration/test_backups_ceph.py @@ -12,9 +12,7 @@ from pytest_operator.plugin import OpsTest from . import markers -from .helpers import ( - backup_operations, -) +from .helpers import backup_operations logger = logging.getLogger(__name__) From 47a6bde0951b5ac14dd6ac0e83a8bd27577a74d7 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 3 Sep 2025 00:29:06 +0300 Subject: [PATCH 03/10] Restore region config --- templates/pgbackrest.conf.j2 | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index 2eff36a0c3..4ef31706e7 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -8,9 +8,7 @@ repo1-retention-full={{ retention_full }} repo1-retention-history=365 repo1-type=s3 repo1-path={{ path }} -{%- if enable_tls %} -repo1-s3-region={{ region }} -{%- endif %} +repo1-s3-region="{{ region }}" repo1-s3-endpoint={{ endpoint }} repo1-s3-bucket={{ bucket }} repo1-s3-uri-style={{ s3_uri_style }} From c1e8bbaf4c42df9fc420b547b143b4d0ecf39271 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 3 Sep 2025 14:06:14 +0300 Subject: [PATCH 04/10] Revert template --- templates/pgbackrest.conf.j2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index 4ef31706e7..ce4dbbe2ca 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -8,7 +8,7 @@ repo1-retention-full={{ retention_full }} repo1-retention-history=365 repo1-type=s3 repo1-path={{ path }} -repo1-s3-region="{{ region }}" +repo1-s3-region={{ region }} repo1-s3-endpoint={{ endpoint }} repo1-s3-bucket={{ bucket }} repo1-s3-uri-style={{ s3_uri_style }} From 24bdc0c90ce2ed0d57ef49b6ea9ee2f1972ba432 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 3 Sep 2025 14:50:23 +0300 Subject: [PATCH 05/10] Skip region correctly --- src/backups.py | 1 - templates/pgbackrest.conf.j2 | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backups.py b/src/backups.py index 9217632959..a38f8dc474 100644 --- a/src/backups.py +++ b/src/backups.py @@ -1330,7 +1330,6 @@ def _retrieve_s3_parameters(self) -> tuple[dict, list[str]]: # Add some sensible defaults (as expected by the code) for missing optional parameters s3_parameters.setdefault("endpoint", "https://s3.amazonaws.com") - s3_parameters.setdefault("region", "") s3_parameters.setdefault("path", "") s3_parameters.setdefault("s3-uri-style", "host") s3_parameters.setdefault("delete-older-than-days", "9999999") diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index ce4dbbe2ca..779147cbdb 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -8,7 +8,9 @@ repo1-retention-full={{ retention_full }} repo1-retention-history=365 repo1-type=s3 repo1-path={{ path }} +{%- if region %} repo1-s3-region={{ region }} +{%- endif %} repo1-s3-endpoint={{ endpoint }} repo1-s3-bucket={{ bucket }} repo1-s3-uri-style={{ s3_uri_style }} From 0915707b01af6ed80a3b4dbd19a99abef3a98f90 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 3 Sep 2025 20:37:56 +0300 Subject: [PATCH 06/10] Conditional empty region --- src/backups.py | 33 ++++++++++++++++----------------- templates/pgbackrest.conf.j2 | 2 ++ tests/unit/test_backups.py | 1 - 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/backups.py b/src/backups.py index a38f8dc474..57fa33b872 100644 --- a/src/backups.py +++ b/src/backups.py @@ -21,10 +21,7 @@ from botocore.exceptions import ClientError, ConnectTimeoutError, ParamValidationError, SSLError from botocore.loaders import create_loader from botocore.regions import EndpointResolver -from charms.data_platform_libs.v0.s3 import ( - CredentialsChangedEvent, - S3Requirer, -) +from charms.data_platform_libs.v0.s3 import CredentialsChangedEvent, S3Requirer from charms.operator_libs_linux.v2 import snap from jinja2 import Template from ops.charm import ActionEvent, HookEvent @@ -108,11 +105,13 @@ def _tls_ca_chain_filename(self) -> str: return "" def _get_s3_session_resource(self, s3_parameters: dict): - session = Session( - aws_access_key_id=s3_parameters["access-key"], - aws_secret_access_key=s3_parameters["secret-key"], - region_name=s3_parameters["region"], - ) + kwargs = { + "aws_access_key_id": s3_parameters["access-key"], + "aws_secret_access_key": s3_parameters["secret-key"], + } + if "region" in s3_parameters: + kwargs["region_name"] = s3_parameters["region"] + session = Session(**kwargs) return session.resource( "s3", endpoint_url=self._construct_endpoint(s3_parameters), @@ -200,7 +199,7 @@ def can_use_s3_repository(self) -> tuple[bool, str]: else: if return_code != 0: - logger.error(stderr) + logger.error(f"Failed to run pgbackrest: {stderr}") return False, FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE for stanza in json.loads(stdout): @@ -252,7 +251,7 @@ def _construct_endpoint(self, s3_parameters: dict) -> str: # Construct the endpoint using the region. resolver = EndpointResolver(data) - endpoint_data = resolver.construct_endpoint("s3", s3_parameters["region"]) + endpoint_data = resolver.construct_endpoint("s3", s3_parameters.get("region")) # Use the built endpoint if it is an AWS endpoint. if endpoint_data and endpoint.endswith(endpoint_data["dnsSuffix"]): @@ -640,10 +639,10 @@ def _initialise_stanza(self, event: HookEvent) -> bool: raise Exception(stderr) except TimeoutError as e: raise e - except Exception as e: + except Exception: # If the check command doesn't succeed, remove the stanza name # and rollback the configuration. - logger.exception(e) + logger.exception("Failed to initialise stanza:") self._s3_initialization_set_failure(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) return False @@ -689,10 +688,10 @@ def check_stanza(self) -> bool: if return_code != 0: raise Exception(stderr) self.charm._set_primary_status_message() - except Exception as e: + except Exception: # If the check command doesn't succeed, remove the stanza name # and rollback the configuration. - logger.exception(e) + logger.exception("Failed to check stanza:") self._s3_initialization_set_failure(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) self.charm.update_config() return False @@ -975,9 +974,9 @@ def _run_backup( else: try: backup_id = list(self._list_backups(show_failed=True).keys())[-1] - except ListBackupsError as e: - logger.exception(e) + except ListBackupsError: error_message = "Failed to retrieve backup id" + logger.exception(error_message) logger.error(f"Backup failed: {error_message}") event.fail(error_message) return diff --git a/templates/pgbackrest.conf.j2 b/templates/pgbackrest.conf.j2 index 779147cbdb..ff44a6d4a8 100644 --- a/templates/pgbackrest.conf.j2 +++ b/templates/pgbackrest.conf.j2 @@ -10,6 +10,8 @@ repo1-type=s3 repo1-path={{ path }} {%- if region %} repo1-s3-region={{ region }} +{% else %} +repo1-s3-region="" {%- endif %} repo1-s3-endpoint={{ endpoint }} repo1-s3-bucket={{ bucket }} diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 7cf03ace9c..4267541a2a 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1803,7 +1803,6 @@ def test_retrieve_s3_parameters(harness): "delete-older-than-days": "9999999", "endpoint": "https://s3.amazonaws.com", "path": "/", - "region": "", "s3-uri-style": "host", "secret-key": "test-secret-key", }, From c511e3f8345e8b29c2c70a00b3887b764692627d Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 3 Sep 2025 23:50:15 +0300 Subject: [PATCH 07/10] Generate random path --- tests/integration/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 668459db7c..b54a53212b 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -33,7 +33,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None return { "endpoint": "https://s3.amazonaws.com", "bucket": "data-charms-testing", - "path": f"/postgresql-k8s/{uuid.uuid1()}", + "path": f"/postgresql-k8s/{uuid.uuid4()}", "region": "us-east-1", }, { "access-key": os.environ["AWS_ACCESS_KEY"], @@ -43,7 +43,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None return { "endpoint": "https://storage.googleapis.com", "bucket": "data-charms-testing", - "path": f"/postgresql-k8s/{uuid.uuid1()}", + "path": f"/postgresql-k8s/{uuid.uuid4()}", "region": "", }, { "access-key": os.environ["GCP_ACCESS_KEY"], From 03fc000eab8002ce8a13c84cf52551b169816de0 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 4 Sep 2025 00:41:49 +0300 Subject: [PATCH 08/10] Set region for gcp --- tests/integration/conftest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index b54a53212b..2740fa317d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -33,7 +33,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None return { "endpoint": "https://s3.amazonaws.com", "bucket": "data-charms-testing", - "path": f"/postgresql-k8s/{uuid.uuid4()}", + "path": f"/postgresql-k8s/{uuid.uuid1()}", "region": "us-east-1", }, { "access-key": os.environ["AWS_ACCESS_KEY"], @@ -43,8 +43,8 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None return { "endpoint": "https://storage.googleapis.com", "bucket": "data-charms-testing", - "path": f"/postgresql-k8s/{uuid.uuid4()}", - "region": "", + "path": f"/postgresql-k8s/{uuid.uuid1()}", + "region": "us-east-1", }, { "access-key": os.environ["GCP_ACCESS_KEY"], "secret-key": os.environ["GCP_SECRET_KEY"], From e8d42b7f757ecc8cad6134a8f51e4122bf59d1c8 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 4 Sep 2025 13:27:47 +0300 Subject: [PATCH 09/10] Set default region --- src/backups.py | 13 ++++++------- tests/integration/conftest.py | 2 +- tests/unit/test_backups.py | 1 + 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/backups.py b/src/backups.py index 57fa33b872..bfc8409796 100644 --- a/src/backups.py +++ b/src/backups.py @@ -105,13 +105,11 @@ def _tls_ca_chain_filename(self) -> str: return "" def _get_s3_session_resource(self, s3_parameters: dict): - kwargs = { - "aws_access_key_id": s3_parameters["access-key"], - "aws_secret_access_key": s3_parameters["secret-key"], - } - if "region" in s3_parameters: - kwargs["region_name"] = s3_parameters["region"] - session = Session(**kwargs) + session = Session( + aws_access_key_id=s3_parameters["access-key"], + aws_secret_access_key=s3_parameters["secret-key"], + region_name=s3_parameters["region"], + ) return session.resource( "s3", endpoint_url=self._construct_endpoint(s3_parameters), @@ -1329,6 +1327,7 @@ def _retrieve_s3_parameters(self) -> tuple[dict, list[str]]: # Add some sensible defaults (as expected by the code) for missing optional parameters s3_parameters.setdefault("endpoint", "https://s3.amazonaws.com") + s3_parameters.setdefault("region", "") s3_parameters.setdefault("path", "") s3_parameters.setdefault("s3-uri-style", "host") s3_parameters.setdefault("delete-older-than-days", "9999999") diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 2740fa317d..668459db7c 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -44,7 +44,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None "endpoint": "https://storage.googleapis.com", "bucket": "data-charms-testing", "path": f"/postgresql-k8s/{uuid.uuid1()}", - "region": "us-east-1", + "region": "", }, { "access-key": os.environ["GCP_ACCESS_KEY"], "secret-key": os.environ["GCP_SECRET_KEY"], diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 4267541a2a..1e62906efa 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1802,6 +1802,7 @@ def test_retrieve_s3_parameters(harness): "bucket": "test-bucket", "delete-older-than-days": "9999999", "endpoint": "https://s3.amazonaws.com", + "region": "", "path": "/", "s3-uri-style": "host", "secret-key": "test-secret-key", From 6caae64cd2154f983dd601475976eaeee7f588bd Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 4 Sep 2025 15:54:56 +0300 Subject: [PATCH 10/10] Revert "Set default region" This reverts commit e8d42b7f757ecc8cad6134a8f51e4122bf59d1c8. --- src/backups.py | 13 +++++++------ tests/integration/conftest.py | 2 +- tests/unit/test_backups.py | 1 - 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/backups.py b/src/backups.py index bfc8409796..57fa33b872 100644 --- a/src/backups.py +++ b/src/backups.py @@ -105,11 +105,13 @@ def _tls_ca_chain_filename(self) -> str: return "" def _get_s3_session_resource(self, s3_parameters: dict): - session = Session( - aws_access_key_id=s3_parameters["access-key"], - aws_secret_access_key=s3_parameters["secret-key"], - region_name=s3_parameters["region"], - ) + kwargs = { + "aws_access_key_id": s3_parameters["access-key"], + "aws_secret_access_key": s3_parameters["secret-key"], + } + if "region" in s3_parameters: + kwargs["region_name"] = s3_parameters["region"] + session = Session(**kwargs) return session.resource( "s3", endpoint_url=self._construct_endpoint(s3_parameters), @@ -1327,7 +1329,6 @@ def _retrieve_s3_parameters(self) -> tuple[dict, list[str]]: # Add some sensible defaults (as expected by the code) for missing optional parameters s3_parameters.setdefault("endpoint", "https://s3.amazonaws.com") - s3_parameters.setdefault("region", "") s3_parameters.setdefault("path", "") s3_parameters.setdefault("s3-uri-style", "host") s3_parameters.setdefault("delete-older-than-days", "9999999") diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 668459db7c..2740fa317d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -44,7 +44,7 @@ def get_cloud_config(cloud: str) -> tuple[dict[str, str], dict[str, str]] | None "endpoint": "https://storage.googleapis.com", "bucket": "data-charms-testing", "path": f"/postgresql-k8s/{uuid.uuid1()}", - "region": "", + "region": "us-east-1", }, { "access-key": os.environ["GCP_ACCESS_KEY"], "secret-key": os.environ["GCP_SECRET_KEY"], diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 1e62906efa..4267541a2a 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1802,7 +1802,6 @@ def test_retrieve_s3_parameters(harness): "bucket": "test-bucket", "delete-older-than-days": "9999999", "endpoint": "https://s3.amazonaws.com", - "region": "", "path": "/", "s3-uri-style": "host", "secret-key": "test-secret-key",