Repository: metal3-io/baremetal-operator
Pull request #833 (2021-03-24 21:30:00)

Pass go tests flags via env.
To make local development more convenient this PR add ability to pass go test flags via envars directly to unit-tests that are started in docker/podman instance.

So effectively one could use something like this:
```
CONTAINER_RUNTIME=docker TEST_FLAGS="-failfast" ./hack/unit.sh
```

Setting `failfast` flag and removing verbose output is super-helpful on local dev environment when screen is small (less to scroll) and CPU/MEM resources are limited, so running all tests takes some time.

(mainly this is scroll issue actually, but could also be helpful in other cases) 

Signed-off-by: s3rj1k <evasive.gyron@gmail.com>


Patch:
diff --git a/Makefile b/Makefile
index 9379c05a..04fd365d 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
 RUN_NAMESPACE = metal3
-GO_TEST_FLAGS = $(VERBOSE)
+GO_TEST_FLAGS = $(TEST_FLAGS)
 DEBUG = --debug
 COVER_PROFILE = cover.out
 
@@ -63,7 +63,7 @@ test: generate lint manifests unit ## Run common developer tests
 
 .PHONY: unit
 unit: ## Run unit tests
-	go test ./... $(VERBOSE) -coverprofile $(COVER_PROFILE)
+	go test ./... $(GO_TEST_FLAGS) -coverprofile $(COVER_PROFILE)
 
 .PHONY: unit-cover
 unit-cover: ## Run unit tests with code coverage
@@ -72,7 +72,7 @@ unit-cover: ## Run unit tests with code coverage
 
 .PHONY: unit-verbose
 unit-verbose: ## Run unit tests with verbose output
-	VERBOSE=-v make unit
+	TEST_FLAGS=-v make unit
 
 ## --------------------------------------
 ## Linter Targets
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index b059c031..634a6254 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -118,8 +118,6 @@ const (
 	// has any sort of error.
 	OperationalStatusError OperationalStatus = "error"
 
-	// OperationalStatusDelayed is the status value for when the host
-	// deployment needs to be delayed to limit simultaneous hosts provisioning
 	OperationalStatusDelayed = "delayed"
 )
 
