Fixes #39213 - Import only OS-detection facts at registration time#11701
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Sequence DiagramsequenceDiagram
participant Client
participant RegistrationManager
participant Candlepin
participant SubscriptionFacet
participant HostFactImporter
Client->>RegistrationManager: register_host(host, consumer_params)
RegistrationManager->>Candlepin: create consumer
Candlepin-->>RegistrationManager: consumer created
rect rgba(100,150,200,0.5)
Note over RegistrationManager: extract critical facts
RegistrationManager->>RegistrationManager: critical = consumer_params[:facts].slice(REGISTRATION_CRITICAL_FACTS)
RegistrationManager->>SubscriptionFacet: update_facts(host, critical, additive: true)
SubscriptionFacet->>HostFactImporter: import_facts(rhsm_facts, additive: true)
HostFactImporter-->>SubscriptionFacet: import complete
end
RegistrationManager->>RegistrationManager: finalize_registration (full facts import later)
RegistrationManager-->>Client: registration complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/katello/registration_manager.rb (1)
247-258:⚠️ Potential issue | 🟠 MajorClarify: Should this full fact import remain?
Per the PR objectives, "Import only the three OS-detection facts synchronously at registration (step 2)." This line imports all facts during registration, which contradicts the stated optimization goal. If the intent is to defer the full fact sync to step 6 (PUT /rhsm/consumers/:id), this call should likely be removed.
If this import is intentionally retained for Candlepin synchronization purposes, please clarify in the code comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/katello/registration_manager.rb` around lines 247 - 258, The create_in_candlepin method currently calls ::Katello::Host::SubscriptionFacet.update_facts(host, consumer_params[:facts]) which imports all facts at registration and contradicts the PR objective to import only the three OS-detection facts synchronously; either remove this full-fact import or change it to extract and pass only the three OS-detection facts (e.g., kernel, osrelease, operatingsystem or whatever the project’s canonical three are) before calling update_facts, and if you intentionally must keep the full import for Candlepin sync add a clear code comment above the ::Katello::Host::SubscriptionFacet.update_facts call explaining that Candlepin requires full fact sync at registration and why it cannot be deferred to the later PUT step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 203-209: Remove the full facts import in create_in_candlepin so
only the critical facts are imported during registration: locate the
create_in_candlepin method and remove or disable the call to
::Katello::Host::SubscriptionFacet.update_facts that applies the entire
consumer_params[:facts] payload, leaving the earlier logic that slices
consumer_params[:facts] with REGISTRATION_CRITICAL_FACTS and only updates those
critical facts (additive: true); ensure no other code in create_in_candlepin
re-applies the full facts set so the full fact set will arrive later via PUT
/rhsm/consumers/:id as intended.
---
Outside diff comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 247-258: The create_in_candlepin method currently calls
::Katello::Host::SubscriptionFacet.update_facts(host, consumer_params[:facts])
which imports all facts at registration and contradicts the PR objective to
import only the three OS-detection facts synchronously; either remove this
full-fact import or change it to extract and pass only the three OS-detection
facts (e.g., kernel, osrelease, operatingsystem or whatever the project’s
canonical three are) before calling update_facts, and if you intentionally must
keep the full import for Candlepin sync add a clear code comment above the
::Katello::Host::SubscriptionFacet.update_facts call explaining that Candlepin
requires full fact sync at registration and why it cannot be deferred to the
later PUT step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 762da2c0-c244-4b48-8782-c9dd9f668412
📥 Commits
Reviewing files that changed from the base of the PR and between cc28088 and 0736af9f82284a5212e2bfed9b397fadbd04cc8f.
📒 Files selected for processing (2)
app/models/katello/host/subscription_facet.rbapp/services/katello/registration_manager.rb
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
0736af9 to
25524db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 203-209: The fact-import block for REGISTRATION_CRITICAL_FACTS
(using consumer_params[:facts] and
::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true))
was moved outside the create_in_candlepin rescue/rollback path so any exception
there bypasses the rollback; move this block back inside the existing
create_in_candlepin rescue/ensure cleanup path (or wrap it in the same
begin/rescue that calls the rollback) so failures during critical-fact import
trigger the same rollback/cleanup logic as create_in_candlepin and do not leave
a partially registered host.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 030790c9-6809-4db5-a83c-2bee92365259
📥 Commits
Reviewing files that changed from the base of the PR and between 0736af9f82284a5212e2bfed9b397fadbd04cc8f and 25524dbea0ac7b77407f1792589bb8203dcd8e6d.
📒 Files selected for processing (2)
app/models/katello/host/subscription_facet.rbapp/services/katello/registration_manager.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/katello/host/subscription_facet.rb
| # Import only the facts needed for OS/architecture detection so that | ||
| # RhelLifecycleStatus is correct immediately after registration. | ||
| # The full fact set arrives via PUT /rhsm/consumers/:id (step 6). | ||
| unless consumer_params[:facts].blank? | ||
| critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS) | ||
| ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) if critical.present? | ||
| end |
There was a problem hiding this comment.
Keep the new fact import inside the existing cleanup path.
This block now sits outside the create_in_candlepin rescue, so an exception during fact import skips the rollback logic and can leave a partially registered host behind.
🩹 Proposed fix
User.as_anonymous_admin do
begin
create_in_candlepin(host, content_view_environments, consumer_params, activation_keys)
+ unless consumer_params[:facts].blank?
+ critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS)
+ ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) if critical.present?
+ end
rescue StandardError => e
# we can't call CP here since something bad already happened. Just clean up our DB as best as we can.
host.subscription_facet.try(:destroy!)
new_host ? remove_partially_registered_new_host(host) : remove_host_artifacts(host)
raise e
end
-
- # Import only the facts needed for OS/architecture detection so that
- # RhelLifecycleStatus is correct immediately after registration.
- # The full fact set arrives via PUT /rhsm/consumers/:id (step 6).
- unless consumer_params[:facts].blank?
- critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS)
- ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) if critical.present?
- end
finalize_registration(host)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Import only the facts needed for OS/architecture detection so that | |
| # RhelLifecycleStatus is correct immediately after registration. | |
| # The full fact set arrives via PUT /rhsm/consumers/:id (step 6). | |
| unless consumer_params[:facts].blank? | |
| critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS) | |
| ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) if critical.present? | |
| end | |
| User.as_anonymous_admin do | |
| begin | |
| create_in_candlepin(host, content_view_environments, consumer_params, activation_keys) | |
| unless consumer_params[:facts].blank? | |
| critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS) | |
| ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) if critical.present? | |
| end | |
| rescue StandardError => e | |
| # we can't call CP here since something bad already happened. Just clean up our DB as best as we can. | |
| host.subscription_facet.try(:destroy!) | |
| new_host ? remove_partially_registered_new_host(host) : remove_host_artifacts(host) | |
| raise e | |
| end | |
| finalize_registration(host) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/katello/registration_manager.rb` around lines 203 - 209, The
fact-import block for REGISTRATION_CRITICAL_FACTS (using consumer_params[:facts]
and ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive:
true)) was moved outside the create_in_candlepin rescue/rollback path so any
exception there bypasses the rollback; move this block back inside the existing
create_in_candlepin rescue/ensure cleanup path (or wrap it in the same
begin/rescue that calls the rollback) so failures during critical-fact import
trigger the same rollback/cleanup logic as create_in_candlepin and do not leave
a partially registered host.
25524db to
38878a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 208-213: The re-registration flow currently deletes
host.rhsm_fact_values in remove_host_artifacts (invoked via unregister_host with
unregistering: true) before ::Katello::Host::SubscriptionFacet.update_facts is
called, so using additive: true cannot restore prior RHSM facts; modify the
unregister/remove path to accept and propagate a flag (e.g., clear_rhsm_facts:
false) when performing re-registration cleanup, update remove_host_artifacts to
skip deleting host.rhsm_fact_values when that flag is false, and ensure the
re-registration caller passes clear_rhsm_facts: false so the old RHSM facts
remain available for ::Katello::Host::SubscriptionFacet.update_facts(additive:
true).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fd6a583-d0bf-4c20-9e89-103dd0ec61e6
📥 Commits
Reviewing files that changed from the base of the PR and between 25524dbea0ac7b77407f1792589bb8203dcd8e6d and 38878a14f5d25a13d5453fa374c4ec042cbe1ad0.
📒 Files selected for processing (2)
app/models/katello/host/subscription_facet.rbapp/services/katello/registration_manager.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/katello/host/subscription_facet.rb
| unless consumer_params[:facts].blank? | ||
| critical = consumer_params[:facts].slice(*REGISTRATION_CRITICAL_FACTS) | ||
| if critical.present? | ||
| begin | ||
| ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) | ||
| rescue StandardError => e |
There was a problem hiding this comment.
additive: true cannot preserve the previous RHSM facts after re-registration.
For existing hosts, Line 169 calls unregister_host(..., unregistering: true), and that path reaches remove_host_artifacts, which deletes host.rhsm_fact_values at Line 367 before this block runs. By the time Line 212 executes, the old RHSM fact set is already gone, so the additive import only preserves facts written earlier in this request—not the prior registration facts this change is trying to keep until step 6. Consider threading a clear_rhsm_facts: false-style flag through the re-registration cleanup path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/katello/registration_manager.rb` around lines 208 - 213, The
re-registration flow currently deletes host.rhsm_fact_values in
remove_host_artifacts (invoked via unregister_host with unregistering: true)
before ::Katello::Host::SubscriptionFacet.update_facts is called, so using
additive: true cannot restore prior RHSM facts; modify the unregister/remove
path to accept and propagate a flag (e.g., clear_rhsm_facts: false) when
performing re-registration cleanup, update remove_host_artifacts to skip
deleting host.rhsm_fact_values when that flag is false, and ensure the
re-registration caller passes clear_rhsm_facts: false so the old RHSM facts
remain available for ::Katello::Host::SubscriptionFacet.update_facts(additive:
true).
There was a problem hiding this comment.
The additive: true is intentional and necessary for a specific edge case that the analysis missed.
remove_host_artifacts (which deletes rhsm_fact_values at line 367) is only reached if candlepin_consumer_destroy at line 142 succeeds. When the host is already gone from Candlepin, candlepin_consumer_destroy raises RestClient::Gone, which is rescued at line 170-172 — and remove_host_artifacts is never called. In that case host.rhsm_fact_values still contains the prior fact set.
So the two paths are:
- Normal re-registration:
remove_host_artifactsruns → facts cleared →additive: trueis a no-op (but harmless) RestClient::Gone(host already deleted from Candlepin):remove_host_artifactsnever runs → prior facts still exist →additive: truepreserves them while importing only the critical subset
additive: true is the right choice precisely because of the second path.
There was a problem hiding this comment.
@pablomh, thank you for the detailed walkthrough — you're right, and my original comment was incorrect.
The key I missed is the rescue RestClient::Gone at lines 170–172 of register_host. If RestClient::Gone propagates out of unregister_host (most likely from host.subscription_facet.try(:destroy!) triggering a Candlepin callback, since candlepin_consumer_destroy itself silently swallows that exception), execution jumps past remove_host_artifacts entirely. In that path rhsm_fact_values are still intact from the prior registration, and additive: true correctly preserves them while only importing the critical subset.
The two paths are indeed:
| Path | remove_host_artifacts runs? |
Effect of additive: true |
|---|---|---|
| Normal re-registration | ✅ Yes — facts cleared | No-op (harmless) |
RestClient::Gone (host already gone from CP) |
❌ No — facts intact | Preserves prior facts |
additive: true is the right choice for exactly this reason. I'll withdraw my original comment.
✏️ Learnings added
Learnt from: pablomh
Repo: Katello/katello PR: 11701
File: app/services/katello/registration_manager.rb:208-213
Timestamp: 2026-04-02T20:20:03.389Z
Learning: In `app/services/katello/registration_manager.rb`, the `additive: true` flag passed to `SubscriptionFacet.update_facts` during critical-facts import at registration is intentional. When `RestClient::Gone` is raised during `unregister_host` (e.g., from `subscription_facet.destroy!` Candlepin callbacks), it is rescued at the outer `rescue RestClient::Gone` block inside `register_host`, causing `remove_host_artifacts` to be skipped. In that code path `host.rhsm_fact_values` still hold the prior registration's facts, and `additive: true` correctly preserves them while importing only the critical OS-detection subset. In the normal re-registration path `remove_host_artifacts` runs first and clears the facts, making `additive: true` a harmless no-op.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if critical.present? | ||
| begin | ||
| ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) | ||
| rescue StandardError => e |
There was a problem hiding this comment.
What is this rescue guarding against? Does the FactImporter raise any of its own errors? It's reasonable to believe that fact import (which is ultimately just creating or updating some records in the DB) should succeed.
If the rescue is reached there's no signal that something has gone wrong - it's ultimately swallowed - and could be a long time before a regression is found & reported. By letting the error raise the underlying bug can be caught and addressed - hopefully by unit or integration tests.
There was a problem hiding this comment.
Good catch — rescue StandardError was too broad. Narrowed it to rescue Foreman::Exception with an explanation in the comment.
HostFactImporter#import_facts explicitly raises Foreman::Exception when host.build? is true (raise ::Foreman::Exception.new('Host is pending for Build') if host.build?). This is the one specific expected failure: during the provisioning workflow, subscription-manager registers the host while build=true (the flag hasn't been cleared yet), so Foreman intentionally refuses to import facts. Registration has already succeeded at that point and step 6 will import the full fact set.
Everything else — DB failures, connection errors, bugs in the importer — now propagates.
There was a problem hiding this comment.
We extracted the entire block into import_critical_registration_facts so the rescue scope and intent are explicit rather than buried in register_host.
rescue StandardError is intentional and the comment in the new method explains why: this import is best-effort. By the time it runs, Candlepin registration has already succeeded — the host IS registered. Propagating any failure here would mislead the client into thinking registration failed when it actually did not. Step 6 (PUT /rhsm/consumers/:id) will import the full fact set regardless.
The method name makes the non-fatal contract clear, so the broad rescue is no longer hidden in the middle of a long method.
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
38878a1 to
559ff78
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/services/katello/registration_manager.rb (1)
211-219:⚠️ Potential issue | 🟠 MajorThis rescue does not protect the build-host path it documents.
SubscriptionFacet.update_factsalready returns whenhost.build?, so theForeman::Exceptionrescue here will not fire for the provisioning flow described in Lines 213-216. That leaves this block masking only unexpected importer failures, while any other exception still escapes after Candlepin creation and bypasses the cleanup in Lines 194-200. Please either letupdate_factsfail fast here or move the whole critical-import step under the existing rollback path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/katello/registration_manager.rb` around lines 211 - 219, The rescue around ::Katello::Host::SubscriptionFacet.update_facts masks unexpected failures and doesn't protect the documented build-host path because update_facts already returns when host.build?; remove the rescue so exceptions from ::Katello::Host::SubscriptionFacet.update_facts bubble and trigger the existing rollback/cleanup, or alternatively explicitly guard the call with if host.build? skip logic then call update_facts without rescuing—modify the block around ::Katello::Host::SubscriptionFacet.update_facts (and any HostFactImporter interactions) so that only an explicit host.build? check suppresses the call, but all other exceptions are allowed to propagate to the existing rollback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 211-219: The rescue around
::Katello::Host::SubscriptionFacet.update_facts masks unexpected failures and
doesn't protect the documented build-host path because update_facts already
returns when host.build?; remove the rescue so exceptions from
::Katello::Host::SubscriptionFacet.update_facts bubble and trigger the existing
rollback/cleanup, or alternatively explicitly guard the call with if host.build?
skip logic then call update_facts without rescuing—modify the block around
::Katello::Host::SubscriptionFacet.update_facts (and any HostFactImporter
interactions) so that only an explicit host.build? check suppresses the call,
but all other exceptions are allowed to propagate to the existing rollback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10014a41-3ef3-4611-9eff-f9f56d98c316
📥 Commits
Reviewing files that changed from the base of the PR and between 38878a14f5d25a13d5453fa374c4ec042cbe1ad0 and 559ff788fbadf4d459cdf55c704d3b2e8351fc77.
📒 Files selected for processing (2)
app/models/katello/host/subscription_facet.rbapp/services/katello/registration_manager.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/katello/host/subscription_facet.rb
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
559ff78 to
716b178
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/services/katello/registration_manager.rb (1)
330-341: Keep the expected build-host skip out ofwarn.Line 335 currently warns for every importer failure, but provisioning registrations can legitimately hit the build-mode guard. A small
host.build?fast-path here would keep logs quieter and make the remaining warnings much more actionable.💡 One way to keep the expected path quiet
def import_critical_registration_facts(host, facts) return if facts.blank? + if host.build? + Rails.logger.debug("Skipping critical fact import for build host #{host&.name}") + return + end critical = facts.slice(*REGISTRATION_CRITICAL_FACTS) return if critical.blank? ::Katello::Host::SubscriptionFacet.update_facts(host, critical, additive: true) rescue StandardError => e @@ - Rails.logger.warn("Failed to import critical facts for host #{host&.name}: #{e.message}") + Rails.logger.warn("Failed to import critical facts for host #{host&.name}: #{e.class}: #{e.message}") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/katello/registration_manager.rb` around lines 330 - 341, The rescue in import_critical_registration_facts logs any failure, but expected provisioning build-host skips should not produce warnings; add a fast-path that detects build-mode and suppresses the warning. Modify import_critical_registration_facts (method in registration_manager.rb) to return early if host&.build? (or, in the rescue, skip logging when host&.build? is true) before calling ::Katello::Host::SubscriptionFacet.update_facts or emitting the Rails.logger.warn, so legitimate build-mode bypasses do not create warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/services/katello/registration_manager.rb`:
- Around line 330-341: The rescue in import_critical_registration_facts logs any
failure, but expected provisioning build-host skips should not produce warnings;
add a fast-path that detects build-mode and suppresses the warning. Modify
import_critical_registration_facts (method in registration_manager.rb) to return
early if host&.build? (or, in the rescue, skip logging when host&.build? is
true) before calling ::Katello::Host::SubscriptionFacet.update_facts or emitting
the Rails.logger.warn, so legitimate build-mode bypasses do not create warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62631df4-bd83-4ecb-8c77-a7a92038184d
📥 Commits
Reviewing files that changed from the base of the PR and between 559ff788fbadf4d459cdf55c704d3b2e8351fc77 and 716b1784635888c2f94d51d1c20f477fd252524e.
📒 Files selected for processing (2)
app/models/katello/host/subscription_facet.rbapp/services/katello/registration_manager.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/katello/host/subscription_facet.rb
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
716b178 to
c620d7c
Compare
|
Addressed the two improvements from the latest review:
On the |
Performance testing resultsTested on a medium Satellite (8 CPU, 32 GB RAM) with 3 registration topologies (direct, standalone capsule, load-balanced capsule pair) across concurrency levels from 56 to 560 concurrent hosts. This PR was tested together with its dependency theforeman/foreman#10942 (bulk-insert facts + additive import mode). Direct impact of this PR (193 → 3 facts at step 2): By deferring 190 facts from the synchronous The deferred facts arrive at step 6 ( Combined impact with theforeman/foreman#10942 (measured in the same batch):
The P50 improvement in Note: The 90% success rate is limited by the |
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
c620d7c to
a4632de
Compare
|
CI fails with Would you prefer we split the test class (e.g. a separate |
|
Pausing my review here until #11692 reaches a resolution. |
|
@pablomh can you rebase this? Then we'll get a proper review going |
If this is still applicable (it might not be since I removed some tests from that file recently) then I'm good with disabling that cop for the class you mentioned. |
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
a4632de to
8a9361d
Compare
|
The test failures do look related to this PR: |
|
The remaining failures are tests created based on the addition of the |
|
I forgot about that. Ping me when it's green :) |
|
I'll put it DRAFT mode until it's merged. |
|
Please rebase and fix conflicts here, thx :) |
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
2adee57 to
92b7106
Compare
pr-processor
Bot
added
Needs re-review
Waiting on contributor
and removed
Waiting on contributor
labels
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
47b7493 to
899a87a
Compare
|
@jturel, friendly ping now that all the reqs are met and the CI is green :) |
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
I've already tested this with theforeman/foreman#10980 and am happy with the result. Just leaving one nit below
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
899a87a to
952ad38
Compare
Import only the registration-time facts needed for immediate OS and architecture detection while preserving additive fact-import behavior. Keep the stricter registration failure handling, remove the unnecessary Candlepin UUID fallback, and align the registration tests with the resource layer's indifferent-access return contract.
pablomh
force-pushed
the
registration/surgical-fact-import
branch
from
952ad38 to
b4f022d
Compare
3 participants
Problem
During
POST /rhsm/consumers, subscription-manager sends ~193 RHSM facts. Since 2018, all 193 are imported synchronously into Foreman'sfact_valuestable, then imported again at step 6 (PUT /rhsm/consumers/:id, ~33 seconds later) — a redundant double import inherited from the original Dynflow registration flow that was removed in the same 2018 refactor.The original reason for the step-2 import: step 6 was also async (Dynflow) in 2018, so it was unreliable for immediate status calculation. Step 6 is now synchronous and reliable.
Analysis
Only 3 facts are strictly needed at step 2 for
RhelLifecycleStatusto be immediately correct:distribution.name→ setshost.operatingsystem.namedistribution.version→ setshost.operatingsystem.majoruname.machine→ setshost.architecture.nameHostFactImporter.parse_factsoperates on the in-memory facts hash (notfact_values), so these 3 facts are sufficient to persist the correct OS and architecture on the host beforefinalize_registrationruns and callsrefresh_statuses([RhelLifecycleStatus]).Everything else between step 2 and step 6 — Candlepin compliance checks (~13 calls), content access validation, Insights — operates on data from the POST body or its own sources. None read Foreman's
fact_values.Solution
Import only the 3 critical facts synchronously before
finalize_registration, usingadditive: true(introduced in theforeman/foreman#10942) so existingfact_valuesare not deleted on re-registrations. The full 193-fact sync continues to happen at step 6 unchanged.Test plan
RhelLifecycleStatuscorrect immediately afterPOST /rhsm/consumersadditive:keyword threaded throughSubscriptionFacet.update_factsFixes #39213
Depends on theforeman/foreman#10942 (additive import mode)
Summary by Sourcery
Limit synchronous fact import during host registration to only the OS and architecture detection facts while supporting additive fact updates.
Enhancements:
Summary by CodeRabbit