Merge branch '122002-remove-lfs-storage-project' into 'master'

Remove the lfs_storage_project helper

Closes #122002

See merge request gitlab-org/gitlab!42000

app/controllers/repositories/lfs_api_controller.rb

@@ -46,7 +46,7 @@ module Repositories
     end
 
     def download_objects!
-      existing_oids = project.all_lfs_objects_oids(oids: objects_oids)
+      existing_oids = project.lfs_objects_oids(oids: objects_oids)
 
       objects.each do |object|
         if existing_oids.include?(object[:oid])

app/models/project.rb

@@ -1469,46 +1469,6 @@ class Project < ApplicationRecord
     forked_from_project || fork_network&.root_project
   end
 
-  # TODO: Remove this method once all LfsObjectsProject records are backfilled
-  # for forks.
-  #
-  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
-  def lfs_storage_project
-    @lfs_storage_project ||= begin
-      result = self
-
-      # TODO: Make this go to the fork_network root immediately
-      # dependant on the discussion in: https://gitlab.com/gitlab-org/gitlab-foss/issues/39769
-      result = result.fork_source while result&.forked?
-
-      result || self
-    end
-  end
-
-  # This will return all `lfs_objects` that are accessible to the project and
-  # the fork source. This is needed since older forks won't have access to some
-  # LFS objects directly and have to get it from the fork source.
-  #
-  # TODO: Remove this method once all LfsObjectsProject records are backfilled
-  # for forks. At that point, projects can look at their own `lfs_objects`.
-  #
-  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
-  def all_lfs_objects
-    LfsObject
-      .distinct
-      .joins(:lfs_objects_projects)
-      .where(lfs_objects_projects: { project_id: [self, lfs_storage_project] })
-  end
-
-  # TODO: Remove this method once all LfsObjectsProject records are backfilled
-  # for forks. At that point, projects can look at their own `lfs_objects` so
-  # `lfs_objects_oids` can be used instead.
-  #
-  # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
-  def all_lfs_objects_oids(oids: [])
-    oids(all_lfs_objects, oids: oids)
-  end
-
   def lfs_objects_oids(oids: [])
     oids(lfs_objects, oids: oids)
   end

app/services/projects/unlink_fork_service.rb

@@ -2,21 +2,13 @@
 
 module Projects
   class UnlinkForkService < BaseService
-    # If a fork is given, it:
-    #
-    # - Saves LFS objects to the root project
-    # - Close existing MRs coming from it
-    # - Is removed from the fork network
-    #
-    # If a root of fork(s) is given, it does the same,
-    # but not updating LFS objects (there'll be no related root to cache it).
+    # Close existing MRs coming from the project and remove it from the fork network
     def execute
       fork_network = @project.fork_network
+      forked_from = @project.forked_from_project
 
       return unless fork_network
 
-      save_lfs_objects
-
       merge_requests = fork_network
                          .merge_requests
                          .opened
@@ -41,7 +33,7 @@ module Projects
 
       # When the project getting out of the network is a node with parent
       # and children, both the parent and the node needs a cache refresh.
-      [@project.forked_from_project, @project].compact.each do |project|
+      [forked_from, @project].compact.each do |project|
         refresh_forks_count(project)
       end
     end
@@ -51,22 +43,5 @@ module Projects
     def refresh_forks_count(project)
       Projects::ForksCountService.new(project).refresh_cache
     end
-
-    # TODO: Remove this method once all LfsObjectsProject records are backfilled
-    # for forks.
-    #
-    # See https://gitlab.com/gitlab-org/gitlab/issues/122002 for more info.
-    def save_lfs_objects
-      return unless @project.forked?
-
-      lfs_storage_project = @project.lfs_storage_project
-
-      return unless lfs_storage_project
-      return if lfs_storage_project == @project # that project is being unlinked
-
-      lfs_storage_project.lfs_objects.find_each do |lfs_object|
-        lfs_object.projects << @project unless lfs_object.projects.include?(@project)
-      end
-    end
   end
 end

app/workers/repository_fork_worker.rb

@@ -52,7 +52,7 @@ class RepositoryForkWorker # rubocop:disable Scalability/IdempotentWorker
   def link_lfs_objects(source_project, target_project)
     Projects::LfsPointers::LfsLinkService
         .new(target_project)
