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

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

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Aug 16, 2024

What are the changes introduced in this pull request?

We do have hosts that have no subscription facet, and should not have
one. Adjust the logic for the c2r fact to only create the missing facet
if the fact was sent, and leave hosts w/o the facet and w/o the fact
alone.

Considerations taken when implementing this change?

This is an alternative to #11109

What are the testing steps for this pull request?

Same as #11109

We do have hosts that have no subscription facet, and should not have
one. Adjust the logic for the c2r fact to only create the missing facet
if the fact was sent, and leave hosts w/o the facet and w/o the fact
alone.
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

If you apply this, you can simplify the code below. Simply make it:

convert2rhel_through_foreman: ::Foreman::Cast.to_bool(parser.facts['conversions.env.CONVERT2RHEL_THROUGH_FOREMAN'])

But another approach:

convert2rhel = parser.facts['conversions.env.CONVERT2RHEL_THROUGH_FOREMAN']
return if convert2rhel.nil?

facet = host.subscription_facet
return unless facet

facet.attributes = {
  convert2rhel_through_foreman: ::Foreman::Cast.to_bool(convert2rhel),
}
facet.save

Another thing to notice here: does this actually wipe all other attributes that could be there? Are there other attributes?

@evgeni
Copy link
Member Author

evgeni commented Aug 16, 2024

If you apply this, you can simplify the code below. Simply make it:

convert2rhel_through_foreman: ::Foreman::Cast.to_bool(parser.facts['conversions.env.CONVERT2RHEL_THROUGH_FOREMAN'])

No your can't.
It still can be nil for hosts that do have a subscription facet!

Another thing to notice here: does this actually wipe all other attributes that could be there? Are there other attributes?

Quite unrelated to this PR.
It does (and I have no idea if there are others)

@ekohl
Copy link
Member

ekohl commented Aug 16, 2024

It still can be nil for hosts that do have a subscription facet!

You already have a return unless has_convert2rhel so it's guaranteed to be true.

@evgeni
Copy link
Member Author

evgeni commented Aug 16, 2024

It still can be nil for hosts that do have a subscription facet!

You already have a return unless has_convert2rhel so it's guaranteed to be true.

No. I have unless host.subscription_facet || has_convert2rhel :)

@ekohl
Copy link
Member

ekohl commented Aug 16, 2024

You've convinced me that it's lunch time for me.

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @evgeni

Test results:

Curl of localhost:

[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}}

Host that has custom facts uploading the fact:

08:48:22 rails.1   | 2024-08-16T08:48:22 [D|app|9b31cfe0] Updating Candlepin associations for host toledo8.croberts.io
08:48:22 rails.1   | 2024-08-16T08:48:22 [D|kat|9b31cfe0] Resource PUT request: /candlepin/consumers/e0a027ba-e63e-45fb-b0fc-e7651c1cdfe8
08:48:22 rails.1   | 2024-08-16T08:48:22 [D|kat|9b31cfe0] Headers: {"accept":"application/json","accept-language":"en","content-type":"application/json","X-Correlation-ID":"9b31cfe0-c672-4721-9ff3-1aa0ab4d232d","cp-user":"foreman_admin"}
08:48:22 rails.1   | 2024-08-16T08:48:22 [D|kat|9b31cfe0] Body: "{\"facts\":{\"system.certificate_version\":\"3.2\",\"virt.is_guest\":true,\"virt.host_type\":\"vmware\",\"virt.uuid\":\"6D680D42-BD19-7622-0A94-15F0721F2EBC\",\"dmi.bios.vendor\":\"Phoenix 2620:52:0:868:250:56ff:fe8d:a0c2\",\"net.interface.lo.ipv6_address.host\":\"::1\",\"net.interface.lo.ipv6_address.host_list\":\"::1\",\"net.interface.lo.ipv6_netmask.host\":\"128\",\"net.interface.lo.ipv6_netmask.host_list\":\"128\",\"net.interface.lo.ipv4_address\":\"127.0.0.1\",\"net.interface.lo.ipv4_address_list\":\"127.0.0.1\",\"net.interface.lo.ipv4_netmask\":\"8\",\"net.interface.lo.ipv4_netmask_list\":\"8\",\"net.interface.lo.ipv4_broadcast\":\"Unknown\",\"net.interface.lo.ipv4_broadcast_list\":\"Unknown\",\"net.interface.ens192.mac_address\":\"00:50:56:8d:a0:c2\",\"net.interface.ens192.ipv6_address.global\":\"2620:52:0:868:250:56ff:fe8d:a0c2\",\"net.interface.ens192.ipv6_address.global_list\":\"2620:52:0:868:250:56ff:fe8d:a0c2\",\"net.interface.ens192.ipv6_address.link\":\"fe80::250:56ff:fe8d:a0c2\",\"net.interface.ens192.ipv6_address.link_list\":\"fe80::250:56ff:fe8d:a0c2\",\"net.interface.ens192.ipv6_netmask.global\":\"64\",\"net.interface.ens192.ipv6_netmask.global_list\":\"64\",\"net.interface.ens192.ipv6_netmask.link\":\"64\",\"net.interface.ens192.ipv6_netmask.link_list\":\"64\",\"net.interface.ens192.ipv4_address\":\"10.8.104.105\",\"net.interface.ens192.ipv4_address_list\":\"10.8.104.105\",\"net.interface.ens192.ipv4_netmask\":\"24\",\"net.interface.ens192.ipv4_netmask_list\":\"24\",\"net.interface.ens192.ipv4_broadcast\":\"10.8.104.255\",\"net.interface.ens192.ipv4_broadcast_list\":\"10.8.104.255\",\"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\",\"conversions.run_id\":\"null\",\"kpatch.installed\":\"\",\"kpatch.loaded\":\"\"}}"

Host subscription facet showing the correct value(should be a 1 but my migration is applied from my other pr)

[1] pry(main)> Host.find(2).subscription_facet
=> #<Katello::Host::SubscriptionFacet:0x00007f6b25bb59a8
 id: 1,
 host_id: 2,
 uuid: "e0a027ba-e63e-45fb-b0fc-e7651c1cdfe8",
 last_checkin: Thu, 15 Aug 2024 21:47:16.225438000 UTC +00:00,
 service_level: "",
 release_version: nil,
 autoheal: true,
 registered_at: Thu, 15 Aug 2024 21:47:06.000000000 UTC +00:00,
 registered_through: "toledodevel.satellite.lab.eng.rdu2.redhat.com",
 user_id: 4,
 hypervisor: false,
 hypervisor_host_id: nil,
 purpose_usage: "",
 purpose_role: "",
 dmi_uuid: "6D680D42-BD19-7622-0A94-15F0721F2EBC",
 convert2rhel_through_foreman: true>

I will follow up and make a pr to switch the database migration to boolean(we can see from my output that my migration is working since it's still in the system from my testing) and updated the test.

@evgeni evgeni merged commit 763e72d into Katello:master Aug 16, 2024
27 checks passed
@evgeni evgeni deleted the i37696 branch August 16, 2024 12:55
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