initial commit for multidb by YasenT · Pull Request #7862 · pulp/pulpcore

YasenT · 2026-07-15T09:43:40Z

Initial PR for the multidb implementation. To test github actions. etc

YasenT · 2026-07-15T12:54:03Z

/retest

YasenT · 2026-07-21T07:35:06Z

@gerrod3 What do you think? Can I get a review?

gerrod3

Round 1 of reviews. This is honestly quite unreviewable in its current state. The AI comments are a nightmare and make references to docs and comments I don't have access to. The commits are not logically structured either. I would probably have ordered them something like:

The initial adding of the database-alias and db-router
Fixing management commands and other models (GenericReleation)
Adding the database domain migration command
CI work and tests

I would like to set expectations now that this will require major changes and many iterations before we are close to a state that might be mergeable.

gerrod3 · 2026-07-31T17:27:34Z

+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


Why do we need these two different settings?

The first one makes sure we don't "flag" objects in "flight" as orphans. For example in the case of a migration of an active domain to a new DB, we need some grace period before we start flagging as orphans

And than how regularly we purge is a different setting, the 2x can be very different values. Multiple days vs multiple hours. Makes sense to be 2 phased in a way.

gerrod3 · 2026-07-31T18:47:46Z

+_DOMAIN_WALK_MAX_DEPTH = 2
+
+
+def _resolve_domain_id(value, _depth=0, _seen=None):


Do we really need this? Yeah it's probably the safest way to get the domain of the object, but I would expect that get_domain would always return the correct domain that the object is in. Maybe it doesn't matter since creating GenericRelationships typically never happen in a hot path

I see it more as a protection for the future, just playing safe.

dralley · 2026-08-05T12:55:42Z

Rebase this please

YasenT · 2026-08-10T10:54:11Z

Rebase this please

rebased. tests re-running, had to fix few things, seems okay for now

gerrod3

After more review I am not convinced that this is a good feature to add to Pulp. There are too many unknowns with how this could break and will potentially saddle us with a larger burden of maintenance then we are capable to provide. I would strongly suggest taking another look at running multiple Pulp instances to solve your problem.

If that is still not possible, here are some changes I would like to see.

gerrod3 · 2026-09-09T01:51:42Z

+
+        instance = hints.get("instance")
+        if instance is not None:
+            if "pulp_domain_id" in instance.__dict__:


We should probably add a comment for why we are looking at __dict__ instead of directly accessing it.

gerrod3 · 2026-09-10T17:54:45Z

+def is_multi_db_routing_active():
+    return any(isinstance(r, PulpDomainRouter) for r in django_router.routers)


I feel we should cache this calculation.

I think it's already a very cheap calculation, with djanngo_router.routers already being a cached property. An any/isinstance over something that should already be a very short list is a sub-microsecond as is.
Happy to add caching if you really feel it will help

gerrod3 · 2026-09-10T18:05:55Z

+        from pulpcore.app.db_router import is_multi_db_routing_active
+
+        if not is_multi_db_routing_active():
+            return super().filter(*args, **kwargs)


Instead of checking this every call, we should just swap these methods in the init if the router is selected.

Wouldn't we call it more often if we move it to the init?

gerrod3 · 2026-09-10T18:15:50Z

 def _ensure_default_domain(sender, **kwargs):
+    if kwargs.get("using", "default") != "default":
+        return
    table_names = connection.introspection.table_names()


This is wrong, with multi-db we need to ensure that every domain on a separate DB has that exact domain object in their DB. We might also need to ensure that each DB also has a copy of the default domain.

I believe that what you are looking for is handled already by _ensure_domains_replicated + reconcile_domains_to_alias()

gerrod3 · 2026-09-10T18:15:59Z

    return _current_domain.get() or get_default_domain()


 def get_domain_pk():


The handling here is incorrect. Let's take an example of a Repository:

The Repository model has a ForeignKey (pulp_domain) to Domain, with a default value set to this function. This ensures when a Repository is created it gets its pulp-domain set to the Domain that call was initiated from. With multi-db this would currently fail since the current_domain would live in the default database and wouldn't exist in that Domain's database, creating an invalid lookup.

Why wouldn't the domain exist in the domains database?
A domain has to be already migrated for us to start sending queries to the new db.
And domain_sync handles the sync part.

gerrod3 · 2026-09-10T18:38:35Z

