ACM-27859: Allow PreprovisioningImage finalizer to be removed once BMH finishes deprovisioning #8604
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@CrystalChun: This pull request references ACM-27859 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PreprovisioningImage deletion handler now checks the associated BareMetalHost's provisioning state and only requeues deletion while the host is still deprovisioning; finalizer removal proceeds only when the host is deprovisioned. Tests were added to verify deletion allowed after deprovisioning completes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
openshift-ci
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/controllers/preprovisioningimage_controller_test.go (1)
1445-1459: Critical: This existing test will fail with the new controller logic.The test expects PreprovisioningImage deletion to be blocked when the BareMetalHost has cleaning enabled but is not being deleted. However, the new controller logic at line 776 (in
preprovisioningimage_controller.go) only requeues when:if !bmh.DeletionTimestamp.IsZero() && !alreadyDeprovisioned {This condition requires the BMH to be deleting (
!bmh.DeletionTimestamp.IsZero()) for the requeue to occur. When the BMH is not being deleted (as in this test), the condition evaluates tofalse, causing the finalizer to be removed and allowing PPI deletion.Expected test result with new code:
result.Requeuewill befalse(nottrueas the test expects)- PPI will be deleted (finalizer removed)
- Test assertion
Expect(result.Requeue).To(BeTrue())will failResolution required:
Either update this test to reflect the new intended behavior (allowing deletion when BMH is not being deleted), or adjust the controller logic if this blocking behavior should be preserved.
🧹 Nitpick comments (1)
internal/controller/controllers/preprovisioningimage_controller.go (1)
772-780: Consider includingStateDeletedin the deprovisioning completion check.The current logic checks for
StatePoweringOffBeforeDeleteandStateDeletingto determine if deprovisioning has finished. While these states are correct, you might also want to includeStateDeletedfor completeness, as it represents a terminal state where the BMH definitely no longer needs the PreprovisioningImage.However, if the BMH reaches
StateDeleted, it's likely about to be removed from the system entirely, making this check somewhat redundant. The current implementation is probably sufficient.Optional: Include StateDeleted
- alreadyDeprovisioned := funk.Contains([]metal3_v1alpha1.ProvisioningState{metal3_v1alpha1.StatePoweringOffBeforeDelete, metal3_v1alpha1.StateDeleting}, bmh.Status.Provisioning.State) + alreadyDeprovisioned := funk.Contains([]metal3_v1alpha1.ProvisioningState{metal3_v1alpha1.StatePoweringOffBeforeDelete, metal3_v1alpha1.StateDeleting, metal3_v1alpha1.StateDeleted}, bmh.Status.Provisioning.State)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
internal/controller/controllers/preprovisioningimage_controller.go(1 hunks)internal/controller/controllers/preprovisioningimage_controller_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/controller/controllers/preprovisioningimage_controller_test.gointernal/controller/controllers/preprovisioningimage_controller.go
🔇 Additional comments (3)
internal/controller/controllers/preprovisioningimage_controller_test.go (2)
405-423: LGTM! Test correctly verifies deletion when BMH finishes deprovisioning.The test properly validates that when a BareMetalHost reaches the
StateDeletingprovisioning state (indicating deprovisioning has completed), the PreprovisioningImage finalizer is removed and deletion proceeds. This aligns with the controller logic changes.
425-443: LGTM! Test correctly verifies deletion when BMH enters powering-off state.The test properly validates that when a BareMetalHost reaches the
StatePoweringOffBeforeDeleteprovisioning state, the PreprovisioningImage finalizer is removed and deletion proceeds. This aligns with the controller logic changes.internal/controller/controllers/preprovisioningimage_controller.go (1)
772-780: LGTM! Logic correctly addresses the foreground deletion deadlock.The new deprovisioning-aware deletion logic properly solves the deadlock scenario described in the PR objectives:
- During BMH foreground deletion: PPI finalizer is retained while BMH is still deprovisioning (needs the image), but removed once BMH reaches terminal states (
PoweringOffBeforeDeleteorDeleting)- When BMH is not being deleted: PPI finalizer is removed immediately, allowing independent PPI deletion
- When cleaning is disabled: PPI finalizer is removed immediately
This ensures the PreprovisioningImage can be deleted without creating a circular dependency with its owning BareMetalHost.
…H finishes deprovisioning BMH owns the PreprovisioningImage (PPI), and foreground deletion waits for all children resources (PPI) to be deleted before it removes the owner (BMH) With the PreprovisioningImage finalizer, there is a possibly for a deadlock to happen when foreground deletion is done. The BMH (with foreground deletion) waits for the PPI to be deleted, however, previously, the PPI ends up waiting for the BMH to be deleted before its finalizer can be removed. To prevent this, we'll rely on the state of the BMH to remove the finalizer. If the BMH has finished deprovisioning (state powering off before deleting or deleting) then we assume we can safely remove the PPI finalizer as the image is no longer needed.
CrystalChun
force-pushed
the
ppi-lock
branch
from
5a22d90 to
e4ed6fe
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8604 +/- ##
=======================================
Coverage 43.49% 43.49%
=======================================
Files 411 411
Lines 71244 71250 +6
=======================================
+ Hits 30987 30992 +5
- Misses 37497 37498 +1
Partials 2760 2760
🚀 New features to boost your workflow:
|
|
/test e2e-agent-compact-ipv4 |
2 similar comments
|
/test e2e-agent-compact-ipv4 |
|
/test e2e-agent-compact-ipv4 |
|
@CrystalChun: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
BMH owns the PreprovisioningImage (PPI), and foreground deletion waits for all children resources (PPI) to be deleted before it removes the owner (BMH)
With the PreprovisioningImage finalizer, there is a possibly for a deadlock to happen when foreground deletion is done.
The BMH (with foreground deletion) waits for the PPI to be deleted, however, previously, the PPI ends up waiting for the BMH to be deleted before its finalizer can be removed.
To prevent this, we'll rely on the state of the BMH to remove the finalizer.
If the BMH has finished deprovisioning (state powering off before deleting or deleting) then we assume we can safely remove the PPI finalizer as the image is no longer needed.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
/cc @carbonin