Fixes #39308 - Skip Candlepin update when subscription facet is destroyed#11736
Fixes #39308 - Skip Candlepin update when subscription facet is destroyed#11736jturel wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis PR centralizes host artifact cleanup into HostArtifactCleaner, refactors RegistrationManager to delegate cleanup with preserve_for_provisioning logic, adds guards and explicit error handling for destroyed subscription facets and missing Candlepin consumers, and updates factories and tests to exercise the new behavior. ChangesHost Artifact Cleanup Refactoring
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| elsif organization_destroy | ||
| host.content_facet.try(:destroy!) | ||
| remove_host_artifacts(host, clear_content_facet: false, preserve_for_provisioning: preserve_for_provisioning) | ||
| remove_host_artifacts(host, clear_content_facet: false) |
There was a problem hiding this comment.
preserve_for_provisioning is a no-op when clear_content_facet is false; removed for clarity
|
|
||
| if unregistering | ||
| remove_host_artifacts(host, preserve_for_provisioning: preserve_for_provisioning) | ||
| remove_host_artifacts(host, preserve_for_provisioning: preserve_for_provisioning || Setting[:retain_build_profile_upon_unregistration]) |
There was a problem hiding this comment.
Lifted the setting check higher since it's only relevant during unregistration, and is ultimately an override of preserve_for_provisioning
|
|
||
| def remove_host_artifacts(host, clear_content_facet: true, preserve_for_provisioning: false) | ||
| Rails.logger.debug "Host ID: #{host.id}, clear_content_facet: #{clear_content_facet}" | ||
| if host.content_facet && clear_content_facet |
There was a problem hiding this comment.
I had fixed this in a different way and testing this piece was annoying, so I pulled it into a separate class. It's a copy/paste except for relocation of the Setting check
| begin | ||
| ::Katello::Resources::Candlepin::Consumer.update(subscription_facet.uuid, consumer_params) | ||
| rescue RestClient::Gone, RestClient::NotFound | ||
| raise "Unable to update missing or deleted candlepin consumer uuid=#{subscription_facet.uuid} host_id=#{self.id}" |
There was a problem hiding this comment.
I could go either way on this but as the bug shows RestClient rescues could hide bugs
|
Tagging @ianballou here for visibility because I think you worked on the |
![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
🧹 Nitpick comments (3)
test/factories/content_view_environment_factory.rb (1)
8-10: ⚡ Quick winAvoid depending on FactoryBot’s internal
@instancehere.Lines 9-10 can use dependent attributes directly; reaching into
@instancemakes this factory brittle across FactoryBot strategies and upgrades.Proposed change
- content_view_version { association(:katello_content_view_version) } - content_view { `@instance.content_view_version.content_view` } - environment { association(:katello_environment, organization: `@instance.content_view.organization`) } + content_view_version { association(:katello_content_view_version) } + content_view { content_view_version.content_view } + environment { association(:katello_environment, organization: content_view.organization) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/factories/content_view_environment_factory.rb` around lines 8 - 10, The factory is directly accessing FactoryBot's internal `@instance` (lines defining content_view and environment); remove that dependency by making content_view and environment use the already-declared content_view_version association via lazy evaluation. Replace uses of `@instance` with references to content_view_version (e.g., set content_view to content_view_version.content_view) and build the environment association using the organization from content_view_version.content_view (e.g., association(:katello_environment, organization: content_view_version.content_view.organization)), ensuring both attributes rely on the declared content_view_version rather than `@instance`.test/services/katello/registration/host_artifact_cleaner_test.rb (1)
19-25: ⚡ Quick winThis stub masks the regression this PR is fixing.
Line 20 forces every
update_candlepin_associationscall to succeed, so these tests would still pass ifclean!unexpectedly touched Candlepin after the subscription facet was destroyed. Please add one regression case without the global stub and assert cleanup completes without attempting that update or raisingRestClient::Gone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/services/katello/registration/host_artifact_cleaner_test.rb` around lines 19 - 25, The global stub on ::Host::Managed#update_candlepin_associations masks the regression; remove that global stub for one regression test and add a specific example that calls host_artifact_cleaner.clean! (or the clean! invocation used in this test) without stubbing update_candlepin_associations, then assert the call completes without invoking update_candlepin_associations (use expect(host).never(:update_candlepin_associations) or similar) and that no RestClient::Gone is raised; keep the other existing expectations for installed_packages, rhsm_fact_values, and get_status so the test verifies cleanup succeeds while ensuring clean! does not attempt Candlepin updates after the subscription facet is destroyed.test/services/katello/registration_manager_test.rb (1)
429-438: ⚡ Quick winEnforce call order for the two cleanup expectations.
These two expectations assert arguments but not execution order. Since the test intent is explicitly sequential, add a sequence to prevent order regressions.
Proposed tweak
- # original unregister - Katello::Registration::HostArtifactCleaner.any_instance.expects(:clean!).with( + cleanup_sequence = sequence('cleanup_sequence') + + # original unregister + Katello::Registration::HostArtifactCleaner.any_instance.expects(:clean!).with( clear_content_facet: true, preserve_for_provisioning: true - ) + ).in_sequence(cleanup_sequence) # failure during re-reg Katello::Registration::HostArtifactCleaner.any_instance.expects(:clean!).with( clear_content_facet: true, preserve_for_provisioning: false - ) + ).in_sequence(cleanup_sequence)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/services/katello/registration_manager_test.rb` around lines 429 - 438, The two expectations on Katello::Registration::HostArtifactCleaner#clean! must enforce execution order; create a Mocha sequence (e.g., seq = sequence('artifact_cleanup')) and attach it to both expectations using in_sequence(seq) so the first expectation (preserve_for_provisioning: true) is expected before the second (preserve_for_provisioning: false).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/models/concerns/host_managed_extensions_test.rb`:
- Around line 305-310: The test test_backend_update_needed_facet_destroyed is
stubbing subscription_facet.destroyed? incorrectly; change the expectation on
host.subscription_facet (the line using
host.subscription_facet.expects(:destroyed?).returns(...)) to return true so the
early-return guard in subscription_facet#backend_update_needed? (the `return
false if self.destroyed?` path) is exercised and the test verifies the
facet-destroyed behavior.
---
Nitpick comments:
In `@test/factories/content_view_environment_factory.rb`:
- Around line 8-10: The factory is directly accessing FactoryBot's internal
`@instance` (lines defining content_view and environment); remove that dependency
by making content_view and environment use the already-declared
content_view_version association via lazy evaluation. Replace uses of `@instance`
with references to content_view_version (e.g., set content_view to
content_view_version.content_view) and build the environment association using
the organization from content_view_version.content_view (e.g.,
association(:katello_environment, organization:
content_view_version.content_view.organization)), ensuring both attributes rely
on the declared content_view_version rather than `@instance`.
In `@test/services/katello/registration_manager_test.rb`:
- Around line 429-438: The two expectations on
Katello::Registration::HostArtifactCleaner#clean! must enforce execution order;
create a Mocha sequence (e.g., seq = sequence('artifact_cleanup')) and attach it
to both expectations using in_sequence(seq) so the first expectation
(preserve_for_provisioning: true) is expected before the second
(preserve_for_provisioning: false).
In `@test/services/katello/registration/host_artifact_cleaner_test.rb`:
- Around line 19-25: The global stub on
::Host::Managed#update_candlepin_associations masks the regression; remove that
global stub for one regression test and add a specific example that calls
host_artifact_cleaner.clean! (or the clean! invocation used in this test)
without stubbing update_candlepin_associations, then assert the call completes
without invoking update_candlepin_associations (use
expect(host).never(:update_candlepin_associations) or similar) and that no
RestClient::Gone is raised; keep the other existing expectations for
installed_packages, rhsm_fact_values, and get_status so the test verifies
cleanup succeeds while ensuring clean! does not attempt Candlepin updates after
the subscription facet is destroyed.
🪄 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: f8057a1e-214a-4f9a-a76a-f9c2883e98ec
📒 Files selected for processing (13)
app/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/host/subscription_facet.rbapp/services/katello/registration/host_artifact_cleaner.rbapp/services/katello/registration_manager.rbtest/factories/content_facet_factory.rbtest/factories/content_view_environment_factory.rbtest/factories/host_factory.rbtest/factories/katello_erratum_factory.rbtest/factories/repository_factory.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/subscription_facet_host_extensions_test.rbtest/services/katello/registration/host_artifact_cleaner_test.rbtest/services/katello/registration_manager_test.rb
@parthaa added it but I was involved in the first pass at ensuring provisioning bits aren't broken, I can take a look. |
The fix from #11049 is what is sensitive. If the general logic there hasn't changed, meaning provisioning users won't get missing medium errors, then we're set. I'll try to verify that. |
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
I give my +1 that this does not break the functionality that saves kickstart repo information when reregistering a host. The new tests help prove this.
To be clear this broke on the line that clears the content view environments from the hosts' content facet because it triggers an update to Candlepin. On this code path the candlepin consumer has already been deleted. So it's not really related to anything in RegistrationManager but that's where my original fix was. |
+1, it was tangential to this PR, just a fragile bit of logic that was a pain in the past. |
jturel
force-pushed
the
fix_host_artifact_cleanup
branch
from
1b372c3 to
a989260
Compare
|
Squashed & rebased |
jturel
force-pushed
the
fix_host_artifact_cleanup
branch
from
f0d4c2f to
15df239
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/services/katello/registration_manager_test.rb (1)
297-297: 💤 Low valueDebug message expectation may be fragile.
This expectation verifies a debug log message is called exactly once. While this confirms the code path is executed, debug messages are implementation details that may change, making the test brittle. Consider whether this assertion is essential or could be removed if it becomes a maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/services/katello/registration_manager_test.rb` at line 297, The test currently asserts a fragile exact debug log call via Rails.logger.expects(:debug).with("ContentFacet: Marking content view environments changed for host #{`@host.name`}").times(1); remove this strict expectation (or relax it to a non-strict/no-count expectation) so the test doesn't rely on an implementation-detail debug message—locate the expectation in registration_manager_test (the line containing Rails.logger.expects(:debug) referencing ContentFacet) and either delete that assertion or change it to a permissive stub that doesn't enforce times(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/services/katello/registration_manager_test.rb`:
- Line 297: The test currently asserts a fragile exact debug log call via
Rails.logger.expects(:debug).with("ContentFacet: Marking content view
environments changed for host #{`@host.name`}").times(1); remove this strict
expectation (or relax it to a non-strict/no-count expectation) so the test
doesn't rely on an implementation-detail debug message—locate the expectation in
registration_manager_test (the line containing Rails.logger.expects(:debug)
referencing ContentFacet) and either delete that assertion or change it to a
permissive stub that doesn't enforce times(1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 817eb2f0-f2a4-4141-b490-a2b53556695a
📒 Files selected for processing (13)
app/models/katello/concerns/subscription_facet_host_extensions.rbapp/models/katello/host/subscription_facet.rbapp/services/katello/registration/host_artifact_cleaner.rbapp/services/katello/registration_manager.rbtest/factories/content_facet_factory.rbtest/factories/content_view_environment_factory.rbtest/factories/host_factory.rbtest/factories/katello_erratum_factory.rbtest/factories/repository_factory.rbtest/models/concerns/host_managed_extensions_test.rbtest/models/concerns/subscription_facet_host_extensions_test.rbtest/services/katello/registration/host_artifact_cleaner_test.rbtest/services/katello/registration_manager_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/factories/repository_factory.rb
🚧 Files skipped from review as they are similar to previous changes (9)
- app/models/katello/host/subscription_facet.rb
- test/factories/host_factory.rb
- test/models/concerns/subscription_facet_host_extensions_test.rb
- test/factories/katello_erratum_factory.rb
- test/services/katello/registration/host_artifact_cleaner_test.rb
- test/models/concerns/host_managed_extensions_test.rb
- app/services/katello/registration/host_artifact_cleaner.rb
- test/factories/content_view_environment_factory.rb
- test/factories/content_facet_factory.rb
3 participants
What are the changes introduced in this pull request?
During a recent PR review around host registration I found two broken scenarios stemming from the same bug rooted in host artifact cleanup.
Scenario 1: Candlepin goes down during registration.
Attempts to cleanup certain host artifacts in the DB which issue another HTTP request to Candlepin. Since CP is down it fails and cleanup is left incomplete.
The exception is raised from within
clean_host_artifacts.Scenario 2: Host unregister
Host artifacts are again cleaned up when a host is unregistered.
The DELETE request is emitting logs about a PUT to the candlepin consumer which returns a
410 Gone. When cleanup happens the consumer has already been deleted from Candlepin. Artifact cleanup is again incomplete. The CandlepinProxiesController rescues all RestClient exceptions which hides the fact that something actually went wrong - it just passes the return code on without a stack trace.Considerations taken when implementing this change?
I don't think CandlepinProxiesController rescuing all rest client exceptions is a good idea but I decided to leave it alone and just fix the actual bug.
What are the testing steps for this pull request?
Scenario 1:
remove_host_artifactsshould not be in the stack trace.Scenario 2:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests