Fixes #36563 - Remove orphaned content unit cleanup from index_content (

#10638) * Fixes #36563 - Move remove orphan content units to async task * Fixes #36563 - Run orphan content unit deletion during orphan cleanup only (cherry picked from commit 51f7a12)
Katello · Jul 24, 2023 · 40b2d5c · 40b2d5c
1 parent 393a5ff
commit 40b2d5c
Show file tree

Hide file tree

Showing 12 changed files with 55 additions and 49 deletions.
diff --git a/app/lib/actions/katello/orphan_cleanup/remove_orphaned_content_units.rb b/app/lib/actions/katello/orphan_cleanup/remove_orphaned_content_units.rb
@@ -0,0 +1,22 @@
+module Actions
+  module Katello
+    module OrphanCleanup
+      class RemoveOrphanedContentUnits < Actions::Base
+        def run
+          models = []
+
+          ::Katello::RepositoryTypeManager.enabled_repository_types.each_value do |repo_type|
+            models << repo_type.content_types_to_index
+          end
+          models.flatten.each do |content_type|
+            content_type.model_class.orphaned.destroy_all
+          end
+        end
+
+        def rescue_strategy
+          Dynflow::Action::Rescue::Skip
+        end
+      end
+    end
+  end
+end
diff --git a/app/lib/actions/katello/orphan_cleanup/remove_orphans.rb b/app/lib/actions/katello/orphan_cleanup/remove_orphans.rb
@@ -7,27 +7,16 @@ class RemoveOrphans < Actions::Base
         end
         def plan(proxy)
           sequence do
+            if proxy.pulp_primary?
+              ::Katello::RootRepository.orphaned.destroy_all
+              plan_action(RemoveOrphanedContentUnits)
+            end
             if proxy.pulp3_enabled?
               plan_action(
                 Actions::Pulp3::Orchestration::OrphanCleanup::RemoveOrphans,
                 proxy)
             end
-            plan_self
-          end
-        end
-
-        def run
-          models = []
-          ::Katello::RepositoryTypeManager.enabled_repository_types.each_value do |repo_type|
-            indexable_types = repo_type.content_types_to_index
-            models += indexable_types&.map(&:model_class)
-            models.select! { |model| model.many_repository_associations }
-          end
-          models.each do |model|
-            model.joins("left join katello_#{model.repository_association} on #{model.table_name}.id = katello_#{model.repository_association}.#{model.unit_id_field}").where("katello_#{model.repository_association}.#{model.unit_id_field} IS NULL").destroy_all
           end
-
-          ::Katello::RootRepository.orphaned.destroy_all
         end
       end
     end

diff --git a/app/models/katello/concerns/pulp_database_unit.rb b/app/models/katello/concerns/pulp_database_unit.rb
@@ -169,9 +169,9 @@ def with_identifiers(ids)
 
       def orphaned
         if many_repository_associations
-          where.not(:id => repository_association_class.where(:repository_id => ::Katello::Repository.all).select(unit_id_field))
+          where.not(:id => repository_association_class.select(unit_id_field))
         else
-          where.not(:repository_id => ::Katello::Repository.all)
+          where.not(:repository_id => ::Katello::Repository.select(:id))
         end
       end
 

diff --git a/app/models/katello/repository.rb b/app/models/katello/repository.rb
@@ -935,7 +935,6 @@ def index_content(options = {})
         repository_type.content_types_to_index.each do |type|
           Katello::Logging.time("CONTENT_INDEX", data: {type: type.model_class}) do
             Katello::ContentUnitIndexer.new(content_type: type, repository: self, optimized: !full_index).import_all
-            type.model_class.orphaned.destroy_all
           end
         end
         repository_type.index_additional_data_proc&.call(self)

diff --git a/app/models/katello/root_repository.rb b/app/models/katello/root_repository.rb
@@ -130,7 +130,7 @@ class RootRepository < Katello::Model
       where(:http_proxy_policy => RootRepository::USE_SELECTED_HTTP_PROXY).
       where(:http_proxy_id => http_proxy_id)
     }
