Fixes #38003 - more efficient handle_acs_product_removal #11214

pmoravec · 2024-11-11T18:48:46Z

What are the changes introduced in this pull request?

Simplified calculation of content types that remain in the product after the repo removal. One (or two) ActiveRecord queries are much faster and much less memory demanding than traversing potentially hundreds or thousands of repos in the product.

Considerations taken when implementing this change?

The PR was tested in one call flow only (CV version deletion).

What are the testing steps for this pull request?

See the foreman issue.

pmoravec · 2024-11-12T13:35:14Z

I dont understand the failed tests:

Error: Failure: test_0804_route api/v2/job_invocations/hosts should have a permission that grants access

should be independent.

Error: Failure: test_0009_plans ACS product removal when removing the deleting the last repo with URL

That seems relevant, but I can not reproduce it - what does the test do differently than me? 1) create a product and a repo with feed URL, 2) create ACS referring to the product, 3) delete the repo - here, some extra debugs show me that repo_content_types=[] (originally it was <Set: {}>, but inclusion test works the same for lists and sets) and also I do see Removing product zoo_acs with ID 352 from ACS acs_2 with ID 2 log.

Maybe this is a consequence of (unrelated?) error

ERROR: Missing required option for --name HTTP_PROXY_NAMEERROR: Missing required option for --url HTTP_PROXY_URLDEPRECATED: global use of must_include from katello/test/actions/katello/repository_set_test.rb:97. Use _(obj).must_include instead. This will fail in Minitest 6.

?

chris1984 · 2024-11-12T20:42:33Z

I dont understand the failed tests:

Error: Failure: test_0804_route api/v2/job_invocations/hosts should have a permission that grants access

should be independent.

Error: Failure: test_0009_plans ACS product removal when removing the deleting the last repo with URL

That seems relevant, but I can not reproduce it - what does the test do differently than me? 1) create a product and a repo with feed URL, 2) create ACS referring to the product, 3) delete the repo - here, some extra debugs show me that repo_content_types=[] (originally it was <Set: {}>, but inclusion test works the same for lists and sets) and also I do see Removing product zoo_acs with ID 352 from ACS acs_2 with ID 2 log.

Maybe this is a consequence of (unrelated?) error

ERROR: Missing required option for --name HTTP_PROXY_NAMEERROR: Missing required option for --url HTTP_PROXY_URLDEPRECATED: global use of must_include from katello/test/actions/katello/repository_set_test.rb:97. Use _(obj).must_include instead. This will fail in Minitest 6.

?

Test failures are unrelated, restarting them. There was an issue with remote_execution that caused those.

parthaa · 2024-11-13T19:36:47Z

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

-            repo_content_types.add(test_repo.content_type) if (repository.id != test_repo.id) && test_repo.url.present?
-          end
+          root_ids = product.repositories.where.not(:id => repository.id).pluck(:root_id).uniq
+          repo_content_types = ::Katello::RootRepository.where(:id => root_ids).where.not(:url => [nil, '']).pluck(:content_type).uniq


How about

repo_content_types = ::Katello::RootRepository.where(:id => product.repositories.where.not(:id => repository.id).select(:root_id)).where.not(:url => [nil, '']).distinct.pluck(:content_type)

That was my original patch :) But wasnt sure about code readability here (or where to split the cmd into multiple lines).

Either patch is fine for me. Let me know if I shall update the PR per this suggestion.

chris1984 · 2024-11-13T20:00:40Z

Before PR:

[root@ip-10-0-168-118 ~]# top
top - 14:31:25 up 22:44,  2 users,  load average: 1.13, 1.17, 1.50
Tasks: 315 total,   2 running, 313 sleeping,   0 stopped,   0 zombie
%Cpu(s): 16.6 us,  0.3 sy,  0.0 ni, 83.0 id,  0.0 wa,  0.0 hi,  0.0 si,  0.0 st
MiB Mem :  23776.6 total,   8386.2 free,  14127.7 used,   1854.1 buff/cache
MiB Swap:   4096.0 total,   4096.0 free,      0.0 used.   9648.9 avail Mem
    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
 162874 foreman   20   0 9849.0m   10.7g  12032 S 100.0  40.9  15:40.76 rails

 2024-11-13T14:38:54 [I|app|fedcbe8b] Completed 202 Accepted in 1107208ms (Views: 136.3ms | ActiveRecord: 26627.4ms | Allocations: 346926573)

