Skip to content

⚠️ Enable removal of ExternallyProvisioned attribute from BareMetalHosts #2566

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.

Sign up for GitHub

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

Open
ethan-gallant wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −16
Open

⚠️ Enable removal of ExternallyProvisioned attribute from BareMetalHosts #2566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/controller/metal3.io/host_state_machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,9 @@ func (hsm *hostStateMachine) handleRegistering(_ *reconcileInfo) actionResult {
// if the credentials change and the Host must be re-registered.
if hsm.Host.Spec.ExternallyProvisioned {
hsm.NextState = metal3api.StateExternallyProvisioned
} else if hsm.Host.Status.Provisioning.State == metal3api.StateExternallyProvisioned {
Copy link
Member

@Rozzii Rozzii Jul 17, 2025

Choose a reason for hiding this comment

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

Wouldn't this implementation move all externally provisioned BMH to "regular provisioned" state even if the user would add an unrelated annotation or the reconciliation would be triggered for whatever other reason?

It might be just a theoretical scenario but I would like to have an an additionl condition that checks that the "ExtarnallyProvisioned" filed is really set to false.

Copy link
Author

@ethan-gallant ethan-gallant Jul 17, 2025

Choose a reason for hiding this comment

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

Since this lands in an else block after the check for externally provisioned I don't believe that would be possible here.

Happy to reformat it as well into a switch statement or similar as the conditional branches here are getting a bit wild already before the change.

Copy link
Member

@zaneb zaneb Jul 18, 2025

Choose a reason for hiding this comment

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

I don't believe this does anything at all, because in handleRegistering() the hsm.Host.Status.Provisioning.State is, by definition, StateRegistering.

Copy link
Member

@Rozzii Rozzii Jul 18, 2025

Choose a reason for hiding this comment

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

Since this lands in an else block after the check for externally provisioned I don't believe that would be possible here.

Happy to reformat it as well into a switch statement or similar as the conditional branches here are getting a bit wild already before the change.

That is true , silly me :D .

But then there is the other thing zaneb pointed out.

Copy link
Member

@dtantsur dtantsur Jul 18, 2025

Choose a reason for hiding this comment

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

The original bug is about the incorrect state in Ironic: once we get to Inspecting, BMO does not expect to find the Node in the active state in Ironic, but that's what actually happens. BMO needs to deprovision the host first. Then make sure that inspection still happen.

// Removing externallyManaged moves hosts to provisioned state
hsm.NextState = metal3api.StateProvisioned
Copy link
Member

@dtantsur dtantsur Aug 13, 2025

Choose a reason for hiding this comment

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

See Zane's comment: this is a wrong place. You need to fix handleExternallyProvisioned instead.

} else if inspectionDisabled(hsm.Host) {
hsm.NextState = metal3api.StatePreparing
} else {
Expand Down
4 changes: 0 additions & 4 deletions internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,6 @@ func (webhook *BareMetalHost) validateChanges(oldObj *metal3api.BareMetalHost, n
errs = append(errs, errors.New("bootMACAddress can not be changed once it is set"))
}

if oldObj.Spec.ExternallyProvisioned != newObj.Spec.ExternallyProvisioned {
Copy link
Member

@zaneb zaneb Jul 18, 2025

Choose a reason for hiding this comment

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

This is a valid bug fix.
According to the state diagram, it is valid to remove the externallyProvisioned flag: https://github.com/metal3-io/baremetal-operator/blob/main/docs/baremetalhost-states.md
I have no idea why the webhook is preventing it.

Copy link
Author

@ethan-gallant ethan-gallant Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The linked issue gives a bit more background, the webhook seems to be a bandaid fix to prevent the host from getting stuck in the provisioning state.

Copy link
Member

@dtantsur dtantsur Jul 18, 2025

Choose a reason for hiding this comment

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

To echo what I said in the bug: if removing externallyProvisioned unconditionally deprovisions the host, we need to make sure that image and customDeploy are not set.

Or we need to permit them to be set and keep the host Provisioned.

Copy link
Author

@ethan-gallant ethan-gallant Jul 18, 2025

Choose a reason for hiding this comment

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

Looks like I may have misunderstood the issue and this will need a bit of additional work.

Just to confirm (since I was not at the working group meetings) the desired behavior is:

  • If the Image & customDeploy are set
    • Move the host to the Provisioned state since the host complies with the standard setup, it was just (as expected) externallyProvisioned
  • If the host does not have the image & customDeploy fields set
    • Deprovision the host since the host can not be managed in the standard fashion

Overall I'm wondering if the approach originally for the externallyProvisioned would have been to follow the cert-manager pattern and had a ProvisionRequest CR where some external system could fulfill the provision instead of hacking around the BareMetalHost CR.

Copy link
Member

@Rozzii Rozzii Jul 22, 2025

Choose a reason for hiding this comment

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

Overall I'm wondering if the approach originally for the externallyProvisioned would have been to follow the cert-manager pattern and had a ProvisionRequest CR where some external system could fulfill the provision instead of hacking around the BareMetalHost CR.

Just to reflect on this point, this would be a nice approach but in practice the system that has done the provisioning might not be cloud native or not even automated at all so a ProvisionRequest CR would provide some optional extra functionality, but it would still not cover all the use cases.

Copy link
Member

@dtantsur dtantsur Jul 22, 2025

Choose a reason for hiding this comment

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

Overall I'm wondering if the approach originally for the externallyProvisioned would have been to follow the cert-manager pattern and had a ProvisionRequest CR where some external system could fulfill the provision instead of hacking around the BareMetalHost CR.

Well, we should have had a separate CRD for provisioning for sure. The HostClaim work metal3-io/metal3-docs#408 is a step in that direction but it does not address externally provisioned.

Just to confirm (since I was not at the working group meetings) the desired behavior is

I think your summary is correct, except that: "If the Image or customDeploy are set" (one is enough).

errs = append(errs, errors.New("externallyProvisioned can not be changed"))
}

return errs
}

Expand Down
8 changes: 0 additions & 8 deletions internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,14 +970,6 @@ func TestValidateUpdate(t *testing.T) {
TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{BootMACAddress: "test-mac"}},
wantedErr: "bootMACAddress can not be changed once it is set",
},
{
name: "updateExternallyProvisioned",
newBMH: &metal3api.BareMetalHost{
TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{}},
oldBMH: &metal3api.BareMetalHost{
TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{ExternallyProvisioned: true}},
wantedErr: "externallyProvisioned can not be changed",
},
}

for _, tt := range tests {
Expand Down
23 changes: 19 additions & 4 deletions test/e2e/externally_provisioned_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/cluster-api/test/framework"
"sigs.k8s.io/cluster-api/util"
kclient "sigs.k8s.io/controller-runtime/pkg/client"
)

var _ = Describe("Create as externally provisioned, deprovision", Label("required", "provision", "deprovision"),
Expand Down Expand Up @@ -79,14 +80,28 @@ var _ = Describe("Create as externally provisioned, deprovision", Label("require
}, &bmh)
Expect(err).NotTo(HaveOccurred())

By("checking that the BMH was not inspected or deployed")
By("Checking that the BMH was not inspected or deployed")
Expect(bmh.Status.OperationHistory.Inspect.Start.IsZero()).To(BeTrue())
Expect(bmh.Status.OperationHistory.Provision.Start.IsZero()).To(BeTrue())
Comment on lines 84 to 85
Copy link
Member

@Rozzii Rozzii Jul 17, 2025

Choose a reason for hiding this comment

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

I am not sure these two conditions mean that the BMH is still in registering as they would never be true in case of an externally provisioned BMH at least that would be my expectation.

I assume you have run this test locally but I wonder how long does the BMH stay in "Registering" state that your implementation relies on.

How do you ensure that the BMH stays in registering state to allow the test code to do the bmh.Spec.ExternallyProvisioned field update? I am not a user of the externally provisioned feature so I might be missing something but I would have suspected the BMH to very quickly transition out of "Registering" and If that is true this test would be very prone to timing issues.

Copy link
Author

@ethan-gallant ethan-gallant Jul 17, 2025

Choose a reason for hiding this comment

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

I did a manual test however I had some issues getting the e2e tests running locally with make test-e2e as the e2e image from quay.io could not be loaded.

I'll try again today as it seems that the tests did not align with what I saw manually testing. If you have any guidance that I might have missed getting the E2E working locally (on Linux / MacOS) that would be greatly appreciated.


By("Removing the ExternallyProvisioned field")
bmh.Spec.ExternallyProvisioned = false

err = clusterProxy.GetClient().Update(ctx, &bmh)
Expect(err).NotTo(HaveOccurred())

Eventually(func(g Gomega) {
Copy link
Member

@dtantsur dtantsur Aug 13, 2025

Choose a reason for hiding this comment

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

Better use WaitForBmhInProvisioningState

err = clusterProxy.GetClient().Get(ctx, kclient.ObjectKeyFromObject(&bmh), &bmh)
g.Expect(bmh.Status.Provisioning.State).To(Equal(metal3api.StateProvisioned))
Copy link
Member

@dtantsur dtantsur Aug 13, 2025

Choose a reason for hiding this comment

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

The way the test is written, the expected state is not Provisioned. Or even: it make become Provisioned briefly, then BMO detects that there is no image and no customDeploy and deprovisions.

}, 2*time.Second, 100*time.Millisecond).Should(Succeed())

By("Deleting the BMH")
// Wait for 2 seconds to allow time to confirm annotation is set
// TODO: fix this so we do not need the sleep
time.Sleep(2 * time.Second)
// Wait for the finalizer to be there, otherwise cleanup logic can't hold the resource
Eventually(func(g Gomega) {
err = clusterProxy.GetClient().Get(ctx, kclient.ObjectKeyFromObject(&bmh), &bmh)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(bmh.Finalizers).To(ContainElement(metal3api.BareMetalHostFinalizer))
}, 2*time.Second, 100*time.Millisecond).Should(Succeed())

err = clusterProxy.GetClient().Delete(ctx, &bmh)
Expect(err).NotTo(HaveOccurred())
Expand Down
Loading