From 7aced7a0e7faecc08f75d4b6925bbd91cafe0125 Mon Sep 17 00:00:00 2001 From: Lucas Gameiro Borges Date: Thu, 4 Jul 2024 01:26:24 +0000 Subject: [PATCH 1/5] add list backup extra info + type check --- src/backups.py | 71 +++++++++++++++++++-- tests/integration/helpers.py | 8 +-- tests/unit/test_backups.py | 119 ++++++++++++++++++++++++++--------- 3 files changed, 159 insertions(+), 39 deletions(-) diff --git a/src/backups.py b/src/backups.py index 217c17b8aa..bf5e0a2848 100644 --- a/src/backups.py +++ b/src/backups.py @@ -342,11 +342,43 @@ def result(): def _format_backup_list(self, backup_list) -> str: """Formats provided list of backups as a table.""" - backups = ["{:<21s} | {:<12s} | {:s}".format("backup-id", "backup-type", "backup-status")] - backups.append("-" * len(backups[0])) - for backup_id, backup_type, backup_status in backup_list: + s3_parameters, _ = self._retrieve_s3_parameters() + backups = [ + "Storage bucket name: {:s}".format(s3_parameters["bucket"]), + "Backups base path: {:s}/backup/\n".format(s3_parameters["path"]), + "{:<20s} | {:<12s} | {:<8s} | {:<20s} | {:<23s} | {:<20s} | {:<20s} | {:s}".format( + "backup-id", + "type", + "status", + "reference-backup-id", + "LSN start/stop", + "start-time", + "finish-time", + "backup-path", + ), + ] + backups.append("-" * len(backups[2])) + for ( + backup_id, + backup_type, + backup_status, + reference, + lsn_start_stop, + start, + stop, + path, + ) in backup_list: backups.append( - "{:<21s} | {:<12s} | {:s}".format(backup_id, backup_type, backup_status) + "{:<20s} | {:<12s} | {:<8s} | {:<20s} | {:<23s} | {:<20s} | {:<20s} | {:s}".format( + backup_id, + backup_type, + backup_status, + reference, + lsn_start_stop, + start, + stop, + path, + ) ) return "\n".join(backups) @@ -368,11 +400,29 @@ def _generate_backup_list_output(self) -> str: backups = json.loads(output)[0]["backup"] for backup in backups: backup_id, backup_type = self._parse_backup_id(backup["label"]) + backup_reference = "None" + if backup["reference"]: + backup_reference, _ = self._parse_backup_id(backup["reference"][-1]) + lsn_start_stop = f'{backup["lsn"]["start"]} / {backup["lsn"]["stop"]}' + time_start, time_stop = ( + datetime.strftime(datetime.fromtimestamp(stamp), "%Y-%m-%dT%H:%M:%SZ") + for stamp in backup["timestamp"].values() + ) + backup_path = f'/{self.stanza_name}/{backup["label"]}' error = backup["error"] backup_status = "finished" if error: backup_status = f"failed: {error}" - backup_list.append((backup_id, backup_type, backup_status)) + backup_list.append(( + backup_id, + backup_type, + backup_status, + backup_reference, + lsn_start_stop, + time_start, + time_stop, + backup_path, + )) return self._format_backup_list(backup_list) def _list_backups(self, show_failed: bool, parse=True) -> OrderedDict[str, str]: @@ -633,6 +683,17 @@ def _on_create_backup_action(self, event) -> None: # noqa: C901 event.fail(error_message) return + if ( + backup_type in ["differential", "incremental"] + and len(self._list_backups(show_failed=False)) == 0 + ): + error_message = ( + f"Invalid backup type: {backup_type}. No previous full backup to reference." + ) + logger.error(f"Backup failed: {error_message}") + event.fail(error_message) + return + logger.info(f"A {backup_type} backup has been requested on unit") can_unit_perform_backup, validation_message = self._can_unit_perform_backup() if not can_unit_perform_backup: diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index d9b491e59a..b7032cbc13 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -1150,8 +1150,8 @@ async def backup_operations( action = await ops_test.model.units.get(replica).run_action("list-backups") await action.wait() backups = action.results.get("backups") - # 2 lines for header output, 1 backup line ==> 3 total lines - assert len(backups.split("\n")) == 3, "full backup is not outputted" + # 5 lines for header output, 1 backup line ==> 6 total lines + assert len(backups.split("\n")) == 6, "full backup is not outputted" await ops_test.model.wait_for_idle(status="active", timeout=1000) # Write some data. @@ -1177,8 +1177,8 @@ async def backup_operations( action = await ops_test.model.units.get(replica).run_action("list-backups") await action.wait() backups = action.results.get("backups") - # 2 lines for header output, 2 backup lines ==> 4 total lines - assert len(backups.split("\n")) == 4, "differential backup is not outputted" + # 5 lines for header output, 2 backup lines ==> 7 total lines + assert len(backups.split("\n")) == 7, "differential backup is not outputted" await ops_test.model.wait_for_idle(status="active", timeout=1000) # Write some data. diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 53018825e9..42b9d67718 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -602,49 +602,98 @@ def test_execute_command(harness): def test_format_backup_list(harness): - # Test when there are no backups. - tc.assertEqual( - harness.charm.backup._format_backup_list([]), - """backup-id | backup-type | backup-status -----------------------------------------------------""", - ) + with patch( + "charms.data_platform_libs.v0.s3.S3Requirer.get_s3_connection_info" + ) as _get_s3_connection_info: + # Test when there are no backups. + _get_s3_connection_info.return_value = { + "bucket": " /test-bucket/ ", + "access-key": " test-access-key ", + "secret-key": " test-secret-key ", + "path": " test-path/ ", + } + assert ( + harness.charm.backup._format_backup_list([]) + == """Storage bucket name: test-bucket +Backups base path: /test-path/backup/ - # Test when there are backups. - backup_list = [ - ("2023-01-01T09:00:00Z", "full", "failed: fake error"), - ("2023-01-01T10:00:00Z", "full", "finished"), - ] - tc.assertEqual( - harness.charm.backup._format_backup_list(backup_list), - """backup-id | backup-type | backup-status ----------------------------------------------------- -2023-01-01T09:00:00Z | full | failed: fake error -2023-01-01T10:00:00Z | full | finished""", - ) +backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path +-----------------------------------------------------------------------------------------------------------------------------------------------------------""" + ) + + # Test when there are backups. + backup_list = [ + ( + "2023-01-01T09:00:00Z", + "full", + "failed: fake error", + "None", + "0/3000000 / 0/5000000", + "2023-01-01T09:00:00Z", + "2023-01-01T09:00:05Z", + "a/b/c", + ), + ( + "2023-01-01T10:00:00Z", + "full", + "finished", + "None", + "0/5000000 / 0/7000000", + "2023-01-01T10:00:00Z", + "2023-01-01T010:00:07Z", + "a/b/d", + ), + ] + assert ( + harness.charm.backup._format_backup_list(backup_list) + == """Storage bucket name: test-bucket +Backups base path: /test-path/backup/ + +backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path +----------------------------------------------------------------------------------------------------------------------------------------------------------- +2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2023-01-01T09:00:00Z | 2023-01-01T09:00:05Z | a/b/c +2023-01-01T10:00:00Z | full | finished | None | 0/5000000 / 0/7000000 | 2023-01-01T10:00:00Z | 2023-01-01T010:00:07Z | a/b/d""" + ) def test_generate_backup_list_output(harness): - with patch("charm.PostgreSQLBackups._execute_command") as _execute_command: + with ( + patch( + "charms.data_platform_libs.v0.s3.S3Requirer.get_s3_connection_info" + ) as _get_s3_connection_info, + patch("charm.PostgreSQLBackups._execute_command") as _execute_command, + ): + _get_s3_connection_info.return_value = { + "bucket": " /test-bucket/ ", + "access-key": " test-access-key ", + "secret-key": " test-secret-key ", + "path": " test-path/ ", + } # Test when no backups are returned. _execute_command.return_value = (0, '[{"backup":[]}]', "") - tc.assertEqual( - harness.charm.backup._generate_backup_list_output(), - """backup-id | backup-type | backup-status -----------------------------------------------------""", + assert ( + harness.charm.backup._generate_backup_list_output() + == """Storage bucket name: test-bucket +Backups base path: /test-path/backup/ + +backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path +-----------------------------------------------------------------------------------------------------------------------------------------------------------""" ) # Test when backups are returned. _execute_command.return_value = ( 0, - '[{"backup":[{"label":"20230101-090000F","error":"fake error"},{"label":"20230101-100000F","error":null}]}]', + '[{"backup":[{"label":"20230101-090000F","error":"fake error","reference":null,"lsn":{"start":"0/3000000","stop":"0/5000000"},"timestamp":{"start":1719866711,"stop":1719866714}}]}]', "", ) - tc.assertEqual( - harness.charm.backup._generate_backup_list_output(), - """backup-id | backup-type | backup-status ----------------------------------------------------- -2023-01-01T09:00:00Z | full | failed: fake error -2023-01-01T10:00:00Z | full | finished""", + assert ( + harness.charm.backup._generate_backup_list_output() + == """Storage bucket name: test-bucket +Backups base path: /test-path/backup/ + +backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path +----------------------------------------------------------------------------------------------------------------------------------------------------------- +2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2024-07-01T20:45:11Z | 2024-07-01T20:45:14Z | /None.postgresql/20230101-090000F""" ) @@ -1230,8 +1279,18 @@ def test_on_create_backup_action(harness): mock_event.fail.assert_called_once() mock_event.set_results.assert_not_called() + # Test when the backup is of type diff/incr when there's no previous full backup. + mock_event.reset_mock() + mock_event.params = {"type": "differential"} + _upload_content_to_s3.return_value = True + _is_primary.return_value = True + harness.charm.backup._on_create_backup_action(mock_event) + mock_event.fail.assert_called_once() + mock_event.set_results.assert_not_called() + # Test when the backup fails. mock_event.reset_mock() + mock_event.params = {"type": "full"} _upload_content_to_s3.return_value = True _is_primary.return_value = True _execute_command.return_value = (1, "", "fake error") From dfa56e4175d1c457e2aafa0e53d818578ce46417 Mon Sep 17 00:00:00 2001 From: Lucas Gameiro Borges Date: Thu, 4 Jul 2024 14:01:31 -0300 Subject: [PATCH 2/5] fix unit test --- tests/unit/test_backups.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 42b9d67718..8afbb17ba4 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -693,7 +693,7 @@ def test_generate_backup_list_output(harness): backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path ----------------------------------------------------------------------------------------------------------------------------------------------------------- -2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2024-07-01T20:45:11Z | 2024-07-01T20:45:14Z | /None.postgresql/20230101-090000F""" +2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2024-07-01T17:45:11Z | 2024-07-01T17:45:14Z | /None.postgresql/20230101-090000F""" ) From 1234b688000d109b46d7d33e99bcd897e478ce39 Mon Sep 17 00:00:00 2001 From: Lucas Gameiro Borges Date: Thu, 4 Jul 2024 14:11:58 -0300 Subject: [PATCH 3/5] revert unit test time --- tests/unit/test_backups.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 8afbb17ba4..42b9d67718 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -693,7 +693,7 @@ def test_generate_backup_list_output(harness): backup-id | type | status | reference-backup-id | LSN start/stop | start-time | finish-time | backup-path ----------------------------------------------------------------------------------------------------------------------------------------------------------- -2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2024-07-01T17:45:11Z | 2024-07-01T17:45:14Z | /None.postgresql/20230101-090000F""" +2023-01-01T09:00:00Z | full | failed: fake error | None | 0/3000000 / 0/5000000 | 2024-07-01T20:45:11Z | 2024-07-01T20:45:14Z | /None.postgresql/20230101-090000F""" ) From a6d3c562dbdc2b7e4ffcdd8af3c9cd3b5fc1a7d9 Mon Sep 17 00:00:00 2001 From: Lucas Gameiro Borges Date: Fri, 5 Jul 2024 11:43:46 -0300 Subject: [PATCH 4/5] add libpq connection string --- src/relations/postgresql_provider.py | 6 ++++++ tests/unit/test_postgresql_provider.py | 12 +++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 6b105fa602..476e04d44b 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -115,6 +115,12 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: # Set the database name self.database_provides.set_database(event.relation.id, database) + # Set connection string URI. + self.database_provides.set_uris( + event.relation.id, + f"postgresql://{user}:{password}@{self.charm.primary_endpoint}:{DATABASE_PORT}/{database}", + ) + self._update_unit_status(event.relation) except ( PostgreSQLCreateDatabaseError, diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index cb8e010943..a1ee3e9038 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -62,7 +62,7 @@ def request_database(_harness): _harness.update_relation_data( rel_id, _harness.charm.app.name, - {"data": "", "username": "", "password": "", "version": "", "database": ""}, + {"data": "", "username": "", "password": "", "uris": "", "version": "", "database": ""}, ) # Simulate the request of a new database plus extra user roles. @@ -94,10 +94,11 @@ def test_on_database_requested(harness): _member_started.side_effect = [False, True, True, True, True, True] _primary_endpoint.side_effect = [ None, - {"1.1.1.1"}, - {"1.1.1.1"}, - {"1.1.1.1"}, - {"1.1.1.1"}, + "1.1.1.1", + "1.1.1.1", + "1.1.1.1", + "1.1.1.1", + "1.1.1.1", ] postgresql_mock.create_user = PropertyMock( side_effect=[None, PostgreSQLCreateUserError, None, None] @@ -141,6 +142,7 @@ def test_on_database_requested(harness): "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', "username": user, "password": "test-password", + "uris": f"postgresql://{user}:test-password@1.1.1.1:5432/{DATABASE}", "version": POSTGRESQL_VERSION, "database": f"{DATABASE}", } From 97a279e3bbcb6220600367c242f835c1ac43e53c Mon Sep 17 00:00:00 2001 From: Lucas Gameiro Borges Date: Fri, 5 Jul 2024 11:44:27 -0300 Subject: [PATCH 5/5] Revert "add libpq connection string" This reverts commit a6d3c562dbdc2b7e4ffcdd8af3c9cd3b5fc1a7d9. --- src/relations/postgresql_provider.py | 6 ------ tests/unit/test_postgresql_provider.py | 12 +++++------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 476e04d44b..6b105fa602 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -115,12 +115,6 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: # Set the database name self.database_provides.set_database(event.relation.id, database) - # Set connection string URI. - self.database_provides.set_uris( - event.relation.id, - f"postgresql://{user}:{password}@{self.charm.primary_endpoint}:{DATABASE_PORT}/{database}", - ) - self._update_unit_status(event.relation) except ( PostgreSQLCreateDatabaseError, diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index a1ee3e9038..cb8e010943 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -62,7 +62,7 @@ def request_database(_harness): _harness.update_relation_data( rel_id, _harness.charm.app.name, - {"data": "", "username": "", "password": "", "uris": "", "version": "", "database": ""}, + {"data": "", "username": "", "password": "", "version": "", "database": ""}, ) # Simulate the request of a new database plus extra user roles. @@ -94,11 +94,10 @@ def test_on_database_requested(harness): _member_started.side_effect = [False, True, True, True, True, True] _primary_endpoint.side_effect = [ None, - "1.1.1.1", - "1.1.1.1", - "1.1.1.1", - "1.1.1.1", - "1.1.1.1", + {"1.1.1.1"}, + {"1.1.1.1"}, + {"1.1.1.1"}, + {"1.1.1.1"}, ] postgresql_mock.create_user = PropertyMock( side_effect=[None, PostgreSQLCreateUserError, None, None] @@ -142,7 +141,6 @@ def test_on_database_requested(harness): "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', "username": user, "password": "test-password", - "uris": f"postgresql://{user}:test-password@1.1.1.1:5432/{DATABASE}", "version": POSTGRESQL_VERSION, "database": f"{DATABASE}", }