With @parthaa suggestion:
2024-11-13T10:59:58 [I|app|dfe43680] Completed 202 Accepted in 49289ms (Views: 492.7ms | ActiveRecord: 3497.9ms | Allocations: 23707580)

ianballou · 2024-11-13T20:41:19Z

Was the reason for this code existing tested as well? You would test it by deleting the last repository with an upstream URL in a product that belongs to a simplified ACS. The code should then remove the empty product from the ACS.

pmoravec · 2024-11-13T20:44:39Z

Was the reason for this code existing tested as well? You would test it by deleting the last repository with an upstream URL in a product that belongs to a simplified ACS. The code should then remove the empty product from the ACS.

Yes, I tested this scenario on a backport of the patch to my Sat6.15. I noticed log Removing product zoo_acs with ID 352 from ACS acs_2 with ID 2 and the ACS has empty list of products now (it had just the one product before).

chris1984 · 2024-11-13T21:12:28Z

@pmoravec Are you able to push up a change with Partha's suggestion in it and rebase(that should fix the test failures) The improvement is so much faster. My satellite UI literally crashed because it took to long to return the task.

pmoravec · 2024-11-13T21:28:06Z

Sure, here you are ;-)

pmoravec · 2024-11-14T09:44:48Z

Still same/similar errors in CI test..? :-o

chris1984 · 2024-11-14T15:56:43Z

This looks like the test failure:

Error: Failure: test_0009_plans ACS product removal when removing the deleting the last repo with URL

Expected: 0
  Actual: 1

@ianballou do you know why that would have failed with the new DB query?

ianballou · 2024-11-14T19:21:06Z

This looks like the test failure:
Error: Failure: test_0009_plans ACS product removal when removing the deleting the last repo with URL

Expected: 0
  Actual: 1
@ianballou do you know why that would have failed with the new DB query?

I'm not sure just by looking at the query, the best way to find out would be to debug the test and see what's actually going wrong.

ianballou · 2024-11-14T20:05:47Z

I'm looking into it a bit, I think the issue might come down to the test data caching old root repository URLs. I was seeing unexpected results during debugging.

ianballou

I did some debugging, hopefully these results help a little:

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

chris1984 · 2024-11-18T14:38:04Z

