Fixes #37279 - Re-create remotes when the type needs to change #10942

quba42 · 2024-03-19T13:25:14Z

When updating a remote, we may need to re-create it instead if the type has changed from uln to rpm or vice versa.

For operations on a remote href, the type is now determined from the href, rather than indirectly from the url. This is more robust since it is the actually relevant source of truth.

All other changes are for readability, deduplication, or consistency

quba42 · 2024-03-19T16:00:03Z

app/services/katello/pulp3/repository.rb

-        if remote_options[:url]&.start_with?('uln')
-          api.remotes_uln_api.partial_update(repo.remote_href, remote_options)
-        else
-          api.remotes_api.partial_update(repo.remote_href, remote_options)
+        url_type = remote_options[:url]&.start_with?('uln') ? 'uln' : 'default'
+        remote_type = repo.remote_href.start_with?('/pulp/api/v3/remotes/rpm/uln/') ? 'uln' : 'default'
+        href = repo.remote_href
+
+        if url_type == remote_type
+          api.get_remotes_api(href: href).partial_update(href, remote_options)
+        else # We need to recreate a remote of the correct type!
+          create_remote
+          delete_remote(href: href)


FYI: This is what actually fixes the bug.

All the rest of the changes are essentially a refactor aimed at improving readability, consistency, and robustness, as well as using the correct source of truth for the remote type (in most cases the remote href itself), and also de-duplicating the remote API selection code.

m-bucher

Overall pretty nice, the get_remotes_api() really cleans up the handling of ULN-remotes.

app/services/katello/pulp3/api/core.rb

app/services/katello/pulp3/repository_mirror.rb

ianballou · 2024-04-22T14:46:33Z

A Katello maintainers should get to reviewing this this week.

app/services/katello/pulp3/api/yum.rb

ianballou · 2024-04-25T16:14:41Z

@quba42 do you have a uln repo URL that I can test with?

quba42 · 2024-04-26T09:04:14Z

@ianballou If you actually want to sync a uln repo, you need an account with login credentials, but you can test how this PR works without actually syncing using a URL like uln://not_a_real_uln_url (and just putting something arbitrary for username/password). You can then switch a repo between the ULN URL and a normal repo URL and see how the Pulp remote is re-created as a remote of type ULN vs RPM. The same workflow without this PR would instead update the remote without re-creating it, which means you end up with an RPM remote with a ULN URL or vice versa in Pulp. This cannot work for syncing.

Thinking about this, it might be worth also adding some validation within Pulp that will throw an error if you try to add a URL starting with uln:// to an RPM remote, or one not starting with uln:// to a ULN remote. I think I will open an issue for this.

Edit: Complementary pulp_rpm issue: pulp/pulp_rpm#3520

ianballou

Overall this is working well! Just one more question, and I'd like input on my other comment from before about the args for get_remotes_api.

app/services/katello/pulp3/repository_mirror.rb

ianballou

Still working, left some code comments.

ianballou · 2024-04-29T20:53:51Z

test/services/katello/pulp3/repository/file/update_remote_test.rb

@@ -20,7 +20,7 @@ def setup
          @file_repo_service = @file_repo.backend_service(@mock_smart_proxy)
          @file_repo.root.update(url: 'my-files.org')
          @file_repo_service.stubs(:api).returns(@mock_api_wrapper)
-          @mock_api_wrapper.stubs(:remotes_api).returns(@mock_pulp3_api)
+          @mock_api_wrapper.stubs(:get_remotes_api).returns(@mock_pulp3_api)


Perhaps this entire method shouldn't be stubbed out so it gets tested? remotes_api should still be stub-able.

Maybe something like blah_repo_service.expects(:remotes_api).once.returns(@mock_pulp3_api)

I realize in this file only File repository types are tested. A new test for yum/uln could be in order :)

If I allow it to run get_remotes_api (revert this change), I get:

[vagrant@centos8-katello-devel-stable2 foreman]$ ktest ../katello/test/services/katello/pulp3/repository/file/update_remote_test.rb /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.7/lib/active_record/connection_adapters/postgresql_adapter.rb:883: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/pg-1.5.6/lib/pg/text_decoder/timestamp.rb:8: warning: The called method `initialize' is defined here Emptying /home/vagrant/foreman/jenkins/reports/unit -- execute("SET CONSTRAINTS ALL DEFERRED;") WARNING: SET CONSTRAINTS can only be used in transaction blocks -> 0.0009s # Running tests with run options --seed 65021: FF.Writing XML reports to /home/vagrant/foreman/jenkins/reports/unit Finished tests in 2.028559s, 1.4789 tests/s, 7.3944 assertions/s. Failure: Katello::Service::Repository::UpdateRemoteTest#test_feed_url_exists_and_remote_href_exists_updates_remote [/home/vagrant/katello/app/services/katello/pulp3/repository.rb:99] Minitest::Assertion: unexpected invocation: #<Mock:api_wrapper>.get_remotes_api(:href => "193874298udsfsdf") unsatisfied expectations: - expected exactly once, invoked never: #<Mock:pulp3_api>.partial_update(any_parameters) satisfied expectations: - allowed any number of times, invoked never: #<AnyInstance:Resolv::DNS>.getaddresses(any_parameters) - allowed any number of times, invoked never: #<AnyInstance:Resolv::DNS>.getaddress(any_parameters) - allowed any number of times, invoked never: #<AnyInstance:Resolv::DNS>.getnames(any_parameters) - allowed any number of times, invoked never: #<AnyInstance:Resolv::DNS>.getname(any_parameters) - allowed any number of times, invoked never: #<AnyInstance:Katello::Pulp3::Repository>.test_remote_name(any_parameters) - allowed any number of times, invoked never: Katello::Ping.ping(any_parameters) - allowed any number of times, invoked never: Cert::Certs.ssl_client_key(any_parameters) - allowed any number of times, invoked never: Cert::Certs.ssl_client_cert(any_parameters) - allowed any number of times, invoked never: Cert::Certs.ca_cert(any_parameters) - allowed any number of times, invoked never: #<Mock:response>.pulp_href(any_parameters) - allowed any number of times, invoked never: #<Mock:api_wrapper>.remotes_api(any_parameters) - allowed any number of times, invoked never: #<Mock:pulp3_api>.create(any_parameters) - allowed any number of times, invoked never: #<Mock:smart_proxy>.pulp_primary?(any_parameters) - allowed any number of times, invoked never: #<Mock:smart_proxy>.pulp2_preferred_for_type?(any_parameters) - allowed any number of times, invoked once: #<Mock:smart_proxy>.pulp3_support?(any_parameters) - allowed any number of times, invoked once: #<Katello::Pulp3::Repository::File:0x249c8>.api(any_parameters) Failure: Katello::Service::Repository::UpdateRemoteTest#test_feed_url_is_missing_but_remote_href_exists_deletes_remote [/home/vagrant/katello/app/services/katello/pulp3/service_common.rb:44] Minitest::Assertion: unexpected invocation: get_remotes_api({:href=>"193874298udsfsdf"}) 3 tests, 15 assertions, 2 failures, 0 errors, 0 skips Minitest Reporters: Mean Time Report (Samples: 4, Order: :avg :desc) Avg: 2.07335646 Min: 1.994977516 Max: 2.256206166 Last: 2.026355225 Description: Katello::Service::Repository::UpdateRemoteTest

Adding entirely new tests for additional content types would make a lot of sense.

I will put it on the agenda for our upcoming meeting.

- expected exactly once, invoked never: #<Mock:pulp3_api>.partial_update(any_parameters)

Is there a bit in your code that is expecting partial_update to run?

You should be able to set it up so at least you can run get_remotes_api and stub/expect the internal method calls instead.

When updating a remote, we need to re-create it instead if the type has changed from uln to rpm or vice versa. For operations on a remote href, the type is now determined from the href, rather than indirectly from the url. This is more robust since it is the actually relevant source of truth. In addition the remote API selection code is now deduplicated within the new get_remote_api method.

ianballou

I'm okay acking this since other tests are already creating remotes for yum content. If get_remotes_api didn't work, we'd have failures all over.

Thanks for the fix!

ianballou · 2024-04-30T20:26:05Z

@quba42 just let me know if you're ready for this to merge and I'll do so.

ianballou · 2024-05-07T17:59:06Z

Merging based on a conversation today where the tests around RPM remote creation will be bolstered outside this PR.

pr-processor bot added the Not yet reviewed label Mar 19, 2024

quba42 force-pushed the fix_switch_to_uln_remote branch 2 times, most recently from ae912ad to e7806f5 Compare March 19, 2024 15:07

quba42 commented Mar 19, 2024

View reviewed changes

m-bucher suggested changes Mar 20, 2024

View reviewed changes

app/services/katello/pulp3/api/core.rb Show resolved Hide resolved

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Mar 20, 2024

quba42 force-pushed the fix_switch_to_uln_remote branch from e7806f5 to f5e2097 Compare March 20, 2024 11:00

pr-processor bot added Needs re-review and removed Waiting on contributor labels Mar 20, 2024

quba42 force-pushed the fix_switch_to_uln_remote branch from f5e2097 to c076e35 Compare March 20, 2024 15:38

quba42 commented Mar 21, 2024

View reviewed changes

app/services/katello/pulp3/repository_mirror.rb Outdated Show resolved Hide resolved

quba42 force-pushed the fix_switch_to_uln_remote branch from c076e35 to 7914004 Compare March 21, 2024 10:37

m-bucher approved these changes Mar 22, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Mar 22, 2024

ianballou reviewed Apr 25, 2024

View reviewed changes

app/services/katello/pulp3/api/yum.rb Outdated Show resolved Hide resolved

ianballou reviewed Apr 26, 2024

View reviewed changes

app/services/katello/pulp3/repository_mirror.rb Outdated Show resolved Hide resolved

quba42 force-pushed the fix_switch_to_uln_remote branch from 7914004 to efde1e9 Compare April 29, 2024 14:27

quba42 requested a review from ianballou April 29, 2024 14:28

ianballou reviewed Apr 29, 2024

View reviewed changes

quba42 force-pushed the fix_switch_to_uln_remote branch from efde1e9 to cf7753a Compare April 30, 2024 07:40

ianballou approved these changes Apr 30, 2024

View reviewed changes

ianballou merged commit 97f8a09 into Katello:master May 7, 2024
22 of 23 checks passed

quba42 deleted the fix_switch_to_uln_remote branch May 8, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37279 - Re-create remotes when the type needs to change #10942

Fixes #37279 - Re-create remotes when the type needs to change #10942

quba42 commented Mar 19, 2024

quba42 Mar 19, 2024 •

edited

m-bucher left a comment

ianballou commented Apr 22, 2024

ianballou commented Apr 25, 2024

quba42 commented Apr 26, 2024 •

edited

ianballou left a comment

ianballou left a comment

ianballou Apr 29, 2024

quba42 Apr 30, 2024

quba42 Apr 30, 2024

ianballou Apr 30, 2024

ianballou Apr 30, 2024

ianballou left a comment

ianballou commented Apr 30, 2024

ianballou commented May 7, 2024

Fixes #37279 - Re-create remotes when the type needs to change #10942

Fixes #37279 - Re-create remotes when the type needs to change #10942

Conversation

quba42 commented Mar 19, 2024

quba42 Mar 19, 2024 • edited

Choose a reason for hiding this comment

m-bucher left a comment

Choose a reason for hiding this comment

ianballou commented Apr 22, 2024

ianballou commented Apr 25, 2024

quba42 commented Apr 26, 2024 • edited

ianballou left a comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

ianballou Apr 29, 2024

Choose a reason for hiding this comment

quba42 Apr 30, 2024

Choose a reason for hiding this comment

quba42 Apr 30, 2024

Choose a reason for hiding this comment

ianballou Apr 30, 2024

Choose a reason for hiding this comment

ianballou Apr 30, 2024

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

ianballou commented Apr 30, 2024

ianballou commented May 7, 2024

quba42 Mar 19, 2024 •

edited

quba42 commented Apr 26, 2024 •

edited