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 #36826 - add new Host - Installed Products report for SCA hosts #9863

Merged

Conversation

ianballou
Copy link
Contributor

@ianballou ianballou commented Oct 13, 2023

Makes the old Subscriptions Entitlements Report work for SCA users. Only shows subscription information if there is any. Also adds system purpose statuses to the host output.

To test:

  1. Checkout the Katello PR: Fixes #36828 - add host products report jail methods Katello/katello#10769
  2. Create a report template called 'Host - Products and Subscriptions'
  3. Run the DB migration
  4. Check that the old template was backed up
  5. Register some hosts and generate the new report
  6. Switch an org or two to non-sca mode and regenerate it
    -> There should now be subscription data for the hosts in the report
  7. Ensure that the host filter input works

The above is out of date -- see my later comment for new information.

@ianballou
Copy link
Contributor Author

CC @sideangleside

@ianballou
Copy link
Contributor Author

I need to add the product number along with the name as well.

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 9026521 to f2cf84b Compare October 13, 2023 20:11
@ianballou
Copy link
Contributor Author

I realized users might miss the "days from now" input being gone... it's awkward to introduce into this new report, so I'm thinking that we might want to keep the old report around but rename it to say that it's for non-SCA orgs only.

@jeremylenz
Copy link
Contributor

I realized users might miss the "days from now" input being gone... it's awkward to introduce into this new report, so I'm thinking that we might want to keep the old report around but rename it to say that it's for non-SCA orgs only.

I agree. We also have an email notification that you can set up which emails you the Subscription - Entitlement Report filtered based on that Days from Now input, so I'm not sure how this would work if we delete the report?

I think the old Entitlement Report should stick around as-is and this new report should be SCA-only.

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from f2cf84b to 12b3185 Compare October 16, 2023 19:15
@ianballou
Copy link
Contributor Author

Changes I've made:

  • New report is called Host - Products. It shows host attributes and installed products.
  • The database migration now backs up any reports called Host - Products
  • The subscriptions entitlement report now:
    • Checks if a host's org isn't using SCA before reporting on it.
    • Reports system purpose usage and role

The subscriptions entitlement report will continue to not show anything for SCA hosts. The new host products report will report regardless of SCA status.

@ianballou ianballou changed the title Fixes #36826 - fix subs entitlement report for SCA Fixes #36826 - add new Host - Products report for non-SCA hosts Oct 16, 2023
@ianballou ianballou changed the title Fixes #36826 - add new Host - Products report for non-SCA hosts Fixes #36826 - add new Host - Products report for SCA hosts Oct 16, 2023
@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 12b3185 to 193b079 Compare October 16, 2023 19:24
version: 4.11.0
-%>
<%- report_headers 'Host Name', 'Organization', 'Lifecycle Environment', 'Content View', 'Host Collections', 'Virtual', 'Guest of Host', 'OS', 'Arch', 'Sockets', 'RAM', 'Cores', 'SLA', 'Role', 'Usage', 'Products', 'Last Checkin' -%>
<%- load_hosts(search: input('Hosts filter'), includes: [:operatingsystem, :architecture, :content_view_environments, :organization, :reported_data, :subscription_facet, :pools => [:subscription]]).each_record do |host| -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't need to load :pools => [:subscription] here anymore :)

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch 2 times, most recently from 7856620 to 92212fa Compare October 17, 2023 19:37
@ianballou
Copy link
Contributor Author

ianballou commented Oct 17, 2023

@jeremylenz I added in your suggestions

Edit -- I kept the simple content access check due to the reason in #9863 (comment)

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 92212fa to 1c570fa Compare October 17, 2023 19:51
@ianballou ianballou requested a review from jeremylenz October 17, 2023 19:53
@jeremylenz
Copy link
Contributor

[test katello]

@jeremylenz
Copy link
Contributor

Test failure looks related!

NameError: uninitialized constant IntroduceHostProductsReport

[2023-10-17T20:26:50.140Z] Did you mean?  IntroduceHostProductsAndSubscriptionsReport

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 1c570fa to c329a54 Compare October 17, 2023 20:51
@ianballou
Copy link
Contributor Author

Tests are passing now!

@@ -0,0 +1,40 @@
<%#
name: Host - Products
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like "Installed Products" is a phrase people are used to; we use it elsewhere including the new host details Details tab. Thoughts on calling it Host - Installed Products?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me 👍

Comment on lines 34 to 35
'Role': host.purpose_role_status_label,
'Usage': host.purpose_usage_status_label,
Copy link
Contributor

@jeremylenz jeremylenz Oct 18, 2023

Choose a reason for hiding this comment

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

image

This is recording the role status and usage status, which tell you if the host's assigned role and usage are "supported by a valid subscription."

