Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy #10542

ianballou · 2023-05-02T20:28:20Z

What are the changes introduced in this pull request?

Ensures that content units belonging to no repositories are destroyed at repo indexing time. This is necessary because content units that have been removed from Pulp can still remain attached to content view filters, which will cause blocking errors at CV publish time.

I also improved the error returned from Pulp that occurs when trying to copy units that don't exist.

Note that I didn't rewrite the special rpm copy API error. In testing, I found that the rpm copy API ignores content units that don't exist.

Considerations taken when implementing this change?

I considered if content units should be destroyed during content unit importing, or perhaps if units could be disassociated from filters instead. Finding the filter relations at sync time would be too query-intensive. Determining the content units to remove during ContentUnitIndexer.import_all would require searching other repositories to find out if it's truly orphaned, which I ended up doing higher up in the stack anyway.

What are the testing steps for this pull request?

Create a content view with some module streams and container image tags
Make filters that filter in those tags and module streams
Modify the pulp_hrefs for some module stream and some container image tag to be something that doesn't exist in Pulp
Try publishing the CV. You should see a failed task in Dynflow with the error message rewrite that this PR implements.
Perform a complete sync on the related repositories.
The publish should now work. I think you'll have to skip the failing Dynflow task and try the publish again.

Also, try verifying that orphaned content units are indeed destroyed using something other than module streams and container image tags.

theforeman-bot · 2023-05-02T20:28:23Z

Issues: #36334

sjha4 · 2023-05-17T14:46:38Z

app/models/katello/concerns/pulp_database_unit.rb

+        else
+          where.not(:repository_id => ::Katello::Repository.all)
+        end
+      end


Does this handle module streams that don't exist in pulp that are still referenced by Katello CV repositories. (non-library). I am guessing the testing steps are throwing me off here.

You're right, in that case they wouldn't get removed. To fix this, I'll look again at having some sort of "correct filters" method that will run after a repository is synced. That'll ensure that ID filter rules exist only if they relate to content that is in the library repositories.

app/services/katello/content_unit_indexer.rb

ianballou · 2023-05-19T18:48:55Z

@sjha4 I updated the code to clean filter rules after disassociating module streams, package groups, and errata. I didn't bother putting it in an async task because the disassociation should be pretty rare.

ianballou · 2023-05-19T21:55:18Z

@sjha4 I added the rake task.

sjha4 · 2023-05-22T15:42:17Z

lib/katello/tasks/upgrades/4.9/clean_orphaned_filter_rules.rake

+
+        ::Katello::ContentViewErratumFilterRule.all.each do |rule|
+          content_view = rule.filter.content_view
+          unless ::Katello::Erratum.in_repositories(content_view.repositories).pluck(:errata_id).include?(rule.errata_id)


We should add some nil checks here.

lib/katello/tasks/upgrades/4.9/clean_orphaned_filter_rules.rake

sjha4 · 2023-05-22T15:58:22Z

app/services/katello/content_unit_indexer.rb

-            not(@model_class.unit_id_field => assocication_tracker.unit_ids).delete_all
+          repo_associations_to_destroy = @model_class.repository_association_class.where(repository_id: @repository.id).where.
+            not(@model_class.unit_id_field => assocication_tracker.unit_ids)
+          clean_filter_rules(repo_associations_to_destroy) if repo_associations_to_destroy.present? && [::Katello::ModuleStream, ::Katello::Erratum, ::Katello::PackageGroup].include?(@model_class)


Can we also check affected_content_view_ids.present? here to avoid calling this if no CVs have this repo.

Added a return statement below.

ianballou · 2023-05-22T22:25:56Z

@sjha4 I added rule cleanup when removing repositories from content views. I also did a little refactoring with the content view filter rule queries to dry up the code.

sjha4 · 2023-05-23T12:21:32Z

Code looks good..Will test the updates. 👍🏼

parthaa

APJ

sjha4

Able to reproduce the error of missing content unit and task fails with updated error message:
After complete sync on failing repository, able to publish successfully
Filter rules by Id are removed if their repo association is removed during indexing
Tested the rake task on missing module streams from master and then switching to this branch and running rake task. Able to delete stream filter rules with the task.

Code looks good. Added a commit to fix tests etc which was APJ'ed by @parthaa
Ack on the PR. Will merge after tests are green!

sjha4 · 2023-05-23T20:42:04Z

[test katello]

…ve missing content units error on Pulp 3 copy

ianballou · 2023-05-30T21:34:52Z

@sjha4 my latest push should've fixed the test.

sjha4

Ack.. 👍🏼

