Fixes #25415 - import Hypervisor facts from Candlepin #7821

evgeni · 2018-11-08T11:15:48Z

No description provided.

theforeman-bot · 2018-11-08T11:15:56Z

evgeni · 2018-11-08T11:26:19Z

app/lib/katello/resources/candlepin/consumer.rb

@@ -35,6 +35,15 @@ def get_all(uuids)
            consumers
          end

+          # workaround for https://bugzilla.redhat.com/1647724


this might have a heavy performance impact, as it queries each hypervisor directly, do we have tests that cover this?

the number of hyprevisors won't be too big I think and it also runs asynchronously... we should ask QE for explicit test if we're in doubt, but this should be good until the candlepin provides data on /consumers endpoint

We're currently batching the requests 75 at a time, so if we have 15k hypervisors (and users do), the old method makes 200 HTTPS requests, the new one 15000. I'll run a few tests on my local box to have a few before/after numbers.

with 500 hypervisors, 50 guests each

before patch: 0(run)+110(finalize) sec new, 0(run)+20(finalize) sec update
after patch: 180(run)+200(finalize) sec new, 75(run)+175(finalize) sec update

I'll see how we can speed that up :)

@lzap @jlsherrill if you have ideas, please :)

My idea is to immediately add telemetry to this file to get some real-world numbers, then we can talk about poking the codebase. It's super easy the very same thing is done in this plugin: https://github.com/theforeman/foreman_discovery/pull/408/files#diff-45af6b2f1c078550eba223b206b16cb4

@evgeni yeah, we used to query each host individually and recently changed to not do that to speed things up. I'm not surprised this slows it down quite a bit.

If we are okay with facts being imported eventually (but not necessarily immediately), one option would be to use our 'event_queue' to import them. This would involve just throwing the host id on the queue, and then it would get processed in the background and would eventually be imported.

evgeni · 2018-11-08T15:39:43Z

[test katello]

ares · 2018-11-09T12:20:43Z

I tested with local libvirt and got a duplicate host. Otherwise it works great.

