Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #28656 - Store DMI UUID on Subscription Facet #8489

Merged
merged 1 commit into from
Jan 9, 2020

Conversation

jturel
Copy link
Member

@jturel jturel commented Jan 7, 2020

Not so long ago the DMI UUID (reported by subman) of Content Hosts became critical to the registration process in order to block duplicate host registrations. The value is stored among the system facts, and looking up duplicates at registration time in a very large table can become slow as the number of hosts grow.

This PR helps alleviate the problem by making the DMI UUID a value on the SubscriptionFacet model. The below performance measures are taken from 1000 content hosts each with a distinct DMI UUID fact.

Old query performance

irb(main):002:0> Benchmark.bm do |x|
irb(main):003:1* x.report { 10000.times do; Katello::RegistrationManager.find_existing_hosts('flood0', SecureRandom.uuid); end }
irb(main):004:1> end
       user     system      total        real
   9.863411   0.338413  10.201824 ( 12.369747)
=> [#<Benchmark::Tms:0x000000000d5b16b8 @label="", @real=12.369746921118349, @cstime=0.0, @cutime=0.0, @stime=0.33841299999999985, @utime=9.863411000000001, @total=10.201824>]

New query performance:

irb(main):004:0> Benchmark.bm do |x|
irb(main):005:1* x.report { 10000.times do; Katello::RegistrationManager.find_existing_hosts('flood0', SecureRandom.uuid); end }
irb(main):006:1> end
       user     system      total        real
   1.884269   0.014484   1.898753 (  1.904810)

Looks like the duplicate lookup is 5x faster.

To test

  • checkout PR

  • migrate the DB

  • run the rake task to populate subscription facet: bundle exec rake katello:upgrades:3.15:set_sub_facet_dmi_uuid

  • Registrations should exhibit all of the same behaviors w.r.t preventing duplicate hosts by DMI UUID.

  • Changing the DMI UUID from the client side via fact override should also update the subscription facet field

@theforeman-bot
Copy link

Issues: #28656

@@ -96,16 +94,6 @@ def test_existing_host_null_uuid

assert @klass.validate_hosts(hosts, @org, @host.name, 'different-uuid')
end

def test_existing_uuid_mismatch_unregistered
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is irrelevant b/c facts are now destroyed during unregister

@@ -297,7 +298,7 @@ def remove_host_artifacts(host, clear_content_facet = true)
host.get_status(::Katello::TraceStatus).destroy
host.installed_packages.delete_all

host.rhsm_fact_values.destroy_all
host.rhsm_fact_values.delete_all
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind reverting this, but thought it was simple enough to include. Changed to delete_all since we aren't concerned with the values that are destroyed

@parthaa
Copy link
Contributor

parthaa commented Jan 8, 2020

Couple of things

@jturel
Copy link
Member Author

jturel commented Jan 8, 2020

Couple of things

  • I am not sure why you chose a rake task to upgrade instead of seeding from the migration automatically? This would make sense if I am talking to a service like candlepin, but this is purely copying an attribute right?

Fair point but I want to find some examples to make sure that's what we typically do before moving it to the migration

+1 (if we don't move to the migration)

@parthaa
Copy link
Contributor

parthaa commented Jan 9, 2020

Fair point but I want to find some examples to make sure that's what we typically do before moving it to the migration

https://github.com/Katello/katello/blob/master/db/migrate/20150930183738_migrate_content_hosts.rb

@parthaa
Copy link
Contributor

parthaa commented Jan 9, 2020

Sounds like separate rake task is more beneficial longer term. Just add a test for that rake task

@jturel
Copy link
Member Author

jturel commented Jan 9, 2020

@parthaa updated!

Copy link
Contributor

@parthaa parthaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the performance improvements, almost 8 times faster for me. ACK

@jturel
Copy link
Member Author

jturel commented Jan 9, 2020

[test katello]

@jturel jturel merged commit 7eb48bf into Katello:master Jan 9, 2020
@jturel jturel deleted the register_performance branch January 9, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants