Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

chris1984 · 2024-08-15T22:06:45Z

What are the changes introduced in this pull request?

Changed the int4 column on convert2rhel_through_foreman from init4 to boolean
Return out of the fact parser method for hosts that are not registered such as foreman/satellite instance, so we don't error when those systems upload facts
Got rid of build_subscription_facet because the hosts that come from convert2rhel already are registered and have a subscription facet, so no need to build one which also breaks hosts that are not registered through subscription-manager
If for some reason we hit an edge case where we don't get the convert2rhel fact the first time during registration , we will get it when the client checks in again ~ 4 hours. Talking with Shim and Jeremy this seems like a fine approach since the systems rhsmcertd will send us the filtered fact again, which is more than enough time before rh_cloud would send off it's report to console.redhat.com

https://www.redhat.com/en/blog/converting-centos-rhel-convert2rhel-and-satellite

What are the testing steps for this pull request?

Apply PR
Run DB Migration
Try the following curl command on your devel box and make sure it uploads facts and does not return "error": {"message":"Content host must be unregistered before performing this action."}

curl -k -u admin:changeme -d '{"name":"<devboxfqdn>","facts":{"operatingsystem":"CentOS","operatingsystemrelease":"9.0"}}' -H 'Content-Type: application/json' https://localhost/api/v2/hosts/facts

With PR it should pass like this:

[vagrant@toledodevel ~]$ curl -k -u admin:changeme -d '{"name":"toledodevel.satellite.lab.eng.rdu2.redhat.com","facts":{"operatingsystem":"CentOS","operatingsystemrelease":"9.0"}}' -H 'Content-Type: application/json' https://localhost/api/v2/hosts/facts
{"id":1,"root_pass":null,"grub_pass":null,"lookup_value_matcher":"fqdn=toledodevel.satellite.lab.eng.rdu2.redhat.com","name":"toledodevel.satellite.lab.eng.rdu2.redhat.com","last_compile":null,"last_report":null,"updated_at":"2024-08-15T21:45:13.325Z","created_at":"2024-08-15T16:29:06.317Z","architecture_id":null,"operatingsystem_id":2,"ptable_id":null,"medium_id":null,"build":false,"comment":"","disk":null,"installed_at":null,"model_id":null,"hostgroup_id":null,"owner_id":null,"owner_type":null,"enabled":true,"puppet_ca_proxy_id":null,"managed":false,"use_image":null,"image_file":"","uuid":null,"compute_resource_id":null,"puppet_proxy_id":null,"certname":"toledodevel.satellite.lab.eng.rdu2.redhat.com","image_id":null,"organization_id":1,"location_id":2,"otp":null,"realm_id":null,"compute_profile_id":null,"provision_method":"build","global_status":0,"pxe_loader":null,"initiated_at":null,"build_errors":null,"creator_id":4,"reported_data_attributes":{"id":1,"host_id":1,"boot_time":"2024-08-15T16:23:06.000Z","created_at":"2024-08-15T16:29:09.971Z","updated_at":"2024-08-15T16:29:09.971Z","virtual":true,"sockets":4,"cores":4,"ram":24000,"disks_total":129922760192,"kernel_version":"5.14.0-452.el9.x86_64","bios_vendor":"VMware, Inc.","bios_release_date":"08/07/2020","bios_version":"VMW71.00V.16707776.B64.2008070230"},"infrastructure_facet_attributes":{"id":1,"host_id":1,"foreman_instance":false,"smart_proxy_id":1}}[vagrant@toledodevel ~]$
[vagrant@toledodevel ~]$

On a client that has subscription-manager installed create a custom fact like so

cat /etc/rhsm/facts/convert2rhel.facts {
"conversions.version": "1",
"conversions.activity": "conversion",
"conversions.packages.0.nevra": "convert2rhel-0:2.0.0-1.el8.noarch",
"conversions.packages.0.signature": "RSA/SHA256, Thu May 30 13:31:33 2024, Key ID 199e2f91fd431d51",
"conversions.executed": "/usr/bin/convert2rhel",
"conversions.success": true,
"conversions.activity_started": "2024-07-11T17:28:54.281664Z",
"conversions.activity_ended": "2024-07-11T17:48:47.026664Z",
"conversions.source_os.id": "Cerulean Leopard",
"conversions.source_os.name": "AlmaLinux",
"conversions.source_os.version": "8.10",
"conversions.target_os.id": "Ootpa",
"conversions.target_os.name": "Red Hat Enterprise Linux",
"conversions.target_os.version": "8.10",
"conversions.env.CONVERT2RHEL_THROUGH_FOREMAN": "1", <- what we care about
"conversions.run_id": "null"
}

Run subscription-manager facts --update and see if you see the fact in the hosts subscription_facet

[12] pry(main)> foo.subscription_facet
=> #<HostFacets::ReportedDataFacet:0x00007f6a30b9fb30
 id: 19,
 host_id: 18,
 boot_time: Thu, 11 Jul 2024 17:49:26.000000000 UTC +00:00,
 created_at: Mon, 29 Jul 2024 18:36:51.167876000 UTC +00:00,
 updated_at: Mon, 29 Jul 2024 19:30:23.632255000 UTC +00:00,
 virtual: true,
 sockets: 1,
 cores: 1,
 ram: 7953,
 disks_total: nil,
 kernel_version: "4.18.0-553.8.1.el8_10.x86_64",
 bios_vendor: nil,
 bios_release_date: nil,
 bios_version: nil,
 convert2rhel: 1>

