✨ Set ownerreference on BareMetalHost #263
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maelk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
approved
|
/test-v1a3-integration |
2 similar comments
|
/test-v1a3-integration |
|
/test-v1a3-integration |
|
Could we get a little more detail about why the existing consumerRef field is insufficient for this? I've noticed some other patches merging rather quickly this morning. I would like to have time to review this one, so I'm applying a temporary hold. /hold |
metal3-io-bot
added
the
do-not-merge/hold
|
Sure, this is because clusterctl builds a tree of resources using the ownerreferences. It is not directly used right now because we still have BMO installed out of the scope of clusterctl, but whenever that will not be the case anymore (i.e. we include BMO deployment in infrastructure-components.yaml) then this ownerreference is a must to ensure that the BMH gets moved along during the pivoting. It is duplicating the idea behind consumerRef, but clusterctl does not understand BMH CRs, so it uses a more universal way to build the dependency tree. |
| @@ -346,6 +346,7 @@ func (m *MachineManager) Delete(ctx context.Context) error { | |||
| } | |||
|
|
|||
| if host != nil && host.Spec.ConsumerRef != nil { | |||
| host.OwnerReferences = m.DeleteOwnerRef(host.OwnerReferences) | |||
There was a problem hiding this comment.
For the ConsumerRef, we check that it matches the current machine before making any changes. Should we do the same here?
There was a problem hiding this comment.
The DeleteOwnerRef will only remove the ownerRef pointing to this baremetalmachine only. It will leave the others intact
| return refList | ||
| } | ||
| index, err := m.FindOwnerRef(refList) | ||
| if err == nil { |
There was a problem hiding this comment.
This logic seems inverted from what I'm used to seeing in go. If this line was if err != nil then the block could return immediately, which would place the shortcut return path closer to the test and keep the more common active path in the main block of the function.
| // FindOwnerRef checks if an ownerreference to this baremetal machine exists | ||
| // and returns the index | ||
| func (m *MachineManager) FindOwnerRef(refList []metav1.OwnerReference) (int, error) { | ||
| for i := 0; i < len(refList); i++ { |
There was a problem hiding this comment.
Is there a reason not to use range() here?
| for i := 0; i < len(refList); i++ { | |
| for i, curOwnerRef := range(refList) { |
| @@ -1809,18 +1812,25 @@ var _ = Describe("BareMetalMachine manager", func() { | |||
| } else { | |||
| _, ok := errors.Cause(err).(HasRequeueAfterError) | |||
| Expect(ok).To(BeTrue()) | |||
| return | |||
There was a problem hiding this comment.
This looks like it's going to short circuit some of the test logic. Is that the intent? Why return early when a re-queue is expected? Perhaps this is another place where the logic could be flipped and the return kept inside the if/then block and the else block eliminated.
There was a problem hiding this comment.
indeed, I have tightened the condition for the return
|
/hold cancel |
metal3-io-bot
removed
the
do-not-merge/hold
|
A main reason we did not add such an owner reference to BMH previously was that it does not make sense to cascade-delete a BMH. Is that accounted for somehow with this change? |
|
When deleting the Baremetalmachine, capbm will block the deletion with a finalizer until the BMH is back to "ready" state. The first operation of the deprovisioning is to remove the ownerReference. So when the Baremetalmachine is garbage collected, the BMH does not have an ownerreference to the baremetalmachine anymore and is not deleted. Is this sufficient in your opinion, or do you foresee corner-cases ? |
maelk
force-pushed
the
v1a3-bmh-ownerref-mael
branch
from
bb228cd
to
31d394f
Compare
|
/test-v1a3-integration |
So when a BMM is deleted, first the owner reference is removed and then deprovisioning starts? And after deprovisioning is done then the finalizer is removed from the BMM and it is deleted? |
|
Yes, that's the workflow |
Sounds reasonable to me. Thanks for the explanation. |
|
Looks good to me |
|
/lgtm |
What this PR does / why we need it:
This PR adds an Ownerreference on the baremetalhost, set by CAPBM when selecting the baremetalhost. Clusterctl builds a tree of resources using the ownerreferences. the ownerreference is not directly used right now because we still have BMO installed out of the scope of clusterctl, but whenever that will not be the case anymore (i.e. we include BMO deployment in infrastructure-components.yaml) then this ownerreference is a must to ensure that the BMH gets moved along during the pivoting.