@@ -548,13 +546,10 @@ type CredentialsStatus struct {
 type RebootMode string
 
 const (
-	// RebootModeHard defined for hard reset of a node
 	RebootModeHard RebootMode = "hard"
-	// RebootModeSoft defined for soft reset of a node
 	RebootModeSoft RebootMode = "soft"
 )
 
-// RebootAnnotationArguments defines the arguments of the RebootAnnotation type
 type RebootAnnotationArguments struct {
 	Mode RebootMode `json:"mode"`
 }
@@ -901,7 +896,6 @@ func (host *BareMetalHost) GetImageChecksum() (string, string, bool) {
 	return host.Spec.Image.GetChecksum()
 }
 
-// GetChecksum method returns the checksum of an image
 func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
 	if image == nil {
 		return
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 6688130f..e36e7d2f 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -94,7 +94,6 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 	}()
 
 	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
-	reqLogger.Info("start")
 
 	// Fetch the BareMetalHost
 	host := &metal3v1alpha1.BareMetalHost{}
diff --git a/hack/unit.sh b/hack/unit.sh
index 03fd741a..05bace2a 100755
--- a/hack/unit.sh
+++ b/hack/unit.sh
@@ -5,20 +5,21 @@ set -eux
 IS_CONTAINER=${IS_CONTAINER:-false}
 ARTIFACTS=${ARTIFACTS:-/tmp}
 CONTAINER_RUNTIME="${CONTAINER_RUNTIME:-podman}"
+TEST_FLAGS="${TEST_FLAGS:--v}"
 
 if [ "${IS_CONTAINER}" != "false" ]; then
   eval "$(go env)"
   cd "${GOPATH}"/src/github.com/metal3-io/baremetal-operator
   export XDG_CACHE_HOME="/tmp/.cache"
 
-  export VERBOSE=-v
   export COVER_PROFILE="${ARTIFACTS}"/cover.out
 
-  make -e unit-cover
+  TEST_FLAGS=${TEST_FLAGS} make -e unit-cover
 
 else
   "${CONTAINER_RUNTIME}" run --rm \
     --env IS_CONTAINER=TRUE \
+    --env TEST_FLAGS="${TEST_FLAGS}" \
     --env DEPLOY_KERNEL_URL=http://172.22.0.1/images/ironic-python-agent.kernel \
     --env DEPLOY_RAMDISK_URL=http://172.22.0.1/images/ironic-python-agent.initramfs \
     --env IRONIC_ENDPOINT=http://localhost:6385/v1/ \
diff --git a/main.go b/main.go
index 5b2c9589..0e982b59 100644
--- a/main.go
+++ b/main.go
@@ -121,7 +121,7 @@ func main() {
 	}
 
 	provisionerFactory := func(host metal3iov1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publish provisioner.EventPublisher) (provisioner.Provisioner, error) {
-		isUnmanaged := !host.HasBMCDetails()
+		isUnmanaged := host.Spec.ExternallyProvisioned && !host.HasBMCDetails()
 
 		hostCopy := host.DeepCopy()
 
diff --git a/pkg/bmc/ibmc.go b/pkg/bmc/ibmc.go
index fe39468e..e080e0c3 100644
--- a/pkg/bmc/ibmc.go
+++ b/pkg/bmc/ibmc.go
@@ -91,7 +91,7 @@ func (a *ibmcAccessDetails) PowerInterface() string {
 }
 
 func (a *ibmcAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *ibmcAccessDetails) VendorInterface() string {
diff --git a/pkg/bmc/idrac.go b/pkg/bmc/idrac.go
index 663eff12..48834541 100644
--- a/pkg/bmc/idrac.go
+++ b/pkg/bmc/idrac.go
@@ -87,7 +87,7 @@ func (a *iDracAccessDetails) PowerInterface() string {
 }
 
 func (a *iDracAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *iDracAccessDetails) VendorInterface() string {
diff --git a/pkg/bmc/ilo4.go b/pkg/bmc/ilo4.go
index 9138c617..bc20f41c 100644
--- a/pkg/bmc/ilo4.go
+++ b/pkg/bmc/ilo4.go
@@ -83,7 +83,7 @@ func (a *iLOAccessDetails) PowerInterface() string {
 }
 
 func (a *iLOAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *iLOAccessDetails) VendorInterface() string {
diff --git a/pkg/bmc/ipmi.go b/pkg/bmc/ipmi.go
index 8c13c9e4..cd813612 100644
--- a/pkg/bmc/ipmi.go
+++ b/pkg/bmc/ipmi.go
@@ -86,7 +86,7 @@ func (a *ipmiAccessDetails) PowerInterface() string {
 }
 
 func (a *ipmiAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *ipmiAccessDetails) VendorInterface() string {
diff --git a/pkg/bmc/redfish.go b/pkg/bmc/redfish.go
index 7f9c6526..11ac9df5 100644
--- a/pkg/bmc/redfish.go
+++ b/pkg/bmc/redfish.go
@@ -111,7 +111,7 @@ func (a *redfishAccessDetails) PowerInterface() string {
 }
 
 func (a *redfishAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *redfishAccessDetails) VendorInterface() string {
diff --git a/pkg/bmc/redfish_virtualmedia.go b/pkg/bmc/redfish_virtualmedia.go
index 88e76ac4..5bcfd802 100644
--- a/pkg/bmc/redfish_virtualmedia.go
+++ b/pkg/bmc/redfish_virtualmedia.go
@@ -79,7 +79,7 @@ func (a *redfishVirtualMediaAccessDetails) PowerInterface() string {
 }
 
 func (a *redfishVirtualMediaAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *redfishVirtualMediaAccessDetails) VendorInterface() string {
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index cb1e0d57..109150ad 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -57,7 +57,6 @@ type fixtureProvisioner struct {
 	state *Fixture
 }
 
-// Fixture contains persistent state for a particular host
 type Fixture struct {
 	// counter to set the provisioner as ready
 	BecomeReadyCounter int
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 1fa34d0d..f93368b2 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -72,7 +72,6 @@ func (e macAddressConflictError) Error() string {
 	return fmt.Sprintf("MAC address %s conflicts with existing node %s", e.Address, e.ExistingNode)
 }
 
-// NewMacAddressConflictError is a wrap for macAddressConflictError error
 func NewMacAddressConflictError(address, node string) error {
 	return macAddressConflictError{Address: address, ExistingNode: node}
 }
@@ -144,8 +143,6 @@ type ironicProvisioner struct {
 	inspector *gophercloud.ServiceClient
 	// a logger configured for this host
 	log logr.Logger
-	// a debug logger configured for this host
-	debugLog logr.Logger
 	// an event publisher for recording significant events
 	publisher provisioner.EventPublisher
 }
@@ -191,8 +188,6 @@ func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds
 		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
 	// we need.
 	clientIronic.Microversion = "1.56"
@@ -203,8 +198,7 @@ func newProvisionerWithIronicClients(host metal3v1alpha1.BareMetalHost, bmcCreds
 		bmcCreds:  bmcCreds,
 		client:    clientIronic,
 		inspector: clientInspector,
-		log:       provisionerLogger,
-		debugLog:  provisionerLogger.V(1),
+		log:       log.WithValues("host", host.Name),
 		publisher: publisher,
 	}
 
@@ -297,7 +291,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 		ironicNode, err = nodes.Get(p.client, p.status.ID).Extract()
 		switch err.(type) {
 		case nil:
-			p.debugLog.Info("found existing node by ID")
+			p.log.Info("found existing node by ID")
 			return ironicNode, nil
 		case gophercloud.ErrDefault404:
 			// Look by ID failed, trying to lookup by hostname in case it was
@@ -313,7 +307,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 	ironicNode, err = nodes.Get(p.client, p.host.Name).Extract()
 	switch err.(type) {
 	case nil:
-		p.debugLog.Info("found existing node by name")
+		p.log.Info("found existing node by name")
 		return ironicNode, nil
 	case gophercloud.ErrDefault404:
 		p.log.Info(
@@ -337,7 +331,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 		ironicNode, err = nodes.Get(p.client, nodeUUID).Extract()
 		switch err.(type) {
 		case nil:
-			p.debugLog.Info("found existing node by ID")
+			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 != "" {
@@ -369,7 +363,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, provID string, err error) {
 	var ironicNode *nodes.Node
 
-	p.debugLog.Info("validating management access")
+	p.log.Info("validating management access")
 
 	ironicNode, err = p.findExistingHost()
 	if err != nil {
@@ -590,16 +584,18 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
 		return
 
 	case nodes.Manageable:
+		p.log.Info("have manageable host")
 		return
 
 	case nodes.Available:
 		// The host is fully registered (and probably wasn't cleanly
 		// deleted previously)
+		p.log.Info("have available host")
 		return
 
 	case nodes.Active:
 		// The host is already running, maybe it's a master?
-		p.debugLog.Info("have active host", "image_source", ironicNode.InstanceInfo["image_source"])
+		p.log.Info("have active host", "image_source", ironicNode.InstanceInfo["image_source"])
 		return
 
 	default:
@@ -713,7 +709,7 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu
 		result, err = operationFailed(status.Error)
 		return
 	}
-	if !status.Finished || (nodes.ProvisionState(ironicNode.ProvisionState) == nodes.Inspecting || nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectWait) {
+	if !status.Finished || nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectWait {
 		p.log.Info("inspection in progress", "started_at", status.StartedAt)
 		result, err = operationContinuing(introspectionRequeueDelay)
 		return
@@ -740,7 +736,7 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu
 // is expected to do this in the least expensive way possible, such as
 // reading from a cache.
 func (p *ironicProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) {
-	p.debugLog.Info("updating hardware state")
+	p.log.Info("updating hardware state")
 
 	ironicNode, err := p.findExistingHost()
 	if err != nil {
@@ -1202,9 +1198,10 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er
 					Target: nodes.TargetAdopt,
 				},
 			)
+		} else {
+			return operationFailed(fmt.Sprintf("Host adoption failed: %s",
+				ironicNode.LastError))
 		}
-		return operationFailed(fmt.Sprintf("Host adoption failed: %s",
-			ironicNode.LastError))
 	case nodes.Active:
 	default:
 	}
@@ -1237,7 +1234,7 @@ func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node) (sameImag
 
 func (p *ironicProvisioner) buildManualCleaningSteps() (cleanSteps []nodes.CleanStep, err error) {
 	// Build raid clean steps
-	if p.bmcAccess.RAIDInterface() != "no-raid" {
+	if p.bmcAccess.RAIDInterface() != "" {
 		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())
@@ -1249,7 +1246,7 @@ func (p *ironicProvisioner) buildManualCleaningSteps() (cleanSteps []nodes.Clean
 }
 
 func (p *ironicProvisioner) startManualCleaning(ironicNode *nodes.Node) (success bool, result provisioner.Result, err error) {
-	if p.bmcAccess.RAIDInterface() != "no-raid" {
+	if p.bmcAccess.RAIDInterface() != "" {
 		// Set raid configuration
 		err = setTargetRAIDCfg(p, ironicNode)
 		if err != nil {
@@ -1832,7 +1829,7 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error
 
 // IsReady checks if the provisioning backend is available
 func (p *ironicProvisioner) IsReady() (result bool, err error) {
-	p.debugLog.Info("verifying ironic provisioner dependencies")
+	p.log.Info("verifying ironic provisioner dependencies")
 
 	checker := newIronicDependenciesChecker(p.client, p.inspector, p.log)
 	return checker.IsReady()


Test patch:
diff --git a/pkg/bmc/access_test.go b/pkg/bmc/access_test.go
index c80b7874..d93f3078 100644
--- a/pkg/bmc/access_test.go
+++ b/pkg/bmc/access_test.go
@@ -450,7 +450,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "ipxe",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -462,7 +462,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "ipxe",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -474,7 +474,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "ipxe",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -498,7 +498,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "ipxe",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -510,7 +510,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "redfish-virtual-media",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -522,7 +522,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "redfish-virtual-media",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -534,7 +534,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "redfish-virtual-media",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
@@ -630,7 +630,7 @@ func TestStaticDriverInfo(t *testing.T) {
 			boot:       "ilo-ipxe",
 			management: "",
 			power:      "",
-			raid:       "no-raid",
+			raid:       "",
 			vendor:     "",
 		},
 
diff --git a/pkg/provisioner/ironic/testbmc/testbmc.go b/pkg/provisioner/ironic/testbmc/testbmc.go
index 5fab5f55..39bc03a5 100644
--- a/pkg/provisioner/ironic/testbmc/testbmc.go
+++ b/pkg/provisioner/ironic/testbmc/testbmc.go
@@ -75,7 +75,7 @@ func (a *testAccessDetails) PowerInterface() string {
 }
 
 func (a *testAccessDetails) RAIDInterface() string {
-	return "no-raid"
+	return ""
 }
 
 func (a *testAccessDetails) VendorInterface() string {
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index 8c8c3e94..56e7f2de 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -168,7 +168,6 @@ func (m *IronicMock) NodeError(name string, errorCode int) *IronicMock {
 	return m
 }
 
-// NodeCreateCallback type is the callback mock for CreateNodes
 type NodeCreateCallback func(node nodes.Node)
 
 // CreateNodes configures the server so POSTing to /v1/nodes saves the data