Repository: metal3-io/baremetal-operator
Pull request #987 (2021-09-23 14:09:07)

Fix non-existing fmt target usage
This fixed non-existing `fmt` target in the Makefile. Also, updates `fmt-check` target to actually do the formatting.
Currently, if there is formatting issue running `./hack/gofmt.sh` would print
```
fmuyassarov@est:~/go/src/github/baremetal-operator$ ./hack/gofmt.sh 
+ IS_CONTAINER=false
+ CONTAINER_RUNTIME=docker
+ [ false != false ]
+ docker run --rm --env IS_CONTAINER=TRUE --volume /home/fmuyassarov/go/src/github/baremetal-operator:/workdir:rw,z --entrypoint sh --workdir /workdir registry.hub.docker.com/library/golang:1.16 /workdir/hack/gofmt.sh
+ IS_CONTAINER=TRUE
+ CONTAINER_RUNTIME=podman
+ [ TRUE != false ]
+ TOP_DIR=.
+ export XDG_CACHE_HOME=/tmp/.cache
+ cd .
+ gofmt -l .
+ [ -n apis/metal3.io/v1alpha1/baremetalhost_types.go ]
+ make fmt
make: *** No rule to make target 'fmt'.  Stop.
```

and after this patch, 
```
fmuyassarov@est:~/go/src/github/baremetal-operator$ ./hack/gofmt.sh 
+ IS_CONTAINER=false
+ CONTAINER_RUNTIME=docker
+ [ false != false ]
+ docker run --rm --env IS_CONTAINER=TRUE --volume /home/fmuyassarov/go/src/github/baremetal-operator:/workdir:rw,z --entrypoint sh --workdir /workdir registry.hub.docker.com/library/golang:1.16 /workdir/hack/gofmt.sh
+ IS_CONTAINER=TRUE
+ CONTAINER_RUNTIME=podman
+ [ TRUE != false ]
+ TOP_DIR=.
+ export XDG_CACHE_HOME=/tmp/.cache
+ cd .
+ gofmt -l .
+ [ -n apis/metal3.io/v1alpha1/baremetalhost_types.go ]
+ echo Please run make fmt-check to fix formatting issues
+ exit 1
Please run make fmt-check to fix formatting issues
```
and I'm informed that I need to run make `fmt-check` to fix formatting.





metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **fmuyassarov** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fmuyassarov"]} -->
fmuyassarov: I accidentally spotted it after https://kubernetes.slack.com/archives/CHD49TLE7/p1632318260088000
fmuyassarov: /retitle Fix non-existing fmt target usage
fmuyassarov: /cc @kashifest 
kashifest: /lgtm
kashifest: /test-integration
fmuyassarov: > /test-integration

I think CI failure is not about this patch but the issue which is being fixed in https://github.com/metal3-io/metal3-dev-env/pull/806. So, I will rerun the test again once 806 is merged.
fmuyassarov: /test-integration
metal3-io-bot: New changes are detected. LGTM label has been removed.

Patch:
diff --git a/Makefile b/Makefile
index dba44c90..8cc8aea2 100644
--- a/Makefile
+++ b/Makefile
@@ -182,6 +182,10 @@ generate-check-local:
 fmt-check: ## Run gofmt and report an error if any changes are made
 	./hack/gofmt.sh
 
+.PHONY: fmt
+fmt: ## Run gofmt and fix files with formatting issues
+	gofmt -s -w .
+
 ## --------------------------------------
 ## Documentation
 ## --------------------------------------
diff --git a/hack/gofmt.sh b/hack/gofmt.sh
index 561d7eaa..8e058345 100755
--- a/hack/gofmt.sh
+++ b/hack/gofmt.sh
@@ -13,7 +13,7 @@ if [ "${IS_CONTAINER}" != "false" ]; then
 
   # shellcheck disable=SC2086
   if [ -n "$(gofmt -l .)" ]; then
-      make fmt
+      gofmt -s -d -e .
       exit 1
   fi
 else
diff --git a/ironic-deployment/ironic/ironic.yaml b/ironic-deployment/ironic/ironic.yaml
index cb3dc54b..e6cc3651 100644
--- a/ironic-deployment/ironic/ironic.yaml
+++ b/ironic-deployment/ironic/ironic.yaml
@@ -101,6 +101,14 @@ spec:
           envFrom:
             - configMapRef:
                 name: ironic-bmo-configmap
+        - name: ironic-inspector-log-watch
+          image: quay.io/metal3-io/ironic
+          imagePullPolicy: Always
+          command:
+            - /bin/runlogwatch.sh
+          volumeMounts:
+            - mountPath: /shared
+              name: ironic-data-volume
       initContainers:
         - name: ironic-ipa-downloader
           image: quay.io/metal3-io/ironic-ipa-downloader
