From 82490300af28479ddc8d802f0ee5cc762a8acfeb Mon Sep 17 00:00:00 2001 From: Yasen Date: Tue, 14 Jul 2026 17:42:08 +0200 Subject: [PATCH 1/7] initial commit for multidb --- .ci/ansible/start_container.yaml | 7 + .github/workflows/ci.yml | 2 +- .github/workflows/scripts/before_install.sh | 21 + .github/workflows/scripts/script.sh | 12 +- pulpcore/app/apps.py | 128 ++++++- pulpcore/app/contexts.py | 16 + pulpcore/app/db_router.py | 186 +++++++++ pulpcore/app/domain_move.py | 361 ++++++++++++++++++ pulpcore/app/domain_sync.py | 229 +++++++++++ .../commands/analyze-publication.py | 37 +- .../commands/cleanup-moved-domain.py | 138 +++++++ .../management/commands/datarepair-2327.py | 126 +++--- .../app/management/commands/datarepair.py | 252 ++++++------ .../app/management/commands/domain-size.py | 55 +++ .../commands/dump-publications-to-fs.py | 90 +++-- .../commands/handle-artifact-checksums.py | 132 ++++--- .../app/management/commands/migrate-all.py | 204 ++++++++++ pulpcore/app/management/commands/migrate.py | 40 ++ .../app/management/commands/move-domain.py | 292 ++++++++++++++ .../reconcile-cross-plane-references.py | 82 ++++ .../app/management/commands/remove-plugin.py | 64 +++- .../management/commands/repository-size.py | 64 +++- .../app/management/commands/rotate-db-key.py | 30 +- .../app/management/commands/sync-domains.py | 98 +++++ pulpcore/app/migrations/0101_add_domain.py | 55 ++- ...154_domain_database_alias_domain_moving.py | 30 ++ ...resource_content_object_domain_and_more.py | 62 +++ .../app/migrations/0156_migrationstatus.py | 51 +++ pulpcore/app/migrations/0157_domainmove.py | 63 +++ pulpcore/app/models/__init__.py | 6 + pulpcore/app/models/content.py | 5 +- pulpcore/app/models/domain.py | 53 ++- pulpcore/app/models/domain_move.py | 59 +++ pulpcore/app/models/generic.py | 154 +++++++- pulpcore/app/models/migration_status.py | 41 ++ pulpcore/app/models/publication.py | 3 +- pulpcore/app/models/repository.py | 3 +- pulpcore/app/models/role.py | 21 +- pulpcore/app/queryset.py | 63 +++ pulpcore/app/role_util.py | 63 ++- pulpcore/app/serializers/status.py | 29 ++ pulpcore/app/settings.py | 42 ++ pulpcore/app/tasks/reconciliation.py | 141 +++++++ pulpcore/app/util.py | 66 +++- pulpcore/app/views/status.py | 45 +++ pulpcore/app/viewsets/task.py | 12 +- pulpcore/constants.py | 8 + pulpcore/middleware.py | 51 +++ pulpcore/tasking/redis_worker.py | 36 +- pulpcore/tasking/worker.py | 48 ++- pulpcore/tests/unit/content/test_handler.py | 59 ++- pulpcore/tests/unit/models/test_remote.py | 116 +++++- pulpcore/tests/unit/test_domain_move.py | 132 +++++++ .../tests/unit/test_multi_database_routing.py | 320 ++++++++++++++++ pulpcore/tests/unit/test_reconciliation.py | 142 +++++++ 55 files changed, 4268 insertions(+), 377 deletions(-) create mode 100644 pulpcore/app/db_router.py create mode 100644 pulpcore/app/domain_move.py create mode 100644 pulpcore/app/domain_sync.py create mode 100644 pulpcore/app/management/commands/cleanup-moved-domain.py create mode 100644 pulpcore/app/management/commands/domain-size.py create mode 100644 pulpcore/app/management/commands/migrate-all.py create mode 100644 pulpcore/app/management/commands/migrate.py create mode 100644 pulpcore/app/management/commands/move-domain.py create mode 100644 pulpcore/app/management/commands/reconcile-cross-plane-references.py create mode 100644 pulpcore/app/management/commands/sync-domains.py create mode 100644 pulpcore/app/migrations/0154_domain_database_alias_domain_moving.py create mode 100644 pulpcore/app/migrations/0155_createdresource_content_object_domain_and_more.py create mode 100644 pulpcore/app/migrations/0156_migrationstatus.py create mode 100644 pulpcore/app/migrations/0157_domainmove.py create mode 100644 pulpcore/app/models/domain_move.py create mode 100644 pulpcore/app/models/migration_status.py create mode 100644 pulpcore/app/queryset.py create mode 100644 pulpcore/app/tasks/reconciliation.py create mode 100644 pulpcore/tests/unit/test_domain_move.py create mode 100644 pulpcore/tests/unit/test_multi_database_routing.py create mode 100644 pulpcore/tests/unit/test_reconciliation.py diff --git a/.ci/ansible/start_container.yaml b/.ci/ansible/start_container.yaml index e0891b7ab5d..53359a63794 100644 --- a/.ci/ansible/start_container.yaml +++ b/.ci/ansible/start_container.yaml @@ -74,6 +74,13 @@ retries: 2 delay: 5 + - name: "Wait for postgres-satellite" + ansible.builtin.wait_for: + host: "postgres-satellite" + port: 5432 + timeout: 30 + when: "multi_db_test | default(false)" + - name: "Wait for Pulp" ansible.builtin.uri: url: "http://pulp{{ pulp_scenario_settings.api_root | default(pulp_settings.api_root | default('\/pulp\/', True), True) }}api/v3/status/" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 612346b62ff..65abe1de11a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -107,7 +107,7 @@ jobs: uses: "./.github/workflows/test.yml" with: matrix_env: | - [{"TEST": "pulp"}, {"TEST": "azure"}, {"TEST": "s3"}, {"TEST": "lowerbounds"}] + [{"TEST": "pulp"}, {"TEST": "azure"}, {"TEST": "s3"}, {"TEST": "lowerbounds"}, {"TEST": "multi_db"}] deprecations: runs-on: "ubuntu-latest" diff --git a/.github/workflows/scripts/before_install.sh b/.github/workflows/scripts/before_install.sh index 2d8d709cae9..b85d86a6b23 100755 --- a/.github/workflows/scripts/before_install.sh +++ b/.github/workflows/scripts/before_install.sh @@ -123,6 +123,27 @@ pulp_scenario_env: {} VARSYAML fi +# Domain-aware multi-database routing (architecture/domain-db-offloading-design.md) needs a +# second, independently-addressable Postgres instance to exercise the "data_1" satellite alias in +# pulpcore.tests.unit -- this adds a standalone Postgres service container reachable from the +# "pulp" service over the shared "pulp_ci_bridge" docker network. The "data_1" connection settings +# themselves are passed straight to the pytest invocation in script.sh (mirroring how +# PULP_DATABASES__default__USER=postgres is already passed inline there), so this scenario needs +# no change to the "pulp" service's own baked-in environment. +if [ "$TEST" = "multi_db" ]; then + cat >> .ci/ansible/vars/main.yaml << VARSYAML + - name: "postgres-satellite" + image: "docker.io/library/postgres:16" + env: + POSTGRES_USER: "postgres" + POSTGRES_PASSWORD: "postgres" + POSTGRES_DB: "pulp" +multi_db_test: true +pulp_scenario_settings: null +pulp_scenario_env: {} +VARSYAML +fi + cat >> .ci/ansible/vars/main.yaml << VARSYAML ... VARSYAML diff --git a/.github/workflows/scripts/script.sh b/.github/workflows/scripts/script.sh index 65ad9995e02..e9c450d0f28 100755 --- a/.github/workflows/scripts/script.sh +++ b/.github/workflows/scripts/script.sh @@ -129,7 +129,17 @@ cmd_user_prefix bash -c "django-admin makemigrations file --check --dry-run" cmd_user_prefix bash -c "django-admin makemigrations certguard --check --dry-run" # Run unit tests. -cmd_user_prefix bash -c "PULP_DATABASES__default__USER=postgres pytest -v -r sx --color=yes --suppress-no-test-exit-code -p no:pulpcore --durations=20 --pyargs pulpcore.tests.unit" +# For the "multi_db" scenario, also point the "data_1" alias at the standalone +# postgres-satellite service container (see before_install.sh) so +# pulpcore.tests.unit's domain-aware multi-database routing tests +# (architecture/domain-db-offloading-design.md) actually exercise a second, real database +# instead of being skipped. Every other scenario leaves MULTI_DB_ENV empty, so this is a no-op +# for the existing single-DB matrix entries. +MULTI_DB_ENV="" +if [[ "$TEST" == "multi_db" ]]; then + MULTI_DB_ENV="PULP_DATABASES__data_1__ENGINE=django.db.backends.postgresql PULP_DATABASES__data_1__NAME=pulp PULP_DATABASES__data_1__USER=postgres PULP_DATABASES__data_1__PASSWORD=postgres PULP_DATABASES__data_1__HOST=postgres-satellite PULP_DATABASES__data_1__PORT=5432" +fi +cmd_user_prefix bash -c "PULP_DATABASES__default__USER=postgres $MULTI_DB_ENV pytest -v -r sx --color=yes --suppress-no-test-exit-code -p no:pulpcore --durations=20 --pyargs pulpcore.tests.unit" cmd_user_prefix bash -c "PULP_DATABASES__default__USER=postgres pytest -v -r sx --color=yes --suppress-no-test-exit-code -p no:pulpcore --durations=20 --pyargs pulp_file.tests.unit" cmd_user_prefix bash -c "PULP_DATABASES__default__USER=postgres pytest -v -r sx --color=yes --suppress-no-test-exit-code -p no:pulpcore --durations=20 --pyargs pulp_certguard.tests.unit" # Run functional tests diff --git a/pulpcore/app/apps.py b/pulpcore/app/apps.py index f8e12e2570c..4f770cfdaa3 100644 --- a/pulpcore/app/apps.py +++ b/pulpcore/app/apps.py @@ -1,3 +1,4 @@ +import logging import random from collections import defaultdict from gettext import gettext as _ @@ -6,8 +7,8 @@ from django import apps from django.conf import settings from django.core.exceptions import ImproperlyConfigured -from django.db import connection, transaction -from django.db.models.signals import post_migrate, pre_migrate +from django.db import connection, connections, transaction +from django.db.models.signals import post_delete, post_migrate, post_save, pre_migrate from django.utils.module_loading import module_has_submodule from pulpcore.exceptions.plugin import MissingPlugin @@ -126,6 +127,8 @@ def ready(self): self.import_urls() self.import_modelresources() self.import_replicators() + # AccessPolicy/Role are control-plane models (KI-07/KI-13): these hooks must only run + # once, against the control DB, never once per satellite `migrate --database=`. post_migrate.connect( _populate_access_policies, sender=self, @@ -255,14 +258,57 @@ def ready(self): post_migrate.connect( _populate_system_id, sender=self, dispatch_uid="populate_system_id_identifier" ) + # Also deliberately NOT guarded to `using == "default"` -- see + # `_ensure_domains_replicated`'s docstring for why a satellite's own `Domain` row can't + # simply wait for a later, separate `sync-domains` invocation. Connected here, right + # before `_populate_artifact_serving_distribution` (registration order == dispatch + # order for signals connected to the same sender), so that hook -- and any earlier + # plugin post_migrate hook that also needs `Domain` data on a satellite -- never race it. + post_migrate.connect( + _ensure_domains_replicated, + sender=self, + dispatch_uid="ensure_domains_replicated_identifier", + ) + # KI-24: unlike the other post_migrate hooks above, this one is intentionally NOT + # guarded to `using == "default"`. The system ArtifactDistribution is a data-plane + # object (Artifact is data-plane) that must exist on every alias that ends up hosting + # artifacts, so it is deliberately (re-)created once per `migrate --database=` + # run, satellites included. Pinning it to `default` would be actively wrong once a + # domain's artifacts live on a satellite, not just weaker. post_migrate.connect( _populate_artifact_serving_distribution, sender=self, dispatch_uid="populate_artifact_serving_distribution_identifier", ) + # KI-14/KI-15: write-through replication of Domain rows to every configured satellite + # alias. `sync-domains` provides full reconciliation for anything these signals miss + # (bulk_create/update() bypass signals; a satellite down when the signal fired never + # gets retried once it comes back, until reconciliation runs). + from pulpcore.app.domain_sync import on_domain_post_delete, on_domain_post_save + from pulpcore.app.models import Domain + + post_save.connect( + on_domain_post_save, sender=Domain, dispatch_uid="replicate_domain_post_save" + ) + post_delete.connect( + on_domain_post_delete, sender=Domain, dispatch_uid="replicate_domain_post_delete" + ) + + # KI-05: only connect this (deliberately sender-less, since BaseModel is abstract) when + # multi-DB is actually active -- single-DB deployments' native GenericRelation cascade + # already deletes UserRole/GroupRole correctly, so avoid the dispatch cost otherwise. + if len(settings.DATABASES) > 1: + from pulpcore.app.role_util import on_any_model_post_delete + + post_delete.connect( + on_any_model_post_delete, dispatch_uid="cleanup_cross_plane_roles_post_delete" + ) def _clean_app_status(sender, apps, verbosity, **kwargs): + # KI-07: AppStatus is a control-plane model; only ever clean it up against `default`. + if kwargs.get("using", "default") != "default": + return from django.contrib.postgres.functions import TransactionNow from django.db.models import F @@ -276,6 +322,13 @@ def _clean_app_status(sender, apps, verbosity, **kwargs): def _populate_access_policies(sender, apps, verbosity, **kwargs): + # KI-07: AccessPolicy is a control-plane model. Only populate it when migrating the + # control DB -- running this against a satellite alias would attempt to write + # AccessPolicy rows there, which the router would then bounce right back to `default` + # anyway (silently succeeding against the wrong intent) or, worse, create a divergent copy. + if kwargs.get("using", "default") != "default": + return + from pulpcore.app.util import get_view_urlpattern from pulpcore.app.viewsets import LoginViewSet @@ -320,12 +373,20 @@ def _populate_access_policies(sender, apps, verbosity, **kwargs): def _populate_system_id(sender, apps, verbosity, **kwargs): + # KI-07: SystemID is a control-plane model; only ever populate it on `default`. + if kwargs.get("using", "default") != "default": + return SystemID = apps.get_model("core", "SystemID") if not SystemID.objects.exists(): SystemID().save() def _ensure_default_domain(sender, **kwargs): + # KI-07: Domain is control-plane and authoritative on `default` only. Satellite copies of + # the "default" Domain row are populated by replication/`sync-domains`, not by this hook, + # once `default` has actually migrated (see the `migrate-all` ordering: default first). + if kwargs.get("using", "default") != "default": + return table_names = connection.introspection.table_names() if "core_domain" in table_names: from pulpcore.app.util import get_default_domain @@ -343,7 +404,52 @@ def _ensure_default_domain(sender, **kwargs): default.save(skip_hooks=True) +def _ensure_domains_replicated(sender, **kwargs): + """ + Reconcile `Domain` rows onto a satellite alias, inline in its own `migrate` run. + + Why this can't just wait for a separate `sync-domains` call afterwards (e.g. from + `migrate-all`, see `_migrate_satellite_forward`): Django's `post_migrate` signal fires for + every connected receiver at the end of *every* `migrate` invocation, regardless of which + app/migration was targeted -- there is no way to run only *some* of a satellite's + post_migrate hooks, sync `Domain` in between, then run the rest within one `migrate` + process. Since `_populate_artifact_serving_distribution` (KI-24) is deliberately unguarded + so it runs on every alias, and can create data-plane rows there that FK to `Domain`, `Domain` + must already be current on `alias` *before that same post_migrate wave*, not just before the + next `migrate` invocation. Connected right before it for exactly that reason. + + Guarded to skip `default` (nothing to reconcile *onto* the authoritative alias) and to no-op + gracefully if `core_domain` doesn't exist yet on `alias` (e.g. a `migrate --database= + someapp` run that doesn't happen to touch `core` at all -- `_migrate_satellite_forward` + always migrates the whole `core` app first, but this hook can't assume every caller does). + Best-effort like the `post_save`/`post_delete` replication signals in `domain_sync.py`: logs + loudly and lets `sync-domains` catch it later rather than failing the whole `migrate` run. + """ + using = kwargs.get("using", "default") + if using == "default": + return + if "core_domain" not in connections[using].introspection.table_names(): + return + from pulpcore.app.domain_sync import reconcile_domains_to_alias + + try: + reconcile_domains_to_alias(using) + except Exception: + logging.getLogger(__name__).error( + "Reconciling Domain rows to alias '%s' failed during migration. Data-plane objects " + "created on this alias by later migrations/post_migrate hooks that FK to Domain may " + "fail until 'pulpcore-manager sync-domains' is run.", + using, + exc_info=True, + ) + + def _populate_roles(sender, apps, verbosity, **kwargs): + # KI-07/KI-13: Role is a control-plane model; only ever populate it on `default`. This + # guard also covers every plugin's copy of this hook, since all plugins share this same + # `_populate_roles` function (connected per-plugin-app in `PulpPluginAppConfig.ready()`). + if kwargs.get("using", "default") != "default": + return role_prefix = f"{sender.label}." # collect all plugin defined roles desired_roles = {} @@ -403,6 +509,14 @@ def _get_permission(perm): def _populate_artifact_serving_distribution(sender, apps, verbosity, **kwargs): + # KI-24 correctness: the `using` alias this migration ran against is *not* necessarily + # where the router would send an unqualified `ArtifactDistribution.objects` query (the + # router has no instance/ContextVar hint here, so it would default to `default`). Every + # query below must be pinned to `alias` explicitly, or a `migrate --database=` + # run would silently check/create against `default`'s row instead of creating the + # satellite's own -- defeating the whole point of this hook running unguarded on every + # alias. + alias = kwargs.get("using", "default") if ( settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem" or not settings.REDIRECT_TO_OBJECT_STORAGE @@ -415,15 +529,17 @@ def _populate_artifact_serving_distribution(sender, apps, verbosity, **kwargs): print(_("ArtifactDistribution model does not exist. Skipping initialization.")) return try: - ArtifactDistribution.objects.get() + ArtifactDistribution.objects.using(alias).get() except ArtifactDistribution.DoesNotExist: name = f"{random.getrandbits(256):x}" - with transaction.atomic(): - content_guard, _created = ContentRedirectContentGuard.objects.get_or_create( + with transaction.atomic(using=alias): + content_guard, _created = ContentRedirectContentGuard.objects.using( + alias + ).get_or_create( name=name, pulp_type="core.content_redirect", ) - _dist, _created = ArtifactDistribution.objects.get_or_create( + _dist, _created = ArtifactDistribution.objects.using(alias).get_or_create( name=name, pulp_type="core.artifact", defaults={"base_path": name, "content_guard": content_guard}, diff --git a/pulpcore/app/contexts.py b/pulpcore/app/contexts.py index 9e2d6fd1657..d0825e71577 100644 --- a/pulpcore/app/contexts.py +++ b/pulpcore/app/contexts.py @@ -8,6 +8,13 @@ _current_user_func = ContextVar("current_user", default=lambda: None) _current_domain = ContextVar("current_domain", default=None) x_task_diagnostics_var = ContextVar("x_profile_task") +#: Set for the duration of a `migrate` management command run to the alias being migrated (see +#: `pulpcore.app.management.commands.migrate` and `PulpDomainRouter._resolve_db`). Historical +#: ("frozen state") models used inside `RunPython` migrations pre-date the domain router and were +#: never written with an explicit `.using(...)`, so without this the router would silently pin +#: every control-plane query (and every unrouted data-plane query) to `"default"` regardless of +#: which `--database` a given `migrate` invocation actually targets. +_current_migration_alias = ContextVar("current_migration_alias", default=None) @contextmanager @@ -41,6 +48,15 @@ def with_domain(domain): _current_domain.reset(token) +@contextmanager +def with_migration_alias(alias): + token = _current_migration_alias.set(alias) + try: + yield + finally: + _current_migration_alias.reset(token) + + @contextmanager def with_task_context(task): with with_domain(task.pulp_domain), with_guid(task.logging_cid), with_user(task.user): diff --git a/pulpcore/app/db_router.py b/pulpcore/app/db_router.py new file mode 100644 index 00000000000..e5b773e8c32 --- /dev/null +++ b/pulpcore/app/db_router.py @@ -0,0 +1,186 @@ +""" +Domain-aware database router (Phase 1, Layer 1 of the query architecture). + +`PulpDomainRouter` splits models into a "control plane" (always on the `default` database +alias -- Task, Domain itself, RBAC, Django built-ins, etc.) and a "data plane" (routed by the +owning `Domain.database_alias` -- repositories, content, artifacts, and every plugin-defined +model). See `architecture/domain-db-offloading-design.md` for the full design. + +This router is only registered in `DATABASE_ROUTERS` when more than one database alias is +configured (see `pulpcore/app/settings.py`) -- for the overwhelmingly common single-database +deployment, Django's own routing (`django.db.utils.ConnectionRouter._route_db`) never even +consults `self.routers` when it is empty, so this module has zero runtime cost unless a second +alias is actually configured. +""" + +import logging + +from django.apps import apps as django_apps + +from pulpcore.app.contexts import _current_migration_alias +from pulpcore.app.util import get_domain + +logger = logging.getLogger(__name__) + +#: Control-plane models: coordination/bookkeeping data that must always live on the same +#: physical database as the worker/task coordination primitives (`pg_notify`, advisory locks, +#: `SELECT ... FOR UPDATE SKIP LOCKED`), which require a single PostgreSQL instance (see the +#: design doc's "Why Control Plane Stays on the Original RDS"). `Domain` itself is here too: it +#: is authoritative on `default` and merely replicated (read-only, from the routing +#: perspective) to every other alias. +CONTROL_PLANE_LABELS = frozenset( + { + "core.domain", + "core.task", + "core.taskgroup", + "core.taskschedule", + "core.createdresource", + "core.appstatus", + "core.systemid", + "core.accesspolicy", + "core.role", + "core.userrole", + "core.grouprole", + "core.progressreport", + "core.groupprogressreport", + # Fleet-wide migration-orchestration bookkeeping (phase1-migrate-all) -- describes the + # aliases themselves, not any one domain's data. + "core.migrationstatus", + # Historical record of `move-domain` runs (phase2-move-domain) -- fleet-management + # metadata about *when*/*between which aliases* a domain moved, not domain data itself. + "core.domainmove", + # ProfileArtifact spans Task (control) and Artifact (data) -- KI-03. It is kept on the + # control DB alongside Task, its FK cascade parent; the `artifact` FK is a documented + # cross-plane exception handled explicitly (KI-02), not via the router. + "core.profileartifact", + # SigningService (and every plugin's multi-table-inheritance subtype) has no + # `pulp_domain` field at all -- it's a genuinely global, shared-across-domains resource + # (e.g. one signing key used by repositories in many different domains), not + # domain-scoped data. Without this entry it would fall through to the router's + # ContextVar fallback and get silently written to whatever domain happens to be in + # context for the current request/task (e.g. `add-signing-service` run while a + # domain-scoped task is executing), making it invisible to every other domain + # (management-command-audit.md finding). New plugin SigningService subtypes must be + # added here too. + "core.signingservice", + "core.asciiarmoreddetachedsigningservice", + "container.manifestsigningservice", + "rpm.rpmpackagesigningservice", + } +) + +#: Django/contrib apps whose tables are pure framework bookkeeping and always stay on `default`. +CONTROL_PLANE_APPS = frozenset({"auth", "contenttypes", "admin", "sessions"}) + + +class PulpDomainRouter: + """ + Routes data-plane models to the database alias of the `Domain` they belong to, and pins + control-plane models (and Django's own built-in apps) to `default`. + + Resolution order for data-plane models (see "Known Router Limitations and Mitigations" in + the design doc): + + 1. **Instance hint** -- if Django passes the actual model instance being saved/read (most + reliable; e.g. `instance.save()`), and it already has an *in-memory* `pulp_domain_id` and + a *cached* `pulp_domain` relation object (e.g. via `select_related("pulp_domain")`, or + because the domain was explicitly assigned), use `pulp_domain.database_alias`. This never + triggers a fresh query or a `refresh_from_db()` call -- see the inline comments in + `_resolve_db` (KI-27) for why both of those are unsafe here. + 2. **ContextVar** -- set by `DomainMiddleware` for every HTTP request and by + `with_task_context()` for every task; covers the common API/task code path where Django + does not pass an instance hint (e.g. `Model.objects.filter(...)`). + 3. **Safe default** -- `"default"`. If a domain has since been moved to a satellite and its + rows cleaned up from `default`, this returns empty results rather than corrupt data; if + cleanup hasn't run yet, it returns the (still valid) stale copy. Never guesses wrong in a + way that mixes data from two different domains. + """ + + def _is_control_plane(self, model): + label = f"{model._meta.app_label}.{model._meta.model_name}" + return label in CONTROL_PLANE_LABELS or model._meta.app_label in CONTROL_PLANE_APPS + + def _resolve_db(self, model, **hints): + # `apps.get_model(...)` inside a `RunPython` data migration returns a "historical" model + # bound to a per-migration `StateApps` registry, never to the live global one -- a cheap, + # reliable way to tell "this query was issued from inside a migration" apart from + # ordinary app code (see `pulpcore.app.management.commands.migrate` for the full + # rationale). Every pre-existing migration queries these historical models with no + # explicit `.using(...)`, exactly as Django's single-database docs recommend, so without + # this check the control-plane pin below (or the domain ContextVar fallback) would + # silently redirect the query away from whichever alias `migrate` is actually operating + # on -- a no-op as long as every alias's schema+data happens to be in lockstep, but wrong + # the instant they aren't (e.g. `migrate-all` always finishes migrating `default` first). + if model._meta.apps is not django_apps: + migration_alias = _current_migration_alias.get() + if migration_alias is not None: + return migration_alias + + if self._is_control_plane(model): + return "default" + + instance = hints.get("instance") + if instance is not None: + # Inspect `instance.__dict__` directly instead of `hasattr()`/`getattr()`. Both of + # those go through `pulp_domain_id`'s `DeferredAttribute.__get__` (every concrete + # Django field, FK attnames included, gets one via `Field.contribute_to_class`), and + # that descriptor's fallback path (`data[field_name] not in data -> ... + # instance.refresh_from_db(fields=[field_name])`) fires whenever `pulp_domain_id` + # hasn't been *set* on this exact instance yet -- not just for instances loaded with + # `.defer()`/`.only()`, but for any brand-new, still-under-construction instance + # whose class declares another FK *before* `pulp_domain` in field order (e.g. + # `RemoteArtifact`: `content_artifact`, `remote`, then `pulp_domain` -- see + # `models/content.py`). Assigning one of those earlier FKs re-enters the router via + # `ForwardManyToOneDescriptor.__set__` (`instance._state.db = router.db_for_write(..., + # instance=)`), which passes *this* half-built instance right back in + # as the hint -- and since `instance._is_pk_set()` is already true (the PK field is + # processed before any FK in `_meta.concrete_fields` order and has a `default=uuid4`), + # `DeferredAttribute.__get__` takes the `refresh_from_db()` branch instead of the + # "unsaved, no PK yet" `AttributeError` branch, which calls back into the router with + # the *same* instance and recurses until `RecursionError` (KI-27). A plain + # `"pulp_domain_id" in instance.__dict__` membership check never invokes the + # descriptor at all, so it can't trigger that fallback -- it only ever reports + # whether a value has *actually* been stored on this instance already, which is + # exactly what "instance hint" routing should be asking. + if "pulp_domain_id" in instance.__dict__: + # Likewise, don't `getattr(instance, "pulp_domain", None)`: if the FK id is set + # but the related `Domain` row hasn't been fetched (e.g. the instance was loaded + # without `select_related("pulp_domain")`), the descriptor `__get__` would issue + # a fresh query for it on every single call -- a silent N+1 on top of whatever + # relation access or write triggered this router call in the first place (KI-27; + # reproduced by `test_base.py::test_cast` under multi-DB, which expects exactly 1 + # query for `repository.remote` and got 2 once a real `Domain` fetch was mixed + # in). Reading straight from `instance._state.fields_cache` only ever returns a + # value Django already fetched for some other reason (`select_related`, a prior + # `.pulp_domain` access, or the object being explicitly assigned); it never + # issues a query of its own. If the `Domain` isn't already cached, fall through + # to the ContextVar/default resolution below rather than paying for a fetch here. + domain = instance._state.fields_cache.get("pulp_domain") + if domain is not None: + return getattr(domain, "database_alias", "default") + + domain = get_domain() + if domain is not None: + return getattr(domain, "database_alias", "default") + + return "default" + + def db_for_read(self, model, **hints): + return self._resolve_db(model, **hints) + + def db_for_write(self, model, **hints): + return self._resolve_db(model, **hints) + + def allow_relation(self, obj1, obj2, **hints): + # Cross-DB FKs (e.g. cross-plane models) are allowed at the Django level; the "no + # cross-DB join" restriction is enforced at the query layer (CrossDBQuerySetMixin, + # explicit .using() in Layer 3/4 code), not here. Returning True everywhere avoids + # Django raising spurious "relations across databases" errors for FKs we intentionally + # allow to span planes (e.g. Export.task -- see KI-23). + return True + + def allow_migrate(self, db, app_label, model_name=None, **hints): + # Identical schema on every alias (accepted trade-off, see KI-16): every RDS instance, + # satellite or original, gets the full pulpcore + plugin schema so that `allow_migrate` + # never has to reason about which tables "belong" on which alias. + return True diff --git a/pulpcore/app/domain_move.py b/pulpcore/app/domain_move.py new file mode 100644 index 00000000000..a5801442e5a --- /dev/null +++ b/pulpcore/app/domain_move.py @@ -0,0 +1,361 @@ +""" +Shared helpers for the `move-domain` and `cleanup-moved-domain` management commands +(phase2-move-domain / phase2-cleanup), implementing Strategy A ("Read-Only Cutover") from +`architecture/domain-db-offloading-design.md`'s "Domain Movement Procedure". + +Data copy uses Option B from the design doc (application-level Django read+write, filtered by +`pulp_domain_id`) rather than Option A (`pg_dump --where`, requires pg_dump >= 16) or Option C +(`dblink`/`postgres_fdw`, requires network access configured between the two RDS instances +specifically for this purpose): Option B needs nothing beyond the ordinary Django DB connections +this process already holds to every configured alias, at the cost of being slower for very large +domains. Operators with `pg_dump` >= 16 or `postgres_fdw` connectivity already set up between +their RDS instances may prefer Option A/C for a large domain instead -- this module does not +implement either, but nothing here precludes running them manually and then using +`move-domain --skip-copy` (see the command) to drive the rest of the procedure (verification +through cutover) against data copied out-of-band. +""" + +import logging +from contextlib import contextmanager + +from django.apps import apps as django_apps +from django.db import IntegrityError, connections +from django.db.models import ProtectedError, RestrictedError +from django.db.models.fields.files import FileField +from django_lifecycle.mixins import LifecycleModelMixin + +from pulpcore.app.contexts import with_domain +from pulpcore.app.db_router import PulpDomainRouter + +logger = logging.getLogger(__name__) + +_router = PulpDomainRouter() + +#: Data-plane models (per the router's classification -- `PulpDomainRouter._is_control_plane`) +#: that don't carry their own `pulp_domain` field, so `data_plane_models()`'s generic +#: `hasattr(model, "pulp_domain_id")` check can't find them: mostly `ManyToManyField(through=)` +#: join tables (`ContentArtifact`, `RepositoryContent`, `PublishedArtifact`, ...), plus a handful +#: of models that are domain-scoped only transitively through a required FK +#: (`RepositoryVersion` -> `Repository`, most notably). Maps `app_label.model_name` to the +#: lookup used to filter that model's rows down to one domain -- found by an exhaustive +#: full-codebase pass (every concrete, non-control-plane model without a `pulp_domain` field, +#: for pulpcore core + every plugin enabled while auditing this), not by inspection of any one +#: app in isolation. A new plugin-defined model of this same shape must be added here too, or +#: `move-domain`/`cleanup-moved-domain` will silently skip its rows -- `copy_domain_data`'s +#: retry-pass loop turns a missing entry here into a hard failure (an unresolvable FK +#: dependency, since the referencing model's own domain-scoped rows never get copied at all) +#: rather than a silent data loss, so a gap here is at least loud, not silent. +THROUGH_MODEL_DOMAIN_FILTERS = { + "core.contentartifact": "content__pulp_domain_id", + "core.repositorycontent": "repository__pulp_domain_id", + "core.repositoryversion": "repository__pulp_domain_id", + "core.repositoryversioncontentdetails": "repository_version__repository__pulp_domain_id", + "core.publishedartifact": "publication__pulp_domain_id", + "core.distributedpublication": "distribution__pulp_domain_id", + "core.alternatecontentsourcepath": "alternate_content_source__pulp_domain_id", + "core.pulpimporterrepository": "repository__pulp_domain_id", + "core.uploadchunk": "upload__pulp_domain_id", + # ExportedResource (like CreatedResource/UserRole/GroupRole) has no domain field of its own + # -- its `export` FK is what's actually domain-scoped (unlike CreatedResource's `task`, + # which is control-plane, `Export` itself *is* data-plane, see exporter.py). + "core.exportedresource": "export__pulp_domain_id", + # Plugin models found by the same exhaustive pass, extended to every plugin installed while + # auditing this (pulp_container, pulp_rpm, pulp_python) -- confirmed empty for pulp_npm, + # pulp_maven, pulp_hugging_face(_local) (every data-plane model they define already has its + # own `pulp_domain` field). pulp_ostree was not installed in the environment this audit ran + # in and could not be checked -- verify it before relying on this registry for a pulp_ostree + # domain move. + "container.blobmanifest": "manifest__pulp_domain_id", + "container.manifestlistmanifest": "manifest_list__pulp_domain_id", + "rpm.addon": "distribution_tree__pulp_domain_id", + "rpm.checksum": "distribution_tree__pulp_domain_id", + "rpm.image": "distribution_tree__pulp_domain_id", + "rpm.variant": "distribution_tree__pulp_domain_id", + "rpm.rpmpackagesigningresult": "result_package__pulp_domain_id", + "rpm.updatecollection": "update_record__pulp_domain_id", + "rpm.updatereference": "update_record__pulp_domain_id", + "rpm.updatecollectionpackage": "update_collection__update_record__pulp_domain_id", + "python.pythonblocklistentry": "repository__pulp_domain_id", +} + + +class DomainMoveError(Exception): + """Raised for any unrecoverable problem moving/cleaning up a domain's data. + + Deliberately not a Django `CommandError` -- this module is imported by more than one + management command, none of which should have to import Django's management-command + machinery just to catch this. + """ + + +def data_plane_models(): + """ + Every concrete, non-proxy model the router treats as data-plane (see + `PulpDomainRouter._is_control_plane`) together with the field/lookup used to filter its rows + down to one domain, across pulpcore and every installed plugin. + + Returns a list of `(model, lookup)` tuples. Ordering is whatever `apps.get_models()` returns + (app-registration order) -- callers that care about FK dependency order (`copy_domain_data`, + `delete_domain_data`) use a retry-pass loop instead of relying on this order being correct, + since neither `apps.get_models()` nor a plugin's declaration order is guaranteed to already + be topologically sorted by FK dependency (e.g. multi-table-inheritance parents must be + written/deleted in the opposite order from each other). + """ + models = [] + for model in django_apps.get_models(): + if model._meta.proxy or model._meta.auto_created: + continue + if _router._is_control_plane(model): + continue + if hasattr(model, "pulp_domain_id"): + models.append((model, "pulp_domain_id")) + for label, lookup in THROUGH_MODEL_DOMAIN_FILTERS.items(): + try: + model = django_apps.get_model(label) + except LookupError: + # Not every entry's plugin is necessarily installed in a given deployment (e.g. an + # rpm.* entry when pulp_rpm isn't enabled) -- nothing to copy/delete/verify for a + # model that doesn't exist here at all. + continue + models.append((model, lookup)) + return models + + +def _domain_queryset(model, lookup, alias, domain): + return model.objects.using(alias).filter(**{lookup: domain.pk}) + + +def estimate_domain_size(domain, alias): + """ + Step 1 size estimate (design doc): for every data-plane model, this domain's own row count + on `alias` plus that table's total on-disk size for scale/context. + + Mirrors the design doc's own example SQL exactly (`SELECT COUNT(*), + pg_total_relation_size(...) FROM WHERE pulp_domain_id = `): Postgres has no + built-in function for "the on-disk size of just these rows", so `table_total_size_bytes` + below is the size of the *entire* table (data + indexes + TOAST, shared across every domain + that has rows in it), not this domain's share of it -- read it as "this table could be at + most this big", not "this domain's data is this many bytes". `row_count` is the number that + is actually domain-specific. + + Returns a list of dicts, one per data-plane model, each with `model`, `table`, `row_count`, + and `table_total_size_bytes` keys. + """ + rows = [] + for model, lookup in data_plane_models(): + count = _domain_queryset(model, lookup, alias, domain).count() + table = model._meta.db_table + with connections[alias].cursor() as cursor: + cursor.execute("SELECT pg_total_relation_size(%s)", [table]) + (table_size,) = cursor.fetchone() + rows.append( + { + "model": model._meta.label, + "table": table, + "row_count": count, + "table_total_size_bytes": table_size or 0, + } + ) + return rows + + +def _run_passes(models, action, action_description): + """ + Repeatedly apply `action(model, lookup)` to every entry in `models`, deferring any model + that raises an FK-dependency error to a later pass, until either every model has succeeded + or a full pass makes no further progress at all. + + This is the mechanism that makes `copy_domain_data`/`delete_domain_data` correct without + needing to hand-maintain a topological sort of every pulpcore+plugin model's FK graph: a + multi-table-inheritance child (or any other FK-dependent row) simply fails with an + `IntegrityError` (copy: missing parent row) or `ProtectedError`/`RestrictedError` (delete: + still-referenced row) on whichever pass runs before its dependency is satisfied, and + succeeds on a later one once that dependency has been handled. "No progress in a full pass" + means every remaining model is blocked on something *other* than one of its still-remaining + siblings (e.g. a real bug, or a plugin FK to a model this module doesn't know is + domain-scoped) -- that's the only case treated as a hard failure. + """ + remaining = list(models) + results = {} + while remaining: + blocked = [] + progressed = False + for model, lookup in remaining: + try: + results[model._meta.label] = action(model, lookup) + except (IntegrityError, ProtectedError, RestrictedError): + blocked.append((model, lookup)) + else: + progressed = True + if not progressed: + raise DomainMoveError( + f"Could not {action_description} for: " + f"{', '.join(model._meta.label for model, _ in blocked)} " + f"(unresolved FK dependency -- see data_plane_models()/" + f"THROUGH_MODEL_DOMAIN_FILTERS if this is a plugin model this module doesn't " + f"know about)." + ) + remaining = blocked + return results + + +def _copy_model(model, lookup, domain, source_alias, target_alias): + fields = model._meta.concrete_fields + copied = 0 + for row in _domain_queryset(model, lookup, source_alias, domain).iterator(): + values = {} + for field in fields: + value = getattr(row, field.attname) + if isinstance(field, FileField) and value: + # Object storage is shared/global infrastructure and is never moved by a + # domain move (design doc, "Object storage" section) -- only this row's + # *reference* to the already-existing blob moves, not the blob itself. Passing + # the bound `FieldFile` through as-is would make `ArtifactFileField.pre_save` + # (see pulpcore.app.models.fields) think a *newly uploaded* file needs to be + # moved into place, and try to open it by its bare relative name relative to + # the process's cwd -- not the storage backend -- which fails outright. Passing + # the plain stored path string instead makes the freshly-constructed `FieldFile` + # default to `_committed=True` (see `FieldFile.__init__`), so `pre_save` + # correctly treats it as already in place, with no file I/O at all. + value = value.name + values[field.attname] = value + instance = model(**values) + # `_state.adding` forced to False *before* saving, so Django's own `_save_table()` takes + # its normal "try UPDATE (by pk), fall back to INSERT" path instead of the special-case + # optimization it applies to any freshly *constructed* instance whose pk field has a + # `default=` (true for every pulpcore model's `pulp_id`, via `pulp_uuid`): that + # optimization forces an unconditional INSERT purely because the instance was just + # constructed in Python, *regardless* of whether its pk was then explicitly overwritten + # to an already-existing value -- so re-running a partially-completed copy would always + # raise `IntegrityError` on every row a previous, interrupted run already copied. This + # matters even more for multi-table-inheritance models (e.g. `FileContent`): `_state` + # is shared across the *whole* instance, not per-table, so without this, a `FileContent` + # copy would also try to force-INSERT its already-existing `Content` parent-table row + # (if an earlier, separate pass already copied `Content` on its own) and fail the exact + # same way -- forcing `not adding` makes each table level independently try UPDATE first, + # which is exactly the "insert whichever levels don't exist yet, no-op update the rest" + # behavior a safe-to-re-run copy needs. `domain_sync.py`'s replicator does not need this + # trick because it always fetches the existing target row (if any) *before* constructing, + # rather than relying on this after-the-fact override -- either approach works, but a + # get-or-construct dance for every one of the ~40 data-plane models here (some behind an + # indirect lookup, not a plain pk fetch) would be considerably more code for the same + # result. + instance._state.adding = False + # skip_hooks=True: this is a verbatim copy of already-validated data, not a new + # create/update action -- lifecycle hooks (role creation, validation, etc.) must not + # re-fire for it. Only for models that are actually django-lifecycle-enabled though: a + # handful of data-plane models (e.g. `RepositoryVersionContentDetails`) are plain + # `models.Model` and don't accept the `skip_hooks` kwarg at all. + if isinstance(instance, LifecycleModelMixin): + instance.save(using=target_alias, skip_hooks=True) + else: + instance.save(using=target_alias) + copied += 1 + return copied + + +def copy_domain_data(domain, source_alias, target_alias): + """ + Step 3 (Option B) of the design doc's Read-Only Cutover procedure: copy every data-plane row + belonging to `domain` from `source_alias` to `target_alias`. + + Returns a `{model_label: row_count_copied}` dict. Safe to re-run (see `_copy_model`). + + Runs with the domain ContextVar set to `domain` (`with_domain`): `Artifact`'s storage path + (`pulpcore.app.models.storage.get_artifact_path`) and the field-level `DomainStorage` proxy + both resolve the *current* domain from that ContextVar, not from any instance being saved -- + without this, saving a copied `Artifact` row would compute storage paths as if it belonged + to whichever domain (typically "default") happened to be in context when this function was + called, silently mismatching the path the blob was actually written under. + """ + with with_domain(domain): + return _run_passes( + data_plane_models(), + lambda model, lookup: _copy_model(model, lookup, domain, source_alias, target_alias), + "copy data", + ) + + +def _row_checksum(pks): + import hashlib + + return hashlib.sha256(",".join(sorted(str(pk) for pk in pks)).encode()).hexdigest() + + +def verify_domain_data(domain, source_alias, target_alias): + """ + Step 4 of the design doc's procedure: for every data-plane model, compare row counts and a + checksum (SHA-256 over the sorted set of primary keys) between `source_alias` and + `target_alias` for this domain's rows. + + Returns a list of mismatch dicts (empty list means the copy verified clean). Each dict has + `model`, `source_count`, `target_count`, `source_checksum`, `target_checksum`. + """ + mismatches = [] + for model, lookup in data_plane_models(): + source_pks = list( + _domain_queryset(model, lookup, source_alias, domain).values_list("pk", flat=True) + ) + target_pks = list( + _domain_queryset(model, lookup, target_alias, domain).values_list("pk", flat=True) + ) + source_checksum = _row_checksum(source_pks) + target_checksum = _row_checksum(target_pks) + if len(source_pks) != len(target_pks) or source_checksum != target_checksum: + mismatches.append( + { + "model": model._meta.label, + "source_count": len(source_pks), + "target_count": len(target_pks), + "source_checksum": source_checksum, + "target_checksum": target_checksum, + } + ) + return mismatches + + +def _delete_model(model, lookup, domain, alias): + return _domain_queryset(model, lookup, alias, domain).delete()[0] + + +def delete_domain_data(domain, alias): + """ + Step 7 (cleanup) of the design doc's procedure: delete every data-plane row belonging to + `domain` from `alias`. + + Used by `cleanup-moved-domain` to remove the stale copy left on the original alias after a + move has been verified in production; the retry-pass mechanism (`_run_passes`) handles the + FK ordering needed to delete e.g. a `ContentArtifact` row before the `Artifact`/`Content` + rows it `PROTECT`s, without this module needing to hand-maintain that ordering. + + Returns a `{model_label: row_count_deleted}` dict. + """ + return _run_passes( + data_plane_models(), + lambda model, lookup: _delete_model(model, lookup, domain, alias), + "delete data", + ) + + +@contextmanager +def _advisory_lock(lock_id, error_message): + with connections["default"].cursor() as cursor: + cursor.execute("SELECT pg_try_advisory_lock(%s)", [lock_id]) + (acquired,) = cursor.fetchone() + if not acquired: + raise DomainMoveError(error_message) + try: + yield + finally: + cursor.execute("SELECT pg_advisory_unlock(%s)", [lock_id]) + + +def domain_move_lock(): + """Advisory lock held for the duration of a `move-domain` run (see `DOMAIN_MOVE_LOCK`).""" + from pulpcore.constants import DOMAIN_MOVE_LOCK + + return _advisory_lock( + DOMAIN_MOVE_LOCK, + "Could not acquire the domain-move advisory lock. Another 'move-domain' run is " + "already in progress.", + ) diff --git a/pulpcore/app/domain_sync.py b/pulpcore/app/domain_sync.py new file mode 100644 index 00000000000..29bd97a84a1 --- /dev/null +++ b/pulpcore/app/domain_sync.py @@ -0,0 +1,229 @@ +""" +Write-through replication of the `Domain` table to every configured satellite alias. + +`Domain` is a control-plane model: it is authoritative on the `default` database alias, but a +read-only copy of every `Domain` row must also exist on every other configured `DATABASES` alias +so that per-process code (the router, `for_each_domain()`, `Domain.get_storage()`, etc.) can +resolve `database_alias`/`storage_settings`/`moving` without ever needing a cross-database query +(see KI-14/KI-15 in the design doc). + +Two mechanisms keep satellites in sync: + +* `post_save`/`post_delete` signals (connected in `apps.py`) push individual changes as they + happen, with retry + exponential backoff. Best-effort: a replication failure here is logged + loudly but does not fail the triggering request/task. +* The `sync-domains` management command performs a full reconciliation pass, which is required + to catch anything the signals miss (`bulk_create()`/`.update()` bypass Django signals + entirely -- KI-15 -- and a satellite that was down when a signal fired never gets the retried + write once it comes back, until reconciliation runs). +""" + +import logging +import time + +from django.conf import settings + +logger = logging.getLogger(__name__) + +#: Number of attempts made to replicate a single Domain change to a single alias. +REPLICATION_RETRY_ATTEMPTS = 3 +#: Initial delay (seconds) between replication retries; doubled after each failed attempt. +REPLICATION_RETRY_BACKOFF = 1 + + +def satellite_aliases(): + """Return every configured `DATABASES` alias other than `"default"`.""" + return [alias for alias in settings.DATABASES if alias != "default"] + + +def domain_field_values(domain): + """Return a dict of `{attname: value}` for every concrete field on a Domain instance. + + Used to build the `defaults` payload written to satellite aliases, and (minus + `pulp_last_updated`, see `_comparable_domain_field_values`) by `sync-domains` to compare rows + across aliases. + """ + return {field.attname: getattr(domain, field.attname) for field in domain._meta.concrete_fields} + + +def _comparable_domain_field_values(domain): + """Like `domain_field_values`, but excludes `pulp_last_updated` for drift comparisons. + + `pulp_last_updated` is `auto_now=True` (see `BaseModel`), so it is unconditionally + overwritten to "now" by Django on every `.save()` call -- including every replication write + to a satellite. Comparing it verbatim would make a freshly-replicated row look "stale" again + on the very next `reconcile_domains_to_alias` pass, forcing a needless rewrite every time + `sync-domains` runs even with zero real drift. + """ + values = domain_field_values(domain) + values.pop("pulp_last_updated", None) + return values + + +def replicate_domain_save(domain, using=None, attempts=REPLICATION_RETRY_ATTEMPTS): + """Push a single Domain row to every satellite alias (write-through replication).""" + aliases = satellite_aliases() + if not aliases: + return + values = domain_field_values(domain) + pulp_id = values.pop("pulp_id") + for alias in aliases: + if alias == using: + continue + _replicate_one_save(alias, pulp_id, values, attempts) + + +def replicate_domain_delete(pulp_id, using=None, attempts=REPLICATION_RETRY_ATTEMPTS): + """Delete a single Domain row from every satellite alias.""" + aliases = satellite_aliases() + if not aliases: + return + for alias in aliases: + if alias == using: + continue + _replicate_one_delete(alias, pulp_id, attempts) + + +def reconcile_domains_to_alias(alias, dry_run=False): + """ + Full reconciliation of every `Domain` row from `default` (authoritative) onto `alias`. + + Shared by the `sync-domains` management command (reconciling one or every satellite, as an + explicit operator action or as part of `migrate-all`) and `_ensure_domains_replicated` + (`apps.py`, a post_migrate hook that must guarantee `alias`'s `Domain` table is fully caught + up *before* the same post_migrate wave's `_populate_artifact_serving_distribution`, KI-24, + tries to create data-plane rows on `alias` that FK to it -- see the design doc's discussion + of why `migrate --database=` can't otherwise bootstrap a brand new satellite in a + single pass). `alias` must already have a `core_domain` table matching the *current* `Domain` + model's full schema (i.e. the whole `core` app already migrated on `alias`); raises + `django.db.utils.DatabaseError` (uncaught) if it doesn't, since callers are expected to only + invoke this once that precondition holds. + + Returns a `{"missing": set, "extra": set, "stale": set}` report of `pulp_id`s (always + computed, even in `dry_run` mode; empty dict values mean no drift for that category). + """ + from pulpcore.app.models import Domain + + default_domains = {domain.pulp_id: domain for domain in Domain.objects.using("default")} + default_ids = set(default_domains) + + satellite_ids = set(Domain.objects.using(alias).values_list("pulp_id", flat=True)) + + missing = default_ids - satellite_ids + extra = satellite_ids - default_ids + stale = set() + for pulp_id in default_ids & satellite_ids: + satellite_domain = Domain.objects.using(alias).get(pulp_id=pulp_id) + if _comparable_domain_field_values( + default_domains[pulp_id] + ) != _comparable_domain_field_values(satellite_domain): + stale.add(pulp_id) + + if dry_run: + return {"missing": missing, "extra": extra, "stale": stale} + + for pulp_id in missing | stale: + values = domain_field_values(default_domains[pulp_id]) + values.pop("pulp_id") + try: + instance = Domain.objects.using(alias).get(pulp_id=pulp_id) + for key, value in values.items(): + setattr(instance, key, value) + except Domain.DoesNotExist: + instance = Domain(pulp_id=pulp_id, **values) + # skip_hooks: this is a raw replica write of already-validated data (mirroring + # _replicate_one_save), not a new domain-management action -- role creation/validation + # hooks must not re-fire. + instance.save(using=alias, skip_hooks=True) + for pulp_id in extra: + # `default` is authoritative: a Domain that no longer exists there was deleted, and its + # replicated row on this satellite is orphaned. + Domain.objects.using(alias).filter(pulp_id=pulp_id).delete() + + return {"missing": missing, "extra": extra, "stale": stale} + + +def _replicate_one_save(alias, pulp_id, defaults, attempts): + from pulpcore.app.models import Domain + + delay = REPLICATION_RETRY_BACKOFF + for attempt in range(1, attempts + 1): + try: + manager = Domain.objects.using(alias) + try: + instance = manager.get(pulp_id=pulp_id) + for key, value in defaults.items(): + setattr(instance, key, value) + except Domain.DoesNotExist: + instance = Domain(pulp_id=pulp_id, **defaults) + # skip_hooks: this is a raw replica write of already-validated data, not a new + # domain-management action -- role-creation / validation hooks must not re-fire. + instance.save(using=alias, skip_hooks=True) + return + except Exception: + logger.warning( + "Domain replication to alias '%s' failed (attempt %d/%d) for domain %s.", + alias, + attempt, + attempts, + pulp_id, + exc_info=True, + ) + if attempt < attempts: + time.sleep(delay) + delay *= 2 + logger.error( + "Domain replication to alias '%s' failed after %d attempts for domain %s. " + "Run 'pulpcore-manager sync-domains' to reconcile.", + alias, + attempts, + pulp_id, + ) + + +def _replicate_one_delete(alias, pulp_id, attempts): + from pulpcore.app.models import Domain + + delay = REPLICATION_RETRY_BACKOFF + for attempt in range(1, attempts + 1): + try: + Domain.objects.using(alias).filter(pulp_id=pulp_id).delete() + return + except Exception: + logger.warning( + "Domain delete-replication to alias '%s' failed (attempt %d/%d) for domain %s.", + alias, + attempt, + attempts, + pulp_id, + exc_info=True, + ) + if attempt < attempts: + time.sleep(delay) + delay *= 2 + logger.error( + "Domain delete-replication to alias '%s' failed after %d attempts for domain %s. " + "Run 'pulpcore-manager sync-domains' to reconcile.", + alias, + attempts, + pulp_id, + ) + + +def on_domain_post_save(sender, instance, created, using, **kwargs): + """`post_save` receiver for `Domain`. Connected in `apps.py`. + + Only replicates out from `default` (the authoritative alias). Writes performed by + replication itself specify `using=` explicitly, so this guard also + prevents replication from re-triggering itself in a loop. + """ + if using != "default": + return + replicate_domain_save(instance, using=using) + + +def on_domain_post_delete(sender, instance, using, **kwargs): + """`post_delete` receiver for `Domain`. Connected in `apps.py`.""" + if using != "default": + return + replicate_domain_delete(instance.pulp_id, using=using) diff --git a/pulpcore/app/management/commands/analyze-publication.py b/pulpcore/app/management/commands/analyze-publication.py index 07252c96e2d..cfea762c03c 100644 --- a/pulpcore/app/management/commands/analyze-publication.py +++ b/pulpcore/app/management/commands/analyze-publication.py @@ -3,7 +3,7 @@ from django.core.management import BaseCommand, CommandError from django.urls import reverse -from pulpcore.app.models import Artifact, Distribution, Publication +from pulpcore.app.models import Artifact, Distribution, Domain, Publication from pulpcore.app.util import get_view_name_for_model @@ -19,6 +19,18 @@ def add_arguments(self, parser): "--distribution-base-path", required=False, help=_("A base_path of a distribution.") ) parser.add_argument("--tabular", action="store_true", help=_("Display as a table")) + # management-command-audit.md "single-object commands" finding: a bare + # `Publication.objects.get(pk=...)`/`Distribution.objects.get(base_path=...)` (this + # command's original shape) only ever finds the object if it happens to live on + # `default`; a satellite-hosted object would raise `DoesNotExist` with no hint that it + # actually exists on a different alias. Mirrors `repository-size.py`'s existing + # `--domain` convention. + parser.add_argument( + "--domain", + default="default", + required=False, + help=_("The pulp domain the publication/distribution belongs to."), + ) def handle(self, *args, **options): """Implement the command.""" @@ -33,17 +45,28 @@ def handle(self, *args, **options): raise CommandError("Must provide either --publication or --distribution-base-path") elif options["publication"] and options["distribution_base_path"]: raise CommandError("Cannot provide both --publication and --distribution-base-path") - elif options["publication"]: - publication = Publication.objects.get(pk=options["publication"]) + + try: + domain = Domain.objects.get(name=options["domain"]) + except Domain.DoesNotExist: + raise CommandError(_("Domain '{name}' does not exist.").format(name=options["domain"])) + alias = domain.database_alias + + if options["publication"]: + publication = Publication.objects.using(alias).get(pk=options["publication"]) else: - distribution = Distribution.objects.get(base_path=options["distribution_base_path"]) + distribution = Distribution.objects.using(alias).get( + base_path=options["distribution_base_path"] + ) if distribution.publication: publication = distribution.publication elif distribution.repository: repository = distribution.repository - publication = Publication.objects.filter( - repository_version__in=repository.versions.all(), complete=True - ).latest("repository_version", "pulp_created") + publication = ( + Publication.objects.using(alias) + .filter(repository_version__in=repository.versions.all(), complete=True) + .latest("repository_version", "pulp_created") + ) published_artifacts = publication.published_artifact.select_related( "content_artifact__artifact" diff --git a/pulpcore/app/management/commands/cleanup-moved-domain.py b/pulpcore/app/management/commands/cleanup-moved-domain.py new file mode 100644 index 00000000000..f4cb67886c6 --- /dev/null +++ b/pulpcore/app/management/commands/cleanup-moved-domain.py @@ -0,0 +1,138 @@ +from gettext import gettext as _ + +from django.core.management import BaseCommand, CommandError +from django.utils.timezone import now + +from pulpcore.app.domain_move import DomainMoveError, delete_domain_data +from pulpcore.app.models import Domain, DomainMove + + +class Command(BaseCommand): + """ + Step 7 ("Cleanup") of the design doc's Domain Movement Procedure: delete a moved domain's + stale rows from the database alias it moved *away from*. + + Only proceeds for a domain that is not currently mid-move (`Domain.moving` is `False`) and + whose current `database_alias` is not `default` (a domain can only have been moved *to* a + satellite by `move-domain`, never *to* `default` by this tooling -- if `database_alias` is + `default`, either the domain was never moved or it's already been moved back, and there is + nothing on some other alias for this command to legitimately clean up). Until this command + runs, rollback is a one-line `Domain.database_alias` flip back to the original alias with + no data loss -- this command is what makes that no longer true, hence the confirmation + safeguard. + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument("domain", help=_("Name of the previously-moved domain to clean up.")) + parser.add_argument( + "--from", + dest="from_alias", + help=_( + "The alias to delete the domain's stale rows from. Defaults to the " + "`from_alias` of the domain's most recent completed DomainMove record onto its " + "current alias. Required if no such record exists (e.g. the domain was moved " + "by means other than 'move-domain')." + ), + ) + parser.add_argument( + "--force", + action="store_true", + help=_( + "Required. Explicit acknowledgement that this permanently deletes data from " + "'--from' with no way to roll back afterwards." + ), + ) + + def handle(self, *args, **options): + try: + domain = Domain.objects.using("default").get(name=options["domain"]) + except Domain.DoesNotExist: + raise CommandError(_("No domain named '{name}' exists.").format(name=options["domain"])) + + if domain.moving: + raise CommandError( + _( + "Domain '{name}' has moving=True -- a move is in progress. Wait for it to " + "finish (or fail cleanly) before cleaning up." + ).format(name=domain.name) + ) + if domain.database_alias == "default": + raise CommandError( + _( + "Domain '{name}' is currently on 'default' -- nothing to clean up (either " + "it was never moved, or it was already moved back)." + ).format(name=domain.name) + ) + + move = ( + DomainMove.objects.using("default") + .filter(domain=domain, status="completed", to_alias=domain.database_alias) + .order_by("-cutover_at") + .first() + ) + + from_alias = options["from_alias"] or (move and move.from_alias) + if not from_alias: + raise CommandError( + _( + "No completed DomainMove record found for domain '{name}' onto its current " + "alias '{alias}'. Pass --from explicitly (the alias to delete the domain's " + "stale data from) if this domain was moved by means other than " + "'move-domain'." + ).format(name=domain.name, alias=domain.database_alias) + ) + if from_alias == domain.database_alias: + raise CommandError( + _( + "--from ('{alias}') is the domain's current alias; refusing to clean that up." + ).format(alias=from_alias) + ) + + if move and move.monitoring_until and now() < move.monitoring_until: + self.stdout.write( + self.style.WARNING( + _( + "The recommended monitoring window for this move does not end until " + "{until}. Proceeding anyway since you're running this command, but " + "consider waiting." + ).format(until=move.monitoring_until) + ) + ) + + if not options["force"]: + raise CommandError( + _( + "Refusing to delete domain '{name}''s data from '{alias}' without --force. " + "This is permanent and cannot be rolled back afterwards -- re-run with " + "--force once you are certain." + ).format(name=domain.name, alias=from_alias) + ) + + self.stdout.write( + _("Deleting domain '{name}''s data from '{alias}'...").format( + name=domain.name, alias=from_alias + ) + ) + try: + deleted = delete_domain_data(domain, from_alias) + except DomainMoveError as e: + raise CommandError(str(e)) from e + + for label, count in deleted.items(): + if count: + self.stdout.write(f" {label}: {count} row(s) deleted") + + if move: + move.cleaned_up_at = now() + move.status = "cleaned_up" + move.save(update_fields=["cleaned_up_at", "status"]) + + self.stdout.write( + self.style.SUCCESS( + _("Domain '{name}''s data removed from '{alias}'.").format( + name=domain.name, alias=from_alias + ) + ) + ) diff --git a/pulpcore/app/management/commands/datarepair-2327.py b/pulpcore/app/management/commands/datarepair-2327.py index 56a59bab969..0b47ea68d9e 100644 --- a/pulpcore/app/management/commands/datarepair-2327.py +++ b/pulpcore/app/management/commands/datarepair-2327.py @@ -3,11 +3,12 @@ import cryptography from django.conf import settings from django.core.management import BaseCommand -from django.db import connection +from django.db import connections from django.db.models import Q from django.utils.encoding import force_bytes, force_str from pulpcore.app.models import Remote +from pulpcore.app.util import for_each_domain class Command(BaseCommand): @@ -45,64 +46,75 @@ def handle(self, *args, **options): | Q(client_key__isnull=False) ) - number_unencrypted = 0 - number_multi_encrypted = 0 - - for remote_pk in Remote.objects.filter(possibly_affected_remotes).values_list( - "pk", flat=True - ): - try: - remote = Remote.objects.get(pk=remote_pk) - # if we can get the remote successfully, it is either OK or the fields are - # encrypted more than once - except cryptography.fernet.InvalidToken: - # If decryption fails then it probably hasn't been encrypted yet - # get the raw column value, avoiding any Django field handling - with connection.cursor() as cursor: - cursor.execute( - "SELECT username, password, proxy_username, proxy_password, client_key " - "FROM core_remote WHERE pulp_id = %s", - [str(remote_pk)], - ) - row = cursor.fetchone() - - field_values = {} + counts = {"number_unencrypted": 0, "number_multi_encrypted": 0} + + # KI-08 (design doc's own listed example of this bug): `Remote` is data-plane, so a bare + # `Remote.objects.filter(...)` only ever sees `default`'s remotes. `for_each_domain()` + # (Layer 3) re-runs the sweep once per domain with `.using(alias)`/the raw-SQL fallback + # pinned to that domain's own alias. + def _repair_for_domain(domain, alias): + for remote_pk in ( + Remote.objects.using(alias) + .filter(possibly_affected_remotes) + .values_list("pk", flat=True) + ): + try: + remote = Remote.objects.using(alias).get(pk=remote_pk) + # if we can get the remote successfully, it is either OK or the fields are + # encrypted more than once + except cryptography.fernet.InvalidToken: + # If decryption fails then it probably hasn't been encrypted yet + # get the raw column value, avoiding any Django field handling + with connections[alias].cursor() as cursor: + cursor.execute( + "SELECT username, password, proxy_username, proxy_password, " + "client_key FROM core_remote WHERE pulp_id = %s", + [str(remote_pk)], + ) + row = cursor.fetchone() + + field_values = {} + + for field, value in zip(fields, row): + field_values[field] = value - for field, value in zip(fields, row): - field_values[field] = value - - if not dry_run: - Remote.objects.filter(pk=remote_pk).update(**field_values) - number_unencrypted += 1 - else: - times_decrypted = 0 - keep_trying = True - needs_update = False - - while keep_trying: - for field in fields: - field_value = getattr(remote, field) # value gets decrypted once on access - if not field_value: - continue - - try: - # try to decrypt it again - field_value = force_str(fernet.decrypt(force_bytes(field_value))) - # it was decrypted successfully again time, so it was probably - # encrypted multiple times over. lets re-set the value with the - # newly decrypted value - setattr(remote, field, field_value) - needs_update = True - except cryptography.fernet.InvalidToken: - # couldn't be decrypted again, stop here - keep_trying = False - - times_decrypted += 1 - - if needs_update: if not dry_run: - remote.save() - number_multi_encrypted += 1 + Remote.objects.using(alias).filter(pk=remote_pk).update(**field_values) + counts["number_unencrypted"] += 1 + else: + times_decrypted = 0 + keep_trying = True + needs_update = False + + while keep_trying: + for field in fields: + # value gets decrypted once on access + field_value = getattr(remote, field) + if not field_value: + continue + + try: + # try to decrypt it again + field_value = force_str(fernet.decrypt(force_bytes(field_value))) + # it was decrypted successfully again time, so it was probably + # encrypted multiple times over. lets re-set the value with the + # newly decrypted value + setattr(remote, field, field_value) + needs_update = True + except cryptography.fernet.InvalidToken: + # couldn't be decrypted again, stop here + keep_trying = False + + times_decrypted += 1 + + if needs_update: + if not dry_run: + remote.save() + counts["number_multi_encrypted"] += 1 + + for_each_domain(_repair_for_domain) + number_unencrypted = counts["number_unencrypted"] + number_multi_encrypted = counts["number_multi_encrypted"] if dry_run: print("Remotes with un-encrypted fields: {}".format(number_unencrypted)) diff --git a/pulpcore/app/management/commands/datarepair.py b/pulpcore/app/management/commands/datarepair.py index ab998f45752..15e134659ee 100644 --- a/pulpcore/app/management/commands/datarepair.py +++ b/pulpcore/app/management/commands/datarepair.py @@ -3,11 +3,12 @@ import cryptography from django.conf import settings from django.core.management import BaseCommand, CommandError -from django.db import connection +from django.db import connections from django.db.models import Q from django.utils.encoding import force_bytes, force_str from pulpcore.app import models +from pulpcore.app.util import domain_db, for_each_domain class Command(BaseCommand): @@ -48,59 +49,71 @@ def repair_7272(self, options): number_broken = 0 self.stdout.write() + # KI-08: iterate every domain via `domain_db()` (Layer 3) rather than a bare + # `Domain.objects.all()` loop -- without it, every query below would resolve through the + # router's default fallback (`"default"`) regardless of which domain is being processed, + # silently seeing/fixing nothing for a domain hosted on a satellite. `.using(alias)` is + # applied explicitly on every direct queryset here per the Layer 3 convention; + # `rv._content_relationships()` (a `RepositoryContent` queryset built deep inside the + # model) is left unqualified since it correctly picks up the ContextVar `domain_db()` set. for domain in models.Domain.objects.all(): has_printed_domain = False - for repo in models.Repository.objects.filter(pulp_domain=domain): - for rv in models.RepositoryVersion.objects.filter(repository=repo): - needs_fix = False - if rv.content_ids is not None: - cached_id_set = set(rv.content_ids) - repositorycontent_id_set = set( - rv._content_relationships().values_list("content__pk", flat=True) + with domain_db(domain) as alias: + for repo in models.Repository.objects.using(alias).filter(pulp_domain=domain): + for rv in models.RepositoryVersion.objects.using(alias).filter(repository=repo): + needs_fix = False + if rv.content_ids is not None: + cached_id_set = set(rv.content_ids) + repositorycontent_id_set = set( + rv._content_relationships().values_list("content__pk", flat=True) + ) + if cached_id_set != repositorycontent_id_set: + if not has_printed_domain: + self.stdout.write(f'In domain "{domain.name}"') + has_printed_domain = True + + self.stdout.write( + f'\tRepository "{repo.name}" (type "{repo.pulp_type}") ' + f"version {rv.number} has a mismatch between the " + "RepositoryContent and the cached ID set" + ) + needs_fix = True + + repositorycontent_id_count = rv._content_relationships().count() + if repositorycontent_id_count == 0: + continue + rv_count_details = models.RepositoryVersionContentDetails.objects.using( + alias + ).filter( + repository_version=rv, + count_type=models.RepositoryVersionContentDetails.PRESENT, ) - if cached_id_set != repositorycontent_id_set: + + # need to sum across all content types + total_count = sum(rvcd.count for rvcd in rv_count_details) + + if total_count != repositorycontent_id_count: + needs_fix = True if not has_printed_domain: self.stdout.write(f'In domain "{domain.name}"') has_printed_domain = True - self.stdout.write( f'\tRepository "{repo.name}" (type "{repo.pulp_type}") ' f"version {rv.number} has a mismatch between the " - "RepositoryContent and the cached ID set" + "RepositoryContent and RepositoryVersionContentDetails" ) - needs_fix = True - repositorycontent_id_count = rv._content_relationships().count() - if repositorycontent_id_count == 0: - continue - rv_count_details = models.RepositoryVersionContentDetails.objects.filter( - repository_version=rv, - count_type=models.RepositoryVersionContentDetails.PRESENT, - ) - - # need to sum across all content types - total_count = sum(rvcd.count for rvcd in rv_count_details) - - if total_count != repositorycontent_id_count: - needs_fix = True - if not has_printed_domain: - self.stdout.write(f'In domain "{domain.name}"') - has_printed_domain = True - self.stdout.write( - f'\tRepository "{repo.name}" (type "{repo.pulp_type}") ' - f"version {rv.number} has a mismatch between the " - "RepositoryContent and RepositoryVersionContentDetails" - ) - - if needs_fix: - number_broken += 1 + if needs_fix: + number_broken += 1 - if not dry_run: - rv.content_ids = list( - rv._content_relationships().values_list("content__pk", flat=True) - ) - rv.save() - rv._compute_counts() + if not dry_run: + rv.content_ids = list( + rv._content_relationships().values_list( + "content__pk", flat=True + ) + ) + rv.save() + rv._compute_counts() self.stdout.write() @@ -130,64 +143,77 @@ def repair_2327(self, options): | Q(client_key__isnull=False) ) - number_unencrypted = 0 - number_multi_encrypted = 0 - - for remote_pk in models.Remote.objects.filter(possibly_affected_remotes).values_list( - "pk", flat=True - ): - try: - remote = models.Remote.objects.get(pk=remote_pk) - # if we can get the remote successfully, it is either OK or the fields are - # encrypted more than once - except cryptography.fernet.InvalidToken: - # If decryption fails then it probably hasn't been encrypted yet - # get the raw column value, avoiding any Django field handling - with connection.cursor() as cursor: - cursor.execute( - "SELECT username, password, proxy_username, proxy_password, client_key " - "FROM core_remote WHERE pulp_id = %s", - [str(remote_pk)], - ) - row = cursor.fetchone() - - field_values = {} + counts = {"number_unencrypted": 0, "number_multi_encrypted": 0} + + # KI-08: `Remote` is data-plane, so a bare `Remote.objects.filter(...)` (the original + # shape of this method, and the design doc's own listed example of this bug) only ever + # sees `default`'s remotes. `for_each_domain()` (Layer 3) re-runs the whole sweep once + # per domain with `.using(alias)`/the raw-SQL fallback pinned to that domain's own alias. + def _repair_2327_for_domain(domain, alias): + for remote_pk in ( + models.Remote.objects.using(alias) + .filter(possibly_affected_remotes) + .values_list("pk", flat=True) + ): + try: + remote = models.Remote.objects.using(alias).get(pk=remote_pk) + # if we can get the remote successfully, it is either OK or the fields are + # encrypted more than once + except cryptography.fernet.InvalidToken: + # If decryption fails then it probably hasn't been encrypted yet + # get the raw column value, avoiding any Django field handling + with connections[alias].cursor() as cursor: + cursor.execute( + "SELECT username, password, proxy_username, proxy_password, " + "client_key FROM core_remote WHERE pulp_id = %s", + [str(remote_pk)], + ) + row = cursor.fetchone() - for field, value in zip(fields, row): - field_values[field] = value + field_values = {} - if not dry_run: - models.Remote.objects.filter(pk=remote_pk).update(**field_values) - number_unencrypted += 1 - else: - times_decrypted = 0 - keep_trying = True - needs_update = False - - while keep_trying: - for field in fields: - field_value = getattr(remote, field) # value gets decrypted once on access - if not field_value: - continue + for field, value in zip(fields, row): + field_values[field] = value - try: - # try to decrypt it again - field_value = force_str(fernet.decrypt(force_bytes(field_value))) - # it was decrypted successfully again time, so it was probably - # encrypted multiple times over. lets re-set the value with the - # newly decrypted value - setattr(remote, field, field_value) - needs_update = True - except cryptography.fernet.InvalidToken: - # couldn't be decrypted again, stop here - keep_trying = False - - times_decrypted += 1 - - if needs_update: if not dry_run: - remote.save() - number_multi_encrypted += 1 + models.Remote.objects.using(alias).filter(pk=remote_pk).update( + **field_values + ) + counts["number_unencrypted"] += 1 + else: + times_decrypted = 0 + keep_trying = True + needs_update = False + + while keep_trying: + for field in fields: + # value gets decrypted once on access + field_value = getattr(remote, field) + if not field_value: + continue + + try: + # try to decrypt it again + field_value = force_str(fernet.decrypt(force_bytes(field_value))) + # it was decrypted successfully again time, so it was probably + # encrypted multiple times over. lets re-set the value with the + # newly decrypted value + setattr(remote, field, field_value) + needs_update = True + except cryptography.fernet.InvalidToken: + # couldn't be decrypted again, stop here + keep_trying = False + + times_decrypted += 1 + + if needs_update: + if not dry_run: + remote.save() + counts["number_multi_encrypted"] += 1 + + for_each_domain(_repair_2327_for_domain) + number_unencrypted = counts["number_unencrypted"] + number_multi_encrypted = counts["number_multi_encrypted"] if dry_run: print("Remotes with un-encrypted fields: {}".format(number_unencrypted)) @@ -208,24 +234,28 @@ def repair_7465(self, options): number_missing = 0 self.stdout.write() + # KI-08: see repair_7272's comment above -- same `domain_db()`/`.using(alias)` fix. for domain in models.Domain.objects.all(): has_printed_domain = False - for repo in models.Repository.objects.filter(pulp_domain=domain): - for rv in models.RepositoryVersion.objects.filter(repository=repo): - if rv.content_ids is None: - if not has_printed_domain: - self.stdout.write(f'In domain "{domain.name}"') - has_printed_domain = True - number_missing += 1 - self.stdout.write( - f'\tRepository "{repo.name}" (type "{repo.pulp_type}") ' - f"version {rv.number} has a missing content_ids cache" - ) - if not dry_run: - rv.content_ids = list( - rv._content_relationships().values_list("content__pk", flat=True) + with domain_db(domain) as alias: + for repo in models.Repository.objects.using(alias).filter(pulp_domain=domain): + for rv in models.RepositoryVersion.objects.using(alias).filter(repository=repo): + if rv.content_ids is None: + if not has_printed_domain: + self.stdout.write(f'In domain "{domain.name}"') + has_printed_domain = True + number_missing += 1 + self.stdout.write( + f'\tRepository "{repo.name}" (type "{repo.pulp_type}") ' + f"version {rv.number} has a missing content_ids cache" ) - rv.save() + if not dry_run: + rv.content_ids = list( + rv._content_relationships().values_list( + "content__pk", flat=True + ) + ) + rv.save() if not number_missing: self.stdout.write("Finished. (OK)") diff --git a/pulpcore/app/management/commands/domain-size.py b/pulpcore/app/management/commands/domain-size.py new file mode 100644 index 00000000000..6d9af561bf1 --- /dev/null +++ b/pulpcore/app/management/commands/domain-size.py @@ -0,0 +1,55 @@ +from gettext import gettext as _ + +from django.core.management import BaseCommand, CommandError + +from pulpcore.app.domain_move import estimate_domain_size +from pulpcore.app.models import Domain + + +class Command(BaseCommand): + """ + Report per-model row counts (and each table's total on-disk size, for scale/context) for a + domain's data-plane objects on its current database alias. + + Standalone tooling for Step 1 ("Preparation") of the design doc's Domain Movement + Procedure -- `move-domain` also prints this same report automatically before starting a + move, so this command exists for an operator to check ahead of time (e.g. while deciding + which domain is worth moving) without actually starting one. + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument("domain", help=_("Name of the domain to report on.")) + + def handle(self, *args, **options): + try: + domain = Domain.objects.using("default").get(name=options["domain"]) + except Domain.DoesNotExist: + raise CommandError(_("No domain named '{name}' exists.").format(name=options["domain"])) + + self.stdout.write( + _("Domain '{name}' (currently on alias '{alias}'):").format( + name=domain.name, alias=domain.database_alias + ) + ) + report = estimate_domain_size(domain, domain.database_alias) + total_rows = 0 + any_rows = False + for row in report: + if row["row_count"] == 0: + continue + any_rows = True + total_rows += row["row_count"] + self.stdout.write( + " {model}: {rows} row(s) (table '{table}' total size: {size} bytes)".format( + model=row["model"], + rows=row["row_count"], + table=row["table"], + size=row["table_total_size_bytes"], + ) + ) + if not any_rows: + self.stdout.write(_(" No data-plane rows found for this domain.")) + else: + self.stdout.write(_("Total rows across all models: {n}").format(n=total_rows)) diff --git a/pulpcore/app/management/commands/dump-publications-to-fs.py b/pulpcore/app/management/commands/dump-publications-to-fs.py index 299b0c085b3..01b8fe5b805 100644 --- a/pulpcore/app/management/commands/dump-publications-to-fs.py +++ b/pulpcore/app/management/commands/dump-publications-to-fs.py @@ -5,12 +5,13 @@ from django.core.exceptions import ObjectDoesNotExist from django.core.management import BaseCommand, CommandError -from pulpcore.app.models import Distribution, Publication +from pulpcore.app.models import Distribution, Domain, Publication from pulpcore.app.tasks.export import ( UnexportableArtifactException, _export_location_is_clean, _export_publication_to_file_system, ) +from pulpcore.app.util import for_each_domain from pulpcore.app.viewsets.base import NamedModelViewSet from pulpcore.constants import FS_EXPORT_METHODS @@ -23,6 +24,15 @@ class Command(BaseCommand): def add_arguments(self, parser): """Set up arguments.""" parser.add_argument("--publication", required=False, help=_("A publication ID.")) + # management-command-audit.md "single-object commands" finding, same shape as + # analyze-publication.py's --domain flag: a bare `Publication.objects.get(pk=...)` only + # ever finds the object if it lives on `default`. Only meaningful with --publication. + parser.add_argument( + "--domain", + default="default", + required=False, + help=_("The pulp domain --publication belongs to (ignored otherwise)."), + ) parser.add_argument( "--distribution-path-prefix", required=False, @@ -75,45 +85,55 @@ def handle(self, *args, **options): publication_pk = NamedModelViewSet.extract_pk(options["publication"]) except Exception: publication_pk = options["publication"] - publication = Publication.objects.get(pk=publication_pk) + try: + domain = Domain.objects.get(name=options["domain"]) + except Domain.DoesNotExist: + raise CommandError( + _("Domain '{name}' does not exist.").format(name=options["domain"]) + ) + publication = Publication.objects.using(domain.database_alias).get(pk=publication_pk) to_export.append((options["dest"], publication)) # If no publication was specified go through the distributions and dump them if they - # meet the criteria + # meet the criteria. KI-08 (new finding, not previously catalogued): `Distribution` is + # data-plane, so an unqualified "all distributions" scan only ever sees `default`'s + # distributions. `for_each_domain()` re-runs the whole scan once per domain. else: - # If a base_path prefix was provided, filter out distributions with a base path - # that doesn't start with the prefix - if options.get("distribution_path_prefix"): - distributions = Distribution.objects.filter( - base_path__startswith=options["distribution_path_prefix"] - ) - else: - distributions = Distribution.objects.all() - - # Filter out distributions that don't match the type specified (if any) - if options["type"]: - distributions = distributions.filter(pulp_type__startswith=options["type"]) - - # For all matching distributions, if they have a publication, dump it in a directory - # matching the original distribution structure - for distribution in distributions: - if distribution.publication: - publication = distribution.publication - elif distribution.repository: - repository = distribution.repository - # Account for distributions serving the latest publication of a given repository - try: - publication = Publication.objects.filter( - repository_version__in=repository.versions.all(), complete=True - ).latest("repository_version", "pulp_created") - repo_path = os.path.join(options["dest"], distribution.base_path) - to_export.append((repo_path, publication)) - except ObjectDoesNotExist: - logging.warning( - "No publication found for the repo published at '{}': skipping".format( - distribution.base_path + + def _collect_for_domain(domain, alias): + if options.get("distribution_path_prefix"): + distributions = Distribution.objects.using(alias).filter( + base_path__startswith=options["distribution_path_prefix"] + ) + else: + distributions = Distribution.objects.using(alias).all() + + if options["type"]: + distributions = distributions.filter(pulp_type__startswith=options["type"]) + + for distribution in distributions: + if distribution.publication: + publication = distribution.publication + elif distribution.repository: + repository = distribution.repository + try: + publication = ( + Publication.objects.using(alias) + .filter( + repository_version__in=repository.versions.all(), + complete=True, + ) + .latest("repository_version", "pulp_created") ) - ) + repo_path = os.path.join(options["dest"], distribution.base_path) + to_export.append((repo_path, publication)) + except ObjectDoesNotExist: + logging.warning( + "No publication found for the repo published at '{}' in " + "domain '{}': skipping".format(distribution.base_path, domain.name) + ) + + for_each_domain(_collect_for_domain) # Go through all the target directories first, if any of them are dirty, print warnings # and exit - unless the user explicitly asked to go through with it anyway. diff --git a/pulpcore/app/management/commands/handle-artifact-checksums.py b/pulpcore/app/management/commands/handle-artifact-checksums.py index 11c3ba72233..984b8db88f0 100644 --- a/pulpcore/app/management/commands/handle-artifact-checksums.py +++ b/pulpcore/app/management/commands/handle-artifact-checksums.py @@ -8,6 +8,7 @@ from pulpcore import constants from pulpcore.app import pulp_hashlib +from pulpcore.app.util import for_each_domain from pulpcore.plugin.models import ( Artifact, Content, @@ -51,19 +52,27 @@ def _print_out_repository_version_hrefs(self, repo_versions): ) ) - def _show_on_demand_content(self, checksums): + def _show_on_demand_content(self, checksums, alias): query = Q(pk__in=[]) for checksum in checksums: query |= Q(**{f"{checksum}__isnull": False}) - remote_artifacts = RemoteArtifact.objects.filter(query).filter( - content_artifact__artifact__isnull=True + remote_artifacts = ( + RemoteArtifact.objects.using(alias) + .filter(query) + .filter(content_artifact__artifact__isnull=True) ) ras_size = remote_artifacts.aggregate(Sum("size"))["size__sum"] - content_artifacts = ContentArtifact.objects.filter(remoteartifact__pk__in=remote_artifacts) - content = Content.objects.filter(contentartifact__pk__in=content_artifacts) - repo_versions = RepositoryVersion.objects.with_content(content).select_related("repository") + content_artifacts = ContentArtifact.objects.using(alias).filter( + remoteartifact__pk__in=remote_artifacts + ) + content = Content.objects.using(alias).filter(contentartifact__pk__in=content_artifacts) + repo_versions = ( + RepositoryVersion.objects.using(alias) + .with_content(content) + .select_related("repository") + ) self.stdout.write( "Found {} on-demand content units with forbidden checksums.".format(content.count()) @@ -77,7 +86,7 @@ def _show_on_demand_content(self, checksums): self.stdout.write(_("\nAffected repository versions with remote content:")) self._print_out_repository_version_hrefs(repo_versions) - def _show_immediate_content(self, forbidden_checksums): + def _show_immediate_content(self, forbidden_checksums, alias): allowed_checksums = set( constants.ALL_KNOWN_CONTENT_CHECKSUMS.symmetric_difference(forbidden_checksums) ) @@ -89,10 +98,14 @@ def _show_immediate_content(self, forbidden_checksums): for allowed_checksum in allowed_checksums: query_required |= Q(**{f"{allowed_checksum}__isnull": True}) - artifacts = Artifact.objects.filter(query_forbidden | query_required) - content_artifacts = ContentArtifact.objects.filter(artifact__in=artifacts) - content = Content.objects.filter(contentartifact__pk__in=content_artifacts) - repo_versions = RepositoryVersion.objects.with_content(content).select_related("repository") + artifacts = Artifact.objects.using(alias).filter(query_forbidden | query_required) + content_artifacts = ContentArtifact.objects.using(alias).filter(artifact__in=artifacts) + content = Content.objects.using(alias).filter(contentartifact__pk__in=content_artifacts) + repo_versions = ( + RepositoryVersion.objects.using(alias) + .with_content(content) + .select_related("repository") + ) self.stdout.write( "Found {} downloaded content units with forbidden or missing checksums.".format( @@ -110,11 +123,11 @@ def _show_immediate_content(self, forbidden_checksums): self.stdout.write(_("\nAffected repository versions with present content:")) self._print_out_repository_version_hrefs(repo_versions) - def _download_artifact(self, artifact, checksum, file_path): + def _download_artifact(self, artifact, checksum, file_path, alias): restored = False - for ca in artifact.content_memberships.all(): + for ca in artifact.content_memberships.using(alias).all(): if not restored: - for ra in ca.remoteartifact_set.all(): + for ra in ca.remoteartifact_set.using(alias).all(): remote = ra.remote.cast() if remote.policy == "immediate": self.stdout.write(_("Restoring missing file {}").format(file_path)) @@ -153,8 +166,15 @@ def _report(self, allowed_checksums): allowed_checksums ) - self._show_on_demand_content(forbidden_checksums) - self._show_immediate_content(forbidden_checksums) + # KI-08 (design doc's own listed example of this bug): every model touched below + # (`Artifact`, `Content`, `ContentArtifact`, `RemoteArtifact`, `RepositoryVersion`) is + # data-plane, so this report is re-run once per domain via `for_each_domain()`. + def _report_for_domain(domain, alias): + self.stdout.write(_("\n=== Domain '{name}' ===").format(name=domain.name)) + self._show_on_demand_content(forbidden_checksums, alias) + self._show_immediate_content(forbidden_checksums, alias) + + for_each_domain(_report_for_domain) def handle(self, *args, **options): if options["report"]: @@ -167,30 +187,40 @@ def handle(self, *args, **options): log.setLevel(logging.ERROR) hrefs = set() - for checksum in settings.ALLOWED_CONTENT_CHECKSUMS: - params = {f"{checksum}__isnull": True} - artifacts_qs = Artifact.objects.filter(**params) - artifacts = [] - for a in artifacts_qs.iterator(): - hasher = pulp_hashlib.new(checksum) - try: - with a.file as fp: - for chunk in fp.chunks(CHUNK_SIZE): - hasher.update(chunk) - setattr(a, checksum, hasher.hexdigest()) - except FileNotFoundError: - file_path = os.path.join(settings.MEDIA_ROOT, a.file.name) - restored = self._download_artifact(a, checksum, file_path) - if not restored: - hrefs.add(file_path) - artifacts.append(a) - - if len(artifacts) >= 1000: - Artifact.objects.bulk_update(objs=artifacts, fields=[checksum], batch_size=1000) - artifacts.clear() - - if artifacts: - Artifact.objects.bulk_update(objs=artifacts, fields=[checksum]) + + # KI-08 (design doc's own listed example of this bug): `Artifact` is data-plane, so an + # unqualified `Artifact.objects.filter(...)`/`.bulk_update(...)` (the original shape of + # this loop) only ever touches `default`'s artifacts. `for_each_domain()` re-runs the + # whole populate/repair pass once per domain with `.using(alias)`. + def _populate_missing_for_domain(domain, alias): + for checksum in settings.ALLOWED_CONTENT_CHECKSUMS: + params = {f"{checksum}__isnull": True} + artifacts_qs = Artifact.objects.using(alias).filter(**params) + artifacts = [] + for a in artifacts_qs.iterator(): + hasher = pulp_hashlib.new(checksum) + try: + with a.file as fp: + for chunk in fp.chunks(CHUNK_SIZE): + hasher.update(chunk) + setattr(a, checksum, hasher.hexdigest()) + except FileNotFoundError: + file_path = os.path.join(settings.MEDIA_ROOT, a.file.name) + restored = self._download_artifact(a, checksum, file_path, alias) + if not restored: + hrefs.add(file_path) + artifacts.append(a) + + if len(artifacts) >= 1000: + Artifact.objects.using(alias).bulk_update( + objs=artifacts, fields=[checksum], batch_size=1000 + ) + artifacts.clear() + + if artifacts: + Artifact.objects.using(alias).bulk_update(objs=artifacts, fields=[checksum]) + + for_each_domain(_populate_missing_for_domain) if hrefs: raise CommandError( @@ -200,12 +230,20 @@ def handle(self, *args, **options): forbidden_checksums = set(constants.ALL_KNOWN_CONTENT_CHECKSUMS).difference( settings.ALLOWED_CONTENT_CHECKSUMS ) - for checksum in forbidden_checksums: - search_params = {f"{checksum}__isnull": False} - update_params = {f"{checksum}": None} - artifacts_qs = Artifact.objects.filter(**search_params) - if artifacts_qs.exists(): - self.stdout.write("Removing forbidden checksum {} from database".format(checksum)) - artifacts_qs.update(**update_params) + + def _remove_forbidden_for_domain(domain, alias): + for checksum in forbidden_checksums: + search_params = {f"{checksum}__isnull": False} + update_params = {f"{checksum}": None} + artifacts_qs = Artifact.objects.using(alias).filter(**search_params) + if artifacts_qs.exists(): + self.stdout.write( + "Removing forbidden checksum {} from database (domain '{}')".format( + checksum, domain.name + ) + ) + artifacts_qs.update(**update_params) + + for_each_domain(_remove_forbidden_for_domain) self.stdout.write(_("Finished aligning checksums with settings.ALLOWED_CONTENT_CHECKSUMS")) diff --git a/pulpcore/app/management/commands/migrate-all.py b/pulpcore/app/management/commands/migrate-all.py new file mode 100644 index 00000000000..4dc13260766 --- /dev/null +++ b/pulpcore/app/management/commands/migrate-all.py @@ -0,0 +1,204 @@ +from contextlib import contextmanager +from gettext import gettext as _ + +from django.conf import settings +from django.core.management import BaseCommand, CommandError, call_command +from django.db import connections +from django.utils.timezone import now + +from pulpcore.app.models import MigrationStatus +from pulpcore.constants import MIGRATION_ORCHESTRATOR_LOCK + +#: A brand new satellite's forward migration is split into two `migrate` invocations around +#: this checkpoint -- see `_migrate_satellite_forward` for why neither a single uninterrupted +#: `migrate` run nor `sync-domains` alone can bootstrap a satellite correctly. This must be the +#: whole `core` app (not just the migration that first creates `core_domain`, e.g. +#: `0101_add_domain`): `Domain`'s schema keeps changing in later `core` migrations too (e.g. +#: `0128_domain_pulp_labels`, `0154_domain_database_alias_domain_moving`), and `sync-domains` +#: reads/writes `Domain` through the live ORM model -- which reflects *all* of those fields -- +#: so the satellite's `core_domain` table must already match that full, current schema before +#: `sync-domains` can query it, not just exist. +DOMAIN_TABLE_CHECKPOINT = ["core"] + + +@contextmanager +def _orchestrator_lock(): + """ + Hold a PostgreSQL session-level advisory lock on `default` for the duration of a + `migrate-all` run, so two orchestration runs (e.g. two pods restarting at once, or a second + invocation while one is already in progress) can't race each other across `DATABASES` + aliases. Fails fast (rather than blocking) if the lock is already held -- an operator + re-running `migrate-all` while one is already in progress should see a clear error, not hang. + + Session-level (not transaction-level): released automatically if the holding connection + drops (e.g. the pod running this command crashes), matching the design doc's failure-mode + table ("Pod crashes mid-migrate-all: Advisory lock released on disconnect"). + """ + with connections["default"].cursor() as cursor: + cursor.execute("SELECT pg_try_advisory_lock(%s)", [MIGRATION_ORCHESTRATOR_LOCK]) + (acquired,) = cursor.fetchone() + if not acquired: + raise CommandError( + _( + "Could not acquire the migration-orchestrator advisory lock. Another " + "'migrate-all' run is already in progress." + ) + ) + try: + yield + finally: + cursor.execute("SELECT pg_advisory_unlock(%s)", [MIGRATION_ORCHESTRATOR_LOCK]) + + +class Command(BaseCommand): + """ + Migrate every configured `DATABASES` alias, in the correct order. + + Every RDS instance -- `default` and every satellite -- runs an identical Django schema + (accepted trade-off, see KI-16 in the design doc), so `allow_migrate` never has to reason + about which tables "belong" on which alias; this command just runs Django's own `migrate` + once per alias, in an order that respects the one real cross-alias dependency: satellite + bootstrap logic (e.g. `get_domain_pk()`'s default-domain lookup, run as a migration default + via `Domain.objects.using()` reads) can depend on the `Domain` table already + being current, so `default` -- where `Domain` is authoritative -- always migrates first on + the way forward. Each satellite's own forward migration is further split around the + migration that first creates `core_domain` (see `_migrate_satellite_forward`), with a + `sync-domains --alias=` in between, so its `Domain` row exists before any + later migration or post_migrate hook on that satellite needs to FK against it. Rollback + (`--target`) reverses the alias ordering -- satellites roll back first, `default` last, so + `default`'s schema (and the `Domain` table specifically) is never older than any satellite's + expectation of it while a rollback is in progress -- but does not need the same split, since + rolling back never needs to create new FK-referencing rows. + + Safe to re-run: already-migrated aliases are no-ops for Django's own `migrate`, and a + partial failure only affects the alias it failed on -- aliases already handled in this run + (or a previous one) are unaffected. See `MigrationStatus` for a durable per-alias record of + the last outcome (also surfaced on the `/status/` endpoint). + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument( + "--target", + nargs=2, + metavar=("APP", "MIGRATION"), + help=_( + "Roll back every alias to this migration (Django 'app migration_name' syntax, " + "e.g. 'core 0154'). Without --target, migrates every alias to its latest " + "migration." + ), + ) + + def handle(self, *args, **options): + target = options.get("target") + aliases = [alias for alias in settings.DATABASES if alias != "default"] + if target: + # Rollback ordering: satellites first, `default` last. + ordered_aliases = aliases + ["default"] + else: + # Forward ordering: `default` first (authoritative Domain table + control plane). + ordered_aliases = ["default"] + aliases + + with _orchestrator_lock(): + for alias in ordered_aliases: + if alias == "default": + self._migrate_one(alias, target) + if not target: + # Populate satellites' Domain rows once `default`'s own Domain table is + # current: a satellite migration's data defaults (e.g. `get_domain_pk()`) + # may need to resolve a `Domain` row on that satellite already. + self._sync_domains() + elif target: + # Rollback: no bootstrap dance needed, just roll back like any other alias. + self._migrate_one(alias, target) + else: + self._migrate_satellite_forward(alias) + + def _migrate_satellite_forward(self, alias): + """ + Forward-migrate one brand new (or partially migrated) satellite alias. + + Split into two `migrate` invocations around the point where `core_domain` starts to + exist, because a single uninterrupted `migrate --database=` run can't satisfy + both ordering constraints at once: + + * `sync-domains` needs `alias` to already have a `core_domain` table matching the live + `Domain` model's full current schema, so it can read/write through the ORM -- only + true once the whole `core` app is migrated on `alias` (see `DOMAIN_TABLE_CHECKPOINT`). + * Every app's post_migrate hooks -- including this satellite's own, e.g. + `_populate_artifact_serving_distribution` (KI-24), which is deliberately unguarded so + it runs on every alias -- fire once *all* apps finish migrating, and can create + data-plane rows that FK to `Domain`; they need the `Domain` *row* (not just the table) + already replicated to `alias` by then, i.e. before any plugin app's migrations run. + + So: migrate only as far as the checkpoint, reconcile just this one alias (other, + not-yet-migrated satellites may not have a `core_domain` table yet either, hence + `--alias` rather than a full `sync-domains` sweep), then finish the rest. + """ + self._migrate_one(alias, DOMAIN_TABLE_CHECKPOINT, record_status=False) + self._sync_domains(alias=alias) + self._migrate_one(alias, None) + + def _migrate_one(self, alias, target, record_status=True): + # Note: no "running" pre-write here. `MigrationStatus` itself lives in the `core` app's + # migrations, so on a genuinely fresh alias (first-ever bootstrap of a brand new `default`, + # or a satellite's very first `migrate-all` run) `core_migrationstatus` doesn't exist + # *yet* -- writing to it before `call_command("migrate", ...)` has had a chance to create + # it would itself fail. Only write status after the migrate attempt, by which point the + # table exists on `alias` regardless of outcome (`migrate` creates tables before running + # any RunPython data migrations that could fail). + self.stdout.write(_("Migrating database alias '{alias}'...").format(alias=alias)) + args = ["migrate", "--database", alias, "--noinput"] + if target: + args.extend(target) + try: + call_command(*args) + except Exception as e: + if record_status: + self._record_status(alias, "failed", error=str(e)) + raise CommandError( + _("Migration failed for database alias '{alias}': {error}").format( + alias=alias, error=e + ) + ) from e + else: + if record_status: + self._record_status(alias, "complete", completed_at=now()) + self.stdout.write( + self.style.SUCCESS(_("Database alias '{alias}' migrated.").format(alias=alias)) + ) + + def _record_status(self, alias, status, **defaults): + defaults.setdefault("error", None) + try: + MigrationStatus.objects.update_or_create( + database_alias=alias, defaults={"status": status, **defaults} + ) + except Exception: + # Best-effort bookkeeping only -- never let a MigrationStatus write failure mask + # the actual migration outcome above. + self.stderr.write( + self.style.WARNING( + _("Could not record MigrationStatus for alias '{alias}'.").format(alias=alias) + ) + ) + + def _sync_domains(self, alias=None): + try: + if alias: + call_command("sync-domains", alias=alias) + else: + call_command("sync-domains") + except Exception: + # Best-effort: `sync-domains` can be re-run manually, and a satellite that's + # unreachable right now for reconciliation will simply be migrated in its + # then-current (possibly Domain-stale) state -- not fatal to the orchestration run. + self.stderr.write( + self.style.WARNING( + _( + "Domain sync to satellite '{alias}' failed; continuing with satellite " + "migrations. Run 'pulpcore-manager sync-domains' manually afterwards." + ).format(alias=alias or "*") + ) + ) diff --git a/pulpcore/app/management/commands/migrate.py b/pulpcore/app/management/commands/migrate.py new file mode 100644 index 00000000000..3f0d3f98a27 --- /dev/null +++ b/pulpcore/app/management/commands/migrate.py @@ -0,0 +1,40 @@ +from django.core.management.commands.migrate import Command as _DjangoMigrateCommand +from django.db.utils import DEFAULT_DB_ALIAS + +from pulpcore.app.contexts import with_migration_alias + + +class Command(_DjangoMigrateCommand): + """ + Thin wrapper around Django's own `migrate` command that records which `--database` alias is + currently being migrated, for the duration of the run, in the `_current_migration_alias` + ContextVar (see `pulpcore.app.contexts`). + + Why this exists: `PulpDomainRouter` (`db_router.py`) normally pins every control-plane model + (e.g. `Task`) to `"default"` and every data-plane model to whatever `Domain` is in context -- + correct for ordinary request/task code, but wrong inside a `RunPython` data migration. Every + pre-existing pulpcore/plugin migration was written before this router existed and queries its + `apps.get_model(...)` ("historical state") models with no explicit `.using(...)`, exactly as + Django's own docs recommend for the *single-database* case. Without this wrapper, migrating a + satellite alias would silently have those bare queries redirected by the router to `default` + instead of the satellite actually being migrated -- harmless (a no-op) as long as `default` has + an identical, in-sync schema, but broken the instant `default` has already progressed further + through the migration graph than the satellite currently being migrated (exactly the case for + `migrate-all`, which always migrates `default` to completion first) -- see the design doc's + router limitations. `PulpDomainRouter` uses this ContextVar to route bare queries against + *historical* (migration-state) models to the alias actually being migrated instead, and only + falls back to its normal control-/data-plane logic for models obtained the ordinary way (i.e. + everywhere outside of a migration). + + This shadows Django's built-in `migrate` command for every entry point -- `manage.py migrate`, + `call_command("migrate", ...)` (as used by `migrate-all`), and any third-party tooling that + invokes it the same way -- because Django's command loader lets an installed app's management + command override a built-in one of the same name. + """ + + help = _DjangoMigrateCommand.__doc__ + + def handle(self, *args, **options): + alias = options.get("database") or DEFAULT_DB_ALIAS + with with_migration_alias(alias): + return super().handle(*args, **options) diff --git a/pulpcore/app/management/commands/move-domain.py b/pulpcore/app/management/commands/move-domain.py new file mode 100644 index 00000000000..1f77654a9ed --- /dev/null +++ b/pulpcore/app/management/commands/move-domain.py @@ -0,0 +1,292 @@ +from datetime import timedelta +from gettext import gettext as _ + +from django.conf import settings +from django.core.management import BaseCommand, CommandError +from django.db import connections +from django.db.migrations.executor import MigrationExecutor +from django.db.utils import OperationalError +from django.utils.timezone import now + +from pulpcore.app.domain_move import ( + DomainMoveError, + copy_domain_data, + domain_move_lock, + estimate_domain_size, + verify_domain_data, +) +from pulpcore.app.models import Domain, DomainMove, Task +from pulpcore.constants import TASK_INCOMPLETE_STATES + +#: Default length of the Step 6 "Monitoring" window (design doc: "Observe for N days +#: (configurable, default 7)"). +DEFAULT_MONITORING_DAYS = 7 + + +class Command(BaseCommand): + """ + Move a domain's data-plane objects to a different `DATABASES` alias. + + Implements Strategy A ("Read-Only Cutover") from the design doc's Domain Movement + Procedure: the domain is set read-only (`Domain.moving = True`) for the duration of the + data copy, which is simpler to implement than Strategy B's incremental sync but leaves the + domain unavailable for writes for the whole copy -- acceptable for the moderate-sized + domains this tooling targets, and the only strategy implemented by this command (Strategy B + remains "backburner" per the design doc; passing `--strategy incremental` fails fast with a + clear message rather than silently falling back to Strategy A). + + Steps 1-6 of the procedure are all handled by a single invocation of this command: + preparation/verification, read-only mode, data copy (Option B -- application-level Django + read+write; see `pulpcore.app.domain_move`), verification (row counts + checksums), cutover, + and recording the Step 6 monitoring window on a new `DomainMove` row. Step 7 (cleanup of the + stale rows left on the original alias) is a separate, deliberately more cautious command: + `cleanup-moved-domain`. + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument("domain", help=_("Name of the domain to move.")) + parser.add_argument( + "--to", + required=True, + dest="to_alias", + help=_("Target DATABASES alias to move the domain's data to."), + ) + parser.add_argument( + "--strategy", + default="read-only", + choices=["read-only", "incremental"], + help=_( + "Movement strategy. Only 'read-only' (Strategy A) is implemented; 'incremental' " + "(Strategy B) remains on the backburner in the design doc pending a sync-" + "strategy decision." + ), + ) + parser.add_argument( + "--skip-copy", + action="store_true", + help=_( + "Skip the data-copy step, assuming the domain's data was already copied to the " + "target alias out-of-band (e.g. via pg_dump/postgres_fdw, see the module " + "docstring in pulpcore.app.domain_move). Verification and cutover still run." + ), + ) + parser.add_argument( + "--monitoring-days", + type=int, + default=DEFAULT_MONITORING_DAYS, + help=_( + "Length, in days, of the Step 6 monitoring window recorded on the DomainMove " + "row. Default: %(default)s." + ), + ) + parser.add_argument( + "--noinput", + "--no-input", + action="store_false", + dest="interactive", + help=_("Do not prompt for confirmation before starting the move."), + ) + + def handle(self, *args, **options): + if options["strategy"] == "incremental": + raise CommandError( + _( + "Strategy B (incremental sync) is not implemented -- see the 'Domain " + "Movement Procedure' section of architecture/domain-db-offloading-design.md. " + "Use --strategy read-only." + ) + ) + + to_alias = options["to_alias"] + if to_alias not in settings.DATABASES: + raise CommandError( + _("'{alias}' is not a configured DATABASES alias.").format(alias=to_alias) + ) + + try: + domain = Domain.objects.using("default").get(name=options["domain"]) + except Domain.DoesNotExist: + raise CommandError(_("No domain named '{name}' exists.").format(name=options["domain"])) + + self._validate_preconditions(domain, to_alias) + + size_report = estimate_domain_size(domain, domain.database_alias) + self._print_size_report(domain, size_report) + + if options["interactive"]: + confirm = input( + _( + "This will make domain '{name}' read-only for the duration of the copy " + "(may take a long time for large domains) and then move its data from " + "'{source}' to '{target}'. Continue? [y/N]: " + ).format(name=domain.name, source=domain.database_alias, target=to_alias) + ) + if confirm.strip().lower() not in ("y", "yes"): + self.stdout.write(_("Aborted.")) + return + + with domain_move_lock(): + self._move(domain, to_alias, options) + + def _validate_preconditions(self, domain, to_alias): + if domain.name == "default": + raise CommandError( + _( + "The 'default' domain can never be moved -- it is the only domain " + "guaranteed to exist on every deployment, and bootstrap/migration code " + "throughout pulpcore assumes it is reachable without any domain context." + ) + ) + if domain.moving: + raise CommandError( + _( + "Domain '{name}' already has moving=True -- either another move is in " + "progress (check for a concurrent 'move-domain' run) or a previous move " + "was interrupted. Resolve manually (inspect the latest DomainMove row for " + "this domain) before retrying." + ).format(name=domain.name) + ) + if domain.database_alias == to_alias: + raise CommandError( + _("Domain '{name}' is already on alias '{alias}'.").format( + name=domain.name, alias=to_alias + ) + ) + + self._validate_alias_ready(to_alias) + + incomplete = Task.objects.using("default").filter( + pulp_domain=domain, state__in=TASK_INCOMPLETE_STATES + ) + if incomplete.exists(): + raise CommandError( + _( + "Domain '{name}' has {n} in-flight task(s) (waiting/running/canceling). " + "Wait for them to finish (or cancel them) before moving this domain -- " + "read-only mode does not preempt already-dispatched work." + ).format(name=domain.name, n=incomplete.count()) + ) + + def _validate_alias_ready(self, alias): + try: + connections[alias].ensure_connection() + except OperationalError as e: + raise CommandError( + _("Target alias '{alias}' is not reachable: {error}").format(alias=alias, error=e) + ) from e + executor = MigrationExecutor(connections[alias]) + targets = executor.loader.graph.leaf_nodes() + if executor.migration_plan(targets): + raise CommandError( + _( + "Target alias '{alias}' has pending migrations. Run 'pulpcore-manager " + "migrate-all' first." + ).format(alias=alias) + ) + + def _print_size_report(self, domain, size_report): + self.stdout.write(_("Size estimate for domain '{name}':").format(name=domain.name)) + for row in size_report: + if row["row_count"] == 0: + continue + self.stdout.write( + " {model}: {rows} row(s) (table total size: {size} bytes)".format( + model=row["model"], rows=row["row_count"], size=row["table_total_size_bytes"] + ) + ) + + def _move(self, domain, to_alias, options): + from_alias = domain.database_alias + move = DomainMove.objects.using("default").create( + domain=domain, from_alias=from_alias, to_alias=to_alias, started_at=now() + ) + try: + # Step 2 -- read-only mode. Deliberately NOT skip_hooks: the post_save signal in + # domain_sync.py must fire so every satellite (including `to_alias`, before any data + # even lands there) sees moving=True too -- DomainMiddleware/PulpcoreWorker consult + # the ContextVar-cached copy of this row on whichever alias/process they happen to + # be running against, not necessarily `default`. + domain.moving = True + domain.save(update_fields=["moving"]) + + if options["skip_copy"]: + self.stdout.write( + self.style.WARNING( + _( + "--skip-copy given: assuming data was already copied to '{alias}'." + ).format(alias=to_alias) + ) + ) + else: + self.stdout.write( + _("Copying data from '{source}' to '{target}'...").format( + source=from_alias, target=to_alias + ) + ) + copied = copy_domain_data(domain, from_alias, to_alias) + for label, count in copied.items(): + if count: + self.stdout.write(f" {label}: {count} row(s) copied") + + self.stdout.write(_("Verifying copied data...")) + mismatches = verify_domain_data(domain, from_alias, to_alias) + if mismatches: + for m in mismatches: + self.stderr.write( + self.style.ERROR( + " {model}: source={source_count} rows (checksum " + "{source_checksum}), target={target_count} rows (checksum " + "{target_checksum})".format(**m) + ) + ) + raise DomainMoveError( + "Verification failed for {n} model(s); see above. Domain '{name}' left " + "read-only on its original alias ('{alias}') -- no cutover performed. Fix " + "the discrepancy (e.g. re-run without --skip-copy) and retry.".format( + n=len(mismatches), name=domain.name, alias=from_alias + ) + ) + + # Step 5 -- cutover. + domain.database_alias = to_alias + domain.moving = False + domain.save(update_fields=["database_alias", "moving"]) + + cutover_at = now() + move.cutover_at = cutover_at + move.monitoring_until = cutover_at + timedelta(days=options["monitoring_days"]) + move.status = "completed" + move.save(update_fields=["cutover_at", "monitoring_until", "status"]) + + self.stdout.write( + self.style.SUCCESS( + _( + "Domain '{name}' moved from '{source}' to '{target}'. Monitor until " + "{until} before running 'cleanup-moved-domain {name}' (DomainMove " + "{move_id})." + ).format( + name=domain.name, + source=from_alias, + target=to_alias, + until=move.monitoring_until, + move_id=move.pk, + ) + ) + ) + except Exception as e: + move.status = "failed" + move.error = str(e) + move.save(update_fields=["status", "error"]) + # Always clear moving=True on failure -- an admin re-running the command (or just + # using the domain read-only in the meantime) needs writes to work again; the + # original alias still has the authoritative (unmodified, since we only ever copy + # *to* the target, never delete from the source until 'cleanup-moved-domain') data, + # so clearing it is always safe regardless of how far the copy/verify got. + if domain.moving: + domain.moving = False + domain.save(update_fields=["moving"]) + if isinstance(e, DomainMoveError): + raise CommandError(str(e)) from e + raise diff --git a/pulpcore/app/management/commands/reconcile-cross-plane-references.py b/pulpcore/app/management/commands/reconcile-cross-plane-references.py new file mode 100644 index 00000000000..f62785f72db --- /dev/null +++ b/pulpcore/app/management/commands/reconcile-cross-plane-references.py @@ -0,0 +1,82 @@ +from gettext import gettext as _ + +from django.conf import settings +from django.core.management import BaseCommand + +from pulpcore.app.tasks.reconciliation import reconcile_cross_plane_references + + +class Command(BaseCommand): + """ + KI-11: sweep for orphaned cross-plane `GenericForeignKey` references. + + Checks every `CreatedResource`/`ExportedResource`/`UserRole`/`GroupRole` row that records a + cross-plane target (a `content_object` living on a different alias than the row itself, per + the KI-18 `content_object_domain` field) and reports any whose target can no longer be + resolved on its recorded alias -- e.g. because the referencing task crashed before its + data-plane write landed (no distributed transactions, see the design doc's KI-11), or because + a domain was moved/cleaned-up while the row still pointed at the old alias. + + Safe to run at any time, including on a single-database deployment (there are no cross-plane + rows to find, so it's a fast no-op). Intended to also run periodically via a `TaskSchedule` + (see `pulpcore.app.util.configure_cleanup`); this command exists for on-demand/manual runs and + for operators who want a report without waiting for the next scheduled sweep. + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help=_("Report orphans without purging any of them, regardless of --purge-after-days."), + ) + parser.add_argument( + "--grace-period-minutes", + type=int, + default=None, + help=_( + "Skip rows updated more recently than this many minutes ago. Defaults to " + "settings.CROSS_PLANE_RECONCILIATION_GRACE_MINUTES (currently {default})." + ).format(default=settings.CROSS_PLANE_RECONCILIATION_GRACE_MINUTES), + ) + parser.add_argument( + "--purge-after-days", + type=int, + default=None, + help=_( + "Delete confirmed-orphaned rows older than this many days. 0 disables purging " + "(the default). Defaults to settings.CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS " + "(currently {default})." + ).format(default=settings.CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS), + ) + + def handle(self, *args, **options): + report = reconcile_cross_plane_references( + grace_period_minutes=options["grace_period_minutes"], + purge_after_days=options["purge_after_days"], + dry_run=options["dry_run"], + ) + + self.stdout.write( + _("Checked {checked} cross-plane row(s); found {orphaned} orphan(s).").format( + checked=report["checked"], orphaned=report["orphaned"] + ) + ) + for orphan in report["orphans"]: + self.stdout.write( + self.style.WARNING( + " {model} pk={pk} (recorded alias='{alias}', age={age}d)".format( + model=orphan["model"], + pk=orphan["pk"], + alias=orphan["alias"], + age=orphan["age_days"], + ) + ) + ) + if report["purged"]: + self.stdout.write( + self.style.SUCCESS(_("Purged {n} orphan(s).").format(n=report["purged"])) + ) + if not report["orphaned"]: + self.stdout.write(self.style.SUCCESS(_("No orphans found."))) diff --git a/pulpcore/app/management/commands/remove-plugin.py b/pulpcore/app/management/commands/remove-plugin.py index 7531670b03e..f397bd06560 100644 --- a/pulpcore/app/management/commands/remove-plugin.py +++ b/pulpcore/app/management/commands/remove-plugin.py @@ -5,7 +5,7 @@ from django.conf import settings from django.contrib.contenttypes.models import ContentType from django.core.management import BaseCommand, CommandError, call_command -from django.db import IntegrityError, connection +from django.db import IntegrityError, connections from django.db.migrations.exceptions import IrreversibleError from django.db.models.signals import post_migrate @@ -84,24 +84,40 @@ def _remove_indirect_plugin_data(self, app_label): def _remove_plugin_data(self, app_label): """ - Remove all plugin data. + Remove all plugin data, on every configured database alias. Removal happens via ORM to be sure that all relations are cleaned properly as well, e.g. Master-Detail, FKs to various content plugins in pulp-2to3-migration. In some cases, the order in which models are removed matters, e.g. FK is a part of uniqueness constraint. Try to remove such problematic models later. + + management-command-audit.md finding (new, not previously in the Known Issues Registry): + a plugin's data-plane rows exist on every alias (identical schema everywhere, KI-16), and + any domain hosted on a satellite has its own copy of that plugin's rows there too. The + original single-connection version of this method only ever deleted `default`'s rows, + permanently orphaning the plugin's tables/rows on every satellite. Looping every alias + here is unconditional, matching `rotate-db-key.py`'s KI-22 fix -- this is an infrequent, + explicitly operator-invoked, destructive command where correctness matters far more than + the overhead of looping. """ + for alias in settings.DATABASES: + self._remove_plugin_data_from_alias(app_label, alias) + # Control-plane only (ContentType, AccessPolicy, Role -- see CONTROL_PLANE_LABELS): + # authoritative on `default` alone, so this runs exactly once, not once per alias. + self._remove_indirect_plugin_data(app_label) + def _remove_plugin_data_from_alias(self, app_label, alias): + self.stdout.write(_("Removing {} plugin data from alias '{}'...").format(app_label, alias)) models_to_delete = set(apps.all_models[app_label].values()) prev_model_count = len(models_to_delete) + 1 while models_to_delete and len(models_to_delete) < prev_model_count: # while there is something to delete and something is being deleted on each iteration removed_models = set() for model in models_to_delete: - self.stdout.write(_("Removing model: {}").format(model)) + self.stdout.write(_("Removing model: {} (alias '{}')").format(model, alias)) try: - model.objects.filter().delete() + model.objects.using(alias).filter().delete() except IntegrityError: continue else: @@ -114,23 +130,24 @@ def _remove_plugin_data(self, app_label): # Never-happen case raise CommandError( ( - "Data for the following models can't be removed: {}. Please contact plugin " - "maintainers." - ).format(list(models_to_delete)) + "Data for the following models can't be removed on alias '{}': {}. Please " + "contact plugin maintainers." + ).format(alias, list(models_to_delete)) ) - self._remove_indirect_plugin_data(app_label) - - def _drop_plugin_tables(self, app_label): + def _drop_plugin_tables(self, app_label, alias): """ - Drop plugin table with raw SQL. + Drop plugin table with raw SQL, on the given database alias. """ - with connection.cursor() as cursor: + with connections[alias].cursor() as cursor: cursor.execute(DROP_PLUGIN_TABLES_QUERY.format(app_label=app_label)) def _unapply_migrations(self, app_label): """ - Unapply migrations so the plugin can be installed/run django migrations again if needed. + Unapply migrations so the plugin can be installed/run django migrations again if needed, + on every configured database alias (same rationale as `_remove_plugin_data` -- the + plugin's schema exists on every alias, KI-16, so it must be unmigrated everywhere, not + just on `default`). Make sure no post migration signals are connected/run (it's enough to disable only `populate_access_policy` and `populate_roles` for the requested plugin, so after @@ -148,13 +165,20 @@ def _unapply_migrations(self, app_label): if app_config.label == "core": post_migrate.disconnect(sender=app_config, dispatch_uid="delete_anon_identifier") - try: - call_command("migrate", app_label=app_label, migration_name="zero") - except (IrreversibleError, Exception): - # a plugin has irreversible migrations or some other problem, drop the tables and fake - # that migrations are unapplied. - self._drop_plugin_tables(app_label) - call_command("migrate", app_label=app_label, migration_name="zero", fake=True) + for alias in settings.DATABASES: + try: + call_command("migrate", app_label=app_label, migration_name="zero", database=alias) + except (IrreversibleError, Exception): + # a plugin has irreversible migrations or some other problem, drop the tables and + # fake that migrations are unapplied. + self._drop_plugin_tables(app_label, alias) + call_command( + "migrate", + app_label=app_label, + migration_name="zero", + fake=True, + database=alias, + ) def handle(self, *args, **options): plugin_name = options["plugin_name"] diff --git a/pulpcore/app/management/commands/repository-size.py b/pulpcore/app/management/commands/repository-size.py index a8ab13f7705..9072053d659 100644 --- a/pulpcore/app/management/commands/repository-size.py +++ b/pulpcore/app/management/commands/repository-size.py @@ -7,8 +7,8 @@ from django.conf import settings from django.core.management import BaseCommand, CommandError -from pulpcore.app.models import Repository -from pulpcore.app.util import extract_pk, get_url +from pulpcore.app.models import Domain, Repository +from pulpcore.app.util import extract_pk, for_each_domain, get_url def gather_repository_sizes(repositories, include_versions=False, include_on_demand=False): @@ -106,22 +106,58 @@ def add_arguments(self, parser): def handle(self, *args, **options): """Implement the command.""" - domain = options.get("domain") + domain_name = options.get("domain") repository_hrefs = options.get("repositories") - if domain and repository_hrefs: + if domain_name and repository_hrefs: raise CommandError(_("--domain and --repositories are mutually exclusive")) - repositories = Repository.objects.all() + # KI-08: `Repository` is data-plane, so an unqualified `Repository.objects.all()` (the + # original shape of this command) -- or a `--domain` filter that only ever adds + # `.filter(pulp_domain__name=...)` without also switching `.using(alias)` -- would only + # ever see repositories that happen to live on `default`, even though the `--domain` + # flag's own help text implies otherwise. All three call shapes below resolve the + # relevant `Domain`/alias(es) explicitly and query with `.using(alias)`. + report = [] if repository_hrefs: repos_ids = [extract_pk(r) for r in repository_hrefs] - repositories = repositories.filter(pk__in=repos_ids) - elif domain: - repositories = repositories.filter(pulp_domain__name=domain) - - report = gather_repository_sizes( - repositories, - include_versions=options["include_versions"], - include_on_demand=options["include_on_demand"], - ) + # An href doesn't self-identify which alias its repository lives on, so check every + # configured alias -- a given pk only ever matches on the one alias its domain + # actually resides on; every other alias contributes an empty queryset. + for alias in settings.DATABASES: + repositories = Repository.objects.using(alias).filter(pk__in=repos_ids) + report.extend( + gather_repository_sizes( + repositories, + include_versions=options["include_versions"], + include_on_demand=options["include_on_demand"], + ) + ) + elif domain_name: + try: + domain = Domain.objects.get(name=domain_name) + except Domain.DoesNotExist: + raise CommandError(_("Domain '{name}' does not exist.").format(name=domain_name)) + repositories = Repository.objects.using(domain.database_alias).filter( + pulp_domain=domain + ) + report = gather_repository_sizes( + repositories, + include_versions=options["include_versions"], + include_on_demand=options["include_on_demand"], + ) + else: + + def _gather_for_domain(domain, alias): + repositories = Repository.objects.using(alias).filter(pulp_domain=domain) + report.extend( + gather_repository_sizes( + repositories, + include_versions=options["include_versions"], + include_on_demand=options["include_on_demand"], + ) + ) + + for_each_domain(_gather_for_domain) + json.dump(report, sys.stdout, indent=4) print() diff --git a/pulpcore/app/management/commands/rotate-db-key.py b/pulpcore/app/management/commands/rotate-db-key.py index 6bbec206e10..7bfd346b7bb 100644 --- a/pulpcore/app/management/commands/rotate-db-key.py +++ b/pulpcore/app/management/commands/rotate-db-key.py @@ -2,8 +2,9 @@ from gettext import gettext as _ from django.apps import apps +from django.conf import settings from django.core.management import BaseCommand -from django.db import connection, transaction +from django.db import connections, transaction from pulpcore.app.models import MasterModel from pulpcore.app.models.fields import EncryptedJSONField, EncryptedTextField @@ -40,6 +41,19 @@ def add_arguments(self, parser): def handle(self, *args, **options): dry_run = options["dry_run"] + # KI-22: an encrypted value can live on *any* configured alias -- a data-plane model's + # encrypted field on a domain hosted on a satellite is just as much in need of rotation + # as one on `default`. The original single-connection version of this command only ever + # rotated `default`, silently leaving every satellite's copy of the old key in place. + # Looping every alias here is deliberately unconditional (not gated behind + # `len(settings.DATABASES) > 1`) since correctness, not overhead, is what matters for an + # explicitly operator-invoked, infrequent maintenance command -- unlike the router/ + # queryset mixin's hot-path fast paths. + for alias in settings.DATABASES: + self._rotate_alias(alias, dry_run) + + def _rotate_alias(self, alias, dry_run): + print(_("Rotating encrypted fields on database alias '{alias}'.").format(alias=alias)) for model in apps.get_models(): if issubclass(model, MasterModel) and model._meta.master_model is None: # This is a master model, and we will handle all it's descendents. @@ -51,23 +65,23 @@ def handle(self, *args, **options): ] if field_names: print( - _("Updating {fields} on {model}.").format( - model=model.__name__, fields=",".join(field_names) + _("Updating {fields} on {model} (alias '{alias}').").format( + model=model.__name__, fields=",".join(field_names), alias=alias ) ) exclude_filters = {f"{field_name}": None for field_name in field_names} - qs = model.objects.exclude(**exclude_filters).only(*field_names) - with suppress(DryRun), transaction.atomic(): + qs = model.objects.using(alias).exclude(**exclude_filters).only(*field_names) + with suppress(DryRun), transaction.atomic(using=alias): batch = [] for item in qs.iterator(): batch.append(item) if len(batch) >= 1024: - model.objects.bulk_update(batch, field_names) + model.objects.using(alias).bulk_update(batch, field_names) batch = [] if batch: - model.objects.bulk_update(batch, field_names) + model.objects.using(alias).bulk_update(batch, field_names) batch = [] if dry_run: - with connection.cursor() as cursor: + with connections[alias].cursor() as cursor: cursor.execute("SET CONSTRAINTS ALL IMMEDIATE") raise DryRun() diff --git a/pulpcore/app/management/commands/sync-domains.py b/pulpcore/app/management/commands/sync-domains.py new file mode 100644 index 00000000000..60328a9bf71 --- /dev/null +++ b/pulpcore/app/management/commands/sync-domains.py @@ -0,0 +1,98 @@ +from gettext import gettext as _ + +from django.core.management import BaseCommand, CommandError + +from pulpcore.app.domain_sync import reconcile_domains_to_alias, satellite_aliases + + +class Command(BaseCommand): + """ + Full reconciliation of the `Domain` table against every configured satellite alias. + + `Domain`'s `post_save`/`post_delete` signals (see `pulpcore.app.domain_sync`) push individual + changes to every satellite as they happen, but they can miss rows: `bulk_create()`/ + `.update()` bypass Django signals entirely, and a satellite that's unreachable when a signal + fires never gets retried once it comes back, until this command runs. Run this after + provisioning a new satellite (before pointing any domain at it), after any bulk `Domain` + changes, or periodically as a reconciliation job. + """ + + help = __doc__ + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help=_("Report drift without writing any changes."), + ) + parser.add_argument( + "--alias", + help=_( + "Only reconcile this one satellite alias, instead of every configured alias. " + "Used by 'migrate-all' to populate a brand new satellite's `Domain` table " + "partway through its own migration run, before other, not-yet-migrated " + "satellites even have a `core_domain` table to reconcile against." + ), + ) + + def handle(self, *args, **options): + dry_run = options["dry_run"] + if options.get("alias"): + if options["alias"] not in satellite_aliases(): + raise CommandError( + _("'{alias}' is not a configured satellite alias.").format( + alias=options["alias"] + ) + ) + aliases = [options["alias"]] + else: + aliases = satellite_aliases() + if not aliases: + self.stdout.write( + self.style.WARNING( + _("Only one database alias is configured; nothing to reconcile.") + ) + ) + return + + any_drift = False + for alias in aliases: + self.stdout.write(_("Reconciling alias '{alias}'...").format(alias=alias)) + report = reconcile_domains_to_alias(alias, dry_run=dry_run) + missing, extra, stale = report["missing"], report["extra"], report["stale"] + + if not (missing or extra or stale): + self.stdout.write(_(" No drift detected.")) + continue + + any_drift = True + if missing: + self.stdout.write( + _(" {n} domain(s) missing on '{alias}': {ids}").format( + n=len(missing), alias=alias, ids=", ".join(str(i) for i in missing) + ) + ) + if extra: + self.stdout.write( + _(" {n} domain(s) exist only on '{alias}' (orphaned): {ids}").format( + n=len(extra), alias=alias, ids=", ".join(str(i) for i in extra) + ) + ) + if stale: + self.stdout.write( + _(" {n} domain(s) out of sync on '{alias}': {ids}").format( + n=len(stale), alias=alias, ids=", ".join(str(i) for i in stale) + ) + ) + + if not dry_run: + self.stdout.write( + self.style.SUCCESS(_(" Reconciled alias '{alias}'.").format(alias=alias)) + ) + + if dry_run and any_drift: + self.stdout.write( + self.style.WARNING(_("Dry run: no changes were written. Re-run without --dry-run.")) + ) + elif not any_drift: + self.stdout.write(self.style.SUCCESS(_("All satellite aliases are in sync."))) diff --git a/pulpcore/app/migrations/0101_add_domain.py b/pulpcore/app/migrations/0101_add_domain.py index ac5363050ca..3db5cd79933 100644 --- a/pulpcore/app/migrations/0101_add_domain.py +++ b/pulpcore/app/migrations/0101_add_domain.py @@ -7,7 +7,6 @@ import pulpcore.app.models.fields import uuid - DEFAULT_DELETE_TRIGGER = """ CREATE OR REPLACE FUNCTION protect_default() RETURNS TRIGGER as $protect_default$ BEGIN @@ -28,42 +27,62 @@ def create_default_domain(apps, schema_editor): - Domain = apps.get_model('core', 'Domain') + # Domain-aware database routing (see architecture/domain-db-offloading-design.md): `Domain` + # is control-plane and authoritative on `default` only -- every satellite's copy is + # populated by replication/`sync-domains`, never created independently. This migration + # predates that design and, left as-is, would run unguarded on every alias (schema is + # identical everywhere, see KI-16, so `allow_migrate` never filters it out) and create its + # *own* "default"-named `Domain` row on each satellite the very first time `migrate-all` + # migrates it -- a duplicate, unreplicated row with a different `pulp_id` than `default`'s, + # which then collides with `sync-domains` (unique constraint on `name`). Guard it the same + # way `_ensure_default_domain`/KI-07 guards its own post_migrate hook. + if schema_editor.connection.alias != "default": + return + Domain = apps.get_model("core", "Domain") try: - default_domain = Domain.objects.get(name="default") + default_domain = Domain.objects.using("default").get(name="default") except Domain.DoesNotExist: default_domain = Domain( name="default", storage_class=settings.STORAGES["default"]["BACKEND"] ) - default_domain.save(skip_hooks=True) + default_domain.save(using="default", skip_hooks=True) class Migration(migrations.Migration): dependencies = [ - ('contenttypes', '0002_remove_content_type_name'), + ("contenttypes", "0002_remove_content_type_name"), migrations.swappable_dependency(settings.AUTH_USER_MODEL), - ('core', '0100_upstreampulp'), + ("core", "0100_upstreampulp"), ] operations = [ migrations.CreateModel( - name='Domain', + name="Domain", fields=[ - ('pulp_id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)), - ('pulp_created', models.DateTimeField(auto_now_add=True)), - ('pulp_last_updated', models.DateTimeField(auto_now=True, null=True)), - ('name', models.SlugField(unique=True)), - ('description', models.TextField(null=True)), - ('storage_class', models.TextField()), - ('storage_settings', pulpcore.app.models.fields.EncryptedJSONField(default=dict)), - ('redirect_to_object_storage', models.BooleanField(default=True)), - ('hide_guarded_distributions', models.BooleanField(default=False)), + ( + "pulp_id", + models.UUIDField( + default=uuid.uuid4, editable=False, primary_key=True, serialize=False + ), + ), + ("pulp_created", models.DateTimeField(auto_now_add=True)), + ("pulp_last_updated", models.DateTimeField(auto_now=True, null=True)), + ("name", models.SlugField(unique=True)), + ("description", models.TextField(null=True)), + ("storage_class", models.TextField()), + ("storage_settings", pulpcore.app.models.fields.EncryptedJSONField(default=dict)), + ("redirect_to_object_storage", models.BooleanField(default=True)), + ("hide_guarded_distributions", models.BooleanField(default=False)), ], options={ - 'permissions': [('manage_roles_domain', 'Can manage role assignments on domain')], + "permissions": [("manage_roles_domain", "Can manage role assignments on domain")], }, - bases=(django_lifecycle.mixins.LifecycleModelMixin, models.Model, pulpcore.app.models.access_policy.AutoAddObjPermsMixin), + bases=( + django_lifecycle.mixins.LifecycleModelMixin, + models.Model, + pulpcore.app.models.access_policy.AutoAddObjPermsMixin, + ), ), migrations.RunSQL(DEFAULT_DELETE_TRIGGER, reverse_sql=REMOVE_DEFAULT_DELETE_TRIGGER), migrations.RunPython(code=create_default_domain, reverse_code=migrations.RunPython.noop), diff --git a/pulpcore/app/migrations/0154_domain_database_alias_domain_moving.py b/pulpcore/app/migrations/0154_domain_database_alias_domain_moving.py new file mode 100644 index 00000000000..74af97f466f --- /dev/null +++ b/pulpcore/app/migrations/0154_domain_database_alias_domain_moving.py @@ -0,0 +1,30 @@ +# Generated by Django 5.2.13 on 2026-07-09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("core", "0153_taskschedule_pulp_domain_alter_taskschedule_name_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="domain", + name="database_alias", + field=models.SlugField( + default="default", + help_text="DATABASES alias where this domain's data-plane objects reside.", + ), + ), + migrations.AddField( + model_name="domain", + name="moving", + field=models.BooleanField( + default=False, + help_text="True while this domain's data is being moved between database " + "aliases.", + ), + ), + ] diff --git a/pulpcore/app/migrations/0155_createdresource_content_object_domain_and_more.py b/pulpcore/app/migrations/0155_createdresource_content_object_domain_and_more.py new file mode 100644 index 00000000000..544ae2f1419 --- /dev/null +++ b/pulpcore/app/migrations/0155_createdresource_content_object_domain_and_more.py @@ -0,0 +1,62 @@ +# Generated by Django 5.2.15 on 2026-07-09 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + """KI-18: denormalize the target domain of every GenericForeignKey `content_object`. + + `CreatedResource`/`ExportedResource`/`UserRole`/`GroupRole` are control-plane models + (always on `default`) whose `content_object` may point at a data-plane object living on a + satellite alias. See `pulpcore.app.models.generic.DomainResolvedGenericRelation` for why + this field is required for `content_object` to resolve correctly once a domain has moved + off `default`. + """ + + dependencies = [ + ("core", "0154_domain_database_alias_domain_moving"), + ] + + operations = [ + migrations.AddField( + model_name="createdresource", + name="content_object_domain", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="+", + to="core.domain", + ), + ), + migrations.AddField( + model_name="exportedresource", + name="content_object_domain", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="+", + to="core.domain", + ), + ), + migrations.AddField( + model_name="grouprole", + name="content_object_domain", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="+", + to="core.domain", + ), + ), + migrations.AddField( + model_name="userrole", + name="content_object_domain", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="+", + to="core.domain", + ), + ), + ] diff --git a/pulpcore/app/migrations/0156_migrationstatus.py b/pulpcore/app/migrations/0156_migrationstatus.py new file mode 100644 index 00000000000..ada8e92ff93 --- /dev/null +++ b/pulpcore/app/migrations/0156_migrationstatus.py @@ -0,0 +1,51 @@ +# Generated by Django 5.2.15 on 2026-07-09 + +import django_lifecycle.mixins +import pulpcore.app.models.base +from django.db import migrations, models + + +class Migration(migrations.Migration): + """phase1-migrate-all: per-alias bookkeeping for the `migrate-all` orchestration command.""" + + dependencies = [ + ("core", "0155_createdresource_content_object_domain_and_more"), + ] + + operations = [ + migrations.CreateModel( + name="MigrationStatus", + fields=[ + ( + "pulp_id", + models.UUIDField( + default=pulpcore.app.models.base.pulp_uuid, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("pulp_created", models.DateTimeField(auto_now_add=True)), + ("pulp_last_updated", models.DateTimeField(auto_now=True, null=True)), + ("database_alias", models.TextField(unique=True)), + ( + "status", + models.TextField( + choices=[ + ("pending", "Pending"), + ("running", "Running"), + ("complete", "Complete"), + ("failed", "Failed"), + ], + default="pending", + ), + ), + ("completed_at", models.DateTimeField(null=True)), + ("error", models.TextField(null=True)), + ], + options={ + "verbose_name_plural": "migration statuses", + }, + bases=(django_lifecycle.mixins.LifecycleModelMixin, models.Model), + ), + ] diff --git a/pulpcore/app/migrations/0157_domainmove.py b/pulpcore/app/migrations/0157_domainmove.py new file mode 100644 index 00000000000..1836d434e57 --- /dev/null +++ b/pulpcore/app/migrations/0157_domainmove.py @@ -0,0 +1,63 @@ +# Generated by Django 5.2.15 on 2026-07-09 17:21 + +import django.db.models.deletion +import django_lifecycle.mixins +import pulpcore.app.models.base +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("core", "0156_migrationstatus"), + ] + + operations = [ + migrations.CreateModel( + name="DomainMove", + fields=[ + ( + "pulp_id", + models.UUIDField( + default=pulpcore.app.models.base.pulp_uuid, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("pulp_created", models.DateTimeField(auto_now_add=True)), + ("pulp_last_updated", models.DateTimeField(auto_now=True, null=True)), + ("from_alias", models.SlugField()), + ("to_alias", models.SlugField()), + ("started_at", models.DateTimeField()), + ("cutover_at", models.DateTimeField(null=True)), + ("monitoring_until", models.DateTimeField(null=True)), + ("cleaned_up_at", models.DateTimeField(null=True)), + ( + "status", + models.TextField( + choices=[ + ("in_progress", "In Progress"), + ("completed", "Completed"), + ("failed", "Failed"), + ("cleaned_up", "Cleaned Up"), + ], + default="in_progress", + ), + ), + ("error", models.TextField(null=True)), + ( + "domain", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="moves", + to="core.domain", + ), + ), + ], + options={ + "ordering": ["-pulp_created"], + }, + bases=(django_lifecycle.mixins.LifecycleModelMixin, models.Model), + ), + ] diff --git a/pulpcore/app/models/__init__.py b/pulpcore/app/models/__init__.py index a1bf1ca1147..87e48500482 100644 --- a/pulpcore/app/models/__init__.py +++ b/pulpcore/app/models/__init__.py @@ -84,6 +84,10 @@ from .analytics import SystemID +from .migration_status import MigrationStatus + +from .domain_move import DomainMove + from .upload import ( Upload, UploadChunk, @@ -161,6 +165,8 @@ "TaskGroup", "TaskSchedule", "SystemID", + "MigrationStatus", + "DomainMove", "Upload", "UploadChunk", "GroupProgressReport", diff --git a/pulpcore/app/models/content.py b/pulpcore/app/models/content.py index e73d4b480ed..42da25bd240 100644 --- a/pulpcore/app/models/content.py +++ b/pulpcore/app/models/content.py @@ -26,6 +26,7 @@ from pulpcore.app import pulp_hashlib from pulpcore.app.models import BaseModel, MasterModel, fields, storage +from pulpcore.app.queryset import CrossDBQuerySetMixin from pulpcore.app.util import get_domain_pk, gpg_verify from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS from pulpcore.exceptions import ( @@ -97,7 +98,7 @@ def bulk_get_or_create(self, objs, batch_size=None): return objs -class BulkTouchQuerySet(models.QuerySet): +class BulkTouchQuerySet(CrossDBQuerySetMixin, models.QuerySet): """ A query set that provides ``touch()``. """ @@ -694,7 +695,7 @@ def sort_key(ca): return c_key, a_key -class RemoteArtifactQuerySet(models.QuerySet): +class RemoteArtifactQuerySet(CrossDBQuerySetMixin, models.QuerySet): """QuerySet that provides methods for querying RemoteArtifact.""" def acs(self): diff --git a/pulpcore/app/models/domain.py b/pulpcore/app/models/domain.py index aff0dc4b22e..656004329a3 100644 --- a/pulpcore/app/models/domain.py +++ b/pulpcore/app/models/domain.py @@ -1,7 +1,9 @@ +from django.conf import settings from django.contrib.postgres.fields import HStoreField +from django.core.exceptions import ValidationError from django.core.files.storage import default_storage from django.db import models -from django_lifecycle import BEFORE_DELETE, BEFORE_UPDATE, hook +from django_lifecycle import BEFORE_CREATE, BEFORE_DELETE, BEFORE_UPDATE, hook from pulpcore.app.models import AutoAddObjPermsMixin, BaseModel from pulpcore.exceptions import DomainProtectedError @@ -32,6 +34,16 @@ class Domain(BaseModel, AutoAddObjPermsMixin): storage_settings (EncryptedJSONField): Settings needed to configure storage backend redirect_to_object_storage (models.BooleanField): Redirect to object storage in content app hide_guarded_distributions (models.BooleanField): Hide guarded distributions in content app + database_alias (models.SlugField): The `settings.DATABASES` alias where this domain's + data-plane objects (repositories, content, artifacts, etc.) reside. Defaults to + "default" so existing single-database deployments are unaffected. This field is + internal/operational only -- it is never exposed through the public API (see + `DomainSerializer`) and is only ever changed by admin-run `pulpcore-manager` + tooling (`move-domain`), never by end users. + moving (models.BooleanField): Whether this domain is currently in the middle of being + moved between database aliases by the `move-domain` command. While `True`, write + operations for this domain are rejected and no new tasks are dispatched for it. + Internal/operational only, same visibility rules as `database_alias`. """ name = models.SlugField(null=False, unique=True) @@ -43,6 +55,18 @@ class Domain(BaseModel, AutoAddObjPermsMixin): # Pulp settings that are appropriate to be set on a "per domain" level redirect_to_object_storage = models.BooleanField(default=True) hide_guarded_distributions = models.BooleanField(default=False) + # Internal/operational-only fields for domain-aware database routing. Never exposed via the + # public API (intentionally excluded from DomainSerializer.Meta.fields) and only ever written + # by admin-run `pulpcore-manager` CLI tooling (`move-domain`, `sync-domains`), never by + # end users or any REST endpoint. + database_alias = models.SlugField( + default="default", + help_text="DATABASES alias where this domain's data-plane objects reside.", + ) + moving = models.BooleanField( + default=False, + help_text="True while this domain's data is being moved between database aliases.", + ) def get_storage(self): """Returns this domain's instantiated storage class.""" @@ -65,13 +89,34 @@ def get_storage(self): def prevent_default_deletion(self): raise models.ProtectedError("Default domain can not be updated/deleted.", [self]) + @hook(BEFORE_CREATE) + @hook(BEFORE_UPDATE, when="database_alias", has_changed=True) + def _validate_database_alias(self): + """Ensure `database_alias` always refers to a real, configured DATABASES alias.""" + if self.database_alias not in settings.DATABASES: + raise ValidationError( + { + "database_alias": ( + f"'{self.database_alias}' is not a configured DATABASES alias." + ) + } + ) + @hook(BEFORE_DELETE, when="name", is_not="default") def _cleanup_orphans_pre_delete(self): - protected_content_set = self.content_set.exclude(version_memberships__isnull=True) + # This domain's data-plane objects may live on a satellite alias (KI-04): the reverse + # FK managers below must be explicitly routed there, since the router's instance-hint + # check only recognizes models with a `pulp_domain` attribute, which `self` (the Domain + # itself) does not have. + protected_content_set = self.content_set.using(self.database_alias).exclude( + version_memberships__isnull=True + ) if protected_content_set.exists(): raise DomainProtectedError() - self.content_set.filter(version_memberships__isnull=True).delete() - for artifact in self.artifact_set.all().iterator(): + self.content_set.using(self.database_alias).filter( + version_memberships__isnull=True + ).delete() + for artifact in self.artifact_set.using(self.database_alias).all().iterator(): # Delete on by one to properly cleanup the storage. artifact.delete() diff --git a/pulpcore/app/models/domain_move.py b/pulpcore/app/models/domain_move.py new file mode 100644 index 00000000000..03b60cafd28 --- /dev/null +++ b/pulpcore/app/models/domain_move.py @@ -0,0 +1,59 @@ +""" +Historical record of `move-domain` runs (Phase 2, Strategy A -- "Read-Only Cutover"). + +Purely operational bookkeeping, always on `default`: fleet-management metadata about *when* and +*between which aliases* a domain moved, not data belonging to any one domain. Gives an operator +something concrete to consult during the design doc's Step 6 "Monitoring" window (there is no +real monitoring/alerting infra in this implementation pass -- see the design doc and +`architecture/domain-db-offloading-runbook.md` -- this is just a durable timestamped record, not +a substitute for it) and lets `cleanup-moved-domain` refuse to run before `monitoring_until` has +elapsed. +""" + +from django.db import models + +from pulpcore.app.models import BaseModel + +DOMAIN_MOVE_STATUS_CHOICES = ( + ("in_progress", "In Progress"), + ("completed", "Completed"), + ("failed", "Failed"), + ("cleaned_up", "Cleaned Up"), +) + + +class DomainMove(BaseModel): + """ + One row per `move-domain` invocation for a given `Domain`. + + Fields: + domain (models.ForeignKey): The domain that was (or is being) moved. + from_alias (models.SlugField): The `DATABASES` alias the domain's data-plane objects + were moved from. + to_alias (models.SlugField): The `DATABASES` alias the domain's data-plane objects were + moved to. + started_at (models.DateTimeField): When this move began (`Domain.moving` set to `True`). + cutover_at (models.DateTimeField): When `Domain.database_alias` was updated to + `to_alias` (Step 5 -- null until the move reaches cutover). + monitoring_until (models.DateTimeField): End of the recommended Step 6 monitoring + window (default 7 days after `cutover_at`; see `move-domain --monitoring-days`). + `cleanup-moved-domain` warns (but does not block, since the operator is the one + asserting the move looks healthy) if run before this. + cleaned_up_at (models.DateTimeField): When `cleanup-moved-domain` deleted the stale rows + from `from_alias` (Step 7 -- null until cleanup has run). + status (models.TextField): One of "in_progress", "completed", "failed", "cleaned_up". + error (models.TextField): Error message if `status` is "failed". + """ + + domain = models.ForeignKey("Domain", on_delete=models.CASCADE, related_name="moves") + from_alias = models.SlugField() + to_alias = models.SlugField() + started_at = models.DateTimeField() + cutover_at = models.DateTimeField(null=True) + monitoring_until = models.DateTimeField(null=True) + cleaned_up_at = models.DateTimeField(null=True) + status = models.TextField(choices=DOMAIN_MOVE_STATUS_CHOICES, default="in_progress") + error = models.TextField(null=True) + + class Meta: + ordering = ["-pulp_created"] diff --git a/pulpcore/app/models/generic.py b/pulpcore/app/models/generic.py index 2d8420d4a01..af372b6dddd 100644 --- a/pulpcore/app/models/generic.py +++ b/pulpcore/app/models/generic.py @@ -5,14 +5,158 @@ https://docs.djangoproject.com/en/3.2/ref/contrib/contenttypes/#generic-relations """ +import logging + from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.db import models from pulpcore.app.models.base import BaseModel +_logger = logging.getLogger(__name__) + + +#: Sentinel distinguishing "no in-memory value cached yet" from a legitimately cached `None` +#: (a model-level `UserRole`/`GroupRole` grant has no target at all). +_UNSET = object() + + +class DomainResolvedGenericRelation: + """ + Mixin providing a cross-database-safe `content_object` for models with a + `content_type`/`object_id` `GenericForeignKey` pair whose target may be a data-plane object + living on a different database alias than the row holding the FK (KI-18, Critical). + + Every model in pulpcore/plugins that declares a `GenericForeignKey` (`CreatedResource`, + `ExportedResource`, `UserRole`, `GroupRole` -- no plugin declares its own, confirmed by + full-codebase audit) is itself a control-plane model, always on `default`, while its target + may be any model at all, including a data-plane one that has since moved to a satellite. + Django's stock `GenericForeignKey` has *two* cross-DB landmines for exactly this shape, + both found empirically (real multi-Postgres integration testing), not just by reading the + design doc's KI-18 writeup (which only covers the first): + + 1. `__get__` resolves the target using `instance._state.db` -- the *FK-holding row's* + database, not the target's -- and silently swallows `ObjectDoesNotExist`, returning + `None` with no error at all. For a moved domain, `.content_object` would quietly start + returning `None` for every affected row: no exception, no log line, just wrong data (a + task's created-resources page silently empty, an incremental export silently missing a + repository version). + 2. `__set__` resolves the `ContentType` row for the target via + `ContentType.objects.db_manager(value._state.db).get_for_model(value)` -- i.e. against + *the target's own alias's* `django_content_type` table, not `default`'s. `ContentType.pk` + is an independent per-database auto-increment (each alias runs its own + `create_contenttypes` during `migrate`, in whatever order that alias's installed apps + happen to migrate in), so the same `app_label`/`model` pair can end up with a *different* + pk on a satellite than on `default`. Since this row is itself control-plane and always + lives on `default`, its stored `content_type_id` must always be `default`'s id for that + model -- storing the target-alias's id means a later read (which resolves `content_type` + against `default`, since ContentType is control-plane) silently decodes it as whatever + *different* model happens to have that id on `default`. Reproduced in integration testing + as a `CreatedResource` for a `Repository` resolving to an unrelated `Remote` instead. + + Locked fix for #1 (see `architecture/domain-db-offloading-design.md`, KI-18): denormalize a + nullable `content_object_domain` FK onto the model, auto-populated by the `content_object` + setter below from `content_object.pulp_domain_id` whenever a target is set -- no call site + needs to set it explicitly, and it's set eagerly (not deferred to `save()`) specifically so + that `bulk_create()` -- which never calls `save()` -- still populates it correctly; see + `pulpcore.app.viewsets.base.NamedModelViewSet.add_role` for a real `bulk_create()` call + site. The `content_object` property below then resolves the target via + `.using(content_object_domain.database_alias)` when that field is set, raising loudly on + failure instead of silently swallowing it, and falls back to resolving on this row's own + alias whenever there's no recorded cross-plane domain (no target at all -- e.g. a + model-level `UserRole`/`GroupRole` grant -- or a control-plane target, which lives on the + same alias as this row anyway). + + Fix for #2: the setter below never delegates to Django's `GenericForeignKey.__set__` (would + reintroduce the bug) -- it sets `content_type`/`object_id` directly, always resolving + `ContentType` via `ContentType.objects.db_manager("default")` regardless of which alias the + target object itself came from. + + Both the getter and setter also avoid Django's `GenericForeignKey.__get__`/`is_cached()` + entirely for in-memory caching (a plain `_UNSET`-sentinelled instance attribute is used + instead): `__get__`'s cache-validation re-resolves the cached value's `ContentType` via + `value._state.db` again on every access (the same #2 landmine), which would otherwise + spuriously invalidate a just-set cross-plane value on every subsequent access within the + same process, even before any DB round-trip. + + Including models must declare the real `GenericForeignKey` under the name + `_content_object` (not `content_object` -- kept only so `for_concrete_model` and the + content_type/object_id field names stay introspectable/documented in one place, not because + its `__get__`/`__set__` are actually used) and a nullable `content_object_domain` FK to + `core.Domain`. This mixin then makes `content_object` behave exactly like the original + attribute at every one of pulpcore's/plugins' existing `content_object=` call sites -- + zero call-site changes required, including in code this audit didn't see (unaudited or + future plugin code). + """ + + def __init__(self, *args, **kwargs): + has_content_object = "content_object" in kwargs + content_object = kwargs.pop("content_object", None) + super().__init__(*args, **kwargs) + if has_content_object: + self.content_object = content_object + + @property + def content_object(self): + cached = self.__dict__.get("_content_object_cache", _UNSET) + if cached is not _UNSET: + return cached + if self.content_type_id is None or self.object_id is None: + return None + model_class = self.content_type.model_class() + if self.content_object_domain_id is not None: + # Cross-plane target loaded fresh from the DB: resolve on the target's own alias, + # not this (control-plane) row's alias -- this is the KI-18 fix. + alias = self.content_object_domain.database_alias + try: + resolved = model_class.objects.using(alias).get(pk=self.object_id) + except model_class.DoesNotExist: + _logger.error( + "content_object for %s (pk=%s) not found on alias '%s' " + "(content_type_id=%s, object_id=%s). The referenced object may have been " + "deleted, or Domain replication for this row's domain may be stale -- run " + "'pulpcore-manager sync-domains' to check.", + self._meta.label, + self.pk, + alias, + self.content_type_id, + self.object_id, + ) + raise + else: + # No cross-plane domain recorded: the target is itself control-plane, so it lives on + # this (control-plane) row's own alias -- resolve there, mirroring normal Django GFK + # behavior but without touching `_content_object`/`__get__` (see class docstring). + try: + resolved = model_class._base_manager.using(self._state.db or "default").get( + pk=self.object_id + ) + except model_class.DoesNotExist: + resolved = None + self.__dict__["_content_object_cache"] = resolved + return resolved + + @content_object.setter + def content_object(self, value): + self.__dict__["_content_object_cache"] = value + if value is None: + self.content_type = None + self.object_id = None + self.content_object_domain_id = None + return + gfk = type(self)._content_object + # Always resolve against `default`, regardless of which alias `value` itself lives on -- + # see the class docstring's landmine #2. `db_manager` bypasses the router deliberately + # (mirroring Django's own `get_content_type`), since ContentType is control-plane and + # `.get_for_model()`'s cache is keyed per-alias already. + self.content_type = ContentType.objects.db_manager("default").get_for_model( + value, for_concrete_model=gfk.for_concrete_model + ) + self.object_id = value.pk + self.content_object_domain_id = getattr(value, "pulp_domain_id", None) + -class GenericRelationModel(BaseModel): +class GenericRelationModel(DomainResolvedGenericRelation, BaseModel): """Base model class for implementing Generic Relations. This class provides the required fields to implement generic relations. Instances of @@ -22,7 +166,13 @@ class GenericRelationModel(BaseModel): content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) object_id = models.UUIDField() - content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + _content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + # KI-18: denormalized target domain, auto-populated by DomainResolvedGenericRelation's + # `content_object` setter. Internal/operational only, like Domain.database_alias/moving -- + # not exposed via any serializer. + content_object_domain = models.ForeignKey( + "core.Domain", null=True, on_delete=models.SET_NULL, related_name="+" + ) class Meta: abstract = True diff --git a/pulpcore/app/models/migration_status.py b/pulpcore/app/models/migration_status.py new file mode 100644 index 00000000000..bdbe8fe5a63 --- /dev/null +++ b/pulpcore/app/models/migration_status.py @@ -0,0 +1,41 @@ +""" +Tracks the outcome of `migrate-all`'s per-alias `migrate` invocations (Phase 1). + +Purely operational bookkeeping: gives an operator/the `/status/` endpoint a durable record of +which `DATABASES` alias is at which migration state, without needing to `showmigrations` every +alias individually. Always lives on `default` -- it is control-plane data about the fleet, not +data belonging to any one domain. +""" + +from django.db import models + +from pulpcore.app.models import BaseModel + +MIGRATION_STATUS_CHOICES = ( + ("pending", "Pending"), + ("running", "Running"), + ("complete", "Complete"), + ("failed", "Failed"), +) + + +class MigrationStatus(BaseModel): + """ + Per-`DATABASES`-alias record of the last `migrate-all` outcome. + + Fields: + database_alias (models.TextField): The `DATABASES` alias this row tracks. Unique -- + one row per alias, updated in place on every `migrate-all` run. + status (models.TextField): One of "pending", "running", "complete", "failed". + completed_at (models.DateTimeField): When this alias last reached "complete". + error (models.TextField): The exception message from the most recent failed attempt, + if any. Cleared out on the next successful run. + """ + + database_alias = models.TextField(unique=True) + status = models.TextField(choices=MIGRATION_STATUS_CHOICES, default="pending") + completed_at = models.DateTimeField(null=True) + error = models.TextField(null=True) + + class Meta: + verbose_name_plural = "migration statuses" diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py index b953be30db9..9b5d0ba6b4a 100644 --- a/pulpcore/app/models/publication.py +++ b/pulpcore/app/models/publication.py @@ -21,6 +21,7 @@ from pulpcore.app.files import PulpTemporaryUploadedFile from pulpcore.app.models import AutoAddObjPermsMixin +from pulpcore.app.queryset import CrossDBQuerySetMixin from pulpcore.app.util import cache_key, get_domain_pk, get_url, retain_distributed_pub_enabled from pulpcore.cache import Cache from pulpcore.responses import ArtifactResponse @@ -33,7 +34,7 @@ _logger = logging.getLogger(__name__) -class PublicationQuerySet(models.QuerySet): +class PublicationQuerySet(CrossDBQuerySetMixin, models.QuerySet): """A queryset that provides publication filtering methods.""" def with_content(self, content): diff --git a/pulpcore/app/models/repository.py b/pulpcore/app/models/repository.py index 5470fb5d238..aa0ce1dd8a8 100644 --- a/pulpcore/app/models/repository.py +++ b/pulpcore/app/models/repository.py @@ -18,6 +18,7 @@ from django_lifecycle import AFTER_UPDATE, BEFORE_CREATE, BEFORE_DELETE, hook from rest_framework.exceptions import APIException +from pulpcore.app.queryset import CrossDBQuerySetMixin from pulpcore.app.util import ( batch_qs, cache_key, @@ -882,7 +883,7 @@ class Meta: ) -class RepositoryVersionQuerySet(models.QuerySet): +class RepositoryVersionQuerySet(CrossDBQuerySetMixin, models.QuerySet): """A queryset that provides repository version filtering methods.""" def complete(self): diff --git a/pulpcore/app/models/role.py b/pulpcore/app/models/role.py index c7db1d3fa24..0a229966494 100644 --- a/pulpcore/app/models/role.py +++ b/pulpcore/app/models/role.py @@ -5,6 +5,7 @@ from django.db import models from pulpcore.app.models import BaseModel, Group +from pulpcore.app.models.generic import DomainResolvedGenericRelation class Role(BaseModel): @@ -27,7 +28,7 @@ class Role(BaseModel): permissions = models.ManyToManyField(Permission) -class UserRole(BaseModel): +class UserRole(DomainResolvedGenericRelation, BaseModel): """ Join table for user to role associations with optional content object. @@ -46,7 +47,13 @@ class UserRole(BaseModel): role = models.ForeignKey(Role, related_name="object_users", on_delete=models.CASCADE) content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, null=True) object_id = models.TextField(null=True) - content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + _content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + # KI-18: denormalized target domain, auto-populated by DomainResolvedGenericRelation's + # `content_object` setter; see pulpcore.app.models.generic for why this is needed. + # Internal/operational only. + content_object_domain = models.ForeignKey( + "Domain", null=True, on_delete=models.SET_NULL, related_name="+" + ) domain = models.ForeignKey("Domain", null=True, on_delete=models.CASCADE) class Meta: @@ -57,7 +64,7 @@ class Meta: ] -class GroupRole(BaseModel): +class GroupRole(DomainResolvedGenericRelation, BaseModel): """ Join table for group to role associations with optional content object. @@ -74,7 +81,13 @@ class GroupRole(BaseModel): role = models.ForeignKey(Role, related_name="object_groups", on_delete=models.CASCADE) content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, null=True) object_id = models.TextField(null=True) - content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + _content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) + # KI-18: denormalized target domain, auto-populated by DomainResolvedGenericRelation's + # `content_object` setter; see pulpcore.app.models.generic for why this is needed. + # Internal/operational only. + content_object_domain = models.ForeignKey( + "Domain", null=True, on_delete=models.SET_NULL, related_name="+" + ) domain = models.ForeignKey("Domain", null=True, on_delete=models.CASCADE) class Meta: diff --git a/pulpcore/app/queryset.py b/pulpcore/app/queryset.py new file mode 100644 index 00000000000..7467086b241 --- /dev/null +++ b/pulpcore/app/queryset.py @@ -0,0 +1,63 @@ +""" +Layer 2 of the query architecture: cross-database-safe subquery resolution. + +Django compiles `queryset.filter(field__in=other_queryset)` (or a `Q(...)` containing a nested +queryset) as a SQL subquery. That fails outright when `other_queryset` and the outer queryset +live on two different physical PostgreSQL instances -- there is no way for one Postgres server +to run a subquery against rows that live on a different server. The clearest example is RBAC: +`role_util.get_objects_for_user_roles()` builds `Q(pk_str__in=user_role_pks)` where +`user_role_pks` is a lazy `UserRole` queryset (control-plane, always `default`) and the outer +queryset is a data-plane model that may be routed to a satellite (KI-01, KI-25). + +`CrossDBQuerySetMixin` detects exactly this situation -- a queryset-valued kwarg/`Q` child whose +`.db` differs from the outer queryset's `.db` -- and materializes it with `list()` before letting +the normal `filter()`/`exclude()` machinery run, at which point Django emits an `IN (values...)` +clause instead of a subquery. For single-database deployments `value.db != self.db` is always +`False`, so the fast-path below (`len(settings.DATABASES) <= 1`) skips the extra work entirely +and every one of Django's original semantics/optimizations are preserved -- see the design doc's +Feasibility & Impact Assessment §1 for why that fast path matters (a permanent unconditional tax +on `.filter()`/`.exclude()` for every install otherwise, most of whom never configure a second +database). +""" + +from django.conf import settings +from django.db import models +from django.db.models import Q + + +class CrossDBQuerySetMixin: + """ + Mix into any base `QuerySet` class whose `.filter()`/`.exclude()` calls might receive a + queryset (directly, or nested in a `Q()`) that lives on a different database alias than + `self`. Safe to mix into every queryset unconditionally: the `len(settings.DATABASES) <= 1` + fast path makes this a no-op for the common single-database case. + """ + + def filter(self, *args, **kwargs): + if len(settings.DATABASES) <= 1: + return super().filter(*args, **kwargs) + args = tuple(self._resolve_cross_db_q(a) if isinstance(a, Q) else a for a in args) + self._resolve_cross_db_kwargs(kwargs) + return super().filter(*args, **kwargs) + + def exclude(self, *args, **kwargs): + if len(settings.DATABASES) <= 1: + return super().exclude(*args, **kwargs) + args = tuple(self._resolve_cross_db_q(a) if isinstance(a, Q) else a for a in args) + self._resolve_cross_db_kwargs(kwargs) + return super().exclude(*args, **kwargs) + + def _resolve_cross_db_kwargs(self, kwargs): + for key, value in list(kwargs.items()): + if isinstance(value, models.QuerySet) and value.db != self.db: + kwargs[key] = list(value) + + def _resolve_cross_db_q(self, q): + for i, child in enumerate(q.children): + if isinstance(child, Q): + self._resolve_cross_db_q(child) + elif isinstance(child, tuple): + key, value = child + if isinstance(value, models.QuerySet) and value.db != self.db: + q.children[i] = (key, list(value)) + return q diff --git a/pulpcore/app/role_util.py b/pulpcore/app/role_util.py index 103c68541e3..a049da12eec 100644 --- a/pulpcore/app/role_util.py +++ b/pulpcore/app/role_util.py @@ -132,9 +132,16 @@ def get_objects_for_user_roles( ): return qs - user_role_pks = user.object_roles.filter( - domain__isnull=True, role__permissions=permission - ).values_list("object_id", flat=True) + # KI-01: UserRole/GroupRole are control-plane models (always on `default`), while `qs` may + # be routed to a satellite alias once domains can move. A lazy queryset here would compile + # to a cross-database subquery, which PostgreSQL cannot execute. `CrossDBQuerySetMixin` + # (Layer 2) already guards this at the `qs.filter()` call below, but materialize explicitly + # here too, belt-and-suspenders, since this value is also reused directly in `final_q`. + user_role_pks = list( + user.object_roles.filter(domain__isnull=True, role__permissions=permission).values_list( + "object_id", flat=True + ) + ) final_q = Q(pk_str__in=user_role_pks) if accept_domain_perms and hasattr(qs.model, "pulp_domain"): domains = list( @@ -155,9 +162,12 @@ def get_objects_for_user_roles( ) if use_groups: - group_role_pks = GroupRole.objects.filter( - group__in=user.groups.all(), role__permissions=permission, domain__isnull=True - ).values_list("object_id", flat=True) + # KI-01: same cross-DB-subquery concern as `user_role_pks` above. + group_role_pks = list( + GroupRole.objects.filter( + group__in=user.groups.all(), role__permissions=permission, domain__isnull=True + ).values_list("object_id", flat=True) + ) final_q |= Q(pk_str__in=group_role_pks) return qs.annotate(pk_str=Cast("pk", output_field=CharField())).filter(final_q) @@ -575,3 +585,44 @@ def get_groups_with_perms( for_concrete_model=for_concrete_model, ) return qs.distinct() + + +def cleanup_roles_for_deleted_object(instance): + """ + Explicitly delete `UserRole`/`GroupRole` rows for a deleted object (Layer 4, KI-05). + + `BaseModel.user_roles`/`group_roles` are `GenericRelation`s, so Django's cascade-delete + `Collector` treats them like an `on_delete=CASCADE` FK and deletes matching `UserRole`/ + `GroupRole` rows *using the same database alias as the object being deleted* + (`Collector(using=alias)`). That's correct for control-plane objects (already on + `default`, same alias as `UserRole`/`GroupRole`), but for a data-plane object routed to a + satellite, the cascade instead runs `UserRole.objects.using()...delete()` -- + `.using()` always overrides the router, so this silently deletes zero rows against the + (schema-identical but empty-of-this-data) satellite copy of the `core_userrole` table, + while the *actual* `UserRole`/`GroupRole` rows on `default` are never touched. This function + is the explicit fix: always target `default` directly by `(content_type, object_id)`. + """ + content_type = ContentType.objects.get_for_model(instance, for_concrete_model=False) + object_id = str(instance.pk) + UserRole.objects.using("default").filter( + content_type=content_type, object_id=object_id + ).delete() + GroupRole.objects.using("default").filter( + content_type=content_type, object_id=object_id + ).delete() + + +def on_any_model_post_delete(sender, instance, **kwargs): + """ + `post_delete` receiver for `cleanup_roles_for_deleted_object` (KI-05). + + Connected (in `apps.py`, without a `sender` filter, since `BaseModel` is abstract and every + concrete model -- core and plugin -- inherits the `user_roles`/`group_roles` + `GenericRelation`s) only when `len(settings.DATABASES) > 1`: for single-database + deployments Django's own cascade delete already handles this correctly, so the receiver + isn't connected at all rather than paying a per-delete dispatch cost for a no-op. + """ + from pulpcore.app.models import BaseModel + + if isinstance(instance, BaseModel): + cleanup_roles_for_deleted_object(instance) diff --git a/pulpcore/app/serializers/status.py b/pulpcore/app/serializers/status.py index 8adb8458973..92aa1801fd8 100644 --- a/pulpcore/app/serializers/status.py +++ b/pulpcore/app/serializers/status.py @@ -47,6 +47,27 @@ class DatabaseConnectionSerializer(serializers.Serializer): ) +class DatabaseStatusSerializer(serializers.Serializer): + """ + Serializer for the per-alias connectivity + migration-completeness status of a single + `settings.DATABASES` entry (phase1-status-endpoint). + """ + + alias = serializers.CharField(help_text=_("The settings.DATABASES alias this entry reports on")) + + connected = serializers.BooleanField( + help_text=_("Info about whether the app can connect to this database alias") + ) + + migrations_complete = serializers.BooleanField( + help_text=_( + "Whether this database alias has no pending migrations. Null if connectivity " + "could not be established, since migration status can't be determined in that case." + ), + allow_null=True, + ) + + class RedisConnectionSerializer(serializers.Serializer): """ Serializer for information about the Redis connection @@ -126,6 +147,14 @@ class StatusSerializer(serializers.Serializer): help_text=_("Database connection information") ) + databases = DatabaseStatusSerializer( + help_text=_( + "Per-alias connectivity and migration-completeness status for every configured " + "settings.DATABASES alias (one entry per alias, including 'default')." + ), + many=True, + ) + redis_connection = RedisConnectionSerializer( required=False, help_text=_("Redis connection information"), diff --git a/pulpcore/app/settings.py b/pulpcore/app/settings.py index 11b05424538..5db6a783862 100644 --- a/pulpcore/app/settings.py +++ b/pulpcore/app/settings.py @@ -309,6 +309,24 @@ TASK_PROTECTION_TIME = 0 TMPFILE_PROTECTION_TIME = 0 +# KI-11: how long to wait, in minutes, after a cross-plane GenericForeignKey row +# (CreatedResource/ExportedResource/UserRole/GroupRole with `content_object_domain` set) is +# created/updated before the reconciliation sweep will flag an unresolvable `content_object` as +# an orphan. Guards against false positives from ordinary in-flight replication/task lag rather +# than an actual orphan (e.g. Domain replication hasn't caught up yet, or the referenced object's +# own creating transaction hasn't committed on its alias yet). +CROSS_PLANE_RECONCILIATION_GRACE_MINUTES = 60 + +# KI-11: how long, in days, a confirmed-orphaned cross-plane row is kept (logged/alerted on every +# sweep) before the reconciliation sweep deletes it outright. 0 disables purging entirely -- +# orphans are only ever logged, never deleted, which is the safe default. +CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS = 0 + +# KI-11: how often, in minutes, the reconciliation sweep itself is dispatched as a scheduled +# task. 0 disables the periodic schedule entirely (the 'reconcile-cross-plane-references' +# management command remains available for on-demand/manual runs either way). +CROSS_PLANE_RECONCILIATION_INTERVAL_MINUTES = 24 * 60 + REMOTE_USER_ENVIRON_NAME = "REMOTE_USER" REMOTE_USER_OPENAPI_SECURITY_SCHEME = {"type": "mutualTLS"} @@ -680,3 +698,27 @@ def saml2_settings_hook(settings): settings.set("V3_DOMAIN_API_ROOT", api_root + "/api/v3/") settings.set("V3_API_ROOT_NO_FRONT_SLASH", settings.V3_API_ROOT.lstrip("/")) settings.set("V3_DOMAIN_API_ROOT_NO_FRONT_SLASH", settings.V3_DOMAIN_API_ROOT.lstrip("/")) + +# Domain-aware database routing (see architecture/domain-db-offloading-design.md). Only register +# the router when more than one DATABASES alias is actually configured, so single-database +# deployments -- the overwhelming majority of installs today -- see zero behavior change: +# Django's ConnectionRouter never consults an empty `routers` list, so PulpDomainRouter isn't +# merely a no-op, it's never instantiated or called at all. +# +# Deliberately set as a *plain module global* (`DATABASE_ROUTERS = [...]`) in addition to +# `settings.set(...)`, not `settings.set(...)` alone like `V3_API_ROOT` above: dynaconf's Django +# integration (`DjangoDynaconf()`, called above) replaces `sys.modules["django.conf"]` with a +# wrapper whose `.settings` resolves to its own live `lazy_settings` object -- but only for code +# that imports `django.conf.settings` *after* that replacement happens. Some Django internals +# (e.g. `django.db.utils`, which owns the global `ConnectionRouter` singleton every query +# consults) do `from django.conf import settings` earlier than that and keep a reference to the +# original, vanilla `LazySettings` object, which resolves this settings module the normal +# Django way (`Settings.__init__` snapshotting `dir(mod)`) whenever it's first touched -- and +# that snapshot only sees real module-level globals, not anything added purely via +# `settings.set(...)` on the dynaconf side. Setting the plain global here ensures both the +# dynaconf-backed settings object (the overwhelming majority of settings consumers, via +# `settings.set(...)`) and any such early/stale consumer (via `dir(mod)`) resolve +# `DATABASE_ROUTERS` consistently. +if len(settings.DATABASES) > 1: + DATABASE_ROUTERS = ["pulpcore.app.db_router.PulpDomainRouter"] + settings.set("DATABASE_ROUTERS", DATABASE_ROUTERS) diff --git a/pulpcore/app/tasks/reconciliation.py b/pulpcore/app/tasks/reconciliation.py new file mode 100644 index 00000000000..eb734dcb366 --- /dev/null +++ b/pulpcore/app/tasks/reconciliation.py @@ -0,0 +1,141 @@ +""" +KI-11 reconciliation: periodic sweep for orphaned cross-plane `GenericForeignKey` references. + +There is no distributed transaction spanning `default` (control plane) and a satellite alias +(data plane) -- e.g. a task can write its `CreatedResource` row on `default` and then crash (or +the process can be killed) before/without the corresponding data-plane write ever landing on the +satellite, or a domain can be moved/cleaned-up (`move-domain` / `cleanup-moved-domain`) while a +handful of in-flight cross-plane rows still point at the old alias. This is an accepted trade-off +(see the design doc's KI-11 and "No Distributed Transactions" sections), mitigated by detecting +and reporting (optionally purging) the resulting orphans here rather than by trying to eliminate +them. + +This reuses the KI-18 `content_object` resolver (`DomainResolvedGenericRelation`, see +`pulpcore.app.models.generic`) as the *only* detection mechanism: every model in scope already +raises loudly (logs + re-raises `DoesNotExist`) when a cross-plane `content_object` can't be +resolved on its recorded alias, so this sweep doesn't need any separate orphan-detection logic -- +it just needs to iterate the candidate rows and call `.content_object` on each. +""" + +from datetime import timedelta +from logging import getLogger + +from django.conf import settings +from django.core.exceptions import ObjectDoesNotExist +from django.utils import timezone + +from pulpcore.app.models import CreatedResource, ExportedResource +from pulpcore.app.models.role import GroupRole, UserRole + +log = getLogger(__name__) + +#: Every model that mixes in `DomainResolvedGenericRelation` (KI-18) and can therefore hold a +#: stale cross-plane `content_object` reference. Kept in one place so a new such model only needs +#: adding here to be covered by this sweep. +_GFK_MODELS = (CreatedResource, ExportedResource, UserRole, GroupRole) + + +def _candidate_rows(model, cutoff): + """ + Rows of `model` worth checking: those with a recorded cross-plane target + (`content_object_domain_id` set -- same-plane targets can't suffer from KI-18 at all, since + they're always resolved on this row's own alias) that are old enough that a resolution + failure is meaningfully suspicious rather than ordinary in-flight lag. + """ + return model.objects.using("default").filter( + content_object_domain_id__isnull=False, + pulp_last_updated__lt=cutoff, + ) + + +def reconcile_cross_plane_references( + grace_period_minutes=None, + purge_after_days=None, + dry_run=False, +): + """ + Sweep `CreatedResource`/`ExportedResource`/`UserRole`/`GroupRole` for orphaned cross-plane + `content_object` references and report (optionally purge) them. + + Args: + grace_period_minutes (int): Rows updated more recently than this are skipped (still + "fresh enough" that a resolution failure is more likely ordinary replication lag than + a real orphan). Defaults to `settings.CROSS_PLANE_RECONCILIATION_GRACE_MINUTES`. + purge_after_days (int): Confirmed orphans older than this many days are deleted outright. + `0`/`None` (the default, from `settings.CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS`) + disables purging -- orphans are only ever logged/reported. + dry_run (bool): If True, never delete anything regardless of `purge_after_days`; only + report what would happen. Used by `reconcile-cross-plane-references --dry-run`. + + Returns: + dict: Per-model counts of `checked`, `orphaned`, and `purged` rows, plus a flat list of + `orphans` describing each unresolvable row (model label, pk, alias, age). + """ + if grace_period_minutes is None: + grace_period_minutes = settings.CROSS_PLANE_RECONCILIATION_GRACE_MINUTES + if purge_after_days is None: + purge_after_days = settings.CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS + + cutoff = timezone.now() - timedelta(minutes=grace_period_minutes) + purge_cutoff = timezone.now() - timedelta(days=purge_after_days) if purge_after_days else None + + report = {"checked": 0, "orphaned": 0, "purged": 0, "orphans": []} + + # Deliberately no `ProgressReport` here (unlike e.g. `orphan_cleanup`): `ProgressReport.task` + # is a required (non-nullable) FK, so it can only be used from inside a dispatched task's own + # execution context (`Task.current()` set). This function is also called directly by the + # 'reconcile-cross-plane-references' management command for on-demand/manual runs, with no + # task at all -- plain logging works in both contexts. + for model in _GFK_MODELS: + qs = _candidate_rows(model, cutoff) + for row in qs.iterator(): + report["checked"] += 1 + try: + row.content_object + except ObjectDoesNotExist: + # The resolver (DomainResolvedGenericRelation.content_object) already logged the + # specifics (model/pk/alias/content_type); we just tally here. + report["orphaned"] += 1 + alias = ( + row.content_object_domain.database_alias + if row.content_object_domain_id + else None + ) + age = timezone.now() - row.pulp_last_updated + report["orphans"].append( + { + "model": model._meta.label, + "pk": str(row.pk), + "alias": alias, + "age_days": age.days, + } + ) + if not dry_run and purge_cutoff and row.pulp_last_updated < purge_cutoff: + log.warning( + "Purging orphaned cross-plane row %s (pk=%s, alias=%s, age=%sd) -- " + "unresolvable content_object older than " + "CROSS_PLANE_RECONCILIATION_PURGE_AFTER_DAYS=%s.", + model._meta.label, + row.pk, + alias, + age.days, + purge_after_days, + ) + row.delete() + report["purged"] += 1 + + if report["orphaned"]: + log.error( + "Cross-plane reconciliation found %s orphaned reference(s) out of %s checked " + "(%s purged). See preceding log lines for details on each. Run " + "'pulpcore-manager reconcile-cross-plane-references' for a full report.", + report["orphaned"], + report["checked"], + report["purged"], + ) + else: + log.info( + "Cross-plane reconciliation checked %s row(s), found no orphans.", report["checked"] + ) + + return report diff --git a/pulpcore/app/util.py b/pulpcore/app/util.py index 237ad50e67a..e7d48efc658 100644 --- a/pulpcore/app/util.py +++ b/pulpcore/app/util.py @@ -2,7 +2,7 @@ import os import socket import zlib -from contextlib import ExitStack +from contextlib import ExitStack, contextmanager from datetime import timedelta from functools import lru_cache from gettext import gettext as _ @@ -15,14 +15,14 @@ from django.apps import apps from django.conf import settings -from django.db import connection +from django.db import connections from django.db.models import Model, UUIDField from rest_framework.reverse import reverse as drf_reverse from rest_framework.serializers import ValidationError from pulpcore.app import models from pulpcore.app.apps import pulp_plugin_configs -from pulpcore.app.contexts import _current_domain, _current_user_func +from pulpcore.app.contexts import _current_domain, _current_user_func, with_domain from pulpcore.app.loggers import deprecation_logger from pulpcore.exceptions.validation import InvalidSignatureError @@ -506,6 +506,17 @@ def configure_cleanup(): ), ("tasks", "pulpcore.app.tasks.purge.purge", settings.TASK_PROTECTION_TIME), ("content", "pulpcore.app.tasks.orphan.orphan_cleanup", settings.ORPHAN_PROTECTION_TIME), + ( + # KI-11: periodic sweep for orphaned cross-plane GenericForeignKey references. + # Control-plane only (Task/CreatedResource/UserRole/GroupRole all always live on + # `default`), and each row's target is resolved on *its own* recorded alias by the + # KI-18 resolver -- unlike orphan/upload/tmpfile cleanup, this needs no per-domain + # dispatch (KI-21) to reach satellite data; a single run against `default` already + # covers every domain. + "cross-plane references", + "pulpcore.app.tasks.reconciliation.reconcile_cross_plane_references", + settings.CROSS_PLANE_RECONCILIATION_INTERVAL_MINUTES, + ), ]: if protection_time > 0: dispatch_interval = timedelta(minutes=protection_time) @@ -637,6 +648,18 @@ def get_domain_pk(): This is ran in plugin migrations so we can not move this implemenation or change its semantics, EVER! + + KI-06: the raw-SQL fallback below (reached when there is neither a domain ContextVar nor a + cached default domain -- notably true the first time this runs during `migrate + --database=` before that satellite's `core_domain` table has been + populated by Domain replication/`sync-domains`) must always read from the `default` alias + explicitly. `default` is the only alias guaranteed to be migrated and to hold the + authoritative "default" Domain row at this point in the bootstrap sequence (`migrate-all` + always migrates `default` first, then replicates Domain rows, then migrates satellites). + Do not use the bare `django.db.connection` proxy here -- it happens to default to the + "default" alias today, but that's an implicit coincidence, not a guarantee, and this + function's raw SQL must never silently start querying a satellite that doesn't have the + domain data yet. """ if domain := _current_domain.get(): return domain.pk @@ -644,7 +667,7 @@ def get_domain_pk(): if default_domain: return default_domain.pk # If we haven't cached the default_domain then use raw SQL to get its PK - with connection.cursor() as cursor: + with connections["default"].cursor() as cursor: cursor.execute("SELECT pulp_id FROM core_domain WHERE name = 'default'") row = cursor.fetchone() return row[0] @@ -656,6 +679,41 @@ def set_domain(new_domain): return new_domain +@contextmanager +def domain_db(domain): + """ + Set the domain ContextVar for `domain` and yield its `database_alias`. + + Layer 3 of the query architecture (see the design doc's "Query Architecture" section): + for admin/management-command code that needs to iterate over every domain and run queries + against each domain's actual database, wrap each iteration in this context manager and use + the yielded alias with an explicit `.using(alias)` on every queryset touched -- do not rely + on the router alone, since it cannot see queryset filters (only the ContextVar, which this + sets). + + Example: + for domain in Domain.objects.all(): + with domain_db(domain) as alias: + for repo in Repository.objects.using(alias).filter(pulp_domain=domain): + ... + """ + with with_domain(domain): + yield domain.database_alias + + +def for_each_domain(callback): + """ + Call `callback(domain, alias)` once per `Domain`, with the domain ContextVar set and the + domain's database alias passed in for explicit `.using(alias)` calls. + + See `domain_db()` for the underlying context manager and the design doc's Layer 3 for the + broader convention this implements (KI-08, KI-19, KI-21). + """ + for domain in models.Domain.objects.all(): + with domain_db(domain) as alias: + callback(domain, alias) + + def cache_key(base_path): """Returns the base-key(s) used in the Cache for the passed base_path(s).""" if settings.DOMAIN_ENABLED: diff --git a/pulpcore/app/views/status.py b/pulpcore/app/views/status.py index 7d494e272f2..c5c5aaa3df2 100644 --- a/pulpcore/app/views/status.py +++ b/pulpcore/app/views/status.py @@ -4,6 +4,8 @@ from gettext import gettext as _ from django.conf import settings +from django.db import connections, utils as db_utils +from django.db.migrations.executor import MigrationExecutor from django.db.models import Sum from drf_spectacular.utils import extend_schema from rest_framework.response import Response @@ -77,6 +79,7 @@ def get(self, request): redis_status = {"connected": False} db_status = {"connected": self._get_db_conn_status()} + databases = self._get_databases_status() online_workers = AppStatus.objects.online().filter(app_type="worker") online_api_apps = AppStatus.objects.online().filter(app_type="api") @@ -93,6 +96,7 @@ def get(self, request): "online_api_apps": online_api_apps, "online_content_apps": online_content_apps, "database_connection": db_status, + "databases": databases, "redis_connection": redis_status, "storage": _disk_usage(), "content_settings": content_settings, @@ -119,6 +123,47 @@ def _get_db_conn_status(): else: return True + @staticmethod + def _get_databases_status(): + """ + Per-alias connectivity + migration-completeness report, one entry per configured + `settings.DATABASES` alias (phase1-status-endpoint; see the design doc's "Updated + `/status/` endpoint" section for the exact shape). For a single-database deployment + this is a one-element list equivalent to `database_connection` above; it becomes + actionable once satellite aliases exist, e.g. for an operator/monitoring check that + wants to know *which* alias is unreachable or mid-migration rather than just "the + database" in aggregate. + + `migrations_complete` is `None` (not `False`) when connectivity itself fails, since + migration completeness can't be determined without a connection -- mirrors the JSON + shape in the design doc, and avoids conflating "definitely has pending migrations" + with "unknown, because we can't even connect". + """ + results = [] + for alias in settings.DATABASES: + connected = False + migrations_complete = None + try: + connections[alias].ensure_connection() + connected = True + executor = MigrationExecutor(connections[alias]) + targets = executor.loader.graph.leaf_nodes() + migrations_complete = not executor.migration_plan(targets) + except db_utils.OperationalError: + _logger.exception( + _("Cannot connect to database alias '%(alias)s' during status check."), + {"alias": alias}, + ) + except Exception: + _logger.exception( + _("Failed to determine migration status for database alias '%(alias)s'."), + {"alias": alias}, + ) + results.append( + {"alias": alias, "connected": connected, "migrations_complete": migrations_complete} + ) + return results + @staticmethod def _get_redis_conn_status(): """ diff --git a/pulpcore/app/viewsets/task.py b/pulpcore/app/viewsets/task.py index c24131dd250..8fbf05bed90 100644 --- a/pulpcore/app/viewsets/task.py +++ b/pulpcore/app/viewsets/task.py @@ -12,6 +12,7 @@ from pulpcore.app.models import ( AppStatus, + Artifact, CreatedResource, ProfileArtifact, RepositoryVersion, @@ -289,8 +290,15 @@ def profile_artifacts(self, request, pk): task = self.get_object() data = {} - for pa in ProfileArtifact.objects.select_related("artifact").filter(task=task): - data[pa.name] = get_artifact_url(pa.artifact) + # KI-02/KI-03: ProfileArtifact is control-plane (task=task, always on `default`), but + # its `artifact` FK is data-plane and may live on a different physical database once + # the task's domain has moved to a satellite. `select_related("artifact")` compiles to + # a same-connection SQL JOIN, which cannot span two databases -- load the artifact + # explicitly via the task's own domain alias instead. + alias = task.pulp_domain.database_alias + for pa in ProfileArtifact.objects.filter(task=task): + artifact = Artifact.objects.using(alias).get(pk=pa.artifact_id) + data[pa.name] = get_artifact_url(artifact) return Response({"urls": data}) diff --git a/pulpcore/constants.py b/pulpcore/constants.py index 9b48a5f91e1..95bc9d26125 100644 --- a/pulpcore/constants.py +++ b/pulpcore/constants.py @@ -9,6 +9,14 @@ TASK_UNBLOCKING_LOCK = 84 TASK_METRICS_LOCK = 74 WORKER_CLEANUP_LOCK = 11 +#: Held (on the `default` alias) for the duration of a `migrate-all` run so two orchestration +#: runs (e.g. two pods restarting at once) can't race each other across DATABASES aliases. +MIGRATION_ORCHESTRATOR_LOCK = 137 +#: Held (on the `default` alias) for the duration of a `move-domain` run so two moves (of the +#: same or different domains) can't race each other -- `move-domain` is a rare, admin-run, +#: inherently-serial operation, so a single fleet-wide lock (rather than one keyed per domain) +#: keeps the locking logic as simple as `migrate-all`'s at negligible cost. +DOMAIN_MOVE_LOCK = 138 # Reasons to send along a task worker wakeup call. TASK_WAKEUP_UNBLOCK = "unblock" diff --git a/pulpcore/middleware.py b/pulpcore/middleware.py index dd6c98a6f1b..4d54a528d91 100644 --- a/pulpcore/middleware.py +++ b/pulpcore/middleware.py @@ -1,9 +1,13 @@ import re import time +from gettext import gettext as _ from os import environ from django.conf import settings from django.core.exceptions import MiddlewareNotUsed +from django.db import connections +from django.db.utils import Error as DjangoDBError +from django.http import JsonResponse from django.http.response import Http404 from pulpcore.app.contexts import x_task_diagnostics_var @@ -16,6 +20,10 @@ ) from pulpcore.metrics import init_otel_meter +#: HTTP methods that don't need to be rejected for a domain mid-move (`Domain.moving`) -- only +#: writes are unsafe while the target alias may not yet have consistent data (KI-12). +_SAFE_HTTP_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) + class DomainMiddleware: """ @@ -47,10 +55,53 @@ def process_view(self, request, view_func, view_args, view_kwargs): domain = Domain.objects.get(name=domain_name) except Domain.DoesNotExist: raise Http404() + degraded_response = self._degraded_response(request, domain) + if degraded_response is not None: + return degraded_response set_domain(domain) setattr(request, "pulp_domain", domain) return None + @staticmethod + def _degraded_response(request, domain): + """ + Graceful degradation for a domain whose data-plane alias is unreachable, or that is + mid-move (phase1-graceful-degradation / KI-12). Returns a 503 `JsonResponse` to short + -circuit `process_view` (Django dispatches whatever a `process_view` hook returns + instead of calling the view), or `None` if the request should proceed normally. + + Deliberately handled here rather than in `PulpDomainRouter` itself: routers only return + an alias name, they have no mechanism to short-circuit a request with an HTTP response + (see the design doc's Problem 2 discussion of KI-12) -- by the time a router's + `db_for_read`/`db_for_write` raises, the error surfaces from deep inside whatever + viewset/serializer code first touched the database, with no guarantee of a clean, single + error message. + """ + alias = domain.database_alias + if alias != "default": + try: + connections[alias].ensure_connection() + except DjangoDBError: + return JsonResponse( + { + "detail": _( + "Database for domain '{name}' is currently unavailable." + ).format(name=domain.name) + }, + status=503, + ) + if domain.moving and request.method not in _SAFE_HTTP_METHODS: + return JsonResponse( + { + "detail": _( + "Domain '{name}' is currently being moved to a different database; " + "write operations are temporarily unavailable." + ).format(name=domain.name) + }, + status=503, + ) + return None + class APIRootRewriteMiddleware: """ diff --git a/pulpcore/tasking/redis_worker.py b/pulpcore/tasking/redis_worker.py index d9449a885df..c6d8465820f 100644 --- a/pulpcore/tasking/redis_worker.py +++ b/pulpcore/tasking/redis_worker.py @@ -20,7 +20,7 @@ import redis from django.conf import settings -from django.db import DatabaseError, IntegrityError, connection, transaction +from django.db import DatabaseError, IntegrityError, connection, connections, transaction from django.utils import timezone from pulpcore.app.apps import pulp_plugin_configs @@ -421,6 +421,36 @@ def is_compatible(self, task): return False return True + def is_domain_available(self, task): + """ + phase1-graceful-degradation / KI-12 (task-dispatch half): see the identical method on + `pulpcore.tasking.worker.Worker` for the full rationale (this is the Redis-backend + equivalent of that Postgres-advisory-lock-backend check) -- don't attempt to run `task` + while its domain's data-plane alias is unreachable or mid-move; leave it `waiting` + instead of failing it with a confusing `OperationalError` partway through. + """ + domain = task.pulp_domain + if domain.moving: + _logger.info( + _("Domain '%s' is being moved between databases; deferring task %s."), + domain.name, + task.pk, + ) + return False + alias = domain.database_alias + if alias != "default": + try: + connections[alias].ensure_connection() + except DatabaseError: + _logger.warning( + _("Database alias '%s' for domain '%s' is unavailable; deferring task %s."), + alias, + domain.name, + task.pk, + ) + return False + return True + def fetch_task(self): """ Fetch an available waiting task using Redis locks. @@ -648,8 +678,8 @@ def handle_tasks(self): # No task found break - if not self.is_compatible(task): - # Incompatible task, add to ignored list + if not self.is_compatible(task) or not self.is_domain_available(task): + # Incompatible (or domain-unavailable/moving), add to ignored list self.ignored_task_ids.append(task.pk) # Atomically release task lock + resource locks so other workers can attempt it self._maybe_release_locks(task, mark_released=False) diff --git a/pulpcore/tasking/worker.py b/pulpcore/tasking/worker.py index eae0d248cc7..a621ecc8f4a 100644 --- a/pulpcore/tasking/worker.py +++ b/pulpcore/tasking/worker.py @@ -10,7 +10,7 @@ from tempfile import TemporaryDirectory from django.conf import settings -from django.db import DatabaseError, IntegrityError, connection, transaction +from django.db import DatabaseError, IntegrityError, connection, connections, transaction from django.db.models import Case, Count, F, Max, Value, When from django.utils import timezone from packaging.version import parse as parse_version @@ -328,6 +328,46 @@ def is_compatible(self, task): return False return True + def is_domain_available(self, task): + """ + phase1-graceful-degradation / KI-12 (task-dispatch half): don't let this worker attempt + to run `task` while its domain's data-plane alias is unreachable or mid-move -- doing so + would just fail the task with a confusing `OperationalError` partway through, when the + correct behavior is for it to stay `waiting` and be retried later (by this worker or + another) once the alias is healthy/the move completes. Mirrors `is_compatible()`'s + "don't run it now, but don't fail it either" shape and is combined with it the same way + in `handle_unblocked_tasks`. + + Caveat shared with `is_compatible()`: a task skipped here is added to + `self.ignored_task_ids` by the caller, and `cleanup_ignored_tasks()` only re-considers + ignored tasks once they leave `TASK_INCOMPLETE_STATES` -- so *this* worker process won't + retry a domain-unavailable task again until it restarts (a version-incompatible task has + the identical limitation today). Other worker processes, and this one after a restart, + are unaffected. Acceptable for v1: satellite outages are expected to be handled + operationally (see the runbook), not by busy-polling every waiting task every cycle. + """ + domain = task.pulp_domain # Hidden db roundtrip, same as is_compatible() above + if domain.moving: + _logger.info( + _("Domain '%s' is being moved between databases; deferring task %s."), + domain.name, + task.pk, + ) + return False + alias = domain.database_alias + if alias != "default": + try: + connections[alias].ensure_connection() + except DatabaseError: + _logger.warning( + _("Database alias '%s' for domain '%s' is unavailable; deferring task %s."), + alias, + domain.name, + task.pk, + ) + return False + return True + def unblock_tasks(self): """Iterate over waiting tasks and mark them unblocked accordingly. @@ -605,7 +645,11 @@ def handle_unblocked_tasks(self): elif task.state == TASK_STATES.RUNNING: # A running task without a lock must be abandoned. self.cancel_abandoned_task(task, TASK_STATES.FAILED, "Worker has gone missing.") - elif task.state == TASK_STATES.WAITING and self.is_compatible(task): + elif ( + task.state == TASK_STATES.WAITING + and self.is_compatible(task) + and self.is_domain_available(task) + ): if task.immediate: self.supervise_immediate_task(task) else: diff --git a/pulpcore/tests/unit/content/test_handler.py b/pulpcore/tests/unit/content/test_handler.py index 48285e483a6..4385364857a 100644 --- a/pulpcore/tests/unit/content/test_handler.py +++ b/pulpcore/tests/unit/content/test_handler.py @@ -8,7 +8,8 @@ from django.db import IntegrityError from django_guid import clear_guid, set_guid -from pulpcore.app.models import AppStatus +from pulpcore.app.contexts import with_domain +from pulpcore.app.models import AppStatus, Domain from pulpcore.constants import TASK_STATES from pulpcore.content.handler import CheckpointListings, Handler, PathNotResolved from pulpcore.plugin.models import ( @@ -22,6 +23,7 @@ Repository, RepositoryVersion, ) +from pulpcore.tests.unit.test_multi_database_routing import SATELLITE_ALIAS, requires_multi_db @pytest.fixture @@ -318,6 +320,61 @@ def test_pull_through_save_single_artifact_content( assert ra is not None +@requires_multi_db +@pytest.mark.django_db(databases=["default", SATELLITE_ALIAS]) +def test_pull_through_save_single_artifact_content_multi_db( + request123, download_result_mock, monkeypatch, tmp_path +): + """KI-27 regression: the real pull-through save path (`Handler._save_artifact`) must not + crash with `RecursionError` under a multi-DB configuration, and the objects it creates must + end up on the domain's actual database alias, not silently on `default`. + + Builds the `RemoteArtifact` the exact same way `content/handler.py`'s pull-through code does + (`ContentArtifact(relative_path=...)` -- unsaved, no `pulp_domain` set yet -- followed by + `RemoteArtifact(remote=..., url=..., content_artifact=...)`, mirroring lines ~909/~1073 of + `content/handler.py`), while a satellite-routed `Domain` is the active ContextVar -- this is + exactly `test_pull_through_save_single_artifact_content` (which passes even pre-fix on a + single-DB deployment, since the router is never registered/invoked there at all) re-run with + `PulpDomainRouter` actually active, which is what turns the latent bug into a crash. + """ + domain = Domain.objects.create( + name="ki27-pull-through-domain", + storage_class="pulpcore.app.models.storage.FileSystem", + storage_settings={"location": str(tmp_path)}, + database_alias=SATELLITE_ALIAS, + ) + try: + with with_domain(domain): + remote = Remote.objects.create(name="123", url="https://123") + handler = Handler() + remote.get_remote_artifact_content_type = Mock(return_value=Content) + content_init_mock = Mock(return_value=Content()) + monkeypatch.setattr(Content, "init_from_artifact_and_relative_path", content_init_mock) + ca = ContentArtifact(relative_path="c123") + ra = RemoteArtifact(url=f"{remote.url}/c123", remote=remote, content_artifact=ca) + + content_artifacts = handler._save_artifact(download_result_mock, ra, request=request123) + artifact = content_artifacts[ra.content_artifact.relative_path].artifact + + assert Artifact.objects.using(SATELLITE_ALIAS).filter(pk=artifact.pk).exists() + assert not Artifact.objects.using("default").filter(pk=artifact.pk).exists() + saved_ra = ( + RemoteArtifact.objects.using(SATELLITE_ALIAS) + .filter(url=f"{remote.url}/c123", remote=remote) + .first() + ) + assert saved_ra is not None + assert saved_ra.pulp_domain_id == domain.pk + finally: + for alias in {SATELLITE_ALIAS, "default"}: + RemoteArtifact.objects.using(alias).filter(pulp_domain=domain).delete() + ContentArtifact.objects.using(alias).filter(content__pulp_domain=domain).delete() + Content.objects.using(alias).filter(pulp_domain=domain).delete() + Artifact.objects.using(alias).filter(pulp_domain=domain).delete() + Remote.objects.using(alias).filter(pulp_domain=domain).delete() + domain.delete() + + def test_pull_through_save_multi_artifact_content( remote123, request123, download_result_mock, monkeypatch, tmp_path ): diff --git a/pulpcore/tests/unit/models/test_remote.py b/pulpcore/tests/unit/models/test_remote.py index bb394816d37..8b2554ac761 100644 --- a/pulpcore/tests/unit/models/test_remote.py +++ b/pulpcore/tests/unit/models/test_remote.py @@ -1,16 +1,26 @@ +from pathlib import Path from uuid import uuid4 import pytest from cryptography.fernet import InvalidToken +from django.conf import settings from django.core.management import call_command from django.db import connection +from pulpcore.app.contexts import with_domain from pulpcore.app.models import Domain, Remote from pulpcore.app.models.fields import EncryptedTextField, _fernet TEST_KEY1 = b"hPCIFQV/upbvPRsEpgS7W32XdFA2EQgXnMtyNAekebQ=" TEST_KEY2 = b"6Xyv+QezAQ+4R870F5qsgKcngzmm46caDB2gyo9qnpc=" +#: KI-22/KI-27: `rotate-db-key` (management/commands/rotate-db-key.py) unconditionally loops +#: over every alias in `settings.DATABASES`, not just `default` -- see +#: `test_multi_database_routing.py`'s module docstring for why a real second alias must be +#: configured via `PULP_DATABASES__data_1__*` env vars for the satellite-hosted half of +#: `test_rotate_db_key` below to actually exercise anything. +SATELLITE_ALIAS = "data_1" + @pytest.fixture def fake_fernet(tmp_path, settings): @@ -50,27 +60,93 @@ def test_encrypted_proxy_password(fake_fernet): assert proxy_password == "test" -@pytest.mark.django_db +@pytest.mark.django_db(databases=list(settings.DATABASES)) def test_rotate_db_key(fake_fernet): + """KI-22/KI-27: `rotate-db-key` must re-encrypt encrypted fields on *every* configured + database alias, not just `default`. + + The `databases=list(settings.DATABASES)` marker (rather than a hardcoded list, or the bare + `@pytest.mark.django_db` this test used before) is what makes this test pass in both + configurations described in the KI-27 writeup: with only `default` configured (today's CI, + where `rotate-db-key`'s per-alias loop only ever has one alias to iterate and this is exactly + the original test), and with a real `data_1` alias configured (`PULP_DATABASES__data_1__*` + env vars -- see `test_multi_database_routing.py`'s module docstring), where the command's + per-alias loop (`management/commands/rotate-db-key.py`) actually touches a second + connection and a hardcoded `@pytest.mark.django_db` (declaring only `default`) would make + pytest-django raise `DatabaseOperationForbidden` the moment the command reaches `data_1`. + """ remote = Remote.objects.create(name=uuid4(), proxy_password="test") domain = Domain.objects.create(name=uuid4(), storage_settings={"base_path": "/foo"}) - next(fake_fernet) # new + old key - - call_command("rotate-db-key") - - next(fake_fernet) # new key - - del remote.proxy_password - assert remote.proxy_password == "test" - del domain.storage_settings - assert domain.storage_settings == {"base_path": "/foo"} - - next(fake_fernet) # old key - - del remote.proxy_password - with pytest.raises(InvalidToken): - remote.proxy_password - del domain.storage_settings - with pytest.raises(InvalidToken): - domain.storage_settings + satellite_remote = None + satellite_domain = None + if SATELLITE_ALIAS in settings.DATABASES: + # Prove KI-22 is actually fixed, not just that the command doesn't crash: create a + # satellite-hosted encrypted-field object (routed to `data_1` via the domain's + # `database_alias`, exactly like production data on a moved domain would be), and later + # assert *that* object's ciphertext was rewritten with the new key too. + satellite_domain = Domain.objects.create( + name=uuid4(), + storage_class="pulpcore.app.models.storage.FileSystem", + storage_settings={"base_path": "/satellite"}, + database_alias=SATELLITE_ALIAS, + ) + with with_domain(satellite_domain): + satellite_remote = Remote.objects.create( + name=uuid4(), proxy_password="satellite-secret" + ) + assert not Remote.objects.using("default").filter(pk=satellite_remote.pk).exists() + assert Remote.objects.using(SATELLITE_ALIAS).filter(pk=satellite_remote.pk).exists() + + try: + next(fake_fernet) # new + old key + + call_command("rotate-db-key") + + next(fake_fernet) # new key + + del remote.proxy_password + assert remote.proxy_password == "test" + del domain.storage_settings + assert domain.storage_settings == {"base_path": "/foo"} + + if satellite_remote is not None: + satellite_remote = Remote.objects.using(SATELLITE_ALIAS).get(pk=satellite_remote.pk) + assert satellite_remote.proxy_password == "satellite-secret" + + next(fake_fernet) # old key + + del remote.proxy_password + with pytest.raises(InvalidToken): + remote.proxy_password + del domain.storage_settings + with pytest.raises(InvalidToken): + domain.storage_settings + + if satellite_remote is not None: + # The satellite copy was actually re-encrypted with the new key (not left with the + # old key's ciphertext, and not silently skipped) -- it now fails to decrypt with + # the *old* key too, exactly like the `default`-hosted `remote` above. Use an + # explicit `.using(SATELLITE_ALIAS)` fetch (rather than `del` + implicit + # `refresh_from_db()`, as used for `remote` above): `satellite_remote` was created + # outside of any `with_domain()` context by this point, so there's no ContextVar to + # inform routing, and its `pulp_domain` relation was never fetched/cached either + # (only `pulp_domain_id`, via the `default=get_domain_pk` callable) -- exactly the + # scenario the router's instance-hint path intentionally no longer guesses at + # post-KI-27, to avoid the N+1 bug. `from_db_value` decrypts eagerly during the + # fetch itself, so the raise happens inside `.get()`, not on attribute access. + with pytest.raises(InvalidToken): + Remote.objects.using(SATELLITE_ALIAS).get(pk=satellite_remote.pk) + finally: + if satellite_remote is not None or satellite_domain is not None: + # Deleting these rows requires Django's collector to SELECT them first (for + # signals/cascades), which decrypts every encrypted field on the row regardless of + # whether the test cares about its value -- restore a key file with every key used + # above so that read succeeds no matter which rotation step we stopped at. + key_file = Path(settings.DB_ENCRYPTION_KEY) + key_file.write_bytes(TEST_KEY2 + b"\n" + TEST_KEY1) + _fernet.cache_clear() + if satellite_remote is not None: + Remote.objects.using(SATELLITE_ALIAS).filter(pk=satellite_remote.pk).delete() + if satellite_domain is not None: + satellite_domain.delete() diff --git a/pulpcore/tests/unit/test_domain_move.py b/pulpcore/tests/unit/test_domain_move.py new file mode 100644 index 00000000000..18636d96348 --- /dev/null +++ b/pulpcore/tests/unit/test_domain_move.py @@ -0,0 +1,132 @@ +""" +Integration tests for `move-domain`/`cleanup-moved-domain`/`domain-size` (phase2-move-domain, +phase2-cleanup), implementing Strategy A ("Read-Only Cutover") from +`architecture/domain-db-offloading-design.md`. + +See `test_multi_database_routing.py`'s module docstring for why these require a real `data_1` +alias (set via `PULP_DATABASES__data_1__*` env vars) to run at all, rather than skipping. +""" + +import hashlib + +import pytest +from django.core.files.base import ContentFile +from django.core.management import call_command + +from pulpcore.app.contexts import with_domain +from pulpcore.app.models import ( + Artifact, + ContentArtifact, + Domain, + DomainMove, +) +from pulp_file.app.models import FileContent, FileRepository + +from .test_multi_database_routing import SATELLITE_ALIAS, requires_multi_db + +pytestmark = [requires_multi_db, pytest.mark.django_db(databases=["default", SATELLITE_ALIAS])] + + +@pytest.fixture +def hot_domain(tmp_path): + domain = Domain.objects.create( + name="move-test-domain", + storage_class="pulpcore.app.models.storage.FileSystem", + storage_settings={"location": str(tmp_path)}, + ) + with with_domain(domain): + repo = FileRepository.objects.create(name="move-test-repo", pulp_domain=domain) + data = b"move-domain integration test content" + digests = { + alg: hashlib.new(alg, data).hexdigest() + for alg in ("sha224", "sha256", "sha384", "sha512") + } + artifact = Artifact.objects.create( + file=ContentFile(data, name="x"), size=len(data), pulp_domain=domain, **digests + ) + content = FileContent.objects.create( + relative_path="a/b.txt", digest=digests["sha256"], pulp_domain=domain + ) + ContentArtifact.objects.create(artifact=artifact, content=content, relative_path="a/b.txt") + version = repo.new_version() + with version: + version.add_content(FileContent.objects.filter(pk=content.pk)) + yield domain + domain.refresh_from_db() + for alias in {domain.database_alias, "default"}: + # Order matters: Repository first (cascades RepositoryVersion/RepositoryContent), then + # Content (cascades ContentArtifact), then Artifact last -- ContentArtifact.artifact is + # PROTECT, so deleting Artifact first (while a ContentArtifact still references it) + # raises ProtectedError. + FileRepository.objects.using(alias).filter(pulp_domain=domain).delete() + FileContent.objects.using(alias).filter(pulp_domain=domain).delete() + Artifact.objects.using(alias).filter(pulp_domain=domain).delete() + domain.delete() + + +class TestMoveDomain: + def test_move_domain_relocates_data_and_verifies_cleanly(self, hot_domain): + call_command("move-domain", hot_domain.name, "--to", SATELLITE_ALIAS, "--noinput") + + hot_domain.refresh_from_db() + assert hot_domain.database_alias == SATELLITE_ALIAS + assert hot_domain.moving is False + + assert FileRepository.objects.using(SATELLITE_ALIAS).filter(pulp_domain=hot_domain).exists() + assert Artifact.objects.using(SATELLITE_ALIAS).filter(pulp_domain=hot_domain).exists() + # Strategy A / Step 7: the stale copy is deliberately left on the original alias until + # 'cleanup-moved-domain' runs, so rollback is a one-line alias flip with no data loss. + assert FileRepository.objects.using("default").filter(pulp_domain=hot_domain).exists() + + move = DomainMove.objects.using("default").get(domain=hot_domain, status="completed") + assert move.from_alias == "default" + assert move.to_alias == SATELLITE_ALIAS + assert move.cutover_at is not None + assert move.monitoring_until is not None + + def test_move_domain_refuses_default_domain(self): + default_domain = Domain.objects.using("default").get(name="default") + with pytest.raises(Exception, match="default"): + call_command("move-domain", default_domain.name, "--to", SATELLITE_ALIAS, "--noinput") + + def test_move_domain_refuses_unconfigured_alias(self, hot_domain): + with pytest.raises(Exception, match="not a configured"): + call_command("move-domain", hot_domain.name, "--to", "not-a-real-alias", "--noinput") + + def test_move_domain_refuses_same_alias(self, hot_domain): + with pytest.raises(Exception, match="already on alias"): + call_command( + "move-domain", hot_domain.name, "--to", hot_domain.database_alias, "--noinput" + ) + + +class TestCleanupMovedDomain: + def test_cleanup_requires_force(self, hot_domain): + call_command("move-domain", hot_domain.name, "--to", SATELLITE_ALIAS, "--noinput") + with pytest.raises(Exception, match="--force"): + call_command("cleanup-moved-domain", hot_domain.name) + + def test_cleanup_deletes_stale_source_copy(self, hot_domain): + call_command("move-domain", hot_domain.name, "--to", SATELLITE_ALIAS, "--noinput") + call_command("cleanup-moved-domain", hot_domain.name, "--force") + + hot_domain.refresh_from_db() + assert not FileRepository.objects.using("default").filter(pulp_domain=hot_domain).exists() + assert not Artifact.objects.using("default").filter(pulp_domain=hot_domain).exists() + # The satellite copy (the domain's real, current data) must be untouched. + assert FileRepository.objects.using(SATELLITE_ALIAS).filter(pulp_domain=hot_domain).exists() + + move = DomainMove.objects.using("default").get(domain=hot_domain, status="cleaned_up") + assert move.cleaned_up_at is not None + + def test_cleanup_refuses_domain_still_on_default(self, hot_domain): + with pytest.raises(Exception, match="nothing to clean up"): + call_command("cleanup-moved-domain", hot_domain.name, "--force") + + +class TestDomainSize: + def test_domain_size_reports_row_counts(self, hot_domain, capsys): + call_command("domain-size", hot_domain.name) + out = capsys.readouterr().out + assert "file.FileRepository" in out + assert "core.Artifact" in out diff --git a/pulpcore/tests/unit/test_multi_database_routing.py b/pulpcore/tests/unit/test_multi_database_routing.py new file mode 100644 index 00000000000..64816bcb55c --- /dev/null +++ b/pulpcore/tests/unit/test_multi_database_routing.py @@ -0,0 +1,320 @@ +""" +Integration tests for domain-aware database routing (phase1-integration-tests). + +These tests exercise `PulpDomainRouter`, `migrate-all`, and graceful degradation +(phase1-graceful-degradation / KI-12) against **real** additional database aliases, per Django's +standard multi-database testing support (see "Testing multi-database applications" in the Django +testing docs): a second `settings.DATABASES` alias, `data_1`, and +`@pytest.mark.django_db(databases=[...])` to opt individual tests into it. + +Unlike every other alias in this codebase, which is populated dynamically from +`PULP_DATABASES____` environment variables, `data_1` cannot be conjured up inside a +test: pytest-django creates/migrates the test database for every alias present in +`settings.DATABASES` once, at session start (`django_db_setup`), long before any individual test +runs -- there is no supported way to add a brand new alias from inside a test and have +pytest-django create a test database for it on the fly. So, for this entire module to actually +run against a real second database (rather than a no-op skip), the test environment must define +a `data_1` alias before pytest starts, e.g.: + + export PULP_DATABASES__data_1__ENGINE=django.db.backends.postgresql + export PULP_DATABASES__data_1__NAME=pulp + export PULP_DATABASES__data_1__USER=pulp + export PULP_DATABASES__data_1__PASSWORD=pulp + export PULP_DATABASES__data_1__HOST= + export PULP_DATABASES__data_1__PORT= + +When `data_1` isn't configured (the default for a single-database checkout/CI run), every test +below is skipped with a clear reason rather than silently doing nothing useful -- this mirrors +`PulpDomainRouter` itself only activating when `len(settings.DATABASES) > 1`, so there is no +meaningful "multi-db routing" behavior to test at all without it. +""" + +from contextlib import contextmanager +from unittest import mock + +import pytest +from django.conf import settings +from django.core.management import call_command +from django.db.utils import OperationalError + +from pulpcore.app.contexts import with_domain +from pulpcore.app.models import ( + ContentArtifact, + Domain, + MigrationStatus, + Remote, + RemoteArtifact, + Repository, + Task, +) +from pulpcore.constants import TASK_STATES + +SATELLITE_ALIAS = "data_1" + +requires_multi_db = pytest.mark.skipif( + SATELLITE_ALIAS not in settings.DATABASES, + reason=( + f"Multi-database routing tests require a '{SATELLITE_ALIAS}' alias in settings.DATABASES " + f"(set PULP_DATABASES__{SATELLITE_ALIAS}__* env vars to a second real Postgres instance)." + ), +) + +pytestmark = [requires_multi_db, pytest.mark.django_db(databases=["default", SATELLITE_ALIAS])] + + +@contextmanager +def _satellite_domain(**extra_fields): + """Create+clean up a Domain whose data-plane objects are routed to `data_1`.""" + domain = Domain.objects.create( + name=f"test-satellite-domain-{extra_fields.get('_suffix', '')}".rstrip("-"), + storage_class="pulpcore.app.models.storage.FileSystem", + database_alias=SATELLITE_ALIAS, + **{k: v for k, v in extra_fields.items() if k != "_suffix"}, + ) + try: + yield domain + finally: + domain.delete() + + +class TestMigrateAll: + """`migrate-all` (phase1-migrate-all) against a real second alias.""" + + def test_migrate_all_migrates_every_alias(self): + # pytest-django's `django_db_setup` already created+migrated the test database for + # every configured alias (that's how `data_1` exists at all by the time this test runs), + # so this call is expected to be a no-op -- the point is that it *completes cleanly* and + # leaves a "complete" MigrationStatus row for every alias, exactly as it would against a + # freshly provisioned satellite. + call_command("migrate-all") + + statuses = {m.database_alias: m.status for m in MigrationStatus.objects.all()} + for alias in settings.DATABASES: + assert statuses.get(alias) == "complete", ( + f"Expected MigrationStatus for alias '{alias}' to be 'complete', got " + f"{statuses.get(alias)!r}" + ) + + def test_migrate_all_reconciles_domain_table_to_satellite(self): + call_command("migrate-all") + + default_domain = Domain.objects.using("default").get(name="default") + assert Domain.objects.using(SATELLITE_ALIAS).filter(pk=default_domain.pk).exists(), ( + "The 'default' Domain row should have been replicated onto the satellite alias by " + "migrate-all's Domain-sync step." + ) + + +class TestPulpDomainRouter: + """Data-plane routing by `Domain.database_alias` (phase1-router).""" + + def test_data_plane_object_routes_to_satellite_alias(self): + with _satellite_domain(_suffix="routing") as domain: + with with_domain(domain): + repo = Repository.objects.create(name=f"{domain.name}-repo", pulp_domain=domain) + try: + assert Repository.objects.using(SATELLITE_ALIAS).filter(pk=repo.pk).exists(), ( + "Repository created under a satellite-domain context should exist on the " + "satellite alias." + ) + assert not Repository.objects.using("default").filter(pk=repo.pk).exists(), ( + "Repository created under a satellite-domain context must NOT exist on " + "'default' -- routing to the wrong alias would silently duplicate/leak data." + ) + finally: + Repository.objects.using(SATELLITE_ALIAS).filter(pk=repo.pk).delete() + + def test_instance_hint_routes_without_contextvar(self): + """Saving an already-loaded satellite-domain instance routes correctly with no + ContextVar set at all -- purely from `instance.pulp_domain` (the router's instance-hint + path, `PulpDomainRouter._resolve_db`). + + Requires `select_related("pulp_domain")` on the fetch: post-KI-27, the router's + instance-hint path only ever consults an *already-cached* `pulp_domain` relation object + (never issues its own query for one -- that was the KI-27 N+1 bug), so a caller that + wants correct instance-hint routing without a ContextVar must make sure the `Domain` is + actually loaded, the same way `test_base.py::test_cast` does for `remote`. + """ + with _satellite_domain(_suffix="instancehint") as domain: + with with_domain(domain): + repo = Repository.objects.create(name=f"{domain.name}-repo", pulp_domain=domain) + try: + # No `with_domain()`/ContextVar here: the previous `with` block has already + # exited, so only the loaded instance's own `pulp_domain` FK can inform routing. + repo_fresh = ( + Repository.objects.using(SATELLITE_ALIAS) + .select_related("pulp_domain") + .get(pk=repo.pk) + ) + repo_fresh.description = "updated via instance hint, no ContextVar" + repo_fresh.save() + assert ( + Repository.objects.using(SATELLITE_ALIAS).get(pk=repo.pk).description + == "updated via instance hint, no ContextVar" + ) + finally: + Repository.objects.using(SATELLITE_ALIAS).filter(pk=repo.pk).delete() + + def test_control_plane_model_always_routes_to_default(self): + """Task (control-plane) must stay on `default` even while a satellite domain is the + active ContextVar -- KI-11/Locked Decisions: task coordination never moves off the + control DB, regardless of which alias the *data* it operates on lives on.""" + with _satellite_domain(_suffix="controlplane") as domain: + with with_domain(domain): + task = Task.objects.create(name="test-task", state=TASK_STATES.WAITING) + try: + assert Task.objects.using("default").filter(pk=task.pk).exists() + assert not Task.objects.using(SATELLITE_ALIAS).filter(pk=task.pk).exists() + finally: + Task.objects.using("default").filter(pk=task.pk).delete() + + +class TestRouterInstanceHintSafety: + """KI-27 regressions against `PulpDomainRouter._resolve_db`'s instance-hint path. + + Both bugs share a root cause: the old code used `hasattr()`/`getattr()` to read + `pulp_domain_id`/`pulp_domain` off the hinted instance, which can trigger Django's + `DeferredAttribute.__get__` fallback (a fresh query, or -- worse -- a `refresh_from_db()` + call that re-enters the router with the very same not-yet-constructed instance and recurses + until `RecursionError`). The fix inspects `instance.__dict__` / + `instance._state.fields_cache` directly instead, which can never trigger either fallback. + """ + + def test_remote_artifact_construction_does_not_recurse(self): + """Reproduces the crash directly: `RemoteArtifact` declares `content_artifact` and + `remote` *before* `pulp_domain` in field order (see `models/content.py`), so + constructing one with an unsaved `ContentArtifact` -- exactly how + `content/handler.py`'s pull-through path builds it, e.g. around line 909: + `RemoteArtifact(remote=remote, url=url, content_artifact=ca)` -- used to recurse + infinitely the moment `len(settings.DATABASES) > 1` activated the router: assigning + `content_artifact` makes Django's `ForwardManyToOneDescriptor.__set__` ask the router to + resolve a DB for the (unsaved) `ContentArtifact` value, hinting the half-built + `RemoteArtifact` itself back into `_resolve_db`, which then tried to read + `pulp_domain_id` off of it before that field had been assigned at all. + """ + with _satellite_domain(_suffix="norecursion") as domain: + with with_domain(domain): + remote = Remote.objects.create(name="ki27-remote", url="https://example.com") + # Unsaved, with no `pulp_domain` set yet -- matches how handler.py builds it. + ca = ContentArtifact(relative_path="ki27/path") + try: + ra = RemoteArtifact(remote=remote, url=f"{remote.url}/x", content_artifact=ca) + except RecursionError: + pytest.fail( + "PulpDomainRouter._resolve_db recursed while constructing a " + "RemoteArtifact with a preceding unsaved FK -- KI-27 regression" + ) + try: + # Not just "didn't crash" -- resolved the *correct* domain via the ContextVar + # fallback once the instance-hint path safely declined to use a not-yet-set + # `pulp_domain_id`. + assert ra.pulp_domain_id == domain.pk + finally: + Remote.objects.using(SATELLITE_ALIAS).filter(pk=remote.pk).delete() + + def test_relation_access_does_not_issue_extra_domain_query(self, django_assert_num_queries): + """KI-27's N+1 half: reading a data-plane relation (`repository.remote`) off an + already-loaded instance that has `pulp_domain_id` set but no cached `pulp_domain` + relation object must resolve through the router in exactly the number of queries the + relation access itself needs -- zero extra queries to fetch `Domain` just to ask the + router which alias to use. Mirrors `test_base.py::test_cast`, scoped explicitly to a + real multi-DB configuration (`len(settings.DATABASES) > 1`, the condition that actually + activates `PulpDomainRouter` -- see the module docstring in `db_router.py`). + """ + from pulp_file.app.models import FileRemote, FileRepository + + remote = FileRemote.objects.create(name="ki27-cast-remote") + repository = FileRepository.objects.create(name="ki27-cast-repo", remote=remote) + try: + with django_assert_num_queries(1): + fetched = Repository.objects.get(pk=repository.pk) + with django_assert_num_queries(1): + fetched = fetched.cast() + with django_assert_num_queries(1): + assert fetched.remote.pk == remote.pk + finally: + repository.delete() + remote.delete() + + +class TestGracefulDegradation: + """503 on unreachable/moving satellite (phase1-graceful-degradation / KI-12).""" + + def test_503_when_satellite_unreachable(self): + from pulpcore.middleware import DomainMiddleware + + with _satellite_domain(_suffix="unreachable") as domain: + request = mock.Mock(method="GET") + with mock.patch("pulpcore.middleware.connections") as mock_connections: + mock_connections.__getitem__.return_value.ensure_connection.side_effect = ( + OperationalError("could not connect") + ) + response = DomainMiddleware._degraded_response(request, domain) + + assert response is not None + assert response.status_code == 503 + assert domain.name in response.content.decode() + + def test_no_503_when_satellite_reachable(self): + with _satellite_domain(_suffix="reachable") as domain: + from pulpcore.middleware import DomainMiddleware + + request = mock.Mock(method="GET") + # Real connection check against the actual (reachable, in this test run) satellite -- + # no mocking -- to prove the happy path doesn't false-positive. + response = DomainMiddleware._degraded_response(request, domain) + assert response is None + + def test_503_rejects_writes_to_moving_domain(self): + with _satellite_domain(_suffix="moving", moving=True) as domain: + from pulpcore.middleware import DomainMiddleware + + write_request = mock.Mock(method="POST") + response = DomainMiddleware._degraded_response(write_request, domain) + assert response is not None + assert response.status_code == 503 + + read_request = mock.Mock(method="GET") + assert DomainMiddleware._degraded_response(read_request, domain) is None + + def test_task_dispatch_skips_moving_domain(self): + """A worker must not attempt to run a task whose domain is mid-move -- it should stay + `waiting`, not be picked up (and definitely not fail).""" + from pulpcore.tasking.worker import PulpcoreWorker + + with _satellite_domain(_suffix="taskmoving", moving=True) as domain: + with with_domain(domain): + task = Task.objects.create(name="test-task", state=TASK_STATES.WAITING) + try: + worker = mock.Mock(spec=PulpcoreWorker) + assert PulpcoreWorker.is_domain_available(worker, task) is False + finally: + Task.objects.using("default").filter(pk=task.pk).delete() + + def test_task_dispatch_skips_unreachable_satellite(self): + from pulpcore.tasking.worker import PulpcoreWorker + + with _satellite_domain(_suffix="taskunreachable") as domain: + with with_domain(domain): + task = Task.objects.create(name="test-task", state=TASK_STATES.WAITING) + try: + worker = mock.Mock(spec=PulpcoreWorker) + with mock.patch("pulpcore.tasking.worker.connections") as mock_connections: + mock_connections.__getitem__.return_value.ensure_connection.side_effect = ( + OperationalError("could not connect") + ) + assert PulpcoreWorker.is_domain_available(worker, task) is False + finally: + Task.objects.using("default").filter(pk=task.pk).delete() + + def test_task_dispatch_allows_healthy_domain(self): + from pulpcore.tasking.worker import PulpcoreWorker + + with _satellite_domain(_suffix="taskhealthy") as domain: + with with_domain(domain): + task = Task.objects.create(name="test-task", state=TASK_STATES.WAITING) + try: + worker = mock.Mock(spec=PulpcoreWorker) + assert PulpcoreWorker.is_domain_available(worker, task) is True + finally: + Task.objects.using("default").filter(pk=task.pk).delete() diff --git a/pulpcore/tests/unit/test_reconciliation.py b/pulpcore/tests/unit/test_reconciliation.py new file mode 100644 index 00000000000..1ed187cb7a4 --- /dev/null +++ b/pulpcore/tests/unit/test_reconciliation.py @@ -0,0 +1,142 @@ +""" +Integration tests for the KI-11 cross-plane reconciliation sweep (phase3-monitoring). + +See `test_multi_database_routing.py`'s module docstring for why these require a real `data_1` +alias (set via `PULP_DATABASES__data_1__*` env vars) to run at all, rather than skipping. +""" + +from datetime import timedelta + +import pytest +from django.core.management import call_command +from django.utils import timezone + +from pulpcore.app.contexts import with_domain, with_task_context +from pulpcore.app.models import CreatedResource, Domain, Task +from pulpcore.app.tasks.reconciliation import reconcile_cross_plane_references +from pulp_file.app.models import FileRepository + +from .test_multi_database_routing import SATELLITE_ALIAS, requires_multi_db + +pytestmark = [requires_multi_db, pytest.mark.django_db(databases=["default", SATELLITE_ALIAS])] + + +@pytest.fixture +def satellite_domain(): + domain = Domain.objects.create( + name="reconcile-test-domain", + storage_class="pulpcore.app.models.storage.FileSystem", + storage_settings={"location": "/tmp/reconcile-test-domain"}, + database_alias=SATELLITE_ALIAS, + ) + yield domain + domain.delete() + + +@pytest.fixture +def task(): + t = Task.objects.create(name="reconcile-test-task") + yield t + t.delete() + + +def _backdate(created_resource, minutes): + CreatedResource.objects.using("default").filter(pk=created_resource.pk).update( + pulp_last_updated=timezone.now() - timedelta(minutes=minutes) + ) + created_resource.refresh_from_db() + + +class TestReconcileCrossPlaneReferences: + def test_healthy_cross_plane_reference_is_not_flagged(self, satellite_domain, task): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-healthy-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + _backdate(cr, minutes=120) + + report = reconcile_cross_plane_references(grace_period_minutes=60) + + assert report["checked"] >= 1 + assert report["orphaned"] == 0 + assert str(cr.pk) not in {o["pk"] for o in report["orphans"]} + + def test_orphaned_reference_is_detected_but_not_purged_by_default(self, satellite_domain, task): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-orphan-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + repo.delete(using=SATELLITE_ALIAS) + _backdate(cr, minutes=120) + + report = reconcile_cross_plane_references(grace_period_minutes=60, purge_after_days=0) + + assert report["orphaned"] == 1 + assert report["purged"] == 0 + assert CreatedResource.objects.using("default").filter(pk=cr.pk).exists() + orphan = report["orphans"][0] + assert orphan["pk"] == str(cr.pk) + assert orphan["alias"] == SATELLITE_ALIAS + + def test_orphaned_reference_within_grace_period_is_skipped(self, satellite_domain, task): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-fresh-orphan-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + repo.delete(using=SATELLITE_ALIAS) + # No backdating: this row is "fresh" and should be skipped regardless of the grace period. + + report = reconcile_cross_plane_references(grace_period_minutes=60) + + assert str(cr.pk) not in {o["pk"] for o in report["orphans"]} + + def test_purge_after_days_deletes_old_confirmed_orphans(self, satellite_domain, task): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-purge-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + repo.delete(using=SATELLITE_ALIAS) + _backdate(cr, minutes=60 * 24 * 10) + + report = reconcile_cross_plane_references( + grace_period_minutes=60, purge_after_days=7, dry_run=False + ) + + assert report["orphaned"] == 1 + assert report["purged"] == 1 + assert not CreatedResource.objects.using("default").filter(pk=cr.pk).exists() + + def test_dry_run_never_purges(self, satellite_domain, task): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-dry-run-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + repo.delete(using=SATELLITE_ALIAS) + _backdate(cr, minutes=60 * 24 * 10) + + report = reconcile_cross_plane_references( + grace_period_minutes=60, purge_after_days=7, dry_run=True + ) + + assert report["orphaned"] == 1 + assert report["purged"] == 0 + assert CreatedResource.objects.using("default").filter(pk=cr.pk).exists() + + def test_management_command_reports_orphans(self, satellite_domain, task, capsys): + with with_task_context(task), with_domain(satellite_domain): + repo = FileRepository.objects.create( + name="reconcile-cmd-repo", pulp_domain=satellite_domain + ) + cr = CreatedResource.objects.create(content_object=repo) + repo.delete(using=SATELLITE_ALIAS) + _backdate(cr, minutes=120) + + call_command("reconcile-cross-plane-references", "--grace-period-minutes", "60") + + out = capsys.readouterr().out + assert "1 orphan(s)" in out or "found 1 orphan" in out.lower() From 6f02afe691ad172b9a3ba6826f208f53e631881a Mon Sep 17 00:00:00 2001 From: Yasen Date: Wed, 15 Jul 2026 11:47:37 +0200 Subject: [PATCH 2/7] fix lints --- pulpcore/tests/unit/test_reconciliation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pulpcore/tests/unit/test_reconciliation.py b/pulpcore/tests/unit/test_reconciliation.py index 1ed187cb7a4..2ba40f6788b 100644 --- a/pulpcore/tests/unit/test_reconciliation.py +++ b/pulpcore/tests/unit/test_reconciliation.py @@ -14,6 +14,7 @@ from pulpcore.app.contexts import with_domain, with_task_context from pulpcore.app.models import CreatedResource, Domain, Task from pulpcore.app.tasks.reconciliation import reconcile_cross_plane_references + from pulp_file.app.models import FileRepository from .test_multi_database_routing import SATELLITE_ALIAS, requires_multi_db From 9907cd5e9508ed5e8e24ac210d02bc9812f46bd9 Mon Sep 17 00:00:00 2001 From: Yasen Date: Wed, 15 Jul 2026 12:33:31 +0200 Subject: [PATCH 3/7] fix lints --- pulpcore/app/views/status.py | 3 ++- pulpcore/tests/unit/test_domain_move.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pulpcore/app/views/status.py b/pulpcore/app/views/status.py index c5c5aaa3df2..b238d4f54fd 100644 --- a/pulpcore/app/views/status.py +++ b/pulpcore/app/views/status.py @@ -4,7 +4,8 @@ from gettext import gettext as _ from django.conf import settings -from django.db import connections, utils as db_utils +from django.db import connections +from django.db import utils as db_utils from django.db.migrations.executor import MigrationExecutor from django.db.models import Sum from drf_spectacular.utils import extend_schema diff --git a/pulpcore/tests/unit/test_domain_move.py b/pulpcore/tests/unit/test_domain_move.py index 18636d96348..788002a9ee5 100644 --- a/pulpcore/tests/unit/test_domain_move.py +++ b/pulpcore/tests/unit/test_domain_move.py @@ -20,6 +20,7 @@ Domain, DomainMove, ) + from pulp_file.app.models import FileContent, FileRepository from .test_multi_database_routing import SATELLITE_ALIAS, requires_multi_db From 80f7b6fb4af38ed17fce30de1036bcff88764410 Mon Sep 17 00:00:00 2001 From: Yasen Date: Wed, 15 Jul 2026 12:56:31 +0200 Subject: [PATCH 4/7] fix tests --- pulpcore/tests/unit/test_middleware.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pulpcore/tests/unit/test_middleware.py b/pulpcore/tests/unit/test_middleware.py index dbb2649eedd..c603bcebb7a 100644 --- a/pulpcore/tests/unit/test_middleware.py +++ b/pulpcore/tests/unit/test_middleware.py @@ -29,6 +29,11 @@ def test_does_db_lookup_when_flag_not_set(self, mock_set_domain, mock_domain_obj view_class = type("NormalView", (), {}) view_func = MagicMock(view_class=view_class) view_kwargs = {"pulp_domain": "default"} + # database_alias/moving must be set explicitly: an unconfigured MagicMock() would make + # `_degraded_response`'s `alias != "default"` check true, sending it into a real + # `connections[alias]` lookup with a MagicMock alias, which isn't what this test is + # about (it only cares whether the DB lookup/skip-flag behavior is exercised). + mock_domain_objects.get.return_value = MagicMock(database_alias="default", moving=False) self.middleware.process_view(self.request, view_func, [], view_kwargs) From 0445cdb8eaae1470faeb7610275f05db8d235ea6 Mon Sep 17 00:00:00 2001 From: Yasen Date: Wed, 15 Jul 2026 15:13:01 +0200 Subject: [PATCH 5/7] fix early cancel --- pulpcore/tests/functional/api/test_tasking.py | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/pulpcore/tests/functional/api/test_tasking.py b/pulpcore/tests/functional/api/test_tasking.py index 92b9afe7381..476147810dc 100644 --- a/pulpcore/tests/functional/api/test_tasking.py +++ b/pulpcore/tests/functional/api/test_tasking.py @@ -13,7 +13,7 @@ from pulpcore.app import settings from pulpcore.client.pulpcore import ApiException from pulpcore.constants import IMMEDIATE_TIMEOUT -from pulpcore.tests.functional.utils import PulpTaskError, download_file +from pulpcore.tests.functional.utils import SLEEP_TIME, PulpTaskError, download_file @pytest.fixture(scope="module") @@ -476,10 +476,34 @@ def test_finalizer_task_runs_after_all_siblings(dispatch_task_group, monitor_tas @pytest.mark.parallel def test_cancel_task_group(pulpcore_bindings, dispatch_task_group, gen_user): """Test that task groups can be canceled.""" + # `dummy_group_task` keeps dispatching new child tasks for several seconds after the group + # is created, and a `running` child (blocked in a plain, uninterruptible `time.sleep()`) can + # take a bit to actually finish transitioning once canceled. `cancel_task_group` only cancels + # the tasks that exist *at the moment it runs* and reports 409 if, right after that sweep, + # any task is still `running`/`waiting` (see `TaskGroupViewSet.partial_update` and + # `pulpcore.tasking.tasks.cancel_task_group`) -- so a task dispatched (or still running) in + # that narrow window can make an otherwise-successful cancel call race and 409. Retrying is + # safe (canceling an already-canceling/canceled task is a no-op) and lets a legitimately + # permitted cancel eventually succeed without weakening what this test actually verifies + # (that task groups -- and their RBAC-gated cancellation -- work). + cancel_retry_timeout = 60 + + def _cancel_task_group_retrying(): + deadline = time.monotonic() + cancel_retry_timeout + while True: + try: + return pulpcore_bindings.TaskGroupsApi.task_groups_cancel( + tgroup_href, {"state": "canceled"} + ) + except ApiException as e: + if e.status != 409 or time.monotonic() >= deadline: + raise + time.sleep(SLEEP_TIME) + kwargs = {"inbetween": 1, "intervals": [10, 10, 10, 10, 10]} tgroup_href = dispatch_task_group("pulpcore.app.tasks.test.dummy_group_task", kwargs=kwargs) - tgroup = pulpcore_bindings.TaskGroupsApi.task_groups_cancel(tgroup_href, {"state": "canceled"}) + tgroup = _cancel_task_group_retrying() for task in tgroup.tasks: assert task.state in ["canceled", "canceling"] @@ -497,7 +521,7 @@ def test_cancel_task_group(pulpcore_bindings, dispatch_task_group, gen_user): assert "You do not have permission" in e.value.message with gen_user(model_roles=["core.task_owner"]): - pulpcore_bindings.TaskGroupsApi.task_groups_cancel(tgroup_href, {"state": "canceled"}) + _cancel_task_group_retrying() LT_TIMEOUT = IMMEDIATE_TIMEOUT / 2 From eea6a47e7ec2534dbad461414114f2826e888abd Mon Sep 17 00:00:00 2001 From: Yasen Date: Wed, 15 Jul 2026 16:52:59 +0200 Subject: [PATCH 6/7] take care of task test --- pulpcore/app/models/generic.py | 27 ++++++++--- pulpcore/app/tasks/reconciliation.py | 54 +++++++++++++++------- pulpcore/tests/unit/models/test_generic.py | 51 ++++++++++++++++++++ 3 files changed, 108 insertions(+), 24 deletions(-) create mode 100644 pulpcore/tests/unit/models/test_generic.py diff --git a/pulpcore/app/models/generic.py b/pulpcore/app/models/generic.py index af372b6dddd..a227a524af3 100644 --- a/pulpcore/app/models/generic.py +++ b/pulpcore/app/models/generic.py @@ -61,11 +61,13 @@ class DomainResolvedGenericRelation: that `bulk_create()` -- which never calls `save()` -- still populates it correctly; see `pulpcore.app.viewsets.base.NamedModelViewSet.add_role` for a real `bulk_create()` call site. The `content_object` property below then resolves the target via - `.using(content_object_domain.database_alias)` when that field is set, raising loudly on - failure instead of silently swallowing it, and falls back to resolving on this row's own - alias whenever there's no recorded cross-plane domain (no target at all -- e.g. a - model-level `UserRole`/`GroupRole` grant -- or a control-plane target, which lives on the - same alias as this row anyway). + `.using(content_object_domain.database_alias)` when that field is set, and falls back to + resolving on this row's own alias whenever there's no recorded cross-plane domain (no + target at all -- e.g. a model-level `UserRole`/`GroupRole` grant -- or a control-plane + target, which lives on the same alias as this row anyway). Either way, a target that can't + be resolved (deleted, or -- for the cross-plane case -- possibly stale Domain replication) + logs and returns `None`, exactly like Django's own `GenericForeignKey.__get__` and like + every existing call site already expects; it does not raise. Fix for #2: the setter below never delegates to Django's `GenericForeignKey.__set__` (would reintroduce the bug) -- it sets `content_type`/`object_id` directly, always resolving @@ -111,7 +113,18 @@ def content_object(self): try: resolved = model_class.objects.using(alias).get(pk=self.object_id) except model_class.DoesNotExist: - _logger.error( + # Mirror Django's own GenericForeignKey.__get__ semantics (and the "no + # cross-plane domain recorded" branch below): a missing target is far more + # commonly a legitimately deleted object (e.g. a Task's created_resources + # pointing at a since-destroyed Repository/Export) than stale Domain + # replication, and every existing call site (RelatedResourceField, + # CreatedResourcePrnField, delete_incomplete_resources, etc.) already treats + # `content_object is None` as "gone, render/skip gracefully". Raising here + # instead turned that everyday case into an unhandled 500 + # (see pulp task list rendering a Task whose created Export was deleted). + # Still log so a genuinely-stale satellite is discoverable via + # 'pulpcore-manager sync-domains', just without crashing the caller. + _logger.warning( "content_object for %s (pk=%s) not found on alias '%s' " "(content_type_id=%s, object_id=%s). The referenced object may have been " "deleted, or Domain replication for this row's domain may be stale -- run " @@ -122,7 +135,7 @@ def content_object(self): self.content_type_id, self.object_id, ) - raise + resolved = None else: # No cross-plane domain recorded: the target is itself control-plane, so it lives on # this (control-plane) row's own alias -- resolve there, mirroring normal Django GFK diff --git a/pulpcore/app/tasks/reconciliation.py b/pulpcore/app/tasks/reconciliation.py index eb734dcb366..c27042fdd14 100644 --- a/pulpcore/app/tasks/reconciliation.py +++ b/pulpcore/app/tasks/reconciliation.py @@ -10,18 +10,21 @@ and reporting (optionally purging) the resulting orphans here rather than by trying to eliminate them. -This reuses the KI-18 `content_object` resolver (`DomainResolvedGenericRelation`, see -`pulpcore.app.models.generic`) as the *only* detection mechanism: every model in scope already -raises loudly (logs + re-raises `DoesNotExist`) when a cross-plane `content_object` can't be -resolved on its recorded alias, so this sweep doesn't need any separate orphan-detection logic -- -it just needs to iterate the candidate rows and call `.content_object` on each. +This does its own existence check per candidate row (`_target_exists()` below) rather than +resolving through `DomainResolvedGenericRelation.content_object` (`pulpcore.app.models.generic`): +that property must return `None` -- not raise -- for an unresolvable target, since every ordinary +caller (`RelatedResourceField`, `CreatedResourcePrnField`, etc.) treats "target is gone" as a +routine, gracefully-rendered case (e.g. a `Task.created_resources` entry for a since-deleted +`Repository`), not a systemic failure worth surfacing as a 500. This sweep's job is exactly the +opposite -- it needs to *notice* rows whose recorded cross-plane target can't be resolved on the +alias it should live on -- so it queries for existence directly instead of piggy-backing on that +lenient, widely-depended-on property. """ from datetime import timedelta from logging import getLogger from django.conf import settings -from django.core.exceptions import ObjectDoesNotExist from django.utils import timezone from pulpcore.app.models import CreatedResource, ExportedResource @@ -48,6 +51,20 @@ def _candidate_rows(model, cutoff): ) +def _target_exists(row): + """ + Resolve whether `row`'s recorded cross-plane target actually exists on its recorded alias. + + Mirrors `DomainResolvedGenericRelation.content_object`'s own resolution (content_type's + model class, `content_object_domain`'s alias, `object_id`) but as a direct `.exists()` + check rather than a `.get()` -- this sweep needs a boolean, not the object itself, and must + not depend on that property's (deliberately lenient) exception behavior. + """ + model_class = row.content_type.model_class() + alias = row.content_object_domain.database_alias + return model_class.objects.using(alias).filter(pk=row.object_id).exists() + + def reconcile_cross_plane_references( grace_period_minutes=None, purge_after_days=None, @@ -90,18 +107,21 @@ def reconcile_cross_plane_references( qs = _candidate_rows(model, cutoff) for row in qs.iterator(): report["checked"] += 1 - try: - row.content_object - except ObjectDoesNotExist: - # The resolver (DomainResolvedGenericRelation.content_object) already logged the - # specifics (model/pk/alias/content_type); we just tally here. - report["orphaned"] += 1 - alias = ( - row.content_object_domain.database_alias - if row.content_object_domain_id - else None - ) + if not _target_exists(row): + alias = row.content_object_domain.database_alias age = timezone.now() - row.pulp_last_updated + log.error( + "content_object for %s (pk=%s) not found on alias '%s' " + "(content_type_id=%s, object_id=%s). The referenced object may have been " + "deleted, or Domain replication for this row's domain may be stale -- run " + "'pulpcore-manager sync-domains' to check.", + model._meta.label, + row.pk, + alias, + row.content_type_id, + row.object_id, + ) + report["orphaned"] += 1 report["orphans"].append( { "model": model._meta.label, diff --git a/pulpcore/tests/unit/models/test_generic.py b/pulpcore/tests/unit/models/test_generic.py new file mode 100644 index 00000000000..ec0f29653f2 --- /dev/null +++ b/pulpcore/tests/unit/models/test_generic.py @@ -0,0 +1,51 @@ +from uuid import uuid4 + +import pytest + +from pulpcore.app.contexts import with_task_context +from pulpcore.app.models import CreatedResource, Task + +from pulp_file.app.models import FileRepository + + +@pytest.fixture +def task(): + t = Task.objects.create(name="test-generic-relation-task") + yield t + t.delete() + + +@pytest.mark.django_db +def test_content_object_returns_none_for_deleted_domain_scoped_target(task): + """A CreatedResource whose target was deleted must resolve to None, not raise. + + Every data-plane model has ``pulp_domain_id`` set, so ``content_object_domain`` is always + populated for a resource like ``FileRepository`` -- even in a single-database deployment + with no satellites configured. Regression test for the KI-18 cross-plane ``content_object`` + resolution (`pulpcore.app.models.generic.DomainResolvedGenericRelation`): deleting the + target used to raise ``DoesNotExist`` out of the property instead of returning ``None`` + like Django's own ``GenericForeignKey`` (and every caller, e.g. ``RelatedResourceField``) + expects. + """ + with with_task_context(task): + repository = FileRepository.objects.create(name=str(uuid4())) + created_resource = CreatedResource.objects.create(content_object=repository) + assert created_resource.content_object_domain_id is not None + + repository.delete() + + # Force a fresh lookup instead of the in-memory cache populated by the setter above. + created_resource = CreatedResource.objects.get(pk=created_resource.pk) + assert created_resource.content_object is None + + +@pytest.mark.django_db +def test_content_object_resolves_existing_domain_scoped_target(task): + with with_task_context(task): + repository = FileRepository.objects.create(name=str(uuid4())) + created_resource = CreatedResource.objects.create(content_object=repository) + + created_resource = CreatedResource.objects.get(pk=created_resource.pk) + resolved = created_resource.content_object + assert resolved is not None + assert resolved.pk == repository.pk From 20a8b980052d44b8cf0dfa0a3780c6524f585ddc Mon Sep 17 00:00:00 2001 From: Yasen Date: Fri, 17 Jul 2026 13:27:31 +0200 Subject: [PATCH 7/7] fix(KI-18): resolve content_object_domain_id via transitive FK walk getattr(value, 'pulp_domain_id', None) silently returns None for models that reach their domain transitively rather than through their own field (RepositoryVersion.repository, RepositoryContent.repository, ContentArtifact.content, PublishedArtifact.publication). RepositoryVersion is the single most common CreatedResource target in pulpcore (every sync/publish creates one), so this meant most real-world post-move CreatedResource rows for satellite-hosted domains silently resolved content_object on the wrong (control-plane) alias instead of raising or logging -- the exact KI-18 landmine, reached via an uncovered path. Found via live validation against a real two-Postgres environment: a genuine post-move CreatedResource pointing at a satellite RepositoryVersion resolved to a stale pre-cleanup copy on 'default' instead of the live row on the satellite alias. Fixes by walking the target's concrete FK/O2O fields (bounded depth, cycle-guarded) to find any related object exposing pulp_domain_id, rather than a hardcoded model list -- covers all four known cases generically and any future/plugin model with the same shape. Co-authored-by: Cursor --- pulpcore/app/models/generic.py | 62 ++++++++++++++++++++-- pulpcore/tests/unit/models/test_generic.py | 37 ++++++++++++- 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/pulpcore/app/models/generic.py b/pulpcore/app/models/generic.py index a227a524af3..87ce6c24455 100644 --- a/pulpcore/app/models/generic.py +++ b/pulpcore/app/models/generic.py @@ -9,6 +9,7 @@ from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ObjectDoesNotExist from django.db import models from pulpcore.app.models.base import BaseModel @@ -20,6 +21,61 @@ #: (a model-level `UserRole`/`GroupRole` grant has no target at all). _UNSET = object() +#: Max hops when walking FKs to find a `pulp_domain` (see `_resolve_domain_id`). Two hops covers +#: every known transitive case (`RepositoryVersion`/`RepositoryContent` -> `repository`, +#: `ContentArtifact` -> `content`/`artifact`, `PublishedArtifact` -> `publication`) with room to +#: spare, while still bounding the walk for models this can't resolve. +_DOMAIN_WALK_MAX_DEPTH = 2 + + +def _resolve_domain_id(value, _depth=0, _seen=None): + """ + Best-effort resolution of the `Domain` pk a `content_object` target belongs to. + + Most data-plane models (`Repository`, `Content`, `Remote`, `Publication`, `Distribution`, + ...) carry their own `pulp_domain` FK and `getattr(value, "pulp_domain_id", None)` alone + would suffice. But several models that are common `content_object` targets -- notably + `RepositoryVersion`, `RepositoryContent`, `ContentArtifact`, `PublishedArtifact` -- have no + such field of their own; they only reach a domain transitively through a parent FK + (`.repository`, `.content`, `.publication`). Found empirically: a `CreatedResource` for a + satellite-hosted `RepositoryVersion` (the single most common created-resource shape -- + every sync/publish creates one) silently got `content_object_domain_id=None`, which made + `content_object`'s getter fall back to resolving on *this row's own* (control-plane, + `default`) alias instead of the target's real satellite alias -- i.e. exactly the KI-18 + landmine the denormalized field exists to prevent, just reached via a different route than + the one KI-18 originally documented. + + Walks concrete forward FK/O2O fields (bounded depth, cycle-guarded) looking for any related + object that itself resolves to a domain id. Not on any hot/read path -- only called from the + `content_object` setter, which already isn't -- so the extra FK traversal (each hop is at + most one query) is an acceptable one-time cost at write time. + """ + domain_id = getattr(value, "pulp_domain_id", None) + if domain_id is not None: + return domain_id + if _depth >= _DOMAIN_WALK_MAX_DEPTH: + return None + if _seen is None: + _seen = set() + if value.pk is not None: + key = (type(value), value.pk) + if key in _seen: + return None + _seen.add(key) + for field in value._meta.get_fields(): + if not (field.many_to_one or field.one_to_one) or not getattr(field, "concrete", False): + continue + try: + related = getattr(value, field.name) + except ObjectDoesNotExist: + continue + if related is None or not hasattr(related, "_meta"): + continue + resolved = _resolve_domain_id(related, _depth + 1, _seen) + if resolved is not None: + return resolved + return None + class DomainResolvedGenericRelation: """ @@ -56,8 +112,8 @@ class DomainResolvedGenericRelation: Locked fix for #1 (see `architecture/domain-db-offloading-design.md`, KI-18): denormalize a nullable `content_object_domain` FK onto the model, auto-populated by the `content_object` - setter below from `content_object.pulp_domain_id` whenever a target is set -- no call site - needs to set it explicitly, and it's set eagerly (not deferred to `save()`) specifically so + setter below via `_resolve_domain_id()` whenever a target is set -- no call site needs to + set it explicitly, and it's set eagerly (not deferred to `save()`) specifically so that `bulk_create()` -- which never calls `save()` -- still populates it correctly; see `pulpcore.app.viewsets.base.NamedModelViewSet.add_role` for a real `bulk_create()` call site. The `content_object` property below then resolves the target via @@ -166,7 +222,7 @@ def content_object(self, value): value, for_concrete_model=gfk.for_concrete_model ) self.object_id = value.pk - self.content_object_domain_id = getattr(value, "pulp_domain_id", None) + self.content_object_domain_id = _resolve_domain_id(value) class GenericRelationModel(DomainResolvedGenericRelation, BaseModel): diff --git a/pulpcore/tests/unit/models/test_generic.py b/pulpcore/tests/unit/models/test_generic.py index ec0f29653f2..bcde7af2bba 100644 --- a/pulpcore/tests/unit/models/test_generic.py +++ b/pulpcore/tests/unit/models/test_generic.py @@ -3,7 +3,8 @@ import pytest from pulpcore.app.contexts import with_task_context -from pulpcore.app.models import CreatedResource, Task +from pulpcore.app.models import CreatedResource, RepositoryVersion, Task +from pulpcore.app.models.generic import _resolve_domain_id from pulp_file.app.models import FileRepository @@ -49,3 +50,37 @@ def test_content_object_resolves_existing_domain_scoped_target(task): resolved = created_resource.content_object assert resolved is not None assert resolved.pk == repository.pk + + +@pytest.mark.django_db +def test_resolve_domain_id_walks_transitive_fk(task): + """Regression test for the 2026-07-17 KI-18 correction. + + ``RepositoryVersion`` has no ``pulp_domain`` field of its own -- only its parent + ``Repository`` does -- so a bare ``getattr(value, "pulp_domain_id", None)`` always returns + ``None`` for it. ``_resolve_domain_id()`` must walk the ``.repository`` FK to find it. + """ + with with_task_context(task): + repository = FileRepository.objects.create(name=str(uuid4())) + version = RepositoryVersion.objects.create(repository=repository, number=1) + + assert getattr(version, "pulp_domain_id", None) is None + assert _resolve_domain_id(version) == repository.pulp_domain_id + + +@pytest.mark.django_db +def test_content_object_domain_id_set_for_repository_version(task): + """``RepositoryVersion`` is the single most common ``CreatedResource`` target in pulpcore + (every sync/publish creates one) -- this must not silently regress to ``domain_id=None``. + """ + with with_task_context(task): + repository = FileRepository.objects.create(name=str(uuid4())) + version = RepositoryVersion.objects.create(repository=repository, number=1) + + created_resource = CreatedResource.objects.create(content_object=version) + assert created_resource.content_object_domain_id == repository.pulp_domain_id + + created_resource = CreatedResource.objects.get(pk=created_resource.pk) + resolved = created_resource.content_object + assert resolved is not None + assert resolved.pk == version.pk