Skip to content

Commit

Permalink
Refactor and rename ValidateManagementAccess
Browse files Browse the repository at this point in the history 
Validating access is not its primary responsibility: it's main job is to
make sure the Ironic node exists and is correctly configured. For a lack
of a perfect name, I'm calling it Register here.

The renamed method is moved to a new file and split into more manageable
pieces. In the future, we can even unit-test them separately.

Signed-off-by: Dmitry Tantsur <[email protected]>
  • Loading branch information
@dtantsur
dtantsur committed Dec 24, 2024
1 parent ee1cda2 commit 2e96d2a
Show file tree
Hide file tree
Showing 8 changed files with 363 additions and 332 deletions.
2 changes: 1 addition & 1 deletion controllers/metal3.io/baremetalhost_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
return recordActionFailure(info, metal3api.RegistrationError, "failed to read preprovisioningNetworkData")
}

provResult, provID, err := prov.ValidateManagementAccess(
provResult, provID, err := prov.Register(
provisioner.ManagementAccessData{
BootMode: info.host.Status.Provisioning.BootMode,
AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
Expand Down
2 changes: 1 addition & 1 deletion controllers/metal3.io/host_state_machine_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1329,7 +1329,7 @@ func (m *mockProvisioner) calledNoError(methodName string) bool {
return m.callsNoError[methodName]
}

func (m *mockProvisioner) ValidateManagementAccess(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
func (m *mockProvisioner) Register(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
return m.getNextResultByMethod("ValidateManagementAccess"), "", err
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/provisioner/demo/demo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ func (p *demoProvisioner) HasCapacity() (result bool, err error) {
return true, nil
}

// ValidateManagementAccess tests the connection information for the
// Register tests the connection information for the
// host to verify that the location and credentials work.
func (p *demoProvisioner) ValidateManagementAccess(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
func (p *demoProvisioner) Register(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
p.log.Info("testing management access")

hostName := p.objectMeta.Name
Expand Down
4 changes: 2 additions & 2 deletions pkg/provisioner/fixture/fixture.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ func (p *fixtureProvisioner) HasCapacity() (result bool, err error) {
return true, nil
}

// ValidateManagementAccess tests the connection information for the
// Register tests the connection information for the
// host to verify that the location and credentials work.
func (p *fixtureProvisioner) ValidateManagementAccess(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
func (p *fixtureProvisioner) Register(_ provisioner.ManagementAccessData, _, _ bool) (result provisioner.Result, provID string, err error) {
p.log.Info("testing management access")

if p.state.validateError != "" {
Expand Down
257 changes: 0 additions & 257 deletions pkg/provisioner/ironic/ironic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import (
"context"
"fmt"
"net"
"reflect"
"regexp"
"strings"
"time"

Expand Down Expand Up @@ -305,261 +303,6 @@ func (p *ironicProvisioner) createPXEEnabledNodePort(uuid, macAddress string) er
return nil
}

// ValidateManagementAccess registers the host with the provisioning
// system and tests the connection information for the host to verify
// that the location and credentials work.
//
// FIXME(dhellmann): We should rename this method to describe what it
// actually does.
func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.ManagementAccessData, credentialsChanged, restartOnFailure bool) (result provisioner.Result, provID string, err error) {
bmcAccess, err := p.bmcAccess()
if err != nil {
result, err = operationFailed(err.Error())
return result, "", err
}

var ironicNode *nodes.Node
updater := clients.UpdateOptsBuilder(p.log)

p.debugLog.Info("validating management access")

ironicNode, err = p.findExistingHost(p.bootMACAddress)
if err != nil {
switch err.(type) {
case macAddressConflictError:
result, err = operationFailed(err.Error())
default:
result, err = transientError(errors.Wrap(err, "failed to find existing host"))
}
return result, "", err
}

// 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 bmcAccess.NeedsMAC() && p.bootMACAddress == "" {
msg := fmt.Sprintf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type())
p.log.Info(msg)
result, err = operationFailed(msg)
return result, "", err
}

driverInfo := bmcAccess.DriverInfo(p.bmcCreds)
driverInfo = setExternalURL(p, driverInfo)

// If we have not found a node yet, we need to create one
if ironicNode == nil {
p.log.Info("registering host in ironic")

if data.BootMode == metal3api.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 result, "", err
}

inspectInterface, driverErr := p.getInspectInterface(bmcAccess)
if driverErr != nil {
result, err = transientError(driverErr)
return result, "", err
}

nodeCreateOpts := nodes.CreateOpts{
Driver: bmcAccess.Driver(),
BIOSInterface: bmcAccess.BIOSInterface(),
BootInterface: bmcAccess.BootInterface(),
Name: ironicNodeName(p.objectMeta),
DriverInfo: driverInfo,
DeployInterface: p.deployInterface(data),
InspectInterface: inspectInterface,
ManagementInterface: bmcAccess.ManagementInterface(),
PowerInterface: bmcAccess.PowerInterface(),
RAIDInterface: bmcAccess.RAIDInterface(),
VendorInterface: bmcAccess.VendorInterface(),
Properties: map[string]interface{}{
"capabilities": buildCapabilitiesValue(nil, data.BootMode),
},
}

if p.availableFeatures.HasFirmwareUpdates() {
nodeCreateOpts.FirmwareInterface = bmcAccess.FirmwareInterface()
}

ironicNode, err = nodes.Create(p.ctx, p.client, nodeCreateOpts).Extract()
if err == nil {
p.publisher("Registered", "Registered new host")
} else if gophercloud.ResponseCodeIs(err, 409) {
p.log.Info("could not register host in ironic, busy")
result, err = retryAfterDelay(provisionRequeueDelay)
return result, "", err
} else {
result, err = transientError(errors.Wrap(err, "failed to register host in ironic"))
return result, "", err
}

// Store the ID so other methods can assume it is set and so
// we can find the node again later.
provID = ironicNode.UUID

// If we know the MAC, create a port. Otherwise we will have
// to do this after we run the introspection step.
if p.bootMACAddress != "" {
err = p.createPXEEnabledNodePort(ironicNode.UUID, p.bootMACAddress)
if err != nil {
result, err = transientError(err)
return result, provID, err
}
}
} else {
// FIXME(dhellmann): At this point we have found an existing
// node in ironic by looking it up. We need to check its
// settings against what we have in the host, and change them
// if there are differences.
provID = ironicNode.UUID

updater.SetTopLevelOpt("name", ironicNodeName(p.objectMeta), ironicNode.Name)

var bmcAddressChanged bool
newAddress := make(map[string]interface{})
ironicAddress := make(map[string]interface{})
reg := regexp.MustCompile("_address$")
for key, value := range driverInfo {
if reg.MatchString(key) {
newAddress[key] = value
break
}
}
for key, value := range ironicNode.DriverInfo {
if reg.MatchString(key) {
ironicAddress[key] = value
break
}
}
if !reflect.DeepEqual(newAddress, ironicAddress) {
bmcAddressChanged = true
}

// When node exists but has no assigned port to it by Ironic and actuall address (MAC) is present
// in host config and is not allocated to different node lets try to create port for this node.
if p.bootMACAddress != "" {
var nodeHasAssignedPort, addressIsAllocatedToPort bool

nodeHasAssignedPort, err = p.nodeHasAssignedPort(ironicNode)
if err != nil {
result, err = transientError(err)
return result, provID, err
}

if !nodeHasAssignedPort {
addressIsAllocatedToPort, err = p.isAddressAllocatedToPort(p.bootMACAddress)
if err != nil {
result, err = transientError(err)
return result, provID, err
}

if !addressIsAllocatedToPort {
err = p.createPXEEnabledNodePort(ironicNode.UUID, p.bootMACAddress)
if err != nil {
result, err = transientError(err)
return result, provID, err
}
}
}
}

// The actual password is not returned from ironic, so we want to
// update the whole DriverInfo only if the credentials or BMC address
// has changed, otherwise we will be writing on every call to this
// function.
if credentialsChanged || bmcAddressChanged {
p.log.Info("Updating driver info because the credentials and/or the BMC address changed")
updater.SetTopLevelOpt("driver_info", driverInfo, ironicNode.DriverInfo)
}

// We don't return here because we also have to set the
// target provision state to manageable, which happens
// below.
}

// If no PreprovisioningImage builder is enabled we set the Node network_data
// this enables Ironic to inject the network_data into the ramdisk image
if !p.config.havePreprovImgBuilder {
networkDataRaw := data.PreprovisioningNetworkData
if networkDataRaw != "" {
var networkData map[string]interface{}
if yamlErr := yaml.Unmarshal([]byte(networkDataRaw), &networkData); yamlErr != nil {
p.log.Info("failed to unmarshal networkData from PreprovisioningNetworkData")
result, err = transientError(fmt.Errorf("invalid preprovisioningNetworkData: %w", yamlErr))
return result, provID, err
}
numUpdates := len(updater.Updates)
updater.SetTopLevelOpt("network_data", networkData, ironicNode.NetworkData)
if len(updater.Updates) != numUpdates {
p.log.Info("adding preprovisioning network_data for node", "node", ironicNode.UUID)
}
}
}

ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
if !success {
return result, provID, err
}

p.log.Info("current provision state",
"lastError", ironicNode.LastError,
"current", ironicNode.ProvisionState,
"target", ironicNode.TargetProvisionState,
)

// Ensure the node is marked manageable.
switch nodes.ProvisionState(ironicNode.ProvisionState) {
case nodes.Enroll:

// If ironic is reporting an error, stop working on the node.
if ironicNode.LastError != "" && !(credentialsChanged || restartOnFailure) {
result, err = operationFailed(ironicNode.LastError)
return result, provID, err
}

if ironicNode.TargetProvisionState == string(nodes.TargetManage) {
// We have already tried to manage the node and did not
// get an error, so do nothing and keep trying.
result, err = operationContinuing(provisionRequeueDelay)
return result, provID, err
}

result, err = p.changeNodeProvisionState(
ironicNode,
nodes.ProvisionStateOpts{Target: nodes.TargetManage},
)
return result, provID, err

case nodes.Verifying:
// If we're still waiting for the state to change in Ironic,
// return true to indicate that we're dirty and need to be
// reconciled again.
result, err = operationContinuing(provisionRequeueDelay)
return result, provID, err

case nodes.CleanWait,
nodes.Cleaning,
nodes.DeployWait,
nodes.Deploying,
nodes.Inspecting:
// Do not try to update the node if it's in a transient state other than InspectWait - will fail anyway.
result, err = operationComplete()
return result, provID, err

case nodes.Active:
// The host is already running, maybe it's a controlplane host?
p.debugLog.Info("have active host", "image_source", ironicNode.InstanceInfo["image_source"])
fallthrough

default:
result, err = p.configureImages(data, ironicNode, bmcAccess)
return result, provID, err
}
}

func (p *ironicProvisioner) configureImages(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
updater := clients.UpdateOptsBuilder(p.log)

Expand Down
Loading

0 comments on commit 2e96d2a

Please sign in to comment.