@pmoravec @qcjames53 it looks like the same test is failing :(

pmoravec · 2024-11-18T15:10:51Z

Yeah.. The own manual test case "add product with last repo to ACS, delete the repo, check the ACS" against the current code (backported to a Satellite) passes to me.

Is the test case updated in upstream? It is https://github.com/Katello/katello/blob/master/test/actions/katello/repository_test.rb#L210-L228 ?

Also, should not I update the test on my branch? I mean, what test is executed by the CI jobs - from the given PR or from upstream?

ianballou · 2024-11-18T19:41:35Z

Yeah.. The own manual test case "add product with last repo to ACS, delete the repo, check the ACS" against the current code (backported to a Satellite) passes to me.

Just to be super safe, does it also work if you have two yum repositories in a product with an ACS, one with a URL and one without a URL, and you delete the repo with the URL?

ianballou · 2024-11-18T19:43:17Z

Also, should not I update the test on my branch? I mean, what test is executed by the CI jobs - from the given PR or from upstream?

You can replace the failing test with the fixed one that @qcjames53 provided in the comments above. The test CI runs the tests that are in your own branch used in this PR.

pmoravec · 2024-11-19T07:55:25Z

Just to be super safe, does it also work if you have two yum repositories in a product with an ACS, one with a URL and one without a URL, and you delete the repo with the URL?

Running test:

hammer product create --organization-id 1 --name acs_test_product
hammer repository create --organization-id 1 --product acs_test_product --name zoo_repo --content-type yum --url https://repos.fedorapeople.org/repos/pulp/pulp/demo_repos/zoo/
hammer repository synchronize --organization-id 1 --product acs_test_product --name zoo_repo
hammer repository create --organization-id 1 --product acs_test_product --name dummy_repo --content-type yum
productid=$(hammer product info --organization-id 1 --name acs_test_product | head -n1 | awk '{ print $2 }')
hammer alternate-content-source create --name acs_repodel_test --alternate-content-source-type simplified --product-ids $productid
hammer alternate-content-source show --name acs_repodel_test
hammer repository delete --organization-id 1 --product acs_test_product --name zoo_repo
hammer alternate-content-source show --name acs_repodel_test

I got:

Product created.
Repository created.
[.....................................................................................................................................................................................] [100%]
Added Rpms: 32, Errata: 4
Total steps: 84/84
--------------------------------
Associating Content: 39/39
Downloading Artifacts: 0/0
Downloading Metadata Files: 6/6
Parsed Advisories: 4/4
Parsed Comps: 3/3
Parsed Packages: 32/32
Skipping Packages: 0/0
Un-Associating Content: 0/0
Repository created.
Alternate Content Source created.
ID:                            4
Name:                          acs_repodel_test
Label:                         acs_repodel_test
Content type:                  yum
Alternate content source type: simplified
Products:                      
 1) Id:              355
    Organization ID: 1
    Name:            acs_test_product
    Label:           acs_test_product
Smart proxies:

Repository deleted.
ID:                            4
Name:                          acs_repodel_test
Label:                         acs_repodel_test
Content type:                  yum
Alternate content source type: simplified
Products:                      

Smart proxies:

So here the ACS<->Product relation was properly removed.

I updated the test in my PR as well, let see now..

pmoravec · 2024-11-19T08:40:28Z

I dont want to fight for my change, it can be possible I have a mistake there, just a thought about the slight difference in checks. Original check applied test:

.. if (repository.id != test_repo.id) && test_repo.url.present?

for each repository individually. My approach applies the has repo its URL?" per product. BUT since repository's urlmethod is delegated to:root`, the above code is equivalent to:

.. if (repository.id != test_repo.id) && test_repo.root_repository.url.present?

and that is the same like my check does, just in different ordering of conditions.

I replaced .present? method by .where.not(:url => [nil, '']) that is also slightly different (" ".present? or [].present? is evaluated to false but my code ignores that) - but the (original and also updated) test uses nil, as well as katello code in general (I think)..?

About the modified test case: what is the reference to published_rhel7_repository? I can find it in proposed test but nowhere else..?

ianballou · 2024-11-19T14:33:23Z

With the testing being done I'm pretty confident that your PR is okay. Since your fix here brings high value, if we can't get the test working in a couple more days I suggest we temporarily disable it. We can get the test working again after the fact to release you Pavel :)

ianballou · 2024-11-21T19:05:23Z

test/actions/katello/repository_test.rb


      # manually remove the URLs from all repos in product except repository
-      repository.product.repositories.each do |repo|
-        repo.root.update!(url: nil) unless repo.id == repository.id
+      testing_repo = repository.product.repositories[4]


@qcjames53 I'm not sure if the repository at index [4] will consistently be the same one, we'd probably need to look up by name instead.

Actually.... it looks like this was applied to the wrong test? The failing test is plans ACS product removal when removing the deleting the last repo with URL. I'll see about updating that other test to see if it helps.

ianballou · 2024-11-21T19:30:24Z

@pmoravec this diff should fix all of the tests:

diff --git a/test/actions/katello/repository_test.rb b/test/actions/katello/repository_test.rb
index baa80b7774..112112217f 100644
--- a/test/actions/katello/repository_test.rb
+++ b/test/actions/katello/repository_test.rb
@@ -208,23 +208,21 @@ module ::Actions::Katello::Repository
     end
 
     it 'plans ACS product removal when removing the last URL for a product' do