diff --git a/pkg/provisioner/ironic/errors.go b/pkg/provisioner/ironic/errors.go
new file mode 100644
index 00000000..8a69c0cb
--- /dev/null
+++ b/pkg/provisioner/ironic/errors.go
@@ -0,0 +1,19 @@
+package ironic
+
+// SoftPowerOffUnsupportedError is returned when the BMC does not
+// support soft power off.
+type SoftPowerOffUnsupportedError struct {
+}
+
+func (e SoftPowerOffUnsupportedError) Error() string {
+	return "soft power off is unsupported on BMC"
+}
+
+// HostLockedError is returned when the BMC host is
+// locked.
+type HostLockedError struct {
+}
+
+func (e HostLockedError) Error() string {
+	return "BMC host is locked"
+}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index a3c98fc5..d12ee570 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -32,9 +32,9 @@ var (
 
 const (
 	// See nodes.Node.PowerState for details
-	powerOn              = string(nodes.PowerOn)
-	powerOff             = string(nodes.PowerOff)
-	softPowerOff         = string(nodes.SoftPowerOff)
+	powerOn              = "power on"
+	powerOff             = "power off"
+	softPowerOff         = "soft power off"
 	powerNone            = "None"
 	nameSeparator        = "~"
 	customDeployPriority = 80
@@ -1492,20 +1492,6 @@ func (p *ironicProvisioner) Detach() (result provisioner.Result, err error) {
 	return p.Delete()
 }
 
-// softPowerOffUnsupportedError is returned when the BMC does not
-// support soft power off.
-type softPowerOffUnsupportedError struct {
-	cause error
-}
-
-func (e softPowerOffUnsupportedError) Unwrap() error {
-	return e.cause
-}
-
-func (e softPowerOffUnsupportedError) Error() string {
-	return "soft power off is unsupported on BMC"
-}
-
 func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.TargetPowerState) (result provisioner.Result, err error) {
 	p.log.Info("changing power state")
 
@@ -1520,7 +1506,7 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
 	powerStateOpts := nodes.PowerStateOpts{
 		Target: target,
 	}
-	if target == nodes.SoftPowerOff {
+	if target == softPowerOff {
 		powerStateOpts.Timeout = int(softPowerOffTimeout.Seconds())
 	}
 
@@ -1532,25 +1518,19 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
 	switch changeResult.Err.(type) {
 	case nil:
 		p.log.Info("power change OK")
-		event := map[nodes.TargetPowerState]struct{ Event, Reason string }{
-			nodes.PowerOn:      {Event: "PowerOn", Reason: "Host powered on"},
-			nodes.PowerOff:     {Event: "PowerOff", Reason: "Host powered off"},
-			nodes.SoftPowerOff: {Event: "PowerOff", Reason: "Host soft powered off"},
-		}[target]
-		p.publisher(event.Event, event.Reason)
 		return operationContinuing(0)
 	case gophercloud.ErrDefault409:
 		p.log.Info("host is locked, trying again after delay", "delay", powerRequeueDelay)
-		return retryAfterDelay(powerRequeueDelay)
+		result, _ = retryAfterDelay(powerRequeueDelay)
+		return result, HostLockedError{}
 	case gophercloud.ErrDefault400:
 		// Error 400 Bad Request means target power state is not supported by vendor driver
-		if target == nodes.SoftPowerOff {
-			changeResult.Err = softPowerOffUnsupportedError{changeResult.Err}
-		}
+		p.log.Info("power change error", "message", changeResult.Err)
+		return result, SoftPowerOffUnsupportedError{}
+	default:
+		p.log.Info("power change error", "message", changeResult.Err)
+		return transientError(errors.Wrap(changeResult.Err, "failed to change power state"))
 	}
-	p.log.Info("power change error", "message", changeResult.Err)
-	return transientError(errors.Wrap(changeResult.Err,
-		fmt.Sprintf("failed to %s node", target)))
 }
 
 // PowerOn ensures the server is powered on independently of any image
@@ -1576,7 +1556,16 @@ func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err
 			return operationFailed(fmt.Sprintf("PowerOn operation failed: %s",
 				ironicNode.LastError))
 		}
-		return p.changePower(ironicNode, nodes.PowerOn)
+		if result, err = p.changePower(ironicNode, nodes.PowerOn); err == nil {
+			p.publisher("PowerOn", "Host powered on")
+			return result, nil
+		}
+		switch err.(type) {
+		case HostLockedError:
+			return retryAfterDelay(powerRequeueDelay)
+		default:
+			return transientError(errors.Wrap(err, "failed to PowerOn node"))
+		}
 	}
 	return result, nil
 }
@@ -1586,6 +1575,55 @@ func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err
 func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 	p.log.Info(fmt.Sprintf("ensuring host is powered off (mode: %s)", rebootMode))
 
+	if rebootMode == metal3v1alpha1.RebootModeSoft {
+		return p.softPowerOff()
+	}
+	// Reboot mode is hard or force flag is set
+	return p.hardPowerOff(force)
+}
+
+// hardPowerOff sends 'power off' request to BM node and waits for the result
+func (p *ironicProvisioner) hardPowerOff(force bool) (result provisioner.Result, err error) {
+	p.log.Info("ensuring host is powered off by \"hard power off\" command")
+
+	ironicNode, err := p.getNode()
+	if err != nil {
+		return transientError(err)
+	}
+
+	if ironicNode.PowerState != powerOff {
+		if ironicNode.LastError != "" && !force {
+			p.log.Info("hard power off error", "msg", ironicNode.LastError)
+			return operationFailed(ironicNode.LastError)
+		}
+		if ironicNode.TargetPowerState == powerOff {
+			p.log.Info("waiting for power status to change")
+			return operationContinuing(powerRequeueDelay)
+		}
+		result, err = p.changePower(ironicNode, nodes.PowerOff)
+		if err != nil {
+			switch err.(type) {
+			case HostLockedError:
+				return retryAfterDelay(powerRequeueDelay)
+			default:
+				return transientError(errors.Wrap(err, "failed to power off host"))
+			}
+		}
+		p.publisher("PowerOff", "Host powered off")
+		return result, err
+	}
+
+	return operationComplete()
+}
+
+// softPowerOff sends 'soft power off' request to BM node.
+// If soft power off is not supported, the request ends with an error.
+// Otherwise the request ends with no error and the result should be
+// checked later via node fields "power_state", "target_power_state"
+// and "last_error".
+func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error) {
+	p.log.Info("ensuring host is powered off by \"soft power off\" command")
+
 	ironicNode, err := p.getNode()
 	if err != nil {
 		return transientError(err)
@@ -1599,23 +1637,24 @@ func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force
 			return operationContinuing(powerRequeueDelay)
 		}
 		// If the target state is unset while the last error is set,
-		// then the last execution of power off has failed.
-		if targetState == "" && ironicNode.LastError != "" && !force {
-			p.log.Info("power off error", "msg", ironicNode.LastError)
+		// then the last execution of soft power off has failed.
+		if targetState == "" && ironicNode.LastError != "" {
+			p.log.Info("soft power off error", "msg", ironicNode.LastError)
 			return operationFailed(ironicNode.LastError)
 		}
-
-		if rebootMode == metal3v1alpha1.RebootModeSoft && !force {
-			result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
-			if !errors.As(err, &softPowerOffUnsupportedError{}) {
-				return result, err
+		result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
+		if err != nil {
+			switch err.(type) {
+			case HostLockedError:
+				return retryAfterDelay(powerRequeueDelay)
+			default:
+				return transientError(errors.Wrap(err, "failed to power off host"))
 			}
 		}
-		// Reboot mode is hard, force flag is set, or soft power off is not supported
-		return p.changePower(ironicNode, nodes.PowerOff)
+		p.publisher("PowerOff", "Host soft powered off")
 	}
 
-	return operationComplete()
+	return result, nil
 }
 
 func ironicNodeName(objMeta metav1.ObjectMeta) string {
diff --git a/tools/run_local_ironic.sh b/tools/run_local_ironic.sh
index 8e9e0ae7..917f343a 100755
--- a/tools/run_local_ironic.sh
+++ b/tools/run_local_ironic.sh
@@ -255,3 +255,9 @@ sudo "${CONTAINER_RUNTIME}" run -d --net host --privileged --name ironic-inspect
      --env-file "${IRONIC_DATA_DIR}/ironic-vars.env" \
      --entrypoint /bin/runironic-inspector \
      -v "$IRONIC_DATA_DIR:/shared" "${IRONIC_INSPECTOR_IMAGE}"
+
+# Start ironic-inspector-log-watch
+# shellcheck disable=SC2086
+sudo "${CONTAINER_RUNTIME}" run -d --net host --privileged --name ironic-inspector-log-watch \
+    ${POD} --entrypoint /bin/runlogwatch.sh \
+     -v "$IRONIC_DATA_DIR:/shared" "${IRONIC_INSPECTOR_IMAGE}"


Test patch:
diff --git a/pkg/provisioner/ironic/power_test.go b/pkg/provisioner/ironic/power_test.go
index 628f6cf0..0a899d5f 100644
--- a/pkg/provisioner/ironic/power_test.go
+++ b/pkg/provisioner/ironic/power_test.go
@@ -1,7 +1,6 @@
 package ironic
 
 import (
-	"errors"
 	"net/http"
 	"testing"
 	"time"
@@ -145,8 +144,7 @@ func TestPowerOn(t *testing.T) {
 }
 
 func TestPowerOff(t *testing.T) {
-	hardPowerOffReason := "Host powered off"
-	softPowerOffReason := "Host soft powered off"
+
 	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
 	cases := []struct {
 		name   string
@@ -157,7 +155,6 @@ func TestPowerOff(t *testing.T) {
 		expectedError        bool
 		expectedRequestAfter int
 		expectedErrorResult  bool
-		expectedReason       string
 		rebootMode           metal3v1alpha1.RebootMode
 	}{
 		{
@@ -181,28 +178,29 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off normal",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			rebootMode:     metal3v1alpha1.RebootModeSoft,
-			expectedDirty:  true,
-			expectedReason: softPowerOffReason,
+			expectedDirty: true,
+			rebootMode:    metal3v1alpha1.RebootModeSoft,
 		},
 		{
 			name: "power-off hard",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			expectedDirty:  true,
-			rebootMode:     metal3v1alpha1.RebootModeHard,
-			expectedReason: hardPowerOffReason,
+			expectedDirty: true,
+			rebootMode:    metal3v1alpha1.RebootModeHard,
 		},
 		{
 			name: "power-off wait for Provisioning state",
 			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: string(nodes.TargetDeleted),
 				UUID:                 nodeUUID,
 			}),
@@ -213,6 +211,7 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off wait for locked host",
 			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}).WithNodeStatesPower(nodeUUID, http.StatusConflict).WithNodeStatesPowerUpdate(nodeUUID, http.StatusConflict),
@@ -223,18 +222,19 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off soft with force",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			rebootMode:     metal3v1alpha1.RebootModeSoft,
-			force:          true,
-			expectedDirty:  true,
-			expectedReason: hardPowerOffReason,
+			rebootMode:    metal3v1alpha1.RebootModeSoft,
+			force:         true,
+			expectedDirty: true,
 		},
 		{
 			name: "power-off hard with LastError",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 				LastError:            "hard power off failed",
@@ -247,14 +247,14 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off hard with force",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 				LastError:            "hard power off failed",
 			}),
-			rebootMode:     metal3v1alpha1.RebootModeHard,
-			force:          true,
-			expectedDirty:  true,
-			expectedReason: hardPowerOffReason,
+			rebootMode:    metal3v1alpha1.RebootModeHard,
+			force:         true,
+			expectedDirty: true,
 		},
 	}
 
@@ -273,10 +273,7 @@ func TestPowerOff(t *testing.T) {
 
 			host := makeHost()
 			host.Status.Provisioning.ID = nodeUUID
-			var eventReasons []string
-			publisher := func(reason, message string) {
-				eventReasons = append(eventReasons, message)
-			}
+			publisher := func(reason, message string) {}
 			auth := clients.AuthConfig{Type: clients.NoAuth}
 			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
 				tc.ironic.Endpoint(), auth, inspector.Endpoint(), auth,
@@ -298,52 +295,7 @@ func TestPowerOff(t *testing.T) {
 			if tc.expectedErrorResult {
 				assert.Contains(t, result.ErrorMessage, "hard power off failed")
 			}
-			if tc.expectedReason != "" {
-				assert.Len(t, eventReasons, 1)
-				assert.Contains(t, eventReasons, tc.expectedReason)
-			} else {
-				assert.Empty(t, eventReasons)
-			}
-		})
-	}
-}
-
-func TestSoftPowerOffFallback(t *testing.T) {
-	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
-	node := nodes.Node{
-		PowerState: powerOn,
-		UUID:       nodeUUID,
-	}
-	ironic := testserver.NewIronic(t).Ready().Node(node).WithNodeStatesPowerUpdate(nodeUUID, http.StatusBadRequest)
-	ironic.Start()
-	defer ironic.Stop()
-	inspector := testserver.NewInspector(t).Ready()
-	inspector.Start()
-	defer inspector.Stop()
 
-	host := makeHost()
-	host.Status.Provisioning.ID = nodeUUID
-	var eventReasons []string
-	publisher := func(reason, message string) {
-		eventReasons = append(eventReasons, message)
-	}
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
-		ironic.Endpoint(), auth, inspector.Endpoint(), auth,
-	)
-	if err != nil {
-		t.Fatalf("could not create provisioner: %s", err)
+		})
 	}
-
-	_, err = prov.PowerOff(metal3v1alpha1.RebootModeSoft, false)
-	assert.Error(t, err)
-	assert.False(t, errors.As(err, &softPowerOffUnsupportedError{}))
-
-	_, err = prov.changePower(&node, nodes.PowerOff)
-	assert.Error(t, err)
-	assert.False(t, errors.As(err, &softPowerOffUnsupportedError{}))
-
-	_, err = prov.changePower(&node, nodes.SoftPowerOff)
-	assert.Error(t, err)
-	assert.True(t, errors.As(err, &softPowerOffUnsupportedError{}))
 }