Geo: Check bucket prefix when checking stored blob path in use

Priyanshu Jharequested to merge
gitlab-community/gitlab-org/gitlab:geo-bucket-prefix-stored-location-in-use into master

What does this MR do and why?

With object storage replication and a bucket_prefix, the blob_path that a Geo secondary stores on a registry starts with the prefix. Upload#path, the LFS oid and object_storage_key never include it. So blob_location_in_use? never matches for uploads, LFS objects, Helm metadata caches and NuGet symbols, and the stored-location fallback in Geo::DestroyWorker can delete an object that a newer record reuses.

Geo::BlobReplicatorStrategy#registry_location_params now calls a new private method, stored_location_in_use?, instead of blob_location_in_use?. It treats the stored location as in use when blob_location_in_use? matches either the stored path or the stored path without the uploader's object_store_options.bucket_prefix. Both checks are needed, because a local path can start with the prefix by chance (for example remote_directory: gitlab/uploads gives the prefix uploads, and a local avatar path is uploads/-/system/...). A wrong "in use" result only leaves an orphaned file, but a wrong "not in use" result deletes a file that a live record uses.

The location that registry_location_params returns keeps the prefix, because the removal service needs it to find the object. The default blob_location_in_use? and its replicator overrides are unchanged. This needs to land before geo_blob_removal_uses_registry_location is enabled by default (#635220).

Specs

  • blob_replicator_reusable_blob_path_shared_examples.rb: new #replicate_destroy with a bucket prefix block, used by the LFS, Helm metadata cache, NuGet symbol and Terraform replicator specs. With the prefix stubbed and the registry storing the prefixed path, the removal service receives nil, nil when a record reuses the path, and the prefixed absolute path plus the uploader class when none does.
  • user_upload_replicator_spec.rb: the same two examples for user uploads (AvatarUploader), plus a third example where the prefix is uploads and the stored local path starts with uploads/. It fails if only the shorter path is checked.

Verification

  • rubocop passes on the three changed files. I ran it with the repository's .rubocop.yml, with Gitlab/MarkUsedFeatureFlags excluded, because that cop raises an error on the unchanged line 148 in my sparse checkout.
  • I did not run the specs. I only had a sparse checkout without the GitLab Rails and database environment, so the CI pipeline is the first real run of the new examples. bin/rspec ee/spec/replicators/geo/ ee/spec/workers/geo/destroy_worker_spec.rb is the command from the issue to run them.

References

Screenshots or screen recordings

Not applicable, there are no UI changes.

How to set up and validate locally

  1. Run bin/rspec ee/spec/replicators/geo/ ee/spec/workers/geo/destroy_worker_spec.rb.
  2. To confirm the new examples fail without the fix, revert the change in ee/app/models/concerns/geo/blob_replicator_strategy.rb and rerun. The nil, nil examples should fail for LFS, Helm, NuGet and user uploads.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports