Fixes #25921 Sync Capsules before applying errata at incremental update

[pmoravec]

Identify smart proxies the Hosts are registered through, and invoke Capsule
synchronization of the updated content before applying errata to those Hosts.

This will make some of the automatically triggered Capsule synchronizations
redundant, but such no-op is harmless and preventing it could be error-prone.

Fixes: #25921

Signed-off-by: Pavel Moravec pmoravec@redhat.com

[theforeman-bot]

Issues: #25921 #25921

[theforeman-bot]

There were the following issues with the commit message:

• 2ae6fea must be in the format fixes #redmine_number - brief description
• length of the first commit message line for 2ae6fea exceeds 65 characters
• commit message for 2ae6fea is not wrapped at 72nd column
• commit message for 2ae6fea is not wrapped at 72nd column
• commit message for 2ae6fea is not wrapped at 72nd column
• commit message for 2ae6fea is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[theforeman-bot]

There were the following issues with the commit message:

• 04c47ab must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[pmoravec]

I think the test if ::Katello::CapsuleContent.sync_needed?(env) can be redundant as if no sync is needed for the environment, ::Katello::CapsuleContent.with_environment(env) will be empty and if capsules.any? won't trigger the Capsule sync against empty Capsules.

But a confirmation from somebody else would be appreciated.

[pmoravec]

Current failure is some CI issue (postgres database "test-katello-pr-test-4510-production" does not exist), other tests passed.

[jlsherrill]

[test katello]

[bkearney]

@jlsherrill bump on this, I am looking to close out some older bugs.

[pmoravec]

Rebased to current master, comments incorporated. I am not fully happy with finding nondefault proxies based on their name:

smart_proxies = SmartProxy.where(:name => smart_proxies).where.not(:id => SmartProxy.default_capsule!.id)

that might have a simplier solution with just one where condition - but I havent figured it out how..

[jturel]

[test katello]

[jturel]

@pmoravec ping :)

[jturel]

Here's some rough draft of what tests I think we should have in test/actions/katello/content_view_test.rb#IncrementalUpdatesTest

   it 'plans capsule syncs when needed' do
      Dynflow::Testing::DummyPlannedAction.any_instance.stubs(:new_content_view_version).returns(::Katello::ContentViewVersion.first)

      plan_action(action, [{:content_view_version => content_view.version(library), :environments => [library]}], [],
                  {:errata_ids => ["FOO"]}, true, [], "BadDescription")

      expected_capsules = []
      expected_cv_id = nil
      expected_env_id = nil

      assert_action_planed_with(action, ::Actions::BulkAction, expected_capsules, content_view_id: expected_cv_id, environment_id: expected_env_id)
    end

    it 'plans errata install when needed' do

    end

[pmoravec]

Updated to the smart_proxies.with_environment(env) (I usually prepare and test a patch on older katello that might divert from upstream, so some my constructions are not up-to-date :) ).

For the tests: good point though I have no experience in writing or testing them. I would need to spend more time I dont have now, so I just applied the first improvement.

[pmoravec]

About tests where I am pure newbie and write code without testing it: here is what the two test cases shall look like (I bet there are multiple issues so havent re-spin the PR):

--- test/actions/katello/content_view_test.rb	2019-04-05 08:41:09.775789157 +0200
+++ content_view_test.rb	2019-05-30 13:17:29.095426303 +0200
@@ -353,6 +353,48 @@ module ::Actions::Katello::ContentView
       end
     end
 
+    it 'plans capsule syncs when needed' do
+      Dynflow::Testing::DummyPlannedAction.any_instance.stubs(:new_content_view_version).returns(::Katello::ContentViewVersion.first)
+
+      smart_proxy_service_1 = new_capsule_content(:five)
+      smart_proxy_service_2 = new_capsule_content(:six)
+      smart_proxy_service_1.smart_proxy.add_lifecycle_environment(library)  # just test one proxy will get the content
+
+      plan_action(action, [{:content_view_version => content_view.version(library), :environments => [library]}], [],
+                  {:errata_ids => ["FOO"]}, true, [], "BadDescription")
+
+      assert_action_planned_with(action, ::Actions::BulkAction, ::Actions::Katello::CapsuleContent::Sync,
+                                 [smart_proxy_service_1.smart_proxy],
+                                 :content_view_id => content_view.id, :environment_id => library.id)
+    end
+
+    it 'plans errata install when needed' do
+      Dynflow::Testing::DummyPlannedAction.any_instance.stubs(:new_content_view_version).returns(::Katello::ContentViewVersion.first)
+
+      proxy = FactoryBot.build(:smart_proxy, :url => 'http://fakepath.com/foo')
+      smart_proxy_service = capsule_content(:seven)
+      smart_proxy_service.smart_proxy.add_lifecycle_environment(library)
+      proxy.capsule_content = smart_proxy_service    # ??? how to pair proxy to capsule content?
+      proxy.save!
+
+      errata = katello_errata("bugfix")
+
+      host = FactoryBot.create(:host, :with_content, :content_view => @view,
+                                          :lifecycle_environment => @library)
+      host.content_facet.content_source = proxy
+      host.content_facet.applicable_errata << errata
+      host.save!
+
+      plan_action(action, [{:content_view_version => content_view.version(library), :environments => [library]}], [],
+                  {:errata_ids => [errata.errata_id]}, true, [], "BadDescription")
+
+      assert_action_planned_with(action, ::Actions::BulkAction, Actions::Katello::Host::Erratum::ApplicableErrataInstall, [host], [errata.errata_id]).returns({})  # not sure with the "returns", it was copied from elsewhere
+
+      put :install_content, params: { :included => {:ids => [@host1.id]}, :organization_id => @org.id, :content_type => 'errata', :content => [errata.errata_id] } # ??? copied from errata apply test, not sure if applicable - but we need to verify the errata will be installed on a host registered to the Capsule
+      assert_response :success # ???
+
+    end
+
     it 'generates correct message' do
       data = {:version_outputs =>
                [{:version_id => 1,

[jturel]

I'll take a shot at the tests so we can get this contribution merged in. Will get back in a day or two!

[jturel]

@pmoravec I added some test on my own copy of this PR with some additional tweaks to the code. With this, everything should be good to go. Feel free to merge it to your branch and review that it matches your expectations as well :)

https://github.com/jturel/katello/tree/pr7946

[theforeman-bot]

There were the following issues with the commit message:

• be1ae27 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[pmoravec]

Thanks for collaboration.

I merged the 3 commits into one and rebased the PR.

[jturel]

@jlsherrill you've looked at this PR in the past - would you mind giving it a once-over for the approach taken since pavel and I have worked together on it a bit?

[jlsherrill]

Code review looks good to me!

[jturel]

@pmoravec I was giving this a final test-through and discovered that the query to look up the proxies which I pointed you to will not work as intended. I'll spare the details unless you want me to explain.

@jlsherrill suggested to me that we don't try to isolate which proxies need syncing by which hosts are registered through them, but rather to sync all the proxies which have the affected environment and then run the applicable errata install as before. It's possible we'll sync a few extra proxies, but we'll still guarantee that the affected hosts can install those errata.

It also occurred to us that this change could greatly increase the time it takes to perform the incremental update since the capsule sync becomes synchronous to overall execution as it's written now. We think it's a good idea to effectively mirror what is being done during CV promote in order to run capsule sync and applicable errata install asynchronous to the incremental update Example code is here:

katello/app/lib/actions/katello/content_view/promote_to_environment.rb

Lines 57 to 60 in 3329a4f

	if ::SmartProxy.sync_needed?(environment) && Setting[:foreman_proxy_content_auto_sync]
	ForemanTasks.async_task(ContentView::CapsuleSync,
	::Katello::ContentView.find(input[:content_view_id]),
	environment)

I think we could create a new action to handle this async behavior such as Actions::Katello::ContentView::SyncAndInstallErrata (just a suggested name)

I've emphasized in bold the main ideas of the changes suggested here - let me know if I've explained them well and if you agree with them. If you're not sure how to approach making the changes I can offer more guidance as needed :)

[pmoravec]

I agree with the proposal of changes. Due to the holiday time and some other commitment, I can get to this in a month manner the soonest.

[bkearney]

@pmoravec @jturel what is the status of this pr?

[jturel]

This needs a rebase and I believe there was some feedback pending code changes

[jturel]

@pmoravec any chance you'll be able to get back to this? Do you need us to take it over? (I see there's a BZ for it)

[pmoravec]

Sorry, no time to rework the patch :( A take over would be appreciated.

[jturel]

Thanks Pavel I'll work on it this sprint

[jturel]

Closing this in favor of #8719 - thanks Pavel!