More details: I already had server ibm-x3655-03...com on which I run foreman+katello+libvirt, I configured virt-who on the same machine and restarted virt-who service. A new host `virt-who-ibm-x3655-03...com-1 appeared. I see facts correctly set. But given the history of duplicated hosts, we should try to map it correctly to existing hosts I think as we always have the Foreman host available in DB after installation.

ares · 2018-11-09T13:06:41Z

Sorry got confused, this only adds facts fetching (which works), the duplicity issue is already there.

evgeni · 2018-11-16T14:01:37Z

Not sure why that one test is failing now :/

evgeni · 2018-11-23T10:52:11Z

So, I played a bit more with this. And the slowdown does not come from this PR at all.

It's from bd456f150c00ab782b38f663af9cd6e3880c9a7e, where we started to correctly load data from Candlepin. Before that commit, HypervisorsUpdate.update_subscription_facet was mostly a NOOP, as @candlepin_attributes.key?(uuid) was always false.

katello/app/lib/actions/katello/host/hypervisors_update.rb

Lines 106 to 115 in a0e0f89

    
           def update_subscription_facet(uuid, host) 
        
             host.subscription_facet ||= host.build_subscription_facet(uuid: uuid) 
        
             if @candlepin_attributes.key?(uuid) 
        
               host.subscription_facet.candlepin_consumer.consumer_attributes = @candlepin_attributes[uuid] 
        
               host.subscription_facet.import_database_attributes 
        
               host.subscription_facet.save! 
        
               host.subscription_facet.update_subscription_status(@candlepin_attributes[uuid].try(:[], :entitlementStatus)) 
        
             end 
        
             host.save! 
        
           end

So, to revisit my numbers:

without facts

load_resources: 3-5 sec
update_subscription_facet: 3 min
update_facts: not run

with facts

load_resources: 20-25 sec
update_subscription_facet: 3 min
update_facts: 45 sec

With that said, I would say I am mostly happy with the performance, as I knew it will have an impact.

Still need to figure out why that one test fails.

evgeni · 2018-11-26T09:04:09Z

The test failure is due to https://projects.theforeman.org/issues/25546 and by that unrelated.

evgeni · 2018-11-29T20:29:43Z

[test katello]

evgeni · 2018-11-29T21:34:59Z

Hah. 💚 tests

@jlsherrill if you could have another look, that'd be awesome :)

evgeni

inline comment

evgeni · 2018-12-09T20:28:18Z

app/lib/katello/resources/candlepin/consumer.rb

+          def get_all_with_facts(uuids)
+            consumers = []
+            uuids.each do |uuid|
+              consumers << get(uuid)


This probably can be better written as

uuids.collect { |uuid| get(uuid) }

jlsherrill · 2018-12-11T01:47:21Z

app/lib/actions/katello/host/hypervisors_update.rb

+              @hosts.each do |uuid, host|
+                update_subscription_facet(uuid, host)
+              end
+            end


out of curiosity, why did you move this to the run phase and put it in a transaction ?

The FactImporter needs to run outside of a transaction, so I moved the code to the run phase. But I also wanted the rest of the code to run in transaction, so I wrapped it with one. Should I add a comment with this explanation?

jlsherrill · 2018-12-11T02:17:01Z

Testing this against master, i saw these timings with ~450 hypervisors:

initial load 172s -> 266s
secondary runs: 80s ->141s

This seems like a lot to me, although it seems to conflict with your findings? I can send you some user provided json with these 450 hypervisors if you want to try yourself.

If this performance decrease is accurate, i'd suggest one of a few things:

We push to get https://bugzilla.redhat.com/1647724 fixed sooner (although that would only account for part of the decrease in performance)
We make this optional, so those with a large number of hypervisors can opt-out and choose performance over this functionality
we delegate this to the event queue. This would mean these hypervisor facts are imported asynchronously in the background

evgeni · 2018-12-12T17:14:20Z

Just re-run this on a fresh VM with 500 hypervisors (2 guests each):

without patch:
new: Actions::Katello::Host::HypervisorsUpdate (success) [ 204.57s / 204.57s ]
update: Actions::Katello::Host::HypervisorsUpdate (success) [ 90.58s / 90.58s ]

with patch:
new: Actions::Katello::Host::HypervisorsUpdate (success) [ 325.19s / 325.19s ]
update: Actions::Katello::Host::HypervisorsUpdate (success) [ 180.20s / 180.20s ]

So not far from what you've seen (and I'd say also not too different to the previous numbers, which were in the 60-90sec increase ballpark, even tho the "new data" increase is more 120sec here).

Fixing https://bugzilla.redhat.com/1647724 would "only" make the data collection faster, saving us roughly 20-30 seconds on each run (the collection is identical for new vs updated data)
This task already runs async to the virt-who checkin, so we're not blocking anyone. Would switching to the event queue have any further benefits?
Making this configurable (but I'd prefer on by default) sounds good.

jlsherrill · 2018-12-12T20:40:36Z

@evgeni i think my bigger concern was around db locking and increasing that time. (although the transaction is only be increased by however long it takes to fetch the facts, but not store them).

I wonder if we should just move the entire thing outside of a transaction? Since its written to be idempotent (and can be re-run at any time), i think that would be better? Curious your thoughts.

evgeni · 2018-12-13T07:55:20Z

@evgeni i think my bigger concern was around db locking and increasing that time. (although the transaction is only be increased by however long it takes to fetch the facts, but not store them).

Yeah, I can see this being a concern. We could break that up into two transactions? One for load_resources, one for update_subscription_facet?

I wonder if we should just move the entire thing outside of a transaction? Since its written to be idempotent (and can be re-run at any time), i think that would be better? Curious your thoughts.

I didn't dare to touch this aspect yet. One of the "hidden" "gems" of this task is that load_resources will actually also create new Host resources if they were missing, and I think this should not happen outside a transaction. Updating the SubscriptionFacet and the Facts is probably fine outside, but again, I only have a very high level understanding of how this all works and what we will break.

jlsherrill · 2018-12-17T15:04:09Z

Yeah, I can see this being a concern. We could break that up into two transactions? One for load_resources, one for update_subscription_facet?

Yeah, I think that makes sense!

I didn't dare to touch this aspect yet.

fair enough, i may file an issue and try to tackle this soon in some manner.

evgeni · 2018-12-17T15:45:48Z

@jlsherrill updated with two transactions

jlsherrill · 2019-01-08T20:50:33Z

[test katello]

johnpmitsch · 2019-01-08T21:40:16Z

@evgeni I was able to get facts importing from a virt-who hypervisor with this PR 👍

irb(main):021:0> Host.find(8).facts
=> {"hypervisor::type"=>"QEMU", "cpu::cpu_socket(s)"=>"1", "hypervisor::version"=>"2010001", "_timestamp"=>"2019-01-08 21:03:53 +0000", "hypervisor"=>nil, "cpu"=>nil}

But when I try to get the facts from the API, it still returns null. I couldn't find why this is happening for just the virt-who hypervisors vs. other content hosts. Let me know if you see the same, it would be helpful to use fo this bug

[vagrant@coffee foreman{develop}]$ curl -g -k -u admin:changeme -H "Content-Type: application/json"  https://coffee.jomitsch.example.com/api/v2/hosts/8 | jq '.facts'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3162    0  3162    0     0  10903      0 --:--:-- --:--:-- --:--:-- 10941
null

Also, should hypervisor have a value for the facts hash?

evgeni · 2019-01-09T11:25:17Z

@johnpmitsch I've seen that, it seems to me that it will return fine on /hosts/:host_id/facts but not in /hosts/:host_id and I have no idea why (the templates don't look like they would exclude it or anything).

And no hypervisor does not have a value, it's a dummy fact to allow subfacts :)

johnpmitsch

This worked well for me and code looks good. My other comment seems to be a valid issue, but seems pre-existing and not caused by this PR, so I think we can fix that separately

I haven't tested or evaluated any of the scaling performance, I'll leave it up to others who have been in those conversations to give their approval.

beav · 2019-01-09T22:20:35Z

we discussed this PR during grooming. The performance impact is OK

thanks @evgeni !

evgeni · 2019-01-10T08:21:57Z

💚 thanks everyone for reviewing, helping, discussing! 🎉

theforeman-bot added Needs testing Not yet reviewed labels Nov 8, 2018

evgeni commented Nov 8, 2018

View reviewed changes

evgeni force-pushed the issue25415 branch 2 times, most recently from e2ba5e7 to 3720367 Compare November 8, 2018 15:05

evgeni force-pushed the issue25415 branch from 3720367 to beb5166 Compare November 8, 2018 17:03

evgeni force-pushed the issue25415 branch 2 times, most recently from 49cd832 to b2863a7 Compare November 16, 2018 11:42

evgeni force-pushed the issue25415 branch 4 times, most recently from 8d9a8a0 to c7795ae Compare November 29, 2018 19:34

evgeni force-pushed the issue25415 branch from c7795ae to acfd7cf Compare December 8, 2018 19:02

evgeni commented Dec 9, 2018

View reviewed changes

evgeni mentioned this pull request Dec 10, 2018

implement a FactAnalyzer to determine the host type RedHatSatellite/katello-attach-subscription#30

Merged

jlsherrill reviewed Dec 11, 2018

View reviewed changes

Fixes #25415 - import Hypervisor facts from Candlepin

7776b26

evgeni force-pushed the issue25415 branch from acfd7cf to 7776b26 Compare December 17, 2018 15:43

johnpmitsch approved these changes Jan 9, 2019

View reviewed changes

This was referenced Jan 9, 2019

Fixes #25819 - Show facts for single host API call theforeman/foreman#6398

Closed

Fixes #25818 - Show virt-who hypervisor type #7916

Merged

beav merged commit 81530a0 into Katello:master Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #25415 - import Hypervisor facts from Candlepin #7821

Fixes #25415 - import Hypervisor facts from Candlepin #7821

evgeni commented Nov 8, 2018

theforeman-bot commented Nov 8, 2018

evgeni Nov 8, 2018

ares Nov 9, 2018

evgeni Nov 9, 2018

evgeni Nov 9, 2018

evgeni Nov 9, 2018

lzap Nov 9, 2018

jlsherrill Nov 9, 2018

evgeni commented Nov 8, 2018

ares commented Nov 9, 2018

ares commented Nov 9, 2018

evgeni commented Nov 16, 2018

evgeni commented Nov 23, 2018

evgeni commented Nov 26, 2018

evgeni commented Nov 29, 2018

evgeni commented Nov 29, 2018

evgeni left a comment

evgeni Dec 9, 2018

jlsherrill Dec 11, 2018

evgeni Dec 11, 2018

jlsherrill commented Dec 11, 2018

evgeni commented Dec 12, 2018 •

edited

Loading

jlsherrill commented Dec 12, 2018

evgeni commented Dec 13, 2018

jlsherrill commented Dec 17, 2018

evgeni commented Dec 17, 2018

jlsherrill commented Jan 8, 2019

johnpmitsch commented Jan 8, 2019 •

edited

Loading

evgeni commented Jan 9, 2019

johnpmitsch left a comment

beav commented Jan 9, 2019

evgeni commented Jan 10, 2019

Fixes #25415 - import Hypervisor facts from Candlepin #7821

Fixes #25415 - import Hypervisor facts from Candlepin #7821

Conversation

evgeni commented Nov 8, 2018

theforeman-bot commented Nov 8, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented Nov 8, 2018

ares commented Nov 9, 2018

ares commented Nov 9, 2018

evgeni commented Nov 16, 2018

evgeni commented Nov 23, 2018

without facts

with facts

evgeni commented Nov 26, 2018

evgeni commented Nov 29, 2018

evgeni commented Nov 29, 2018

evgeni 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

jlsherrill commented Dec 11, 2018

evgeni commented Dec 12, 2018 • edited Loading

jlsherrill commented Dec 12, 2018

evgeni commented Dec 13, 2018

jlsherrill commented Dec 17, 2018

evgeni commented Dec 17, 2018

jlsherrill commented Jan 8, 2019

johnpmitsch commented Jan 8, 2019 • edited Loading

evgeni commented Jan 9, 2019

johnpmitsch left a comment

Choose a reason for hiding this comment

beav commented Jan 9, 2019

evgeni commented Jan 10, 2019

evgeni commented Dec 12, 2018 •

edited

Loading

johnpmitsch commented Jan 8, 2019 •

edited

Loading