Repository: metal3-io/baremetal-operator
Pull request #737 (2020-12-04 21:45:42)

Avoid getting stuck in ValidateManagementAccess due to node state
It is not safe to assume in ValidateManagementAccess() that the ironic
state will eventually converge to manageable, available, or active. The
state may in fact be something (like e.g. adopt failed) that requires
action on the part of the actual provisioning function for the current
state to resolve. We previously weren't allowing that, because
ValidateManagementAccess() was always forcing a requeue whenever it
encountered a node in another state.

Since b7b10d79a86bb8e5f04fc86986c7b84fb1a34871 we call
ValidateManagementAccess() in substantially every provisioning state, so
there are even more node states (e.g. inspect failed) that we could get
stuck in.

Move the code for handling the verifying state so that it runs only in
that state, and allow all other states from manageable on to be handled
by the host state machine.


Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 3e915957..7cb7c6ab 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -230,31 +230,6 @@ func (p *ironicProvisioner) validateNode(ironicNode *nodes.Node) (errorMessage s
 	return "", nil
 }
 
-func (p *ironicProvisioner) listAllPorts(address string) ([]ports.Port, error) {
-	var allPorts []ports.Port
-
-	opts := ports.ListOpts{}
-
-	if address != "" {
-		opts.Address = address
-	}
-
-	pager := ports.List(p.client, opts)
-
-	if pager.Err != nil {
-		return allPorts, pager.Err
-	}
-
-	allPages, err := pager.AllPages()
-
-	if err != nil {
-		return allPorts, err
-	}
-
-	return ports.ExtractPorts(allPages)
-
-}
-
 // Look for an existing registration for the host in Ironic.
 func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err error) {
 	// Try to load the node by UUID
@@ -283,48 +258,11 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 		p.log.Info("found existing node by name")
 		return ironicNode, nil
 	case gophercloud.ErrDefault404:
-		p.log.Info(
-			fmt.Sprintf("node with name %s doesn't exist", p.host.Name))
+		return nil, nil
 	default:
 		return nil, errors.Wrap(err,
 			fmt.Sprintf("failed to find node by name %s", p.host.Name))
 	}
-
-	// Try to load the node by port address
-	p.log.Info("looking for existing node by MAC", "MAC", p.host.Spec.BootMACAddress)
-	allPorts, err := p.listAllPorts(p.host.Spec.BootMACAddress)
-
-	if err != nil {
-		p.log.Info("failed to find an existing port with address", "MAC", p.host.Spec.BootMACAddress)
-		return nil, nil
-	}
-
-	if len(allPorts) > 0 {
-		nodeUUID := allPorts[0].NodeUUID
-		ironicNode, err = nodes.Get(p.client, nodeUUID).Extract()
-		switch err.(type) {
-		case nil:
-			p.log.Info("found existing node by ID")
-
-			// If the node has a name, this means we didn't find it above.
-			if ironicNode.Name != "" {
-				return nil, errors.New(fmt.Sprint("node found by MAC but has a name: ", ironicNode.Name))
-			}
-
-			return ironicNode, nil
-		case gophercloud.ErrDefault404:
-			return nil, errors.Wrap(err,
-				fmt.Sprintf("port exists but linked node doesn't %s", nodeUUID))
-		default:
-			return nil, errors.Wrap(err,
-				fmt.Sprintf("port exists but failed to find linked node by ID %s", nodeUUID))
-		}
-	} else {
-		p.log.Info("port with address doesn't exist", "MAC", p.host.Spec.BootMACAddress)
-	}
-
-	return nil, nil
-
 }
 
 // ValidateManagementAccess registers the host with the provisioning
@@ -504,29 +442,6 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 			p.log.Info("setting provisioning id", "ID", p.status.ID)
 		}
 
-		if ironicNode.Name == "" {
-			updates := nodes.UpdateOpts{
-				nodes.UpdateOperation{
-					Op:    nodes.ReplaceOp,
-					Path:  "/name",
-					Value: p.host.Name,
-				},
-			}
-			ironicNode, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
-			switch err.(type) {
-			case nil:
-			case gophercloud.ErrDefault409:
-				p.log.Info("could not update ironic node name, busy")
-				result.Dirty = true
-				result.RequeueAfter = provisionRequeueDelay
-				return result, nil
-			default:
-				return result, errors.Wrap(err, "failed to update ironc node name")
-			}
-			p.log.Info("updated ironic node name")
-
-		}
-
 		// Look for the case where we previously enrolled this node
 		// and now the credentials have changed.
 		if credentialsChanged {
@@ -590,6 +505,14 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
 		)
 
+	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.RequeueAfter = provisionRequeueDelay
+		result.Dirty = true
+		return result, nil
+
 	case nodes.Manageable:
 		p.log.Info("have manageable host")
 		return result, nil
@@ -606,11 +529,6 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 		return result, nil
 
 	default:
-		// 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.RequeueAfter = provisionRequeueDelay
-		result.Dirty = true
 		return result, nil
 	}
 }


Test patch:
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index 6a3dea91..494767c8 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -5,11 +5,9 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
 	"testing"
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
-	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/ports"
 )
 
 // IronicMock is a test server that implements Ironic's semantics
@@ -232,25 +230,3 @@ func (m *IronicMock) WithNodeValidate(nodeUUID string) *IronicMock {
 	m.ResponseWithCode("/v1/nodes/"+nodeUUID+"/validate", "{}", http.StatusOK)
 	return m
 }
-
-// Port configures the server with a valid response for
-//    [GET] /v1/nodes/<node uuid>/ports
-//    [GET] /v1/ports
-//    [GET] /v1/ports?=address=<node uuid>
-func (m *IronicMock) Port(port ports.Port) *IronicMock {
-	if port.NodeUUID == "" {
-		m.MockServer.t.Error("When using withPort(), the port must include a NodeUUID.")
-	}
-
-	resp := map[string][]ports.Port{
-		"ports": {port},
-	}
-
-	address := url.QueryEscape(port.Address)
-
-	m.ResponseJSON(m.buildURL("/v1/nodes/"+port.NodeUUID+"/ports", http.MethodGet), resp)
-	m.ResponseJSON(m.buildURL("/v1/ports", http.MethodGet), resp)
-	m.ResponseJSON(m.buildURL("/v1/ports?address="+address, http.MethodGet), resp)
-
-	return m
-}
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 24d69fe9..cd859fce 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -1,11 +1,9 @@
 package ironic
 
 import (
-	"net/http"
 	"testing"
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
-	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/ports"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
@@ -138,6 +136,116 @@ func TestValidateManagementAccessExistingNode(t *testing.T) {
 	assert.Equal(t, "uuid", host.Status.Provisioning.ID)
 }
 
+func TestValidateManagementAccessExistingNodeContinue(t *testing.T) {
+	statuses := []nodes.ProvisionState{
+		nodes.Manageable,
+		nodes.Available,
+		nodes.Active,
+		nodes.DeployWait,
+		nodes.Deploying,
+		nodes.DeployFail,
+		nodes.DeployDone,
+		nodes.Deleting,
+		nodes.Deleted,
+		nodes.Cleaning,
+		nodes.CleanWait,
+		nodes.CleanFail,
+		nodes.Error,
+		nodes.Rebuild,
+		nodes.Inspecting,
+		nodes.InspectFail,
+		nodes.InspectWait,
+		nodes.Adopting,
+		nodes.AdoptFail,
+		nodes.Rescue,
+		nodes.RescueFail,
+		nodes.Rescuing,
+		nodes.UnrescueFail,
+	}
+
+	for _, status := range statuses {
+		t.Run(string(status), func(t *testing.T) {
+			// Create a host without a bootMACAddress and with a BMC that
+			// does not require one.
+			host := makeHost()
+			host.Spec.BootMACAddress = ""
+			host.Status.Provisioning.ID = "" // so we don't lookup by uuid
+
+			createCallback := func(node nodes.Node) {
+				t.Fatal("create callback should not be invoked for existing node")
+			}
+
+			ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(nodes.Node{
+				Name:           host.Name,
+				UUID:           "", // to match status in host
+				ProvisionState: string(status),
+			})
+			ironic.Start()
+			defer ironic.Stop()
+
+			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)
+			if err != nil {
+				t.Fatalf("error from ValidateManagementAccess: %s", err)
+			}
+			assert.Equal(t, "", result.ErrorMessage)
+			assert.Equal(t, false, result.Dirty)
+		})
+	}
+}
+
+func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) {
+	statuses := []nodes.ProvisionState{
+		nodes.Enroll,
+		nodes.Verifying,
+	}
+
+	for _, status := range statuses {
+		t.Run(string(status), func(t *testing.T) {
+			// Create a host without a bootMACAddress and with a BMC that
+			// does not require one.
+			host := makeHost()
+			host.Spec.BootMACAddress = ""
+			host.Status.Provisioning.ID = "uuid"
+
+			createCallback := func(node nodes.Node) {
+				t.Fatal("create callback should not be invoked for existing node")
+			}
+
+			node := nodes.Node{
+				Name:           host.Name,
+				UUID:           "uuid", // to match status in host
+				ProvisionState: string(status),
+			}
+			ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(node).WithNodeStatesProvisionUpdate(node.UUID)
+			ironic.Start()
+			defer ironic.Stop()
+
+			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)
+			if err != nil {
+				t.Fatalf("error from ValidateManagementAccess: %s", err)
+			}
+			assert.Equal(t, "", result.ErrorMessage)
+			assert.Equal(t, true, result.Dirty)
+		})
+	}
+}
+
 func TestValidateManagementAccessNewCredentials(t *testing.T) {
 	// Create a host without a bootMACAddress and with a BMC that
 	// does not require one.
@@ -181,169 +289,3 @@ func TestValidateManagementAccessNewCredentials(t *testing.T) {
 	newValues := updates[0].Value.(map[string]interface{})
 	assert.Equal(t, "test.bmc", newValues["test_address"])
 }
-
-func TestValidateManagementAccessLinkExistingIronicNodeByMAC(t *testing.T) {
-	// Create an Ironic node, and then create a host with a matching MAC
-	// Test to see if the node was found, and if the link is made
-
-	existingNode := nodes.Node{
-		UUID: "33ce8659-7400-4c68-9535-d10766f07a58",
-	}
-
-	existingNodePort := ports.Port{
-		NodeUUID: existingNode.UUID,
-		Address:  "11:11:11:11:11:11",
-	}
-
-	createCallback := func(node nodes.Node) {
-		t.Fatal("create callback should not be invoked for existing node")
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(existingNode).Port(existingNodePort)
-	ironic.AddDefaultResponse("/v1/nodes/myhost", "GET", http.StatusNotFound, "")
-	ironic.AddDefaultResponse("/v1/nodes/"+existingNode.UUID, "PATCH", http.StatusOK, "{}")
-	ironic.Start()
-	defer ironic.Stop()
-
-	host := makeHost()
-	host.Spec.BootMACAddress = "11:11:11:11:11:11"
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	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)
-	if err != nil {
-		t.Fatalf("error from ValidateManagementAccess: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.NotEqual(t, "", host.Status.Provisioning.ID)
-}
-
-func TestValidateManagementAccessExistingPortWithWrongUUID(t *testing.T) {
-	// Create a node, and a port.  The port has a node uuid that doesn't match the node.
-	// ValidateManagementAccess should return an error.
-
-	existingNode := nodes.Node{
-		UUID: "33ce8659-7400-4c68-9535-d10766f07a58",
-	}
-
-	existingNodePort := ports.Port{
-		NodeUUID: "random-wrong-id",
-		Address:  "11:11:11:11:11:11",
-	}
-
-	createCallback := func(node nodes.Node) {
-		t.Fatal("create callback should not be invoked for existing node")
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(existingNode).Port(existingNodePort)
-	ironic.AddDefaultResponse("/v1/nodes/myhost", "GET", http.StatusNotFound, "")
-	ironic.AddDefaultResponse("/v1/nodes/random-wrong-id", "GET", http.StatusNotFound, "")
-	ironic.Start()
-	defer ironic.Stop()
-
-	host := makeHost()
-	host.Spec.BootMACAddress = "11:11:11:11:11:11"
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	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)
-	}
-
-	_, err = prov.ValidateManagementAccess(false)
-	assert.EqualError(t, err, "failed to find existing host: port exists but linked node doesn't random-wrong-id: Resource not found")
-}
-
-func TestValidateManagementAccessExistingPortButHasName(t *testing.T) {
-	// Create a node, and a port.
-	// The port is linked to the node.
-	// The port address matches the BMH BootMACAddress.
-	// The node has a name, and the name doesn't match the BMH.
-	// ValidateManagementAccess should return an error.
-
-	existingNode := nodes.Node{
-		UUID: "33ce8659-7400-4c68-9535-d10766f07a58",
-		Name: "wrong-name",
-	}
-
-	existingNodePort := ports.Port{
-		NodeUUID: existingNode.UUID,
-		Address:  "11:11:11:11:11:11",
-	}
-
-	createCallback := func(node nodes.Node) {
-		t.Fatal("create callback should not be invoked for existing node")
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(existingNode).Port(existingNodePort)
-	ironic.AddDefaultResponse("/v1/nodes/myhost", "GET", http.StatusNotFound, "")
-	ironic.Start()
-	defer ironic.Stop()
-
-	host := makeHost()
-	host.Spec.BootMACAddress = "11:11:11:11:11:11"
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	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)
-	}
-
-	_, err = prov.ValidateManagementAccess(false)
-	assert.EqualError(t, err, "failed to find existing host: node found by MAC but has a name: wrong-name")
-}
-
-func TestValidateManagementAccessAddTwoHostsWithSameMAC(t *testing.T) {
-
-	existingNode := nodes.Node{
-		UUID: "33ce8659-7400-4c68-9535-d10766f07a58",
-		Name: "myhost",
-	}
-
-	existingNodePort := ports.Port{
-		NodeUUID: existingNode.UUID,
-		Address:  "11:11:11:11:11:11",
-	}
-
-	createCallback := func(node nodes.Node) {
-		t.Fatal("create callback should not be invoked for existing node")
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(existingNode).Port(existingNodePort)
-	ironic.Start()
-	defer ironic.Stop()
-
-	host := makeHost()
-
-	// This value is differen than the port that actually exists
-	host.Spec.BootMACAddress = "22:22:22:22:22:22"
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	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)
-	if err != nil {
-		t.Fatalf("error from ValidateManagementAccess: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.NotEqual(t, "", host.Status.Provisioning.ID)
-}