-    scope :orphaned, -> { where.not(id: Katello::Repository.pluck(:root_id).uniq) }
+    scope :orphaned, -> { where.not(id: Katello::Repository.select(:root_id)) }
     scope :redhat, -> { joins(:provider).merge(Katello::Provider.redhat) }
     scope :custom, -> { where.not(:id => self.redhat) }
     delegate :redhat?, :provider, :organization, to: :product

diff --git a/app/services/katello/pulp3/repository.rb b/app/services/katello/pulp3/repository.rb
@@ -497,7 +497,8 @@ def add_content(content_unit_href, remove_all_units = false)
       rescue api.client_module::ApiError => e
         if e.message.include? 'Could not find the following content units'
           raise ::Katello::Errors::Pulp3Error, "Content units that do not exist in Pulp were requested to be copied."\
-            " Please run a complete sync on the following repository: #{repository_reference.root_repository.name}. Original error: #{e.message}"
+             " Please run `foreman-rake katello:delete_orphaned_content` to fix the following repository:"\
+            " #{repository_reference.root_repository.name}. Original error: #{e.message}"
         else
           raise e
         end
@@ -509,7 +510,7 @@ def add_content_for_repo(repository_href, content_unit_href)
       rescue api.client_module::ApiError => e
         if e.message.include? 'Could not find the following content units'
           raise ::Katello::Errors::Pulp3Error, "Content units that do not exist in Pulp were requested to be copied."\
-            " Please run a complete sync on the following repository:"\
+            " Please run `foreman-rake katello:delete_orphaned_content` to fix the following repository:"\
             " #{::Katello::Pulp3::RepositoryReference.find_by(repository_href: repository_href).root_repository.name}. Original error: #{e.message}"
         else
           raise e

diff --git a/app/services/katello/pulp3/repository/docker.rb b/app/services/katello/pulp3/repository/docker.rb
@@ -70,7 +70,7 @@ def add_content(content_unit_href)
         rescue api.client_module::ApiError => e
           if e.message.include? 'Could not find the following content units'
             raise ::Katello::Errors::Pulp3Error, "Content units that do not exist in Pulp were requested to be copied."\
-              " Please run a complete sync on the following repository: #{repository_reference.root_repository.name}. Original error: #{e.message}"
+              " Please run `foreman-rake katello:delete_orphaned_content` to fix the following repository: #{repository_reference.root_repository.name}. Original error: #{e.message}"
           else
             raise e
           end

diff --git a/test/actions/katello/orphan_cleanup/remove_orphaned_content_units_test.rb b/test/actions/katello/orphan_cleanup/remove_orphaned_content_units_test.rb
@@ -0,0 +1,15 @@
+require 'katello_test_helper'
+module Katello
+  class RemoveOrphanedContentUnitsTest < ActiveSupport::TestCase
+    def setup
+      @rhel6 = Repository.find(katello_repositories(:rhel_6_x86_64).id)
+    end
+
+    def test_orphaned_content_task_destroys_orphans
+      rpm = katello_rpms(:one)
+      rpm.repository_rpms.destroy_all
+      ForemanTasks.sync_task(Actions::Katello::OrphanCleanup::RemoveOrphanedContentUnits)
+      assert_raises(ActiveRecord::RecordNotFound) { rpm.reload }
+    end
+  end
+end
diff --git a/test/actions/katello/orphan_cleanup/remove_orphans_test.rb b/test/actions/katello/orphan_cleanup/remove_orphans_test.rb
@@ -30,36 +30,25 @@ class TestBase < ActiveSupport::TestCase
   class RemoveOrphansPlanTest < TestBase
     let(:action_class) { ::Actions::Katello::OrphanCleanup::RemoveOrphans }
 
-    it 'plans proxy orphans cleanup with pulp3 primary' do
+    it 'plans proxy orphans cleanup and content unit orphan cleanup on pulp3 primary' do
       smart_proxy = SmartProxy.pulp_primary
       tree = plan_action_tree(action_class, smart_proxy)
 
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::RemoveOrphans)
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::DeleteOrphanRepositoryVersions)
+      assert_tree_planned_with(tree, ::Actions::Katello::OrphanCleanup::RemoveOrphanedContentUnits)
     end
 
