Skip to content

test(spanner): fix backup restore TimeoutError and premature instance cleanup #17150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sakthivelmanii wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −7
Open

test(spanner): fix backup restore TimeoutError and premature instance cleanup #17150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/google-cloud-spanner/samples/samples/backup_sample_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ def test_copy_backup_with_multiple_kms_keys(


@pytest.mark.dependency(depends=["create_backup"])
@RetryErrors(exception=DeadlineExceeded, max_tries=2)
@RetryErrors(exception=(DeadlineExceeded, TimeoutError), max_tries=2)
def test_restore_database(capsys, instance_id, sample_database):
backup_sample.restore_database(instance_id, RESTORE_DB_ID, BACKUP_ID)
out, _ = capsys.readouterr()
Expand All @@ -145,7 +145,7 @@ def test_restore_database(capsys, instance_id, sample_database):


@pytest.mark.dependency(depends=["create_backup_with_encryption_key"])
@RetryErrors(exception=DeadlineExceeded, max_tries=2)
@RetryErrors(exception=(DeadlineExceeded, TimeoutError), max_tries=2)
def test_restore_database_with_encryption_key(
capsys,
instance_id,
Expand All @@ -164,7 +164,7 @@ def test_restore_database_with_encryption_key(

@pytest.mark.skip(reason="skipped since the KMS keys are not added on test project")
@pytest.mark.dependency(depends=["create_backup_with_multiple_kms_keys"])
@RetryErrors(exception=DeadlineExceeded, max_tries=2)
@RetryErrors(exception=(DeadlineExceeded, TimeoutError), max_tries=2)
def test_restore_database_with_multiple_kms_keys(
capsys,
multi_region_instance_id,
Expand Down Expand Up @@ -243,7 +243,7 @@ def test_cancel_backup(capsys, instance_id, sample_database):
assert cancel_success or cancel_failure


@RetryErrors(exception=DeadlineExceeded, max_tries=2)
@RetryErrors(exception=(DeadlineExceeded, TimeoutError), max_tries=2)
def test_create_database_with_retention_period(capsys, sample_instance):
backup_sample.create_database_with_version_retention_period(
sample_instance.instance_id,
Expand Down
6 changes: 3 additions & 3 deletions packages/google-cloud-spanner/samples/samples/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,14 @@ def scrub_instance_ignore_not_found(to_scrub):
pass

retry_cleanup(to_scrub.delete)()
except exceptions.NotFound:
except exceptions.GoogleAPICallError:
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching exceptions.GoogleAPICallError is overly broad for a function named scrub_instance_ignore_not_found. This base class includes critical errors such as PermissionDenied or Unauthenticated, which, if ignored, could lead to silent failures and resource leaks in the test environment. If the goal is to handle timeouts during cleanup in addition to missing instances, it is safer to catch specific exceptions to avoid masking configuration or permission issues. As per repository rules, avoid broad exception blocks and consider logging the exception (e.g., using logger.warning) instead of returning silently to aid in debugging.

Suggested change
except exceptions.GoogleAPICallError:
except (exceptions.NotFound, exceptions.DeadlineExceeded):
References
  1. Avoid broad except blocks that can mask underlying issues. Catching specific exceptions and logging them helps prevent masking critical failures like permission or authentication issues.

pass


@pytest.fixture(scope="session")
def cleanup_old_instances(spanner_client):
"""Delete instances, created by samples, that are older than an hour."""
cutoff = int(time.time()) - 1 * 60 * 60
"""Delete instances, created by samples, that are older than 4 hours."""
cutoff = int(time.time()) - 4 * 60 * 60
instance_filter = "labels.cloud_spanner_samples:true"

for instance_pb in spanner_client.list_instances(filter_=instance_filter):
Expand Down
Loading