Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 0 additions & 5 deletions packages/django-google-spanner/noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,11 +232,6 @@ def system_test(session):

@nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS)
def system(session):
# TODO(https://github.com/googleapis/google-cloud-python/issues/16099):
# System tests were disabled in the split repo to speed up the build process.
# See: https://github.com/googleapis/python-spanner-django/pull/929.
session.skip("System tests are temporarily disabled.")

print("System tests with django 5.2")
system_test(session)

Expand Down
13 changes: 6 additions & 7 deletions packages/django-google-spanner/tests/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,14 @@
# https://developers.google.com/open-source/licenses/bsd

import os
import time
import uuid

DEBUG = True
USE_TZ = True

INSTALLED_APPS = [
"django_spanner", # Must be the first entry
"django.contrib.contenttypes",
"django.contrib.auth",
"django.contrib.sites",
"django.contrib.sessions",
"django.contrib.messages",
"django.contrib.staticfiles",
"tests",
]

Expand All @@ -29,7 +25,10 @@
PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT", "emulator-test-project")

# _get_test_db_name method in creation.py addes prefix of 'test_' to db name.
DATABASE_NAME = os.getenv("DJANGO_SPANNER_DB", "django_test_db")
DATABASE_NAME = os.getenv(
"DJANGO_SPANNER_DB",
f"dj_sys_{int(time.time())}_{uuid.uuid4().hex[:6]}",
)
Comment on lines +28 to +31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Generating a completely random database name (dj_sys_{int(time.time())}_{uuid.uuid4().hex[:6]}) per test run can lead to orphaned databases accumulating in the Spanner instance if a test run crashes or is terminated abruptly (e.g., via SIGKILL, CI timeout, or runner cancellation), as atexit handlers do not run in those cases.

Since Spanner has a limit on the number of databases per instance (typically 100), these orphaned databases will eventually cause all subsequent test runs to fail with quota/limit errors.

Consider one of the following improvements:

  1. Use a static database name (or a name based on a stable environment variable like the CI build ID/job ID) so that subsequent runs can reuse or clean up the same database.
  2. Implement a startup cleanup routine in setup_instance() that lists existing databases in the Spanner instance and deletes any databases with the dj_sys_ prefix that are older than a certain threshold (e.g., 2 hours).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added _cleanup_stale_databases() in setup_instance() to drop any stale dj_sys_<timestamp>_* databases older than 1 hour.


DATABASES = {
"default": {
Expand Down
141 changes: 60 additions & 81 deletions packages/django-google-spanner/tests/system/django_spanner/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,29 @@
# license that can be found in the LICENSE file or at
# https://developers.google.com/open-source/licenses/bsd

import atexit
import os
import time

from django.core.management import call_command
from django.db import connection
from django.conf import settings
from django.db import connections
from google.api_core import exceptions
from google.cloud.spanner_v1 import Client
from google.cloud.spanner_v1.instance import Backup, Instance
from test_utils.retry import RetryErrors
from google.cloud.spanner_v1.database import Database

from django_spanner import USE_EMULATOR
from django_spanner.creation import DatabaseCreation

CREATE_INSTANCE = os.getenv("GOOGLE_CLOUD_TESTS_CREATE_SPANNER_INSTANCE") is not None

SPANNER_OPERATION_TIMEOUT_IN_SECONDS = int(
os.getenv("SPANNER_OPERATION_TIMEOUT_IN_SECONDS", 60)
os.getenv("SPANNER_OPERATION_TIMEOUT_IN_SECONDS", 240)
)
EXISTING_INSTANCES = []
INSTANCE_ID = os.environ.get(
"GOOGLE_CLOUD_TESTS_SPANNER_INSTANCE", "spanner-django-python-systest"
"GOOGLE_CLOUD_TESTS_SPANNER_INSTANCE",
settings.DATABASES["default"]["INSTANCE"],
)
PROJECT_ID = os.getenv(
"GOOGLE_CLOUD_PROJECT",
settings.DATABASES["default"]["PROJECT"],
)
PROJECT_ID = os.getenv("GOOGLE_CLOUD_PROJECT", "emulator-test-project")
DATABASE_NAME = os.getenv("DJANGO_SPANNER_DB", "django_test_db")


class Config(object):
Expand All @@ -38,106 +37,86 @@ class Config(object):
"""

CLIENT = None
INSTANCE_CONFIG = None
INSTANCE = None
DATABASE = None


def _list_instances():
return list(Config.CLIENT.list_instances())
def _cleanup_stale_databases(instance):
"""Drop stale dj_sys_<timestamp>_* databases older than 1 hour."""
cutoff = int(time.time()) - 1 * 60 * 60
try:
for db_pb in instance.list_databases():
db = Database.from_pb(db_pb, instance)
parts = db.database_id.split("_")
if len(parts) >= 3 and parts[0] == "dj" and parts[1] == "sys":
try:
created_ts = int(parts[2])
except ValueError:
continue
if created_ts < cutoff:
try:
db.drop()
except Exception:
pass
except Exception:
pass


def setup_instance():
if Config.INSTANCE is not None:
return

if USE_EMULATOR:
from google.auth.credentials import AnonymousCredentials

Config.CLIENT = Client(project=PROJECT_ID, credentials=AnonymousCredentials())
else:
Config.CLIENT = Client()

retry = RetryErrors(exceptions.ServiceUnavailable)

configs = list(retry(Config.CLIENT.list_instance_configs)())

instances = retry(_list_instances)()
EXISTING_INSTANCES[:] = instances

# Delete test instances that are older than an hour.
cutoff = int(time.time()) - 1 * 60 * 60
instance_pbs = Config.CLIENT.list_instances("labels.python-spanner-systests:true")
for instance_pb in instance_pbs:
instance = Instance.from_pb(instance_pb, Config.CLIENT)
if "created" not in instance.labels:
continue
create_time = int(instance.labels["created"])
if create_time > cutoff:
continue
if not USE_EMULATOR:
# Instance cannot be deleted while backups exist.
for backup_pb in instance.list_backups():
backup = Backup.from_pb(backup_pb, instance)
backup.delete()
instance.delete()

if CREATE_INSTANCE:
if not USE_EMULATOR:
# Defend against back-end returning configs for regions we aren't
# actually allowed to use.
configs = [config for config in configs if "-us-" in config.name]

configs = list(Config.CLIENT.list_instance_configs())
if not configs:
raise ValueError("List instance configs failed in module set up.")

Config.INSTANCE_CONFIG = configs[0]
config_name = configs[0].name
create_time = str(int(time.time()))
labels = {"django-spanner-systests": "true", "created": create_time}

Config.INSTANCE = Config.CLIENT.instance(
INSTANCE_ID, config_name, labels=labels
)
Config.INSTANCE = Config.CLIENT.instance(INSTANCE_ID, configs[0].name)
if not Config.INSTANCE.exists():
created_op = Config.INSTANCE.create()
created_op.result(
SPANNER_OPERATION_TIMEOUT_IN_SECONDS
) # block until completion
created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
else:
Config.INSTANCE.reload()

else:
Config.CLIENT = Client(project=PROJECT_ID)
Config.INSTANCE = Config.CLIENT.instance(INSTANCE_ID)
Config.INSTANCE.reload()
_cleanup_stale_databases(Config.INSTANCE)


def teardown_instance():
if CREATE_INSTANCE:
Config.INSTANCE.delete()
pass


def setup_database():
Config.DATABASE = Config.INSTANCE.database(DATABASE_NAME)
if not Config.DATABASE.exists():
creation = DatabaseCreation(connection)
creation._create_test_db(verbosity=0, autoclobber=False, keepdb=True)
if Config.DATABASE is not None:
return

# Running migrations on the db.
call_command("migrate", interactive=False)
active_db_name = connections["default"].settings_dict["NAME"]
Config.DATABASE = Config.INSTANCE.database(active_db_name)
if not Config.DATABASE.exists():
try:
created_op = Config.DATABASE.create()
created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
except exceptions.AlreadyExists:
pass
Comment on lines +94 to +104

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

When running tests via Django's test runner (e.g., manage.py test), Django automatically prefixes the database name with test_ (e.g., test_dj_sys_...) and updates connections['default'].settings_dict['NAME'] accordingly.

However, DATABASE_NAME in utils.py is initialized at module-import time using settings.DATABASES['default']['NAME'], which does not have the test_ prefix (e.g., dj_sys_...).

As a result:

  1. setup_database() checks and creates the un-prefixed database (dj_sys_...).
  2. Django's test runner creates and runs tests against the prefixed database (test_dj_sys_...).
  3. This leads to two databases being created on Spanner, with the un-prefixed one being completely unused and wasted, doubling the creation/deletion overhead.

To fix this, retrieve the active database name dynamically from connections['default'].settings_dict['NAME'] inside setup_database() instead of using the module-level DATABASE_NAME constant.

Suggested change
if Config.DATABASE is not None:
return
Config.DATABASE = Config.INSTANCE.database(DATABASE_NAME)
if not Config.DATABASE.exists():
creation = DatabaseCreation(connection)
creation._create_test_db(verbosity=0, autoclobber=False, keepdb=True)
# Running migrations on the db.
call_command("migrate", interactive=False)
try:
created_op = Config.DATABASE.create()
created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
except exceptions.AlreadyExists:
pass
if Config.DATABASE is not None:
return
active_db_name = connections['default'].settings_dict['NAME']
Config.DATABASE = Config.INSTANCE.database(active_db_name)
if not Config.DATABASE.exists():
try:
created_op = Config.DATABASE.create()
created_op.result(SPANNER_OPERATION_TIMEOUT_IN_SECONDS)
except exceptions.AlreadyExists:
pass

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated setup_database() to read connections['default'].settings_dict['NAME'] dynamically and removed the redundant module-level DATABASE_NAME constant.



def teardown_database():
from django.db import connections

connections.close_all()

Config.DATABASE = Config.INSTANCE.database(DATABASE_NAME)
if Config.DATABASE.exists():
Config.DATABASE.drop()

# Clear the global client cache to avoid Stale Session errors
# when the next test class creates a new database.
try:
import django_spanner.base
def _cleanup_test_resources():
connections.close_all()
if Config.DATABASE is not None:
try:
if Config.DATABASE.exists():
Config.DATABASE.drop()
except Exception:
pass
Config.DATABASE = None

django_spanner.base._SPANNER_CLIENT_CACHE = None
except ImportError:
pass

atexit.register(_cleanup_test_resources)
Loading