From 1b51bba082fbdfccc1ecac811d60a3dfab20a0eb Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 23 Mar 2021 10:36:17 -0400 Subject: [PATCH 1/3] Select empty provisioner inside ironic factory The empty provisioner is a workaround for the fact that the ironic provisioner cannot handle instantiation when there are no BMC details available. The change cadeb28e313602613e8e1944012d153d4b11bafa that added it also had the effect of logging provisioner information on every reconcile when it was intended to be logged only once, at startup. Make it the ironic provisioner factory's responsibility to return an empty Provisioner at runtime when it cannot create an ironic Provisioner, and do only one-time startup configuration in main.go. Fixes #819 --- .../metal3.io/baremetalhost_controller.go | 2 +- main.go | 29 +++++++------------ pkg/provisioner/ironic/ironic.go | 5 ++++ pkg/provisioner/ironic/ironic_test.go | 14 +++++++++ 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index 6688130f2a..ddfce81e36 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -214,7 +214,7 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re request: request, bmcCredsSecret: bmcCredsSecret, } - prov, err := r.ProvisionerFactory(*host, *bmcCreds, info.publishEvent) + prov, err := r.ProvisionerFactory(*host.DeepCopy(), *bmcCreds, info.publishEvent) if err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to create provisioner") } diff --git a/main.go b/main.go index 5b2c95897f..6958e5f0a0 100644 --- a/main.go +++ b/main.go @@ -31,10 +31,8 @@ import ( metal3iov1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" metal3iocontroller "github.com/metal3-io/baremetal-operator/controllers/metal3.io" - "github.com/metal3-io/baremetal-operator/pkg/bmc" "github.com/metal3-io/baremetal-operator/pkg/provisioner" "github.com/metal3-io/baremetal-operator/pkg/provisioner/demo" - "github.com/metal3-io/baremetal-operator/pkg/provisioner/empty" "github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic" "github.com/metal3-io/baremetal-operator/pkg/version" @@ -120,24 +118,17 @@ func main() { os.Exit(1) } - provisionerFactory := func(host metal3iov1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish provisioner.EventPublisher) (provisioner.Provisioner, error) { - isUnmanaged := !host.HasBMCDetails() - - hostCopy := host.DeepCopy() - - if runInTestMode { - ctrl.Log.Info("using test provisioner") - fix := fixture.Fixture{} - return fix.New(*hostCopy, bmcCreds, publish) - } else if runInDemoMode { - ctrl.Log.Info("using demo provisioner") - return demo.New(*hostCopy, bmcCreds, publish) - } else if isUnmanaged { - ctrl.Log.Info("using empty provisioner") - return empty.New(*hostCopy, bmcCreds, publish) - } + var provisionerFactory provisioner.Factory + if runInTestMode { + ctrl.Log.Info("using test provisioner") + fix := fixture.Fixture{} + provisionerFactory = fix.New + } else if runInDemoMode { + ctrl.Log.Info("using demo provisioner") + provisionerFactory = demo.New + } else { ironic.LogStartup() - return ironic.New(*hostCopy, bmcCreds, publish) + provisionerFactory = ironic.New } if err = (&metal3iocontroller.BareMetalHostReconciler{ diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index 1fa34d0df1..d9875972d6 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -20,6 +20,7 @@ import ( "github.com/metal3-io/baremetal-operator/pkg/bmc" "github.com/metal3-io/baremetal-operator/pkg/hardware" "github.com/metal3-io/baremetal-operator/pkg/provisioner" + "github.com/metal3-io/baremetal-operator/pkg/provisioner/empty" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/devicehints" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/hardwaredetails" @@ -214,6 +215,10 @@ func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds // New returns a new Ironic Provisioner using the global configuration // for finding the Ironic services. func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { + if !host.HasBMCDetails() { + return empty.New(host, bmcCreds, publisher) + } + var err error if clientIronicSingleton == nil || clientInspectorSingleton == nil { tlsConf := clients.TLSConfig{ diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go index 77f0d71303..a3561d3132 100644 --- a/pkg/provisioner/ironic/ironic_test.go +++ b/pkg/provisioner/ironic/ironic_test.go @@ -1,11 +1,15 @@ package ironic import ( + "testing" + + "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" logf "sigs.k8s.io/controller-runtime/pkg/log" logz "sigs.k8s.io/controller-runtime/pkg/log/zap" metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" + "github.com/metal3-io/baremetal-operator/pkg/bmc" // We don't use this package directly here, but need it imported // so it registers its test fixture with the other BMC access @@ -81,3 +85,13 @@ func makeHostLiveIso() (host metal3v1alpha1.BareMetalHost) { // Implements provisioner.EventPublisher to swallow events for tests. func nullEventPublisher(reason, message string) {} + +func TestNewNoBMCDetails(t *testing.T) { + // Create a host without BMC details + host := makeHost() + host.Spec.BMC = metal3v1alpha1.BMCDetails{} + + prov, err := New(host, bmc.Credentials{}, nullEventPublisher) + assert.Equal(t, nil, err) + assert.NotEqual(t, nil, prov) +} From beea4d0ead807a8f19b38d538db3502ee3504b97 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 22 Mar 2021 17:28:20 -0400 Subject: [PATCH 2/3] ironic: Lazily create BMC AccessDetails Not every provisioner operation requires BMC access details, so only create them when needed. This allows us to reach the unmanaged state using the ironic provisioner when there are no BMC details specified. This also means that if the BMC details cannot be parsed correctly, rather than an error that causes constant requeues and is recorded only in the logs, we will record a registration failure that will be visible in the UI. --- pkg/provisioner/ironic/ironic.go | 68 +++++++++++-------- .../ironic/validatemanagementaccess_test.go | 23 +++++++ 2 files changed, 62 insertions(+), 29 deletions(-) diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index d9875972d6..bc09154ab5 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -135,8 +135,6 @@ type ironicProvisioner struct { host metal3v1alpha1.BareMetalHost // a shorter path to the provisioning status data structure status *metal3v1alpha1.ProvisionStatus - // access parameters for the BMC - bmcAccess bmc.AccessDetails // credentials to log in to the BMC bmcCreds bmc.Credentials // a client for talking to ironic @@ -186,12 +184,6 @@ func newProvisionerWithSettings(host metal3v1alpha1.BareMetalHost, bmcCreds bmc. } func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, clientIronic *gophercloud.ServiceClient, clientInspector *gophercloud.ServiceClient) (*ironicProvisioner, error) { - - bmcAccess, err := bmc.NewAccessDetails(host.Spec.BMC.Address, host.Spec.BMC.DisableCertificateVerification) - if err != nil { - return nil, errors.Wrap(err, "failed to parse BMC address information") - } - provisionerLogger := log.WithValues("host", host.Name) // Ensure we have a microversion high enough to get the features @@ -200,7 +192,6 @@ func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds p := &ironicProvisioner{ host: host, status: &(host.Status.Provisioning), - bmcAccess: bmcAccess, bmcCreds: bmcCreds, client: clientIronic, inspector: clientInspector, @@ -241,6 +232,14 @@ func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher clientIronicSingleton, clientInspectorSingleton) } +func (p *ironicProvisioner) bmcAccess() (bmc.AccessDetails, error) { + bmcAccess, err := bmc.NewAccessDetails(p.host.Spec.BMC.Address, p.host.Spec.BMC.DisableCertificateVerification) + if err != nil { + return nil, errors.Wrap(err, "failed to parse BMC address information") + } + return bmcAccess, nil +} + func (p *ironicProvisioner) validateNode(ironicNode *nodes.Node) (errorMessage string, err error) { var validationErrors []string @@ -372,6 +371,12 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro // FIXME(dhellmann): We should rename this method to describe what it // actually does. func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) { + bmcAccess, err := p.bmcAccess() + if err != nil { + result, err = operationFailed(err.Error()) + return + } + var ironicNode *nodes.Node p.debugLog.Info("validating management access") @@ -389,14 +394,14 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b // Some BMC types require a MAC address, so ensure we have one // when we need it. If not, place the host in an error state. - if p.bmcAccess.NeedsMAC() && p.host.Spec.BootMACAddress == "" { - msg := fmt.Sprintf("BMC driver %s requires a BootMACAddress value", p.bmcAccess.Type()) + if bmcAccess.NeedsMAC() && p.host.Spec.BootMACAddress == "" { + msg := fmt.Sprintf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type()) p.log.Info(msg) result, err = operationFailed(msg) return } - driverInfo := p.bmcAccess.DriverInfo(p.bmcCreds) + driverInfo := bmcAccess.DriverInfo(p.bmcCreds) // FIXME(dhellmann): We need to get our IP on the // provisioning network from somewhere. driverInfo["deploy_kernel"] = deployKernelURL @@ -408,8 +413,8 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b if ironicNode == nil { p.log.Info("registering host in ironic") - if p.host.Spec.BootMode == metal3v1alpha1.UEFISecureBoot && !p.bmcAccess.SupportsSecureBoot() { - msg := fmt.Sprintf("BMC driver %s does not support secure boot", p.bmcAccess.Type()) + if p.host.Spec.BootMode == metal3v1alpha1.UEFISecureBoot && !bmcAccess.SupportsSecureBoot() { + msg := fmt.Sprintf("BMC driver %s does not support secure boot", bmcAccess.Type()) p.log.Info(msg) result, err = operationFailed(msg) return @@ -418,16 +423,16 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b ironicNode, err = nodes.Create( p.client, nodes.CreateOpts{ - Driver: p.bmcAccess.Driver(), - BootInterface: p.bmcAccess.BootInterface(), + Driver: bmcAccess.Driver(), + BootInterface: bmcAccess.BootInterface(), Name: p.host.Name, DriverInfo: driverInfo, DeployInterface: p.deployInterface(), InspectInterface: "inspector", - ManagementInterface: p.bmcAccess.ManagementInterface(), - PowerInterface: p.bmcAccess.PowerInterface(), - RAIDInterface: p.bmcAccess.RAIDInterface(), - VendorInterface: p.bmcAccess.VendorInterface(), + ManagementInterface: bmcAccess.ManagementInterface(), + PowerInterface: bmcAccess.PowerInterface(), + RAIDInterface: bmcAccess.RAIDInterface(), + VendorInterface: bmcAccess.VendorInterface(), Properties: map[string]interface{}{ "capabilities": bootModeCapabilities[p.host.Status.Provisioning.BootMode], }, @@ -1240,12 +1245,12 @@ func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node) (sameImag return sameImage } -func (p *ironicProvisioner) buildManualCleaningSteps() (cleanSteps []nodes.CleanStep, err error) { +func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails) (cleanSteps []nodes.CleanStep, err error) { // Build raid clean steps - if p.bmcAccess.RAIDInterface() != "no-raid" { + if bmcAccess.RAIDInterface() != "no-raid" { cleanSteps = append(cleanSteps, BuildRAIDCleanSteps(p.host.Status.Provisioning.RAID)...) } else if p.host.Status.Provisioning.RAID != nil { - return nil, fmt.Errorf("RAID settings are defined, but the node's driver %s does not support RAID", p.bmcAccess.Driver()) + return nil, fmt.Errorf("RAID settings are defined, but the node's driver %s does not support RAID", bmcAccess.Driver()) } // TODO: Add manual cleaning steps for host configuration @@ -1253,8 +1258,8 @@ func (p *ironicProvisioner) buildManualCleaningSteps() (cleanSteps []nodes.Clean return } -func (p *ironicProvisioner) startManualCleaning(ironicNode *nodes.Node) (success bool, result provisioner.Result, err error) { - if p.bmcAccess.RAIDInterface() != "no-raid" { +func (p *ironicProvisioner) startManualCleaning(bmcAccess bmc.AccessDetails, ironicNode *nodes.Node) (success bool, result provisioner.Result, err error) { + if bmcAccess.RAIDInterface() != "no-raid" { // Set raid configuration err = setTargetRAIDCfg(p, ironicNode) if err != nil { @@ -1264,7 +1269,7 @@ func (p *ironicProvisioner) startManualCleaning(ironicNode *nodes.Node) (success } // Build manual clean steps - cleanSteps, err := p.buildManualCleaningSteps() + cleanSteps, err := p.buildManualCleaningSteps(bmcAccess) if err != nil { result, err = operationFailed(err.Error()) return @@ -1288,8 +1293,13 @@ func (p *ironicProvisioner) startManualCleaning(ironicNode *nodes.Node) (success // Prepare remove existing configuration and set new configuration. // If `started` is true, it means that we successfully executed `tryChangeNodeProvisionState`. func (p *ironicProvisioner) Prepare(unprepared bool) (result provisioner.Result, started bool, err error) { - var ironicNode *nodes.Node + bmcAccess, err := p.bmcAccess() + if err != nil { + result, err = transientError(err) + return + } + var ironicNode *nodes.Node if ironicNode, err = p.findExistingHost(); err != nil { result, err = transientError(errors.Wrap(err, "could not find host to clean")) return @@ -1302,7 +1312,7 @@ func (p *ironicProvisioner) Prepare(unprepared bool) (result provisioner.Result, switch nodes.ProvisionState(ironicNode.ProvisionState) { case nodes.Available: var cleanSteps []nodes.CleanStep - cleanSteps, err = p.buildManualCleaningSteps() + cleanSteps, err = p.buildManualCleaningSteps(bmcAccess) if err != nil { result, err = operationFailed(err.Error()) return @@ -1318,7 +1328,7 @@ func (p *ironicProvisioner) Prepare(unprepared bool) (result provisioner.Result, case nodes.Manageable: if unprepared { - started, result, err = p.startManualCleaning(ironicNode) + started, result, err = p.startManualCleaning(bmcAccess, ironicNode) return } // Manual clean finished diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go index b3a5510f6a..7369fba723 100644 --- a/pkg/provisioner/ironic/validatemanagementaccess_test.go +++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go @@ -646,3 +646,26 @@ func TestValidateManagementAccessUnsupportedSecureBoot(t *testing.T) { } assert.Contains(t, result.ErrorMessage, "does not support secure boot") } + +func TestValidateManagementAccessNoBMCDetails(t *testing.T) { + ironic := testserver.NewIronic(t).Ready() + ironic.Start() + defer ironic.Stop() + + host := makeHost() + host.Spec.BMC = metal3v1alpha1.BMCDetails{} + + auth := clients.AuthConfig{Type: clients.NoAuth} + prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher, + ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth, + ) + if err != nil { + t.Fatalf("could not create provisioner: %s", err) + } + + result, _, err := prov.ValidateManagementAccess(false, false) + if err != nil { + t.Fatalf("error from ValidateManagementAccess: %s", err) + } + assert.Equal(t, "failed to parse BMC address information: missing BMC address", result.ErrorMessage) +} From da3bd7e31cff025aef3be34d33773f427b2632ca Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Thu, 18 Mar 2021 09:03:42 -0400 Subject: [PATCH 3/3] Remove empty provisioner Now that the ironic provisioner only requires BMC details when registering (which does not occur in the unmanaged state), remove the empty provisioner workaround. --- pkg/provisioner/empty/empty.go | 94 -------------------------------- pkg/provisioner/ironic/ironic.go | 5 -- 2 files changed, 99 deletions(-) delete mode 100644 pkg/provisioner/empty/empty.go diff --git a/pkg/provisioner/empty/empty.go b/pkg/provisioner/empty/empty.go deleted file mode 100644 index a12826ca8e..0000000000 --- a/pkg/provisioner/empty/empty.go +++ /dev/null @@ -1,94 +0,0 @@ -package empty - -import ( - logz "sigs.k8s.io/controller-runtime/pkg/log/zap" - - metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" - "github.com/metal3-io/baremetal-operator/pkg/bmc" - "github.com/metal3-io/baremetal-operator/pkg/provisioner" -) - -var log = logz.New().WithName("provisioner").WithName("empty") - -// Provisioner implements the provisioning.Provisioner interface -type emptyProvisioner struct { -} - -// New returns a new Empty Provisioner -func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { - return &emptyProvisioner{}, nil -} - -// ValidateManagementAccess tests the connection information for the -// host to verify that the location and credentials work. -func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (provisioner.Result, string, error) { - return provisioner.Result{}, "", nil -} - -// InspectHardware updates the HardwareDetails field of the host with -// details of devices discovered on the hardware. It may be called -// multiple times, and should return true for its dirty flag until the -// inspection is completed. -func (p *emptyProvisioner) InspectHardware(force bool) (provisioner.Result, *metal3v1alpha1.HardwareDetails, error) { - return provisioner.Result{}, nil, nil -} - -// UpdateHardwareState fetches the latest hardware state of the server -// and updates the HardwareDetails field of the host with details. It -// is expected to do this in the least expensive way possible, such as -// reading from a cache. -func (p *emptyProvisioner) UpdateHardwareState() (provisioner.HardwareState, error) { - return provisioner.HardwareState{}, nil -} - -// Adopt allows an externally-provisioned server to be adopted. -func (p *emptyProvisioner) Adopt(force bool) (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// Prepare remove existing configuration and set new configuration -func (p *emptyProvisioner) Prepare(unprepared bool) (result provisioner.Result, started bool, err error) { - return provisioner.Result{}, false, nil -} - -// Provision writes the image from the host spec to the host. It may -// be called multiple times, and should return true for its dirty flag -// until the deprovisioning operation is completed. -func (p *emptyProvisioner) Provision(hostConf provisioner.HostConfigData) (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// Deprovision removes the host from the image. It may be called -// multiple times, and should return true for its dirty flag until the -// deprovisioning operation is completed. -func (p *emptyProvisioner) Deprovision(force bool) (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// Delete removes the host from the provisioning system. It may be -// called multiple times, and should return true for its dirty flag -// until the deprovisioning operation is completed. -func (p *emptyProvisioner) Delete() (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// PowerOn ensures the server is powered on independently of any image -// provisioning operation. -func (p *emptyProvisioner) PowerOn() (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// PowerOff ensures the server is powered off independently of any image -// provisioning operation. -func (p *emptyProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (provisioner.Result, error) { - return provisioner.Result{}, nil -} - -// IsReady always returns true for the empty provisioner -func (p *emptyProvisioner) IsReady() (bool, error) { - return true, nil -} - -func (p *emptyProvisioner) HasProvisioningCapacity() (result bool, err error) { - return true, nil -} diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index bc09154ab5..f45506e2b5 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -20,7 +20,6 @@ import ( "github.com/metal3-io/baremetal-operator/pkg/bmc" "github.com/metal3-io/baremetal-operator/pkg/hardware" "github.com/metal3-io/baremetal-operator/pkg/provisioner" - "github.com/metal3-io/baremetal-operator/pkg/provisioner/empty" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/devicehints" "github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/hardwaredetails" @@ -206,10 +205,6 @@ func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds // New returns a new Ironic Provisioner using the global configuration // for finding the Ironic services. func New(host metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) { - if !host.HasBMCDetails() { - return empty.New(host, bmcCreds, publisher) - } - var err error if clientIronicSingleton == nil || clientInspectorSingleton == nil { tlsConf := clients.TLSConfig{