Instead these columns should be simply "Role" and "Usage," and should match the values shown for the hosts on the new host details Overview tab / System Purpose card. I think the values you want are just purpose_role and purpose_usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that makes more sense, alright, I'll update the katello and foreman PRs.

Comment on lines 40 to 41
'Role': host.purpose_role_status_label,
'Usage': host.purpose_usage_status_label,
Copy link
Contributor

Choose a reason for hiding this comment

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

in light of my comments above, these should really be called "Role Status" and "Usage Status"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well if I change them to be purpose role and purpose usage, I think Role and Usage make sense again. I'll just remove the role and usage statuses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant

  • leave the statuses here, in the old Entitlement report, and call the columns 'Role Status' and 'Usage Status'; and
  • in the new Host Installed Products report, call the columns 'Role' and 'Usage' and have the actual values.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also have both (status and value) columns in the Entitlement Report if you want.

'OS': host.operatingsystem,
'Arch': host.architecture,
'Sockets': host.sockets,
'RAM': host.ram,
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the units here?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in MBs seems, at least according to the fact parser. That would make sense given your values there. I can make the top bit say "RAM (MB)"

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me! 👍

'SLA': host_sla(host),
'Role': host.purpose_role_status_label,
'Usage': host.purpose_usage_status_label,
'Products': host_products_names_and_ids(host),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Products': host_products_names_and_ids(host),
'Installed products': host_products_names_and_ids(host),

Comment on lines 34 to 35
'Role': host.purpose_role_status_label,
'Usage': host.purpose_usage_status_label,
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't on the list, but I think it'd be nice to also show the system's Release Version too.

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from c329a54 to 8f35c6f Compare October 18, 2023 21:17
@ianballou ianballou requested a review from jeremylenz October 19, 2023 21:28
@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 8f35c6f to 261324b Compare October 20, 2023 15:02
@ianballou
Copy link
Contributor Author

@jeremylenz I added the usage and role statuses back in.

@jeremylenz
Copy link
Contributor

Since the statuses are only useful for those using entitlement mode, I'd remove the status columns from the new Host Installed Products report, and only have them in the Entitlement Report. So:

  • Subscription - Entitlement: looking good, has both value and status columns
  • Host - Installed Products: remove the status columns, only have the value columns.

apologies again for confusions

@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 261324b to 03283b8 Compare October 20, 2023 15:25
@ianballou
Copy link
Contributor Author

@jeremylenz should actually be fixed now! :D

@jeremylenz
Copy link
Contributor

I spun it up for a final test and tried to run db:rollback, and got this:

PG::ForeignKeyViolation: ERROR:  update or delete on table "templates" violates foreign key constraint "templates_template_id_fk" on table "template_inputs"
DETAIL:  Key (id)=(185) is still referenced from table "template_inputs".
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `block (2 levels) in exec_no_cache'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies/interlock.rb:48:in `block in permit_concurrent_loads'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/share_lock.rb:187:in `yield_shares'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies/interlock.rb:47:in `permit_concurrent_loads'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:671:in `block in exec_no_cache'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/abstract_adapter.rb:696:in `block (2 levels) in log'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/abstract_adapter.rb:695:in `block in log'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/abstract_adapter.rb:687:in `log'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:670:in `exec_no_cache'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:649:in `execute_and_clear'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql/database_statements.rb:72:in `exec_delete'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/abstract/database_statements.rb:185:in `delete'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/abstract/query_cache.rb:22:in `delete'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/persistence.rb:395:in `_delete_record'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/persistence.rb:877:in `_delete_row'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/persistence.rb:523:in `delete'
/home/vagrant/foreman/db/migrate/20231016000000_introduce_host_products_report.rb:8:in `down'

Not sure how to address this, I guess something links the templates and template inputs? maybe .destroy is better than .delete so it can take care of the dependencies?

* Add system purpose role and usage to subscriptions entitlement report
@ianballou ianballou force-pushed the 36826-fix-subs-entitlement-report-sca branch from 03283b8 to 1b15841 Compare October 20, 2023 16:22
@ianballou
Copy link
Contributor Author

@jeremylenz destroy worked. I wonder if the other migrations that use the same technique also never worked :)

Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Thanks @ianballou !

APJ 👍

@jeremylenz
Copy link
Contributor

[test katello]

@ianballou
Copy link
Contributor Author

Waiting for tests to pass on Katello/katello#10769

@ianballou
Copy link
Contributor Author

[test katello]

@ianballou ianballou merged commit 1c1d5e2 into theforeman:develop Oct 24, 2023
6 checks passed
@ianballou ianballou deleted the 36826-fix-subs-entitlement-report-sca branch October 24, 2023 18:59
@jeremylenz jeremylenz changed the title Fixes #36826 - add new Host - Products report for SCA hosts Fixes #36826 - add new Host - Installed Products report for SCA hosts Oct 31, 2023
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.

2 participants