[YasenT]

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

[YasenT]

/retest

[YasenT]

@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:

1. The initial adding of the database-alias and db-router
2. Fixing management commands and other models (GenericReleation)
3. Adding the database domain migration command
4. 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]

Why do we need these two different settings?

[YasenT]

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]

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

[YasenT]

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

[dralley]

Rebase this please

[YasenT]

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]

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

[YasenT]

done

[gerrod3]

I feel we should cache this calculation.

[YasenT]

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]

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

[YasenT]

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

[gerrod3]

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.

[YasenT]

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

[gerrod3]

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.

[YasenT]

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]

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

[YasenT]

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]

Is this needed? Are UserRoles even cleanedup currently?

[YasenT]

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]

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

1. Make Task's CreatedResource.save() a no-op when multi-db is enabled
2. 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.

[YasenT]

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

[gerrod3]

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?

[YasenT]

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.

[ggainey]

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.

[YasenT]

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]

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

[YasenT]

You are right, leftovers, cleaning.

[gerrod3]

Should we still keep this field?

[YasenT]

leftovers, cleaning it

[gerrod3]

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

[YasenT]

done

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

[gerrod3]

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]

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]

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

[YasenT]

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

[gerrod3]

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))

[YasenT]

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

[gerrod3]

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.

[YasenT]

I agree, did some changes.

[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]

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]

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.

[YasenT]

can we rebuild the containers for the tests? I remmeber that you guys were able to rebuild them clean

[YasenT]

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.

what do you think would make for a decent solution here?

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

I reproduced several routing and RBAC regressions at cf5a9c1 that need addressing before merge. The 18 added routing/RBAC tests pass, but nine additional regression cases fail with real PostgreSQL databases in an isolated oci-env environment.

1. Replicate role permission membership changes. Replication currently runs on post_save, before RoleSerializer applies .permissions.set(). New roles therefore reach satellites without permissions, and removing a permission leaves it active on the satellite. Please handle M2M membership changes as well as model saves. rbac_sync.py

2. Honor the database of loaded data-plane instances. Loading a repository with .using("data_1"), then saving it outside domain context, inserts a duplicate in default. Loading a repository version the same way and accessing .repository raises Repository.DoesNotExist because the related query goes to default. The router should respect a saved data-plane instance's _state.db before falling back to ambient context, while preserving creation routing and control-plane handling. db_router.py

3. Make commit callbacks use the transaction's database. Transactions now use the satellite alias, but Artifact.delete() still registers its file deletion callback on default. I reproduced a satellite rollback that restores the artifact row while its stored file is deleted. Please supply the correct using alias to this callback and audit the remaining on_commit() calls alongside the transaction changes. content.py

4. Route object-level grants and checks to the target object's database. In satellite context, a core.task_owner grant is written to the satellite even though the Task lives in default. Task queryset scoping searches grants in default, so the owner cannot see the task. Object-level grant creation/removal should follow the target object's database, including API paths that bypass assign_role(). Direct permission checks also need explicit database selection because the backend currently queries grants through ambient routing. This should be a general rule for object-level grants, covering Tasks, TaskSchedules, Domains, and Groups. Domain-wide grants should continue to follow the domain's database alias. role_util.py, backends.py

5. Classify Pulp's Group proxy as control-plane. Its label is core.group, so the auth app classification misses it. Creating a Group in satellite context writes it to data_1, and the replication handler ignores that save because it did not originate in default. Please classify the proxy explicitly or use its concrete model for this decision. db_router.py

6. Preserve created-resource tracking for targets in default. The current early return disables tracking for every target whenever the router is enabled. Please retain existing tracking when the target lives in default. Satellite targets can be omitted to avoid cross-database relationships; that limitation should be explicit because cancellation cleanup currently depends on these records. task.py

7. Make satellite default content guards compatible with the Domain FK. Setting default_content_guard to a satellite guard violates the FK on the Domain row in default. Replicating the selected guard into default, with the same UUID and owning domain, before updating the FK would address this without a cross-database relationship. The replica needs the concrete subclass data and update/delete synchronization. Composite guards also require their child guards and membership records. domain.py, CompositeContentGuard

[YasenT]

@gerrod Yes i confirmed the bugs and fixed them

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Thanks for working on this. I left inline comments on the issues I found. The main ones:

• Domains that share an alias get processed multiple times by for_each_domain callers.
• Replicating Permission pks assumes identical pks on every alias, which upgrade history doesn't guarantee.
• Group-based grants on satellites never match because the group IDs are a lazy subquery.
• Domain delete and save replication can leave satellites inconsistent (missing PROTECT checks, no on_commit, the default_content_guard FK).
• User saves, including each login's last_login update, write synchronously to every satellite.

Smaller cleanups: the retry/save helpers are repeated in domain_sync, content_guard_sync and rbac_sync (along with satellite_aliases and _field_values) and could share one implementation. Also, the PR doesn't have a changelog entry in CHANGES/ yet.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