-      ::Katello::SmartProxyAlternateContentSource.create!(alternate_content_source_id: simplified_acs.id, smart_proxy_id: proxy.id, repository_id: published_rhel7_repository.id)
+      ::Katello::SmartProxyAlternateContentSource.create!(alternate_content_source_id: simplified_acs.id, smart_proxy_id: proxy.id, repository_id: repository.id)
       action = create_action action_class
-      action.stubs(:action_subject).with(published_rhel7_repository)
+      action.stubs(:action_subject).with(repository)
 
       # manually remove the URLs from all repos in product except repository
-      testing_repo = repository.product.repositories[4]
-      testing_repo.product.repositories.each do |repo|
-        repo.root.url = nil unless repo.id == testing_repo.id || repo.root.id == testing_repo.root.id
-        repo.root.save!(validate: false)
+      repository.product.repositories.each do |repo|
+        repo.root.update!(url: nil) unless repo.id == repository.id
       end
-      url_sum = testing_repo.product.repositories.count do |repo|
+      url_sum = repository.product.repositories.count do |repo|
         repo.root.url.present?
       end
       assert_equal(1, url_sum) # double check there's only one URL left
 
       # Since there is only one URL remaining, the product should be removed
-      plan_action action, published_rhel7_repository, remove_from_content_view_versions: true
+      plan_action action, repository.root, :url => nil
       simplified_acs.reload
       assert_equal(0, simplified_acs.products.length)
     end
@@ -238,6 +236,7 @@ module ::Actions::Katello::Repository
     let(:published_repository) { katello_repositories(:rhel_6_x86_64) }
     let(:published_fedora_repository) { katello_repositories(:fedora_17_x86_64) }
     let(:simplified_acs) { katello_alternate_content_sources(:yum_alternate_content_source) }
+    let(:published_rhel7_repository) { katello_repositories(:rhel_7_no_arch) }
     def setup
       simplified_acs.products << published_repository.product
       ::Katello::SmartProxyAlternateContentSource.create!(alternate_content_source_id: simplified_acs.id, smart_proxy_id: proxy.id)
@@ -380,22 +379,24 @@ module ::Actions::Katello::Repository
       assert_not_equal(0, simplified_acs.products.length)
     end
 
-    it 'plans ACS product removal when removing the deleting the last repo with URL' do
-      ::Katello::SmartProxyAlternateContentSource.create!(alternate_content_source_id: simplified_acs.id, smart_proxy_id: proxy.id, repository_id: published_repository.id)
+    it 'plans ACS product removal when deleting the last repo with URL' do
+      ::Katello::SmartProxyAlternateContentSource.create!(alternate_content_source_id: simplified_acs.id, smart_proxy_id: proxy.id, repository_id: published_rhel7_repository.id)
       action = create_action action_class
-      action.stubs(:action_subject).with(published_repository)
+      action.stubs(:action_subject).with(published_rhel7_repository)
 
       # manually remove the URLs from all repos in product except repository
-      repository.product.repositories.each do |repo|
-        repo.root.url = nil unless repo.id == repository.id
+      testing_repo = repository.product.repositories[4]
+      testing_repo.product.repositories.each do |repo|
+        repo.root.url = nil unless repo.id == testing_repo.id || repo.root.id == testing_repo.root.id
+        repo.root.save!(validate: false)
       end
-      url_sum = repository.product.repositories.count do |repo|
+      url_sum = testing_repo.product.repositories.count do |repo|
         repo.root.url.present?
       end
       assert_equal(1, url_sum) # double check there's only one URL left
 
       # Since there is only one URL remaining, the product should be removed
-      plan_action action, published_repository, remove_from_content_view_versions: true
+      plan_action action, published_rhel7_repository, remove_from_content_view_versions: true
       simplified_acs.reload
       assert_equal(0, simplified_acs.products.length)
     end

Updating also relevant test per qcjames53's and ianballou's comments. Signed-off-by: Pavel Moravec <[email protected]>

pmoravec · 2024-11-22T09:27:27Z

Cool, that helped, thanks! Maybe I copy&pasted wrong patch from the discussion, sorry for the confusion.

Now just rpm-build on RHEL8 is failing - I think due to this:

No matching package to install: 'foreman-assets >= 3.14'
No matching package to install: 'foreman-plugin >= 3.14'
Not all dependencies satisfied
Error: Some packages could not be found.

but I have no idea how to resolve it (though it should be independent on this PR).

chris1984 · 2024-11-22T18:06:46Z

We don't need the EL8 to pass, those are gone now and removed yesterday. Will give it another test and if it looks good will merge it in. Thanks @ianballou @qcjames53 for helping Pavel getting the tests passing.

chris1984

ACK, works great @pmoravec thank you. Big difference with the pr applied:

Before:

2024-11-25T15:13:33 [I|app|9eb151c6] Completed 202 Accepted in 754786ms (Views: 115.9ms | ActiveRecord: 20691.8ms | Allocations: 241932979)

After:

Completed 202 Accepted in 13832ms (Views: 160.9ms | ActiveRecord: 2170.0ms | Allocations: 5362191)

Screenshot from the tasks page showing the time difference:

ekohl · 2024-11-26T11:33:22Z

Since this was merged we see nightly fail:
https://ci.theforeman.org/view/Foreman%20Nightly/job/katello-master-source-release/2741/

Failure:
test_0009_plans ACS product removal when deleting the last repo with URL(Minitest::Result) [/home/jenkins/workspace/katello-master-source-release/test/actions/katello/repository_test.rb:396]:
Expected: 1
  Actual: 7

The next run failed on the same thing.

chris1984 · 2024-11-26T13:01:17Z

@ianballou @qcjames53 do you have any suggestions to fix nightly?

It was all green on the pr, why I merged it

ianballou · 2024-11-26T14:41:34Z

I wonder if there's random behavior and so a different seed caused a failure

ianballou · 2024-11-26T14:48:36Z

ktest test/actions/katello/repository_test.rb --seed 32408 reproduces the failing test.

qcjames53 · 2024-12-02T15:16:07Z

This seems to be an issue with the [4] index I used to select the RHEL7 package. It looks like this index is occasionally incorrect. Sorry about that; I didn't know that was something that could happen. This will need another PR to so we're selecting by package instead of index for this unit test.

ekohl · 2024-12-02T15:18:28Z

I think #11237 has resolved the failure, but wasn't linked here.

qcjames53 · 2024-12-02T15:51:40Z

Thanks Ewoud!

pr-processor bot added the Not yet reviewed label Nov 11, 2024

parthaa reviewed Nov 13, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Nov 13, 2024

chris1984 self-assigned this Nov 13, 2024

pmoravec force-pushed the efficient-handle_acs_product_removal branch from 9febdb8 to b368813 Compare November 13, 2024 21:27

pmoravec force-pushed the efficient-handle_acs_product_removal branch from b368813 to 45b89be Compare November 14, 2024 07:13

ianballou reviewed Nov 14, 2024

View reviewed changes

app/lib/actions/katello/repository/destroy.rb Show resolved Hide resolved

qcjames53 reviewed Nov 14, 2024

View reviewed changes

app/lib/actions/katello/repository/destroy.rb Show resolved Hide resolved

pmoravec force-pushed the efficient-handle_acs_product_removal branch from 45b89be to 7ad9723 Compare November 15, 2024 18:30

pmoravec force-pushed the efficient-handle_acs_product_removal branch from 7ad9723 to 3a63647 Compare November 19, 2024 07:54

ianballou reviewed Nov 21, 2024

View reviewed changes

Fixes #38003 - more efficient handle_acs_product_removal

76a17a8

Updating also relevant test per qcjames53's and ianballou's comments. Signed-off-by: Pavel Moravec <[email protected]>