* Changed the int4 column on convert2rhel_through_foreman from init4 to boolean * Return out of the fact parser method for hosts that are not registered such as foreman/satellite instance, so we don't error when those systems upload facts * Got rid of build_subscription_facet because the hosts that come from convert2rhel already are registered and have a subscription facet, so no need to build one which also breaks hosts that are not registered through subscription-manager If for some reason we hit an edge case where we don't get the convert2rhel fact the first time during registration , we will get it when the client checks in again ~ 4 hours. Talking with Shim and Jeremy this seems like a fine approach since the systems rhsmcertd will send us the filtered fact again, which is more than enough time before rh_cloud would send off it's report to console.redhat.com

evgeni · 2024-08-16T05:52:27Z

app/models/katello/host/subscription_facet.rb

        has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')
        # Add in custom convert2rhel fact if system was converted using convert2rhel through Katello
        # We want the value nil unless the custom fact is present otherwise we get a 0 in the database which if debugging
        # might make you think it was converted2rhel but not with satellite, that is why I have the tenary below.
-        facet = host.subscription_facet || host.build_subscription_facet


You use facet.save unless facet.new_record? below -- now new_record? can never be true, right?

But I also do not exactly understand the reasoning for the unless new_record? -- why didn't we want to store it in the past?

evgeni · 2024-08-16T07:56:57Z

app/models/katello/host/subscription_facet.rb

+        return unless host.subscription_facet # skip method if the host is not a registered host
        has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')


Would the following also work?

Suggested change

return unless host.subscription_facet # skip method if the host is not a registered host

has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')

has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')

return unless (host.subscription_facet || has_convert2rhel) # skip method if the host is not a registered host

That'd create the facet if the host has the special fact, and skip otherwise? (If we keep the build_sub_facet call below)

I tried that in #11110 and ran a pipeline based on the packit build -- it passed.

I think it's an interesting thing to think about. AFAIK populate_fields_from_facts is called for every fact source, so also Puppet. That makes me think the current code will result in unstable values if both RHSM and Puppet are used. Your code to skip if no fact exists is probably more reliable, but also means no mechanism exists to clear the value.

evgeni · 2024-08-16T08:00:19Z

db/migrate/20240815215102_change_convert2_rhel_to_boolean.rb

@@ -0,0 +1,9 @@
+class ChangeConvert2RhelToBoolean < ActiveRecord::Migration[6.1]
+  def up
+    change_column :katello_subscription_facets, :convert2rhel_through_foreman, :boolean, using: 'convert2rhel_through_foreman::boolean'


convert2rhel_through_foreman is a tenary (according to the comment above). It can be nil if the fact is not present, and 0/1 if the fact is.
Wouldn't converting it to bool loose that detail? Or can the column still be NULL?

https://api.rubyonrails.org/v7.2.0/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_column implies it's not specified in the resulting SQL so it's up to the DB implementation. Reading https://www.postgresql.org/docs/current/sql-altertable.html I get the impression the default is to allow NULL, which also makes sense: adding a column means you either need to provide a default value or allow NULL values.

ekohl · 2024-08-16T10:54:54Z

I had a closer look at the method and I think some comments in #11110 (review) also apply to the current code and your patch.

pr-processor bot added the Not yet reviewed label Aug 15, 2024

chris1984 force-pushed the fix-unreghost branch from e8508db to 4a41739 Compare August 15, 2024 22:57

evgeni reviewed Aug 16, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Aug 16, 2024

evgeni reviewed Aug 16, 2024

View reviewed changes

This was referenced Aug 16, 2024

Refs #37696 - only create sub-facet for c2r hosts #11110

Merged

Reapply "Fixes #37726 - Force fact upload to update OS facts" theforeman/foreman-installer#961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

chris1984 commented Aug 15, 2024 •

edited

Loading

evgeni Aug 16, 2024

evgeni Aug 16, 2024

evgeni Aug 16, 2024 •

edited

Loading

evgeni Aug 16, 2024

ekohl Aug 16, 2024

evgeni Aug 16, 2024

ekohl Aug 16, 2024

ekohl commented Aug 16, 2024

		return unless host.subscription_facet # skip method if the host is not a registered host
		has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

Are you sure you want to change the base?

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

Conversation

chris1984 commented Aug 15, 2024 • edited Loading

What are the changes introduced in this pull request?

What are the testing steps for this pull request?

evgeni Aug 16, 2024

Choose a reason for hiding this comment

evgeni Aug 16, 2024

Choose a reason for hiding this comment

evgeni Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

evgeni Aug 16, 2024

Choose a reason for hiding this comment

ekohl Aug 16, 2024

Choose a reason for hiding this comment

evgeni Aug 16, 2024

Choose a reason for hiding this comment

ekohl Aug 16, 2024

Choose a reason for hiding this comment

ekohl commented Aug 16, 2024

chris1984 commented Aug 15, 2024 •

edited

Loading

evgeni Aug 16, 2024 •

edited

Loading