-            with transaction.atomic():
-                content_guard, _created = ContentRedirectContentGuard.objects.get_or_create(
+            with transaction.atomic(using=alias):
+                content_guard, _created = ContentRedirectContentGuard.objects.using(


This would fail without a default domain in the other DB.

I think it's already covered.
_ensure_domains_replicated runs right before this hook in ready()
reconcile_domains_to_alias() always includes default in its desired set regardless of which alias it's reconciling
So by the time this runs, defaults row is already there?

gerrod3 · 2026-09-10T18:39:35Z

+        if is_multi_db_routing_active():
+            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"


Is this needed? Are UserRoles even cleanedup currently?

Django`s deletion collector will treat a GenericRelation like any other reverse relation for cascade purposes and delete the UserRoles when you delete a Repository.
And the current django collector will bypass our domain route, therefore failing silently and leaving orphaned rows behind.

gerrod3 · 2026-09-10T19:02:05Z

I think we shouldn't allow cross DB generic relationships in this initial approach. Instead we should do:

Make Task's CreatedResource.save() a no-op when multi-db is enabled

Don't allow ObjectRolePermissionBackend or DefaultAccessPolicy to be set when multi-db is enabled so that UserRole & GroupRole objects are not used/created

CreatedResource really isn't needed anymore now that we have Task.output, so this isn't that big of a lost. For the RBAC stuff, if services need to use the role based per object we can think of removing User & Group from the control pane and just duplicating the needed users/groups across the DBs. They don't change as often as UserRole/GroupRoles do so I believe there would be less potential for errors this way. But I think that should be done in a future change if necessary.

Hm, Task.output? I am missing something I guess
Ripping all of this away, when we already have the code?

gerrod3 · 2026-09-10T19:09:51Z

I'm going to comment on the whole of this commit adding the migrate commands here:

I believe this whole piece should be moved to a separate PR in the future. We should test out the feature on brand new domains and see what works and what is broken before trying to migrate everything over. I've done some AI review on this PR and most of the bugs, the really bad ones, are from this part. The migration logic is hard, especially for all that you want it to do, so it needs a lot more design and testing that will delay this PR even further if it were to stay.

On the other hand, did you know you can do a pulp-export/import with domains? Have you thought of just using that for the migration?

Our issues, and the reason behind this PR, is the real need to move some big domains to seperate databases.
The rest of the PR without us being able to quickly move stuff, doesn't help us, because the users will keep seeing performance issues.
So I suggest that we keep moving with the review.

The idea here is to separate "having the machinery in place", from "turning the crank". Putting the machinery in place affects everyone who uses Pulp - if that machinery has some edge-cases we haven't considered, it breaks WAY more users than just the Hosted use-case. So, one PR makes the changes that will be there even if you never migrate anyone - because those are global. That lets us address any global problems in isolation from the ...physics of trying to move Lots Of Data.

A second PR would be "OK, now that we have the infrastructure, let's turn the crank", and we can work on addressing problems we discover when we do that.

"We need both parts now" is true - but it's not a good reason to combine them into one PR, that will potentially impact everyone who installs Pulp whether they take advantage of this or not.

While i will remove the migration, so we can move forward quicker.

I believe that the impact is limited just to the hosted-pulp. As this affects only users who are using the multi-db. And try to migrate, etc

It would have been helpful if I knew what the bugs are, so that I can work on these, while maintaining this as a patch on our side for example.

gerrod3

This feels much more manageable now.

gerrod3 · 2026-09-30T02:20:20Z

+from django.db.models import Q
+
+
+class CrossDBQuerySetMixin:


Why is this still here? We ain't doing cross-db queries.

You are right, leftovers, cleaning.

gerrod3 · 2026-09-30T02:21:48Z

+    moving = models.BooleanField(
+        default=False,
+        help_text="True while this domain's data is being moved between database aliases.",
+    )


Should we still keep this field?

leftovers, cleaning it

gerrod3 · 2026-09-30T02:24:35Z

+        return True
+
+
+def is_multi_db_routing_active():


Let's add an LRU cache, max size of 1.

done

And your next comment about the task/artifact .. did changes there too. Just can't comment on it now.

gerrod3 · 2026-09-30T02:29:32Z

-        for pa in ProfileArtifact.objects.select_related("artifact").filter(task=task):
-            data[pa.name] = get_artifact_url(pa.artifact)
+        alias = task.pulp_domain.database_alias
+        for pa in ProfileArtifact.objects.filter(task=task):


I still don't think this is right. We should ensure when a ProfileArtifact is being saved that the associated artifact is saved in the default database to prevent cross db relations.

gerrod3 · 2026-09-30T02:32:34Z

+        post_migrate.connect(
+            _ensure_domains_replicated,
+            sender=self,
+            dispatch_uid="ensure_domains_replicated_identifier",
+        )


For all of these post_migrate and post_save hooks let's group them all into a separate function that is only called if the DBRouter is selected.

gerrod3 · 2026-09-30T02:37:34Z

+    return values
+
+
+def replicate_domain_save(domain, using=None, attempts=REPLICATION_RETRY_ATTEMPTS):


Let's add a comment describing the behavior: "Default changed -> copied to all satellites. Satellite changed -> copied back to default."

We do all the changes against default, and sync towards the satellites only

gerrod3 · 2026-09-30T02:44:03Z

+    desired_domains = {
+        domain.pulp_id: domain
+        for domain in Domain.objects.using("default")
+        if domain.name == "default" or domain.database_alias == alias
+    }


Suggested change

desired_domains = {

domain.pulp_id: domain

for domain in Domain.objects.using("default")

if domain.name == "default" or domain.database_alias == alias

}

desired_domains = Domain.objects.using("default").filter(Q(name="default") | Q(database_alias=alias))

I think this will break the function, as now desired_domains is a dict.
Let me suggest an alternative there, will push it.

gerrod3 · 2026-09-30T03:03:18Z

+deleted. Only User, Group, Role, and grants with no target object (global or domain-wide) need
+active replication, because they aren't naturally tied to -- and thus routed to -- a single alias.


Is this actually true? I'm trying to think of a scenario with a domain role that isn't routed to a single alias. With global roles yes, it could be useful to have them duplicated, but I'm wondering if we should even allow that. My initial thought it to keep it real simple and only duplicate Users & Groups. Even Roles could be moved off the control pane. My understanding is that most large tenants will get their own database that they can have their domains in. So it should be sufficient to just have the Users & Groups duplicated. If a tenant gets so large to have multiple databases maybe then we should worry about duplicated Roles & global User/GroupRoles.

I agree, did some changes.

…ting Introduce the core routing infrastructure needed to split a Pulp instance's domains across multiple database aliases: a `database_alias` field on `Domain`, and a `PulpDomainRouter` that pins control-plane models to `default` and routes data-plane models to their owning domain's alias (via instance hints or the domain ContextVar). `is_multi_db_routing_active` is lru_cache'd: it's a pure function of settings.DATABASE_ROUTERS, fixed for the life of the process, but called from many routing/replication decisions. Also add the supporting ContextVar/util helpers (`with_migration_alias`, `domain_db`, `for_each_domain`, and a `get_domain_pk()` alias fix) used by later commits, and the app-startup guards needed for `migrate` and its existing post_migrate hooks to behave correctly once more than one database alias is configured. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ting Update existing management commands (datarepair, datarepair-2327, remove-plugin, repository-size, rotate-db-key, dump-publications-to-fs, handle-artifact-checksums, analyze-publication) to iterate per-domain and query the correct database alias instead of assuming a single database. Add Domain row replication to every satellite alias (domain_sync.py) -- Domain stays control-plane (always on 'default'), but data-plane rows FK to it with a real, DB-enforced constraint, so a matching row needs to exist locally on each alias too. Replicate Users/Groups/Roles the same way (rbac_sync.py): UserRole/ GroupRole's user/group/role foreign keys are real, DB-enforced constraints, so whichever alias a grant lands on needs a local copy of whatever it references. UserRole/GroupRole themselves are data-plane, not control-plane, and are never replicated -- every grant has exactly one home and is created there directly (role_util.assign_role): object-level grants on their target object's own alias (ambient routing already gets this right, since the request that can see that object is already scoped to that domain); domain-wide grants on their domain's own alias (resolved explicitly, since ambient context can't be trusted the way it can for an object-scoped request); global grants (tied to neither an object nor a domain) on 'default', since they aren't scoped to any single alias at all. Object-level grants are cleaned up by Django's native GenericRelation cascade (BaseModel.user_roles/group_roles) when their target is deleted, same as single-database Pulp. Permission is always resolved against 'default' and trusted when filtering a query pinned to a satellite alias: Permission/ ContentType are provisioned identically (same code, same migrations, same order) on every alias, so a 'default' pk is valid everywhere. All of the Domain/User/Group/Role replication signals are grouped into PulpAppConfig._connect_multi_db_signals(), only called when PulpDomainRouter is actually configured -- a single-database Pulp instance never connects (or pays for firing, on every save/delete of these models) signal receivers it has no use for. Task diagnostic artifacts (memory/pyinstrument/memray profiles, task logs) are internal operational data, not real domain content, so save them directly to 'default' at creation time (tasking/_util.py) instead of wherever the task's domain happens to route to -- ProfileArtifact is control-plane, so this keeps its artifact FK a same-database relationship with no cross-plane lookup needed anywhere that reads it back. CreatedResource.save() becomes a no-op while multi-db routing is active: its content_object is a plain GenericForeignKey that can point at a data-plane object on a different database than this (control-plane) row, and unlike User/Group/Role it can point at literally any model, so replicating whatever it references isn't practical the way it is for those. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Enable multi-database unit tests as part of the normal test suite: data_1 is a second database on the same local Postgres server used by 'default', so Django's test runner creates/tears down its own test database for it, with PulpDomainRouter registered -- no dedicated satellite service/CI job needed. Add unit tests covering the router itself (test_db_router.py), Domain routing/replication and the RBAC design end-to-end -- object-level, domain-wide, and global grants each landing on their one correct alias with no replication involved (test_multi_database_routing.py, test_rbac_sync.py) -- and the DomainMiddleware/content-handler/rotate-db-key changes from the preceding commits. Also fix flakiness in test_cancel_task_group by retrying task-group cancellation on a transient 409 instead of failing outright. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

gerrod3

I think we have a problem with transactions. The django.db.transaction api doesn't consult any configured db_router. It explicitly requires using=alias in order to apply the transaction to the correct DB. Also, nested transactions need to be within the same alias. If there is cross DB transactions the behavior is not well defined, and I believe we would experience this on repository creation which update a task's reserved_resource_record.

gerrod3 · 2026-09-30T20:09:19Z

+  - global grants (tied to no object or domain) always live on 'default'
+A grant is only ever read back from the same alias it was written to (role_util.
+get_objects_for_user_roles queries 'default' for global grants and qs's own alias for the other
+two), so none of them need a copy anywhere else. Object-level grants are cleaned up automatically


This still seems too complicated. We should just have the global grants be per database. You can create a role assignment from any domain if you have permission, but that role assignment must live in the database of that domain.

gerrod3 · 2026-09-30T20:13:02Z

According to Astra, the CI is failing because migration 104_delete_label ends up using the db_router which calls get_default_domain which doesn't use the historical domain model and ends up trying to query for fields that don't yet exist. I believe this happens because you haven't used your with_migration_alias context anywhere yet.

github-actions Bot added no-changelog no-issue multi-commit labels Jul 15, 2026

YasenT marked this pull request as ready for review July 24, 2026 10:30

gerrod3 reviewed Aug 3, 2026

View reviewed changes

YasenT force-pushed the multidb-implementation branch from ed36dcc to 2ad60ec Compare August 10, 2026 10:20

YasenT force-pushed the multidb-implementation branch from 35804e0 to 6c71cca Compare August 13, 2026 19:08

YasenT requested a review from gerrod3 August 19, 2026 09:49

YasenT force-pushed the multidb-implementation branch 3 times, most recently from 124b368 to 1c14902 Compare September 4, 2026 07:28

gerrod3 requested changes Sep 10, 2026

View reviewed changes

YasenT force-pushed the multidb-implementation branch 4 times, most recently from 68c7357 to 1853850 Compare September 29, 2026 16:26

gerrod3 requested changes Sep 30, 2026

View reviewed changes

YasenT and others added 3 commits September 30, 2026 17:00

YasenT force-pushed the multidb-implementation branch from 1853850 to 5e1c0ef Compare September 30, 2026 16:38

gerrod3 requested changes Sep 30, 2026

View reviewed changes

		_DOMAIN_WALK_MAX_DEPTH = 2


		def _resolve_domain_id(value, _depth=0, _seen=None):

		def is_multi_db_routing_active():
		return any(isinstance(r, PulpDomainRouter) for r in django_router.routers)

		return _current_domain.get() or get_default_domain()


		def get_domain_pk():

		return values


		def replicate_domain_save(domain, using=None, attempts=REPLICATION_RETRY_ATTEMPTS):

		deleted. Only User, Group, Role, and grants with no target object (global or domain-wide) need
		active replication, because they aren't naturally tied to -- and thus routed to -- a single alias.

Conversation

YasenT commented Jul 15, 2026

Uh oh!

YasenT commented Jul 15, 2026

Uh oh!

YasenT commented Jul 21, 2026

Uh oh!

gerrod3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dralley commented Aug 5, 2026

Uh oh!

YasenT commented Aug 10, 2026

Uh oh!

gerrod3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gerrod3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

YasenT Sep 30, 2026 •

edited

Loading