pmoravec force-pushed the efficient-handle_acs_product_removal branch from 3a63647 to 76a17a8 Compare November 22, 2024 07:36

chris1984 approved these changes Nov 25, 2024

View reviewed changes

chris1984 added the Ready for merge label Nov 25, 2024

chris1984 merged commit ffbf8b2 into Katello:master Nov 25, 2024
26 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #38003 - more efficient handle_acs_product_removal #11214

Fixes #38003 - more efficient handle_acs_product_removal #11214

pmoravec commented Nov 11, 2024

pmoravec commented Nov 12, 2024

chris1984 commented Nov 12, 2024

parthaa Nov 13, 2024

pmoravec Nov 13, 2024

chris1984 commented Nov 13, 2024 •

edited

Loading

ianballou commented Nov 13, 2024

pmoravec commented Nov 13, 2024

chris1984 commented Nov 13, 2024

pmoravec commented Nov 13, 2024

pmoravec commented Nov 14, 2024

chris1984 commented Nov 14, 2024

ianballou commented Nov 14, 2024

ianballou commented Nov 14, 2024

ianballou left a comment

chris1984 commented Nov 18, 2024

pmoravec commented Nov 18, 2024 •

edited

Loading

ianballou commented Nov 18, 2024

ianballou commented Nov 18, 2024

pmoravec commented Nov 19, 2024 •

edited

Loading

pmoravec commented Nov 19, 2024

ianballou commented Nov 19, 2024

ianballou Nov 21, 2024

ianballou Nov 21, 2024

ianballou commented Nov 21, 2024

pmoravec commented Nov 22, 2024

chris1984 commented Nov 22, 2024

chris1984 left a comment

ekohl commented Nov 26, 2024

chris1984 commented Nov 26, 2024

ianballou commented Nov 26, 2024

ianballou commented Nov 26, 2024

qcjames53 commented Dec 2, 2024

ekohl commented Dec 2, 2024

qcjames53 commented Dec 2, 2024

Fixes #38003 - more efficient handle_acs_product_removal #11214

Fixes #38003 - more efficient handle_acs_product_removal #11214

Conversation

pmoravec commented Nov 11, 2024

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

pmoravec commented Nov 12, 2024

chris1984 commented Nov 12, 2024

parthaa Nov 13, 2024

Choose a reason for hiding this comment

pmoravec Nov 13, 2024

Choose a reason for hiding this comment

chris1984 commented Nov 13, 2024 • edited Loading

ianballou commented Nov 13, 2024

pmoravec commented Nov 13, 2024

chris1984 commented Nov 13, 2024

pmoravec commented Nov 13, 2024

pmoravec commented Nov 14, 2024

chris1984 commented Nov 14, 2024

ianballou commented Nov 14, 2024

ianballou commented Nov 14, 2024

ianballou left a comment

Choose a reason for hiding this comment

chris1984 commented Nov 18, 2024

pmoravec commented Nov 18, 2024 • edited Loading

ianballou commented Nov 18, 2024

ianballou commented Nov 18, 2024

pmoravec commented Nov 19, 2024 • edited Loading

pmoravec commented Nov 19, 2024

ianballou commented Nov 19, 2024

ianballou Nov 21, 2024

Choose a reason for hiding this comment

ianballou Nov 21, 2024

Choose a reason for hiding this comment

ianballou commented Nov 21, 2024

pmoravec commented Nov 22, 2024

chris1984 commented Nov 22, 2024

chris1984 left a comment

Choose a reason for hiding this comment

ekohl commented Nov 26, 2024

chris1984 commented Nov 26, 2024

ianballou commented Nov 26, 2024

ianballou commented Nov 26, 2024

qcjames53 commented Dec 2, 2024

ekohl commented Dec 2, 2024

qcjames53 commented Dec 2, 2024

chris1984 commented Nov 13, 2024 •

edited

Loading

pmoravec commented Nov 18, 2024 •

edited

Loading

pmoravec commented Nov 19, 2024 •

edited

Loading