From 613d5646fc798822b832743d9fe85616092a48d6 Mon Sep 17 00:00:00 2001 From: Satya Prakash G Date: Thu, 22 Jun 2023 08:14:24 +0000 Subject: [PATCH 1/2] feat(default_retention_period): Changes to backup.create API to support null expire time With the default retention period feature, null value for expiration time is a valid value. If the expiration time is not specified (null) at the time of create backup then the backup will be retained for the duration of maximum retention period. In this commit, the precondition check on expiration time in the create backup API path is removed. The condition remains intact for copy backup API and that would be addressed in a follow-up commit. --- google/cloud/spanner_v1/backup.py | 10 +++-- google/cloud/spanner_v1/instance.py | 3 +- tests/unit/test_backup.py | 57 +++++++++++++++++++++++++++-- tests/unit/test_instance.py | 18 +++++++++ 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/google/cloud/spanner_v1/backup.py b/google/cloud/spanner_v1/backup.py index 2f54cf2167..5abe127c91 100644 --- a/google/cloud/spanner_v1/backup.py +++ b/google/cloud/spanner_v1/backup.py @@ -53,8 +53,9 @@ class Backup(object): :type expire_time: :class:`datetime.datetime` :param expire_time: (Optional) The expire time that will be used to - create the backup. Required if the create method - needs to be called. + create the backup. If the expire time is not specified + then the backup will be retained for the duration of + maximum retention period. :type version_time: :class:`datetime.datetime` :param version_time: (Optional) The version time that was specified for @@ -271,8 +272,6 @@ def create(self): :raises BadRequest: if the database or expire_time values are invalid or expire_time is not set """ - if not self._expire_time: - raise ValueError("expire_time not set") if not self._database and not self._source_backup: raise ValueError("database and source backup both not set") @@ -295,6 +294,9 @@ def create(self): metadata = _metadata_with_prefix(self.name) if self._source_backup: + if not self._expire_time: + raise ValueError("expire_time not set") + request = CopyBackupRequest( parent=self._instance.name, backup_id=self.backup_id, diff --git a/google/cloud/spanner_v1/instance.py b/google/cloud/spanner_v1/instance.py index 1b426f8cc2..2614484603 100644 --- a/google/cloud/spanner_v1/instance.py +++ b/google/cloud/spanner_v1/instance.py @@ -532,7 +532,8 @@ def backup( :type expire_time: :class:`datetime.datetime` :param expire_time: Optional. The expire time that will be used when creating the backup. - Required if the create method needs to be called. + If the expire time is not specified then the backup will be retained + for the duration of maximum retention period. :type version_time: :class:`datetime.datetime` :param version_time: diff --git a/tests/unit/test_backup.py b/tests/unit/test_backup.py index 00621c2148..17f104134a 100644 --- a/tests/unit/test_backup.py +++ b/tests/unit/test_backup.py @@ -27,6 +27,8 @@ class _BaseTest(unittest.TestCase): DATABASE_NAME = INSTANCE_NAME + "/databases/" + DATABASE_ID BACKUP_ID = "backup_id" BACKUP_NAME = INSTANCE_NAME + "/backups/" + BACKUP_ID + DST_BACKUP_ID = "dst_backup_id" + DST_BACKUP_NAME = INSTANCE_NAME + "/backups" + DST_BACKUP_ID def _make_one(self, *args, **kwargs): return self._get_target_class()(*args, **kwargs) @@ -329,11 +331,45 @@ def test_create_instance_not_found(self): ) def test_create_expire_time_not_set(self): - instance = _Instance(self.INSTANCE_NAME) - backup = self._make_one(self.BACKUP_ID, instance, database=self.DATABASE_NAME) + from google.cloud.spanner_admin_database_v1 import Backup + from google.cloud.spanner_admin_database_v1 import CreateBackupRequest + from datetime import datetime + from datetime import timedelta + from datetime import timezone - with self.assertRaises(ValueError): - backup.create() + op_future = object() + client = _Client() + api = client.database_admin_api = self._make_database_admin_api() + api.create_backup.return_value = op_future + + instance = _Instance(self.INSTANCE_NAME, client=client) + version_timestamp = datetime.utcnow() - timedelta(minutes=5) + version_timestamp = version_timestamp.replace(tzinfo=timezone.utc) + backup = self._make_one( + self.BACKUP_ID, + instance, + database=self.DATABASE_NAME, + version_time=version_timestamp, + ) + + backup_pb = Backup( + database=self.DATABASE_NAME, + version_time=version_timestamp, + ) + + future = backup.create() + self.assertIs(future, op_future) + + request = CreateBackupRequest( + parent=self.INSTANCE_NAME, + backup_id=self.BACKUP_ID, + backup=backup_pb, + ) + + api.create_backup.assert_called_once_with( + request=request, + metadata=[("google-cloud-resource-prefix", backup.name)], + ) def test_create_database_not_set(self): instance = _Instance(self.INSTANCE_NAME) @@ -413,6 +449,19 @@ def test_create_w_invalid_encryption_config(self): with self.assertRaises(ValueError): backup.create() + def test_copy_expire_time_not_set(self): + client = _Client() + client.database_admin_api = self._make_database_admin_api() + instance = _Instance(self.INSTANCE_NAME, client=client) + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + source_backup=self.BACKUP_ID, + ) + + with self.assertRaises(ValueError): + backup.create() + def test_exists_grpc_error(self): from google.api_core.exceptions import Unknown diff --git a/tests/unit/test_instance.py b/tests/unit/test_instance.py index f9d1fec6b8..2357183eae 100644 --- a/tests/unit/test_instance.py +++ b/tests/unit/test_instance.py @@ -695,6 +695,24 @@ def test_backup_factory_explicit(self): self.assertIs(backup._expire_time, timestamp) self.assertEqual(backup._encryption_config, encryption_config) + def test_backup_factory_without_expiration_time(self): + from google.cloud.spanner_v1.backup import Backup + + client = _Client(self.PROJECT) + instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) + BACKUP_ID = "backup-id" + DATABASE_NAME = "database-name" + + backup = instance.backup( + BACKUP_ID, + database=DATABASE_NAME, + ) + + self.assertIsInstance(backup, Backup) + self.assertEqual(backup.backup_id, BACKUP_ID) + self.assertIs(backup._instance, instance) + self.assertEqual(backup._database, DATABASE_NAME) + def test_list_backups_defaults(self): from google.cloud.spanner_admin_database_v1 import Backup as BackupPB from google.cloud.spanner_admin_database_v1 import DatabaseAdminClient From 2e53490169039b6a7567150855eb93bb295b9568 Mon Sep 17 00:00:00 2001 From: Satya Prakash G Date: Thu, 22 Jun 2023 11:35:30 +0000 Subject: [PATCH 2/2] feat(default_retention_period): Changes to copy backup path to support null expire time With the default retention period feature, null value for expiration time is a valid value. If the expiration time of the destination backup is not specified (null) at the time of copy backup then the destination backup will be created with the expiration time of the source backup. In this commit, the precondition check on expiration time in the copy backup path is removed. --- google/cloud/spanner_v1/backup.py | 3 - google/cloud/spanner_v1/instance.py | 3 +- tests/unit/test_backup.py | 122 ++++++++++++++++++++++++++++ tests/unit/test_instance.py | 75 ++++++++++++++++- 4 files changed, 196 insertions(+), 7 deletions(-) diff --git a/google/cloud/spanner_v1/backup.py b/google/cloud/spanner_v1/backup.py index 5abe127c91..fec55473c9 100644 --- a/google/cloud/spanner_v1/backup.py +++ b/google/cloud/spanner_v1/backup.py @@ -294,9 +294,6 @@ def create(self): metadata = _metadata_with_prefix(self.name) if self._source_backup: - if not self._expire_time: - raise ValueError("expire_time not set") - request = CopyBackupRequest( parent=self._instance.name, backup_id=self.backup_id, diff --git a/google/cloud/spanner_v1/instance.py b/google/cloud/spanner_v1/instance.py index 2614484603..103e6607e4 100644 --- a/google/cloud/spanner_v1/instance.py +++ b/google/cloud/spanner_v1/instance.py @@ -587,7 +587,8 @@ def copy_backup( :type expire_time: :class:`datetime.datetime` :param expire_time: Optional. The expire time that will be used when creating the copy backup. - Required if the create method needs to be called. + If the expire time is not specified then the backup will be created with + the expiration time of the source backup. :type encryption_config: :class:`~google.cloud.spanner_admin_database_v1.types.CopyBackupEncryptionConfig` or :class:`dict` diff --git a/tests/unit/test_backup.py b/tests/unit/test_backup.py index 17f104134a..abd5e1a134 100644 --- a/tests/unit/test_backup.py +++ b/tests/unit/test_backup.py @@ -450,18 +450,140 @@ def test_create_w_invalid_encryption_config(self): backup.create() def test_copy_expire_time_not_set(self): + from google.cloud.spanner_admin_database_v1 import CopyBackupRequest + + op_future = object() + client = _Client() + api = client.database_admin_api = self._make_database_admin_api() + api.copy_backup.return_value = op_future + instance = _Instance(self.INSTANCE_NAME, client=client) + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + source_backup=self.BACKUP_ID, + ) + + future = backup.create() + self.assertIs(future, op_future) + + request = CopyBackupRequest( + parent=self.INSTANCE_NAME, + backup_id=self.DST_BACKUP_ID, + source_backup=self.BACKUP_ID, + ) + + api.copy_backup.assert_called_once_with( + request=request, + metadata=[("google-cloud-resource-prefix", backup.name)], + ) + + def test_copy_fail_database_and_source_backup_not_set(self): client = _Client() client.database_admin_api = self._make_database_admin_api() instance = _Instance(self.INSTANCE_NAME, client=client) + expire_timestamp = self._make_timestamp() + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + expire_time=expire_timestamp, + ) + + with self.assertRaises(ValueError): + backup.create() + + def test_copy_fail_incompatible_encryption_type(self): + from google.cloud.spanner_admin_database_v1 import CreateBackupEncryptionConfig + + client = _Client() + client.database_admin_api = self._make_database_admin_api() + instance = _Instance(self.INSTANCE_NAME, client=client) + expire_timestamp = self._make_timestamp() + encryption_config = { + "encryption_type": CreateBackupEncryptionConfig.EncryptionType.GOOGLE_DEFAULT_ENCRYPTION, + "kms_key_name": "key_name", + } backup = self._make_one( self.DST_BACKUP_ID, instance, source_backup=self.BACKUP_ID, + expire_time=expire_timestamp, + encryption_config=encryption_config, ) with self.assertRaises(ValueError): backup.create() + def test_copy_success(self): + from google.cloud.spanner_admin_database_v1 import CopyBackupRequest + + op_future = object() + client = _Client() + api = client.database_admin_api = self._make_database_admin_api() + api.copy_backup.return_value = op_future + + instance = _Instance(self.INSTANCE_NAME, client=client) + expire_timestamp = self._make_timestamp() + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + source_backup=self.BACKUP_ID, + expire_time=expire_timestamp, + ) + + future = backup.create() + self.assertIs(future, op_future) + + request = CopyBackupRequest( + parent=self.INSTANCE_NAME, + backup_id=self.DST_BACKUP_ID, + source_backup=self.BACKUP_ID, + expire_time=expire_timestamp, + ) + + api.copy_backup.assert_called_once_with( + request=request, + metadata=[("google-cloud-resource-prefix", backup.name)], + ) + + def test_copy_success_with_encryption_params(self): + from google.cloud.spanner_admin_database_v1 import CopyBackupRequest + from google.cloud.spanner_admin_database_v1 import CreateBackupEncryptionConfig + + op_future = object() + client = _Client() + api = client.database_admin_api = self._make_database_admin_api() + api.copy_backup.return_value = op_future + + instance = _Instance(self.INSTANCE_NAME, client=client) + expire_timestamp = self._make_timestamp() + encryption_config = { + "encryption_type": CreateBackupEncryptionConfig.EncryptionType.CUSTOMER_MANAGED_ENCRYPTION, + "kms_key_name": "key_name", + } + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + source_backup=self.BACKUP_ID, + expire_time=expire_timestamp, + encryption_config=encryption_config, + ) + + future = backup.create() + self.assertIs(future, op_future) + + request = CopyBackupRequest( + parent=self.INSTANCE_NAME, + backup_id=self.DST_BACKUP_ID, + source_backup=self.BACKUP_ID, + expire_time=expire_timestamp, + encryption_config=encryption_config, + ) + + api.copy_backup.assert_called_once_with( + request=request, + metadata=[("google-cloud-resource-prefix", backup.name)], + ) + def test_exists_grpc_error(self): from google.api_core.exceptions import Unknown diff --git a/tests/unit/test_instance.py b/tests/unit/test_instance.py index 2357183eae..be4bfa7edb 100644 --- a/tests/unit/test_instance.py +++ b/tests/unit/test_instance.py @@ -40,6 +40,8 @@ class TestInstance(unittest.TestCase): DATABASE_NAME = "%s/databases/%s" % (INSTANCE_NAME, DATABASE_ID) LABELS = {"test": "true"} FIELD_MASK = ["config", "display_name", "processing_units", "labels"] + BACKUP_ID = "backup_id" + DST_BACKUP_ID = "dst_backup_id" def _getTargetClass(self): from google.cloud.spanner_v1.instance import Instance @@ -700,9 +702,8 @@ def test_backup_factory_without_expiration_time(self): client = _Client(self.PROJECT) instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) - BACKUP_ID = "backup-id" - DATABASE_NAME = "database-name" - + BACKUP_ID = self.BACKUP_ID + DATABASE_NAME = self.DATABASE_NAME backup = instance.backup( BACKUP_ID, database=DATABASE_NAME, @@ -713,6 +714,74 @@ def test_backup_factory_without_expiration_time(self): self.assertIs(backup._instance, instance) self.assertEqual(backup._database, DATABASE_NAME) + def test_copy_backup(self): + import datetime + from google.cloud._helpers import UTC + from google.cloud.spanner_v1.backup import Backup + + client = _Client(self.PROJECT) + instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) + BACKUP_ID = self.DST_BACKUP_ID + SOURCE_BACKUP = self.BACKUP_ID + timestamp = datetime.datetime.utcnow().replace(tzinfo=UTC) + backup = instance.copy_backup( + BACKUP_ID, + source_backup=SOURCE_BACKUP, + expire_time=timestamp, + ) + + self.assertIsInstance(backup, Backup) + self.assertEqual(backup.backup_id, self.DST_BACKUP_ID) + self.assertIs(backup._instance, instance) + self.assertEqual(backup._source_backup, self.BACKUP_ID) + self.assertEqual(backup.expire_time, timestamp) + + def test_copy_backup_with_encryption_params(self): + import datetime + from google.cloud._helpers import UTC + from google.cloud.spanner_v1.backup import Backup + from google.cloud.spanner_admin_database_v1 import CreateBackupEncryptionConfig + + client = _Client(self.PROJECT) + instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) + BACKUP_ID = self.DST_BACKUP_ID + SOURCE_BACKUP = self.BACKUP_ID + timestamp = datetime.datetime.utcnow().replace(tzinfo=UTC) + encryption_config = CreateBackupEncryptionConfig( + encryption_type=CreateBackupEncryptionConfig.EncryptionType.CUSTOMER_MANAGED_ENCRYPTION, + kms_key_name="kms_key_name", + ) + backup = instance.copy_backup( + BACKUP_ID, + source_backup=SOURCE_BACKUP, + expire_time=timestamp, + encryption_config=encryption_config, + ) + + self.assertIsInstance(backup, Backup) + self.assertEqual(backup.backup_id, self.DST_BACKUP_ID) + self.assertIs(backup._instance, instance) + self.assertEqual(backup._source_backup, self.BACKUP_ID) + self.assertEqual(backup.expire_time, timestamp) + self.assertEqual(backup._encryption_config, encryption_config) + + def test_copy_backup_without_expiration_time(self): + from google.cloud.spanner_v1.backup import Backup + + client = _Client(self.PROJECT) + instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) + BACKUP_ID = self.DST_BACKUP_ID + SOURCE_BACKUP = self.BACKUP_ID + backup = instance.copy_backup( + BACKUP_ID, + source_backup=SOURCE_BACKUP, + ) + + self.assertIsInstance(backup, Backup) + self.assertEqual(backup.backup_id, self.DST_BACKUP_ID) + self.assertIs(backup._instance, instance) + self.assertEqual(backup._source_backup, self.BACKUP_ID) + def test_list_backups_defaults(self): from google.cloud.spanner_admin_database_v1 import Backup as BackupPB from google.cloud.spanner_admin_database_v1 import DatabaseAdminClient