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

sjha4 · 2023-07-07T16:43:50Z

What are the changes introduced in this pull request?

Move remove content unit orphan deletionout of index_content to avoid increasing indexing times which impacts both syncs and publishes. Instead orphan deletion only happens during OrphanCleanup now.

Considerations taken when implementing this change?

What are the testing steps for this pull request?

~~Sync a repo. You should see the task: "Remove orphaned content units" run after completion of sync or publish CV actions asynchronously.~~
0. Check Katello::Rpm.count in foreman-rake console.

Create a lot of orphaned content. You can create these by running below in foreman-rake console:

i = 1
while(i<50000)
f = Katello::Rpm.new(name: i, pulp_id: i)
f.save!
i +=1
end

Check Katello::Rpm.count in console.
Complete sync any small repo. It shouldn't take ridiculously long. You can compare this against earlier snaps.
In foreman-rake console,
Katello::Rpm.count should stay the same.
Run bundle exec rails katello:delete_orphaned_content . Go to foreman task in UI and wait for it to complete.
Check Katello::Rpm.count again. The orphaned dummy rpm records should get cleaned up..

If you want to use real data, sync several large repos, publish them into a few CVs and create several versions.
That should create enough data to notice slow IndexContent action before this change.

theforeman-bot · 2023-07-07T16:43:54Z

Issues: #36563

app/models/katello/concerns/pulp_database_unit.rb

parthaa

Seems to work ok . Checked sync and saw the new action

parthaa

Tested the remove orphan calls. Behavior looked good. ACK

ianballou · 2023-07-10T20:04:38Z

app/lib/actions/katello/repository/index_content.rb

@@ -31,6 +31,10 @@ def run
            output[:new_content][content_type.label] = new_count - initial_counts[content_type.label]
          end
        end
+
+        def finalize
+          ForemanTasks.async_task(OrphanCleanup::RemoveOrphanedContentUnits, {repo_id: input[:id]})


I'm questioning now if we even need to destroy orphaned units after syncing.

For the orphaned filter rule problem, the solution is this:

katello/app/services/katello/content_unit_indexer.rb

Line 155 in 05f9de2

def clean_filter_rules(repo_associations_to_destroy)

Otherwise, I don't think orphaned content units should cause any other problems.

Plus, if a user syncs 20 repositories, they're going to get 20 orphan cleanup tasks running, which could eat up resources on their system.

Perhaps we should just rely on the weekly orphan cleanup?

If we do that, I just suggest that we change the following error messages to tell the users to run orphan cleanup instead of run a complete sync:

https://github.com/Katello/katello/blob/master/app/services/katello/pulp3/repository.rb#L500

https://github.com/Katello/katello/blob/master/app/services/katello/pulp3/repository.rb#L512

https://github.com/Katello/katello/blob/master/app/services/katello/pulp3/repository/docker.rb#L73

Actually, maybe we should change those error messages with your PR here regardless. Complete sync no longer deletes the orphaned units itself.

ianballou · 2023-07-10T20:10:38Z

app/lib/actions/katello/orphan_cleanup/remove_orphaned_content_units.rb

+        end
+
+        def run
+          content_types_to_index = []


content_types_to_index makes it sound a bit like the types will be indexed instead of checked for orphans.

ianballou · 2023-07-10T20:13:16Z

app/lib/actions/katello/orphan_cleanup/remove_orphaned_content_units.rb

+            repo = ::Katello::Repository.find(input[:id])
+            content_types_to_index = repo.repository_type.content_types_to_index
+          else
+            fail "Pass either a repository to determine content type or destroy_all to destroy all orphaned content units"


What do you think about removing the destroy_all param? That way only repo_id would be needed and the logic would be a bit simplified. Orphan cleanup should only be deleting useless data, so I don't think a safety net is needed.

sjha4 · 2023-07-11T11:05:51Z

Updated the PR to run orphan content unit deletion only in orphan cleanup task which should run once a week in deployed environments.
Removed inputs from task to make it run on all content units during the orphan cleanup.
Updated error messages to point to the rake task.

jeremylenz · 2023-07-11T12:15:35Z

PR title needs an update now ;)

sjha4 · 2023-07-11T12:46:41Z

Should be ready for re-review.

app/services/katello/pulp3/repository.rb

ianballou · 2023-07-11T20:06:19Z

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


Any reason why this was pulled out of the run phase?