…ve missing content units error on Pulp 3 copy (Katello#10542) * Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy * Refs #36334 - remove filter rules when related units get disassociated * Refs #36334 - add rake task to clean up orphaned filter rules * Refs #36334 - clean filter rules when removing repositories * Refs #36334 - Fix rubocop and tests --------- Co-authored-by: Samir Jha <sjha4@ncsu.edu> (cherry picked from commit aed6299)

…ve missing content units error on Pulp 3 copy (#10542) * Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy * Refs #36334 - remove filter rules when related units get disassociated * Refs #36334 - add rake task to clean up orphaned filter rules * Refs #36334 - clean filter rules when removing repositories * Refs #36334 - Fix rubocop and tests --------- Co-authored-by: Samir Jha <sjha4@ncsu.edu> (cherry picked from commit aed6299)

theforeman-bot added Waiting on contributor Not yet reviewed labels May 2, 2023

sjha4 reviewed May 17, 2023

View reviewed changes

ianballou force-pushed the 36334-orphaned-content-units branch from 87c45ae to 09cd628 Compare May 19, 2023 17:51

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 19, 2023

ianballou force-pushed the 36334-orphaned-content-units branch from 09cd628 to 3c97546 Compare May 19, 2023 17:53

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 19, 2023

sjha4 self-assigned this May 19, 2023

sjha4 reviewed May 19, 2023

View reviewed changes

app/services/katello/content_unit_indexer.rb Show resolved Hide resolved

ianballou force-pushed the 36334-orphaned-content-units branch from 3c97546 to 3fd681a Compare May 19, 2023 18:48

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 19, 2023

sjha4 reviewed May 22, 2023

View reviewed changes

lib/katello/tasks/upgrades/4.9/clean_orphaned_filter_rules.rake Show resolved Hide resolved

sjha4 reviewed May 22, 2023

View reviewed changes

ianballou force-pushed the 36334-orphaned-content-units branch from d36bc69 to 2ec7eba Compare May 22, 2023 20:11

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 22, 2023

ianballou requested a review from sjha4 May 22, 2023 20:22

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 22, 2023

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 23, 2023

parthaa approved these changes May 23, 2023

View reviewed changes

sjha4 force-pushed the 36334-orphaned-content-units branch from 10ad887 to afa2d4c Compare May 23, 2023 20:00

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 23, 2023

sjha4 approved these changes May 23, 2023

View reviewed changes

ianballou and others added 5 commits May 30, 2023 20:15

Fixes #36334 - Delete orphaned content units after indexing and impro…

726aad8

…ve missing content units error on Pulp 3 copy

Refs #36334 - remove filter rules when related units get disassociated

53d2cee

Refs #36334 - add rake task to clean up orphaned filter rules

fc918a6

Refs #36334 - clean filter rules when removing repositories

003dd2d

Refs #36334 - Fix rubocop and tests

3d24f23

ianballou force-pushed the 36334-orphaned-content-units branch from afa2d4c to 3d24f23 Compare May 30, 2023 21:34

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels May 30, 2023

sjha4 approved these changes May 31, 2023

View reviewed changes

ianballou merged commit aed6299 into Katello:master May 31, 2023
4 of 5 checks passed

ianballou deleted the 36334-orphaned-content-units branch May 31, 2023 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy #10542

Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy #10542

ianballou commented May 2, 2023

theforeman-bot commented May 2, 2023

sjha4 May 17, 2023

ianballou May 17, 2023

ianballou commented May 19, 2023

ianballou commented May 19, 2023

sjha4 May 22, 2023

sjha4 May 22, 2023

ianballou May 22, 2023

ianballou commented May 22, 2023

sjha4 commented May 23, 2023

parthaa left a comment

sjha4 left a comment •

edited

sjha4 commented May 23, 2023

ianballou commented May 30, 2023

sjha4 left a comment

Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy #10542

Fixes #36334 - Delete orphaned content units after indexing and improve missing content units error on Pulp 3 copy #10542

Conversation

ianballou commented May 2, 2023

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 May 2, 2023

sjha4 May 17, 2023

Choose a reason for hiding this comment

ianballou May 17, 2023

Choose a reason for hiding this comment

ianballou commented May 19, 2023

ianballou commented May 19, 2023

sjha4 May 22, 2023

Choose a reason for hiding this comment

sjha4 May 22, 2023

Choose a reason for hiding this comment

ianballou May 22, 2023

Choose a reason for hiding this comment

ianballou commented May 22, 2023

sjha4 commented May 23, 2023

parthaa left a comment

Choose a reason for hiding this comment

sjha4 left a comment • edited

Choose a reason for hiding this comment

sjha4 commented May 23, 2023

ianballou commented May 30, 2023

sjha4 left a comment

Choose a reason for hiding this comment

sjha4 left a comment •

edited