-        .execute(source_project.all_lfs_objects_oids)
+        .execute(source_project.lfs_objects_oids)
   rescue Projects::LfsPointers::LfsLinkService::TooManyOidsError
     raise_fork_failure(
       source_project,

doc/raketasks/cleanup.md

@@ -10,10 +10,6 @@ DANGER: **Danger:**
 Do not run this within 12 hours of a GitLab upgrade. This is to ensure that all background migrations
 have finished, which otherwise may lead to data loss.
 
-CAUTION: **WARNING:**
-Removing LFS files from a project with forks is currently unsafe. The Rake task
-will refuse to run on projects with forks.
-
 When you remove LFS files from a repository's history, they become orphaned and continue to consume
 disk space. With this Rake task, you can remove invalid references from the database, which
 will allow garbage collection of LFS files.

lib/gitlab/checks/lfs_integrity.rb

@@ -17,7 +17,7 @@ module Gitlab
 
         return false unless new_lfs_pointers.present?
 
-        existing_count = @project.all_lfs_objects
+        existing_count = @project.lfs_objects
                                  .for_oids(new_lfs_pointers.map(&:lfs_oid))
                                  .count
 

lib/gitlab/cleanup/orphan_lfs_file_references.rb

@@ -17,14 +17,6 @@ module Gitlab
       end
 
       def run!
-        # If this project is an LFS storage project (e.g. is the root of a fork
-        # network), what it is safe to remove depends on the sum of its forks.
-        # For now, skip cleaning up LFS for this complicated case
-        if project.forks_count > 0 && project.lfs_storage_project == project
-          log_info("Skipping orphan LFS check for #{project.name_with_namespace} as it is a fork root")
-          return
-        end
-
         log_info("Looking for orphan LFS files for project #{project.name_with_namespace}")
 
         remove_orphan_references

lib/gitlab/import_export/lfs_saver.rb

@@ -16,7 +16,7 @@ module Gitlab
       end
 
       def save
-        project.all_lfs_objects.find_in_batches(batch_size: BATCH_SIZE) do |batch|
+        project.lfs_objects.find_in_batches(batch_size: BATCH_SIZE) do |batch|
           batch.each do |lfs_object|
             save_lfs_object(lfs_object)
           end

spec/lib/gitlab/checks/lfs_integrity_spec.rb

@@ -57,25 +57,5 @@ RSpec.describe Gitlab::Checks::LfsIntegrity do
         expect(subject.objects_missing?).to be_falsey
       end
     end
-
-    context 'for forked project', :sidekiq_might_not_need_inline do
-      let(:parent_project) { create(:project, :repository) }
-      let(:project) { fork_project(parent_project, nil, repository: true) }
-
-      before do
-        allow(project).to receive(:lfs_enabled?).and_return(true)
-      end
-
-      it 'is true parent project is missing LFS objects' do
-        expect(subject.objects_missing?).to be_truthy
-      end
-
-      it 'is false parent project already contains LFS objects for the fork' do
-        lfs_object = create(:lfs_object, oid: blob_object.lfs_oid)
-        create(:lfs_objects_project, project: parent_project, lfs_object: lfs_object)
-
-        expect(subject.objects_missing?).to be_falsey
-      end
-    end
   end
 end

spec/lib/gitlab/cleanup/orphan_lfs_file_references_spec.rb

@@ -87,42 +87,4 @@ RSpec.describe Gitlab::Cleanup::OrphanLfsFileReferences do
       .to receive(:get_all_lfs_pointers)
       .and_return(oids.map { |oid| OpenStruct.new(lfs_oid: oid) })
   end
-
-  context 'LFS for forked projects' do
-    let!(:fork_root) { create(:project, :repository, lfs_enabled: true) }
-    let!(:fork_internal) { fork_project(fork_root, nil, repository: true) }
-    let!(:fork_leaf) { fork_project(fork_internal, nil, repository: true) }
-
-    let(:dry_run) { true }
-
-    context 'root node' do
-      let(:project) { fork_root }
-
-      it 'skips cleanup' do
-        expect(service).not_to receive(:remove_orphan_references)
-
-        service.run!
-      end
-    end
-
-    context 'internal node' do
-      let(:project) { fork_internal }
-
-      it 'runs cleanup' do
-        expect(service).to receive(:remove_orphan_references)
-
-        service.run!
-      end
-    end
-
-    context 'leaf node' do
-      let(:project) { fork_leaf }
-
-      it 'runs cleanup' do
-        expect(service).to receive(:remove_orphan_references)
-
-        service.run!
-      end
-    end
-  end
 end

spec/models/project_spec.rb

@@ -2976,68 +2976,6 @@ RSpec.describe Project do
         expect(project.forks).to contain_exactly(forked_project)
       end
     end
-
-    describe '#lfs_storage_project' do
-      it 'returns self for non-forks' do
-        expect(project.lfs_storage_project).to eq project
-      end
-
-      it 'returns the fork network root for forks' do
-        second_fork = fork_project(forked_project)
-
-        expect(second_fork.lfs_storage_project).to eq project
-      end
-
-      it 'returns self when fork_source is nil' do
-        expect(forked_project).to receive(:fork_source).and_return(nil)
-
-        expect(forked_project.lfs_storage_project).to eq forked_project
-      end
-    end
-
-    describe '#all_lfs_objects' do
-      let(:lfs_object) { create(:lfs_object) }
-
-      context 'when LFS object is only associated to the source' do
-        before do
-          project.lfs_objects << lfs_object
-        end
-
-        it 'returns the lfs object for a project' do
-          expect(project.all_lfs_objects).to contain_exactly(lfs_object)
-        end
-
-        it 'returns the lfs object for a fork' do
-          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
-        end
-      end
-
-      context 'when LFS object is only associated to the fork' do
-        before do
-          forked_project.lfs_objects << lfs_object
-        end
-
-        it 'returns nothing' do
-          expect(project.all_lfs_objects).to be_empty
-        end
-
-        it 'returns the lfs object for a fork' do
-          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
-        end
-      end
-
-      context 'when LFS object is associated to both source and fork' do
-        before do
-          project.lfs_objects << lfs_object
-          forked_project.lfs_objects << lfs_object
-        end
-
-        it 'returns the lfs object for the source and fork' do
-          expect(project.all_lfs_objects).to contain_exactly(lfs_object)
-          expect(forked_project.all_lfs_objects).to contain_exactly(lfs_object)
-        end
-      end
-    end
   end
 
   describe '#set_repository_read_only!' do
@@ -6150,53 +6088,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#all_lfs_objects_oids' do
-    let(:project) { create(:project) }
-    let(:lfs_object) { create(:lfs_object) }
-    let(:another_lfs_object) { create(:lfs_object) }
-
-    subject { project.all_lfs_objects_oids }
-
-    context 'when project has associated LFS objects' do
-      before do
-        create(:lfs_objects_project, lfs_object: lfs_object, project: project)
-        create(:lfs_objects_project, lfs_object: another_lfs_object, project: project)
-      end
-
-      it 'returns OIDs of LFS objects' do
-        expect(subject).to match_array([lfs_object.oid, another_lfs_object.oid])
-      end
-
-      context 'and there are specified oids' do
-        subject { project.all_lfs_objects_oids(oids: [lfs_object.oid]) }
-
-        it 'returns OIDs of LFS objects that match specified oids' do
-          expect(subject).to eq([lfs_object.oid])
-        end
-      end
-    end
-
-    context 'when fork has associated LFS objects to itself and source' do
-      let(:source) { create(:project) }
-      let(:project) { fork_project(source) }
-
-      before do
-        create(:lfs_objects_project, lfs_object: lfs_object, project: source)
-        create(:lfs_objects_project, lfs_object: another_lfs_object, project: project)
-      end
-
-      it 'returns OIDs of LFS objects' do
-        expect(subject).to match_array([lfs_object.oid, another_lfs_object.oid])
-      end
-    end
-
-    context 'when project has no associated LFS objects' do
-      it 'returns empty array' do
-        expect(subject).to be_empty
-      end
-    end
-  end
-
   describe '#lfs_objects_oids' do
     let(:project) { create(:project) }
     let(:lfs_object) { create(:lfs_object) }

spec/services/projects/lfs_pointers/lfs_link_service_spec.rb

@@ -24,11 +24,11 @@ RSpec.describe Projects::LfsPointers::LfsLinkService do
     end
 
     it 'links existing lfs objects to the project' do
-      expect(project.all_lfs_objects.count).to eq 2
+      expect(project.lfs_objects.count).to eq 2
 
       linked = subject.execute(new_oid_list.keys)
 
-      expect(project.all_lfs_objects.count).to eq 3
+      expect(project.lfs_objects.count).to eq 3
       expect(linked.size).to eq 3
     end
 
@@ -52,7 +52,7 @@ RSpec.describe Projects::LfsPointers::LfsLinkService do
       lfs_objects = create_list(:lfs_object, 7)
       linked = subject.execute(lfs_objects.pluck(:oid))
 
-      expect(project.all_lfs_objects.count).to eq 9
+      expect(project.lfs_objects.count).to eq 9
       expect(linked.size).to eq 7
     end
 

spec/services/projects/unlink_fork_service_spec.rb

@@ -58,26 +58,6 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
     expect(source.forks_count).to be_zero
   end
 
-  context 'when the source has LFS objects' do
-    let(:lfs_object) { create(:lfs_object) }
-
-    before do
-      lfs_object.projects << project
-    end
-
-    it 'links the fork to the lfs object before unlinking' do
-      subject.execute
-
-      expect(lfs_object.projects).to include(forked_project)
-    end
-
-    it 'does not fail if the lfs objects were already linked' do
-      lfs_object.projects << forked_project
-
-      expect { subject.execute }.not_to raise_error
-    end
-  end
-
   context 'when the original project was deleted' do
     it 'does not fail when the original project is deleted' do
       source = forked_project.forked_from_project
@@ -152,24 +132,6 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
       expect(project.forks_count).to be_zero
     end
 
-    context 'when given project is a fork of an unlinked parent' do
-      let!(:fork_of_fork) { fork_project(forked_project, user) }
-      let(:lfs_object) { create(:lfs_object) }
-
-      before do
-        lfs_object.projects << project
-      end
-
-      it 'saves lfs objects to the root project' do
-        # Remove parent from network
-        described_class.new(forked_project, user).execute
-
-        described_class.new(fork_of_fork, user).execute
-
-        expect(lfs_object.projects).to include(fork_of_fork)
-      end
-    end
-
     context 'and is node with a parent' do
       subject { described_class.new(forked_project, user) }
 

spec/workers/repository_fork_worker_spec.rb

@@ -87,7 +87,7 @@ RSpec.describe RepositoryForkWorker do
       it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of source project LFS objects' do
         expect_fork_repository(success: true)
         expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service|
-          expect(service).to receive(:execute).with(project.all_lfs_objects_oids)
+          expect(service).to receive(:execute).with(project.lfs_objects_oids)
         end
 
         perform!