chore(bigtable): prevent test leaks#17350
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates Bigtable system tests by changing cleanup fixtures from session to function scope, reversing deletion order, and catching general exceptions during cleanup. It also adds immediate cleanup logic during instance and backup creation. The review feedback correctly identifies a bug where replacing placeholders in the cleanup lists prevents proper removal during a failure, leading to redundant deletion attempts. It recommends simplifying the code by not replacing the placeholders, which also avoids potential race conditions.
| try: | ||
| instance = await operation.result() | ||
|
|
||
| # replace with full instance object | ||
| instances_to_delete[-1] = instance | ||
|
|
||
| # Create a table within the instance | ||
| create_table_request = admin_v2.CreateTableRequest( | ||
| parent=instance_admin_client.instance_path(project_id, instance_id), | ||
| table_id=TEST_TABLE_NAME, | ||
| table=admin_v2.Table( | ||
| column_families={ | ||
| TEST_COLUMMN_FAMILY_NAME: admin_v2.ColumnFamily(), | ||
| } | ||
| ), | ||
| ) | ||
|
|
||
| instance = await operation.result() | ||
| table = await table_admin_client.create_table(create_table_request) | ||
|
|
||
| # replace with full instance object | ||
| instances_to_delete[-1] = instance | ||
| # Populate with dummy data | ||
| await populate_table( | ||
| table_admin_client, data_client, instance, table, INITIAL_CELL_VALUE | ||
| ) | ||
|
|
||
| # Create a table within the instance | ||
| return instance, table | ||
| except Exception: | ||
| # cleanup immediately on failure | ||
| try: | ||
| await instance_admin_client.delete_instance(name=instance_name) | ||
| except Exception: | ||
| pass | ||
| if instance_placeholder in instances_to_delete: | ||
| instances_to_delete.remove(instance_placeholder) | ||
| raise |
There was a problem hiding this comment.
If an exception occurs during table creation or population (lines 182-197) after instances_to_delete[-1] = instance has executed, the except Exception block on line 200 will attempt to remove instance_placeholder from instances_to_delete. However, since instance_placeholder was already replaced by instance in the list, instance_placeholder in instances_to_delete will evaluate to False, leaving the already-deleted instance in instances_to_delete. This causes a redundant deletion attempt in the fixture's finally block.
Additionally, using instances_to_delete[-1] to replace the placeholder is risky if any concurrent operations modify the list.
Since the instances_to_delete fixture only requires the .name attribute of the objects in the list to perform deletion, and instance_placeholder already has the correct name, we can avoid replacing the placeholder entirely, which simplifies the code and avoids this bug.
try:
instance = await operation.result()
# Create a table within the instance
create_table_request = admin_v2.CreateTableRequest(
parent=instance_admin_client.instance_path(project_id, instance_id),
table_id=TEST_TABLE_NAME,
table=admin_v2.Table(
column_families={
TEST_COLUMMN_FAMILY_NAME: admin_v2.ColumnFamily(),
}
),
)
table = await table_admin_client.create_table(create_table_request)
# Populate with dummy data
await populate_table(
table_admin_client, data_client, instance, table, INITIAL_CELL_VALUE
)
return instance, table
except Exception:
# cleanup immediately on failure
try:
await instance_admin_client.delete_instance(name=instance_name)
except Exception:
pass
if instance_placeholder in instances_to_delete:
instances_to_delete.remove(instance_placeholder)
raise|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the system tests for Google Cloud Bigtable by changing the scope of cleanup fixtures from session to function, reversing the deletion order of instances and backups, and catching general exceptions instead of only NotFound errors during resource cleanup. The feedback recommends logging or printing the caught exceptions instead of swallowing them silently, and including exception details in the print statements when table or view deletions fail to make debugging resource leaks easier.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
The bigtable system tests were leaving left-over instances, which could cause future tests to fail until they were manually cleaned up
This PR improves the test logic to make sure resources are properly cleaned up
Also added a fixture to remove test instances created more than a day ago