-    it 'plans proxy orphans cleanup with pulp3 mirror' do
+    it 'plans proxy orphans cleanup without content unit orphan cleanup on pulp3 mirror' do
       smart_proxy = FactoryBot.create(:smart_proxy, :pulp_mirror, :with_pulp3)
+      smart_proxy.stubs(:pulp_primary?).returns(false)
       tree = plan_action_tree(action_class, smart_proxy)
 
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::RemoveOrphans)
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::DeleteOrphanDistributions)
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::DeleteOrphanRemotes)
       assert_tree_planned_with(tree, Actions::Pulp3::OrphanCleanup::DeleteOrphanRepositoryVersions)
-    end
-
-    it 'runs and removes orphan content units' do
-      smart_proxy = SmartProxy.pulp_primary
-      file_unit_orphan = Katello::FileUnit.new(:name => "file_unit", :pulp_id => "orphaned")
-      file_unit_orphan.save!
-      docker_unit_orphan = Katello::DockerTag.new(:name => "docker_unit", :pulp_id => "orphaned_docker")
-      docker_unit_orphan.save!
-      action = create_action(action_class)
-      action.expects(:plan_self)
-      plan_action action, smart_proxy
-      run_action action
-      assert_raises(ActiveRecord::RecordNotFound) { file_unit_orphan.reload }
-      assert_raises(ActiveRecord::RecordNotFound) { docker_unit_orphan.reload }
+      refute_tree_planned(tree, ::Actions::Katello::OrphanCleanup::RemoveOrphanedContentUnits)
     end
   end
 end
diff --git a/test/models/repository_test.rb b/test/models/repository_test.rb
@@ -710,15 +710,6 @@ def test_with_errata
       assert_includes Repository.with_errata([errata]), @rhel6
     end
 
-    def test_index_content_destroys_orphans
-      rpm = katello_rpms(:one)
-      ::Katello::ContentUnitIndexer.any_instance.stubs(:import_all).returns(true)
-      ::Katello::Repository.any_instance.stubs(:import_distribution_data).returns(true)
-      rpm.repository_rpms.destroy_all
-      @rhel6.index_content
-      assert_raises(ActiveRecord::RecordNotFound) { rpm.reload }
-    end
-
     def test_index_content_ordering
       repo_type = @rhel6.repository_type
       SmartProxy.stubs(:pulp_primary).returns(SmartProxy.pulp_primary)

diff --git a/test/services/katello/pulp3/docker_tag_test.rb b/test/services/katello/pulp3/docker_tag_test.rb
@@ -29,7 +29,7 @@ def test_copy_units_rewrites_missing_content_error
           fake_content_href = '/pulp/api/v3/repositories/container/container/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/'
           service = Katello::Pulp3::Repository::Docker.new(@repo, @primary)
           error = assert_raises(::Katello::Errors::Pulp3Error) { service.add_content(fake_content_href) }
-          assert_match(/Please run a complete sync on the following repository: Pulp3 Docker 1./, error.message)
+          assert_match(/Please run `foreman-rake katello:delete_orphaned_content` to fix the following repository: Pulp3 Docker 1./, error.message)
         end
 
         def test_index_on_sync

diff --git a/test/services/katello/pulp3/repository/yum/yum_test.rb b/test/services/katello/pulp3/repository/yum/yum_test.rb
@@ -23,7 +23,7 @@ def test_copy_units_rewrites_missing_content_error
             service = Katello::Pulp3::Repository::Yum.new(@repo, @proxy)
             create_repo(@repo, @proxy)
             error = assert_raises(::Katello::Errors::Pulp3Error) { service.copy_units(@repo, [fake_content_href], false) }
-            assert_match(/Please run a complete sync on the following repository: Fedora 17 x86_64./, error.message)
+            assert_match(/Please run `foreman-rake katello:delete_orphaned_content` to fix the following repository: Fedora 17 x86_64./, error.message)
           end
 
           def test_append_proxy_cacert