Not really..This was the only operation in the run phase after refactoring the content unit deletion..and this doesn't seem like something that needs to be in the run phase..

If there's a DB operation, should try not to put it in the run phase because run doesn't run in a transaction :)

Wouldn't a heavy DB operation in the planning phase eat up a Puma worker? Also we shouldn't need transactions here at least since we're deleting orphans. But even if we did need one, couldn't we do a transaction block?

(I don't think deleting orphaned root repos will be a heavy operation, but I'm a little confused because we stick heavy DB ops in tasks in other places)

I'm a little confused because we stick heavy DB ops in tasks in other places

Usually they're in plan or finalize though, right? Those are fine because they run in transactions by default.

Ack to that..We tend to put only things that need API calls to other products in run phase generally. For this task, I'd be ok keeping this in run phase over the plan phase. Any failures on this are set to rescue=skip and also we do want to still plan other pulp orphan deletion actions even if the orphan deletion in katello fails. A plan phase failure wouldn't plan those pulp tasks.

We could put this in finalize if we want. Let me know if we feel strongly about that and I can update this.

Makes sense to me. I trust your judgment to choose the phase correctly :)

… only

ianballou

Works for me!

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

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

theforeman-bot added the Not yet reviewed label Jul 7, 2023

sjha4 force-pushed the sync_repo_optimization branch 2 times, most recently from a6eb979 to 06f9cd6 Compare July 7, 2023 18:02

parthaa reviewed Jul 7, 2023

View reviewed changes

app/models/katello/concerns/pulp_database_unit.rb Outdated Show resolved Hide resolved

sjha4 force-pushed the sync_repo_optimization branch from 06f9cd6 to 7c403eb Compare July 7, 2023 18:26

parthaa approved these changes Jul 7, 2023

View reviewed changes

sjha4 force-pushed the sync_repo_optimization branch 3 times, most recently from 9dd0f85 to 70fc5ba Compare July 10, 2023 04:15

Fixes #36563 - Move remove orphan content units to async task

442b1da

sjha4 force-pushed the sync_repo_optimization branch from 70fc5ba to 442b1da Compare July 10, 2023 18:13

parthaa approved these changes Jul 10, 2023

View reviewed changes

ianballou reviewed Jul 10, 2023

View reviewed changes

theforeman-bot added the Waiting on contributor label Jul 11, 2023

sjha4 force-pushed the sync_repo_optimization branch from 87f89d9 to 5cc856b Compare July 11, 2023 11:42

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 11, 2023

sjha4 changed the title ~~Fixes #36563 - Move remove orphan content units to async task~~ Fixes #36563 - Remove orphaned content unit cleanup from index_content Jul 11, 2023

ianballou reviewed Jul 11, 2023

View reviewed changes

Fixes #36563 - Run orphan content unit deletion during orphan cleanup…

5bf839e

… only

sjha4 force-pushed the sync_repo_optimization branch from 5cc856b to 5bf839e Compare July 11, 2023 20:46

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 11, 2023

ianballou approved these changes Jul 11, 2023

View reviewed changes

sjha4 merged commit 51f7a12 into Katello:master Jul 12, 2023
3 of 4 checks passed

parthaa mentioned this pull request Jan 18, 2024

Fixes #37058 - Optimize query to identify orphaned content units #10851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sjha4 commented Jul 7, 2023 •

edited

Loading

theforeman-bot commented Jul 7, 2023

parthaa left a comment

parthaa left a comment

ianballou Jul 10, 2023

ianballou Jul 10, 2023

ianballou Jul 10, 2023

ianballou Jul 10, 2023

sjha4 commented Jul 11, 2023

jeremylenz commented Jul 11, 2023

sjha4 commented Jul 11, 2023

ianballou Jul 11, 2023

sjha4 Jul 11, 2023

jeremylenz Jul 11, 2023

ianballou Jul 11, 2023 •

edited

Loading

jeremylenz Jul 11, 2023

sjha4 Jul 12, 2023

jeremylenz Jul 12, 2023

ianballou left a comment

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

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

Conversation

sjha4 commented Jul 7, 2023 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

theforeman-bot commented Jul 7, 2023

parthaa left a comment

Choose a reason for hiding this comment

parthaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjha4 commented Jul 11, 2023

jeremylenz commented Jul 11, 2023

sjha4 commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

sjha4 commented Jul 7, 2023 •

edited

Loading

ianballou Jul 11, 2023 •

edited

Loading