for_each_domain calls the callback once per Domain, but the callers query the whole alias without filtering by pulp_domain. Every domain that shares an alias reprocesses all rows on it.

On a single-DB install with DOMAIN_ENABLED and 5 domains (all on default):

• datarepair-2327 dry run reports 5x the real remote count.
• handle-artifact-checksums --report prints the same data 5 times under different domain headers and retries failed restores 5 times.
• dump-publications-to-fs (the unfiltered Distribution.objects.using(alias).all()) exports each publication 5 times to the same path.

Either filter by pulp_domain=domain in the callbacks, or iterate distinct aliases instead of domains where the command is alias-wide.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Copying raw Permission pks assumes the auto-increment pks match on every alias. Same plugins doesn't guarantee that, because pks depend on each database's migration history:

• default was likely created years ago and upgraded many times, so ContentTypes/Permissions for newer models got appended with higher pks. A satellite created today gets everything in one migrate, in app order.
• Django leaves stale ContentType/Permission rows behind for removed models unless remove_stale_contenttypes is run, so default has gaps a fresh satellite won't.

A satellite added to an existing deployment would end up with roles pointing at the wrong permissions (or an FK error). get_objects_for_user_roles also filters satellite queries with a Permission resolved on default, so this turns into wrong grants rather than a loud failure.

The comment in role_util.get_objects_for_user_roles also says this replication is natural-key-based, which doesn't match the code. A cheap fix: build a (app_label, codename) -> pk map per alias once and translate the IDs before writing the through table. If you'd rather keep the assumption, add a check at migrate time that fails loudly when the pks don't match.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

group_id__in=user_obj.groups.values_list("pk", flat=True) is a lazy subquery, so it runs on the satellite alias, where auth_user_groups is never replicated. Group-based domain and object grants on satellites will never match.

role_util already materializes the group IDs with list(...) first. The same fix is needed here and at lines 38, 81 and 133 (and the group__in=user_obj.groups.all() in get_all_permissions).

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Global grant checks seem to read from different databases. user_obj.object_roles should route to default through the User instance's db hint, while the GroupRole.objects query just below and role_util.get_objects_for_user_roles use the ambient alias or qs.db.

So a global role granted from default context passes has_model_perms, but list endpoints for a satellite domain find no grant and return nothing (and the reverse for a grant created under satellite context). The "ambient routing is correct" comment doesn't hold for the object_roles half.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Two concerns with replicating deletes from here:

1. The PROTECT FKs on default can't see data stored on the satellite, so a domain that still has repositories on data_1 gets deleted on default. The replicated delete then raises ProtectedError on data_1, which is only logged, so the satellite keeps orphaned data with no Domain on the control plane. general_delete (or the domain delete check) should look at the domain's own alias before deleting.
2. Saves and deletes are replicated synchronously in post_save/post_delete. If the default transaction rolls back afterwards, the satellite has already changed. Wrapping the replication in transaction.on_commit would avoid that.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

ensure_replicated_on_alias(alias) never uses alias. replicate_user/group/role loop over every satellite, with 3 attempts and backoff sleeps each.

Running migrate --database data_1 while data_2 isn't migrated yet retries every User, Group and Role against data_2, about 3s per entity. With 1000 users the migrate stalls for close to an hour. Passing the alias through so the backfill only targets that one satellite would fix it.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Moving the reserved_resources_record update and its RuntimeError out of the atomic() block means a failed lock no longer rolls back the repository creation. The repository and its initial version stay committed while the error propagates.

I get that Task and the repository can now be on different aliases, so a single transaction can't cover both. But for single-DB installs this is a regression. Nesting atomic(using="default") around the repository's atomic() would keep the old behavior when the aliases match and get close to it when they don't. Publication.delete and RepositoryVersion.delete have the same pattern with CreatedResource.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Every User save synchronously writes to every satellite. That includes the last_login update on each login (update_last_login calls save(update_fields=["last_login"])), so login latency grows with the number of satellites, and a slow or unavailable satellite delays every login.

post_save receives update_fields, so a small fix is to return early when nothing satellites care about changed, e.g.:

def on_user_post_save(sender, instance, using, update_fields=None, **kwargs):
    if update_fields is not None and set(update_fields) <= {"last_login"}:
        return
    replicate_user(instance, using=using)

Related: Roles and Groups get replicated again on post_save and on every m2m_changed event, so _populate_roles and .set() replicate the same role several times. Collecting the changed pks during the transaction and replicating each one once in transaction.on_commit would remove the duplicates.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Nit: this docstring uses double backticks, and our contributing guidelines ask for docs and comments without them. Single backticks would match the rest of the codebase.

[gerrod3]

Note

Posted by an agent on behalf of @gerrod3.

Nit: this comment is very long for what the code does. Something like "Skip tracking targets on a satellite alias to avoid a cross-database generic relation" would cover it. The same goes for several other comments and docstrings in this PR (e.g. the rbac_sync module docstring, role_util.assign_role, on_group_permissions_changed). Our contributing guidelines ask to keep comments short and concise.