Repository: metal3-io/baremetal-operator
Pull request #1427 (2023-11-03 09:11:49)

🌱 Clean up logging and error handling in the webhooks
Logging:
- Rename the webhook loggers for consistency and easier grepping
- Remove redundant logging from the validation code

Errors:
- Provide wrapped errors in a consistent fashion
- Do not refer to internal constant names in user-visible strings
- Provide allowed values when saying that something is not allowed
- Use the stdlib error routines instead of the deprecated pkg/errors.
- Do not shadow the errors package name by local variables

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

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [dtantsur](https://github.com/dtantsur). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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/main/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":["dtantsur"]} -->
dtantsur: /test-centos-e2e-integration-main
/test-ubuntu-integration-main
dtantsur: /test-centos-e2e-integration-main
/test-ubuntu-integration-main
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1427#pullrequestreview-1714064590" title="Approved">zaneb</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)~~ [zaneb]

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":[]} -->

Patch:
diff --git a/apis/go.mod b/apis/go.mod
index 49a9062e..f90eaec1 100644
--- a/apis/go.mod
+++ b/apis/go.mod
@@ -5,7 +5,6 @@ go 1.20
 require (
 	github.com/google/uuid v1.4.0
 	github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.4.0
-	github.com/pkg/errors v0.9.1
 	github.com/stretchr/testify v1.8.4
 	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1
 	k8s.io/api v0.27.7
@@ -34,6 +33,7 @@ require (
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_golang v1.15.1 // indirect
 	github.com/prometheus/client_model v0.4.0 // indirect
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 6b453bae..e1073d85 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -2,6 +2,7 @@ package v1alpha1
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net"
 	"net/url"
@@ -9,19 +10,20 @@ import (
 	"strings"
 
 	"github.com/google/uuid"
-	"github.com/pkg/errors"
 	"golang.org/x/exp/slices"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
 )
 
-// log is for logging in this package.
-var log = logf.Log.WithName("baremetalhost-validation")
+var (
+	supportedRebootModes           = []string{"hard", "soft", ""}
+	supportedRebootModesString     = "\"hard\", \"soft\" or \"\""
+	inspectAnnotationAllowed       = []string{"disabled", ""}
+	inspectAnnotationAllowedString = "\"disabled\" or \"\""
+)
 
 // validateHost validates BareMetalHost resource for creation
 func (host *BareMetalHost) validateHost() []error {
-	log.Info("validate create", "name", host.Name)
 	var errs []error
 	var bmcAccess bmc.AccessDetails
 
@@ -33,8 +35,8 @@ func (host *BareMetalHost) validateHost() []error {
 		}
 	}
 
-	if raid_errors := validateRAID(host.Spec.RAID); raid_errors != nil {
-		errs = append(errs, raid_errors...)
+	if raidErrors := validateRAID(host.Spec.RAID); raidErrors != nil {
+		errs = append(errs, raidErrors...)
 	}
 
 	errs = append(errs, validateBMCAccess(host.Spec, bmcAccess)...)
@@ -67,7 +69,6 @@ func (host *BareMetalHost) validateHost() []error {
 // validateChanges validates BareMetalHost resource on changes
 // but also covers the validations of creation
 func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error {
-	log.Info("validate update", "name", host.Name)
 	var errs []error
 
 	if err := host.validateHost(); err != nil {
@@ -75,11 +76,11 @@ func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error {
 	}
 
 	if old.Spec.BMC.Address != "" && host.Spec.BMC.Address != old.Spec.BMC.Address {
-		errs = append(errs, fmt.Errorf("BMC address can not be changed once it is set"))
+		errs = append(errs, errors.New("BMC address can not be changed once it is set"))
 	}
 
 	if old.Spec.BootMACAddress != "" && host.Spec.BootMACAddress != old.Spec.BootMACAddress {
-		errs = append(errs, fmt.Errorf("bootMACAddress can not be changed once it is set"))
+		errs = append(errs, errors.New("bootMACAddress can not be changed once it is set"))
 	}
 
 	return errs
@@ -123,7 +124,7 @@ func validateBMCAccess(s BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error
 }
 
 func validateRAID(r *RAIDConfig) []error {
-	var errors []error
+	var errs []error
 
 	if r == nil {
 		return nil
@@ -131,37 +132,37 @@ func validateRAID(r *RAIDConfig) []error {
 
 	// check if both hardware and software RAID are specified
 	if len(r.HardwareRAIDVolumes) > 0 && len(r.SoftwareRAIDVolumes) > 0 {
-		errors = append(errors, fmt.Errorf("hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time"))
+		errs = append(errs, errors.New("hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time"))
 	}
 
 	for index, volume := range r.HardwareRAIDVolumes {
 		// check if physicalDisks are specified without a controller
 		if len(volume.PhysicalDisks) != 0 {
 			if volume.Controller == "" {
-				errors = append(errors, fmt.Errorf("'physicalDisks' specified without 'controller' in hardware RAID volume %d", index))
+				errs = append(errs, fmt.Errorf("'physicalDisks' specified without 'controller' in hardware RAID volume %d", index))
 			}
 		}
 		// check if numberOfPhysicalDisks is not same as len(physicalDisks)
 		if volume.NumberOfPhysicalDisks != nil && len(volume.PhysicalDisks) != 0 {
 			if *volume.NumberOfPhysicalDisks != len(volume.PhysicalDisks) {
-				errors = append(errors, fmt.Errorf("the 'numberOfPhysicalDisks'[%d] and number of 'physicalDisks'[%d] is not same for volume %d", *volume.NumberOfPhysicalDisks, len(volume.PhysicalDisks), index))
+				errs = append(errs, fmt.Errorf("the 'numberOfPhysicalDisks'[%d] and number of 'physicalDisks'[%d] is not same for volume %d", *volume.NumberOfPhysicalDisks, len(volume.PhysicalDisks), index))
 			}
 		}
 	}
 
-	return errors
+	return errs
 }
 
 func validateBMHName(bmhname string) error {
 
 	invalidname, _ := regexp.MatchString(`[^A-Za-z0-9\.\-\_]`, bmhname)
 	if invalidname {
-		return fmt.Errorf("BareMetalHost resource name cannot contain characters other than [A-Za-z0-9._-]")
+		return errors.New("BareMetalHost resource name cannot contain characters other than [A-Za-z0-9._-]")
 	}
 
 	_, err := uuid.Parse(bmhname)
 	if err == nil {
-		return fmt.Errorf("BareMetalHost resource name cannot be a UUID")
+		return errors.New("BareMetalHost resource name cannot be a UUID")
 	}
 
 	return nil
@@ -174,11 +175,7 @@ func validateDNSName(hostaddress string) error {
 	}
 
 	_, err := bmc.GetParsedURL(hostaddress)
-	if err != nil {
-		return errors.Wrap(err, "BMO validation")
-	}
-
-	return nil
+	return err // the error has enough context already
 }
 
 func validateAnnotations(host *BareMetalHost) []error {
@@ -216,7 +213,7 @@ func validateStatusAnnotation(statusAnnotation string) error {
 		deco := json.NewDecoder(strings.NewReader(statusAnnotation))
 		deco.DisallowUnknownFields()
 		if err := deco.Decode(objBMHStatus); err != nil {
-			return fmt.Errorf("error decoding status annotation, error=%w", err)
+			return fmt.Errorf("error decoding status annotation: %w", err)
 		}
 
 		if err := checkStatusAnnotation(objBMHStatus); err != nil {
@@ -231,7 +228,7 @@ func validateImageURL(imageURL string) error {
 
 	_, err := url.ParseRequestURI(imageURL)
 	if err != nil {
-		return fmt.Errorf("Image URL %s is an invalid URL", imageURL)
+		return fmt.Errorf("image URL %s is invalid: %w", imageURL, err)
 	}
 
 	return nil
@@ -244,12 +241,12 @@ func validateRootDeviceHints(rdh *RootDeviceHints) error {
 
 	subpath := strings.TrimPrefix(rdh.DeviceName, "/dev/")
 	if rdh.DeviceName == subpath {
-		return fmt.Errorf("Device Name of root device hint must be a /dev/ path, not \"%s\"", rdh.DeviceName)
+		return fmt.Errorf("device name of root device hint must be a /dev/ path, not \"%s\"", rdh.DeviceName)
 	}
 
 	subpath = strings.TrimPrefix(subpath, "disk/by-path/")
 	if strings.Contains(subpath, "/") {
-		return fmt.Errorf("Device Name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"%s\"", rdh.DeviceName)
+		return fmt.Errorf("device name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"%s\"", rdh.DeviceName)
 	}
 	return nil
 }
@@ -261,11 +258,11 @@ func validateRootDeviceHints(rdh *RootDeviceHints) error {
 func checkStatusAnnotation(bmhStatus *BareMetalHostStatus) error {
 
 	if !slices.Contains(OperationalStatusAllowed, string(bmhStatus.OperationalStatus)) {
-		return fmt.Errorf("invalid OperationalStatus='%s' in StatusAnnotation", string(bmhStatus.OperationalStatus))
+		return fmt.Errorf("invalid operationalStatus '%s' in the %s annotation", string(bmhStatus.OperationalStatus), StatusAnnotation)
 	}
 
 	if !slices.Contains(ErrorTypeAllowed, string(bmhStatus.ErrorType)) {
-		return fmt.Errorf("invalid ErrorType='%s' in StatusAnnotation", string(bmhStatus.ErrorType))
+		return fmt.Errorf("invalid errorType '%s' in the %s annotation", string(bmhStatus.ErrorType), StatusAnnotation)
 	}
 
 	return nil
@@ -277,7 +274,7 @@ func validateHwdDetailsAnnotation(hwdDetAnnotation string, inspect string) error
 	}
 
 	if inspect != "disabled" {
-		return fmt.Errorf("inspection has to be disabled for HardwareDetailsAnnotation, check if {'inspect.metal3.io' : 'disabled'}")
+		return errors.New("when hardware details are provided, the inspect.metal3.io annotation must be set to disabled")
 	}
 
 	objHwdDet := &HardwareDetails{}
@@ -285,18 +282,15 @@ func validateHwdDetailsAnnotation(hwdDetAnnotation string, inspect string) error
 	deco := json.NewDecoder(strings.NewReader(hwdDetAnnotation))
 	deco.DisallowUnknownFields()
 	if err := deco.Decode(objHwdDet); err != nil {
-		return fmt.Errorf("error decoding hardware details annotation, error=%w", err)
+		return fmt.Errorf("error decoding the %s annotation: %w", HardwareDetailsAnnotation, err)
 	}
 
 	return nil
 }
 
 func validateInspectAnnotation(inspectAnnotation string) error {
-
-	inspectAnnotationAllowed := []string{"disabled", ""}
-
 	if !slices.Contains(inspectAnnotationAllowed, inspectAnnotation) {
-		return fmt.Errorf("invalid value for Inspect Annotation")
+		return fmt.Errorf("invalid value for the %s annotation, allowed are %v", InspectAnnotationPrefix, inspectAnnotationAllowedString)
 	}
 
 	return nil
@@ -310,13 +304,11 @@ func validateRebootAnnotation(rebootAnnotation string) error {
 	objStatus := &RebootAnnotationArguments{}
 	err := json.Unmarshal([]byte(rebootAnnotation), objStatus)
 	if err != nil {
-		return fmt.Errorf("failed to fetch Reboot from annotation, %w", err)
+		return fmt.Errorf("failed to unmarshal the data from the %s annotation: %w", RebootAnnotationPrefix, err)
 	}
 
-	supportedRebootModes := []string{"hard", "soft", ""}
-
 	if !slices.Contains(supportedRebootModes, string(objStatus.Mode)) {
-		return fmt.Errorf("invalid RebootMode in RebootAnnotation")
+		return fmt.Errorf("invalid mode in the %s annotation, allowed are %v", RebootAnnotationPrefix, supportedRebootModesString)
 	}
 
 	return nil
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_webhook.go b/apis/metal3.io/v1alpha1/baremetalhost_webhook.go
index 55acaaaf..0e5f646f 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_webhook.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_webhook.go
@@ -23,22 +23,22 @@ import (
 )
 
 // log is for logging in this package.
-var baremetalhostlog = logf.Log.WithName("baremetalhost-resource")
+var baremetalhostlog = logf.Log.WithName("webhooks").WithName("BareMetalHost")
 
 //+kubebuilder:webhook:verbs=create;update,path=/validate-metal3-io-v1alpha1-baremetalhost,mutating=false,failurePolicy=fail,sideEffects=none,admissionReviewVersions=v1;v1beta,groups=metal3.io,resources=baremetalhosts,versions=v1alpha1,name=baremetalhost.metal3.io
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *BareMetalHost) ValidateCreate() (admission.Warnings, error) {
-	baremetalhostlog.Info("validate create", "name", r.Name)
+	baremetalhostlog.Info("validate create", "namespace", r.Namespace, "name", r.Name)
 	return nil, errors.NewAggregate(r.validateHost())
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *BareMetalHost) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	baremetalhostlog.Info("validate update", "name", r.Name)
+	baremetalhostlog.Info("validate update", "namespace", r.Namespace, "name", r.Name)
 	bmh, casted := old.(*BareMetalHost)
 	if !casted {
-		baremetalhostlog.Error(fmt.Errorf("old object conversion error"), "validate update error")
+		baremetalhostlog.Error(fmt.Errorf("old object conversion error for %s/%s", r.Namespace, r.Name), "validate update error")
 		return nil, nil
 	}
 	return nil, errors.NewAggregate(r.validateChanges(bmh))
diff --git a/apis/metal3.io/v1alpha1/bmceventsubscription_validation.go b/apis/metal3.io/v1alpha1/bmceventsubscription_validation.go
index 73b86dd6..96ba7544 100644
--- a/apis/metal3.io/v1alpha1/bmceventsubscription_validation.go
+++ b/apis/metal3.io/v1alpha1/bmceventsubscription_validation.go
@@ -16,36 +16,29 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"errors"
 	"fmt"
 	"net/url"
-
-	"github.com/pkg/errors"
-
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// bmclog is for logging in this package.
-var bmclog = logf.Log.WithName("bmceventsubscription-validation")
-
 // validateSubscription validates BMCEventSubscription resource for creation
 func (s *BMCEventSubscription) validateSubscription() []error {
-	bmclog.Info("validate create", "name", s.Name)
 	var errs []error
 
 	if s.Spec.HostName == "" {
-		errs = append(errs, fmt.Errorf("HostName cannot be empty"))
+		errs = append(errs, errors.New("hostName cannot be empty"))
 	}
 
 	if s.Spec.Destination == "" {
-		errs = append(errs, fmt.Errorf("Destination cannot be empty"))
+		errs = append(errs, errors.New("destination cannot be empty"))
 	} else {
 		destinationUrl, err := url.ParseRequestURI(s.Spec.Destination)
 
 		if err != nil {
-			errs = append(errs, errors.Wrap(err, "Destination is an invalid URL"))
+			errs = append(errs, fmt.Errorf("destination is invalid: %w", err))
 		} else {
 			if destinationUrl.Path == "" {
-				errs = append(errs, fmt.Errorf("Hostname-only destination must have a trailing slash"))
+				errs = append(errs, errors.New("hostname-only destination must have a trailing slash"))
 			}
 		}
 	}
diff --git a/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
index 766820be..43a06a4c 100644
--- a/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
+++ b/apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go
@@ -23,13 +23,13 @@ import (
 )
 
 // bmcsubscriptionlog is for logging in this package.
-var bmcsubscriptionlog = logf.Log.WithName("bmceventsubscription-resource")
+var bmcsubscriptionlog = logf.Log.WithName("webhooks").WithName("BMCEventSubscription")
 
 //+kubebuilder:webhook:verbs=create;update,path=/validate-metal3-io-v1alpha1-bmceventsubscription,mutating=false,failurePolicy=fail,sideEffects=none,admissionReviewVersions=v1;v1beta,groups=metal3.io,resources=bmceventsubscriptions,versions=v1alpha1,name=bmceventsubscription.metal3.io
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (s *BMCEventSubscription) ValidateCreate() (admission.Warnings, error) {
-	bmcsubscriptionlog.Info("validate create", "name", s.Name)
+	bmcsubscriptionlog.Info("validate create", "namespace", s.Namespace, "name", s.Name)
 	return nil, errors.NewAggregate(s.validateSubscription())
 }
 
@@ -37,11 +37,11 @@ func (s *BMCEventSubscription) ValidateCreate() (admission.Warnings, error) {
 //
 // We prevent updates to the spec.  All other updates (e.g. status, finalizers) are allowed.
 func (s *BMCEventSubscription) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	bmcsubscriptionlog.Info("validate update", "name", s.Name)
+	bmcsubscriptionlog.Info("validate update", "namespace", s.Namespace, "name", s.Name)
 
 	bes, casted := old.(*BMCEventSubscription)
 	if !casted {
-		bmcsubscriptionlog.Error(fmt.Errorf("old object conversion error"), "validate update error")
+		bmcsubscriptionlog.Error(fmt.Errorf("old object conversion error for %s/%s", s.Namespace, s.Name), "validate update error")
 		return nil, nil
 	}
 
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 8d4f952a..b2452be4 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -326,7 +326,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	}
 
 	var ironicNode *nodes.Node
-	updater := updateOptsBuilder(p.log)
+	updater := updateOptsBuilder(p.debugLog)
 
 	p.debugLog.Info("validating management access")
 
@@ -541,7 +541,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 }
 
 func (p *ironicProvisioner) configureImages(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
-	updater := updateOptsBuilder(p.log)
+	updater := updateOptsBuilder(p.debugLog)
 
 	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage)
 	updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
@@ -710,13 +710,13 @@ func (p *ironicProvisioner) tryUpdateNode(ironicNode *nodes.Node, updater *nodeU
 		return
 	}
 
-	p.log.Info("updating node settings in ironic", "updateCount", len(updater.Updates))
+	p.log.Info("updating node settings in ironic")
 	updatedNode, err = nodes.Update(p.client, ironicNode.UUID, updater.Updates).Extract()
 	switch err.(type) {
 	case nil:
 		success = true
 	case gophercloud.ErrDefault409:
-		p.log.Info("could not update node settings in ironic, busy or update cannot be applied in the current state")
+		p.log.Info("could not update node settings in ironic, busy")
 		result, err = retryAfterDelay(provisionRequeueDelay)
 	default:
 		result, err = transientError(errors.Wrap(err, "failed to update host settings in ironic"))
@@ -780,7 +780,7 @@ func (p *ironicProvisioner) abortInspection(ironicNode *nodes.Node) (result prov
 func (p *ironicProvisioner) startInspection(data provisioner.InspectData, ironicNode *nodes.Node) (result provisioner.Result, started bool, err error) {
 	_, started, result, err = p.tryUpdateNode(
 		ironicNode,
-		updateOptsBuilder(p.log).
+		updateOptsBuilder(p.debugLog).
 			SetPropertiesOpts(optionsData{
 				"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
 			}, ironicNode),
@@ -1042,7 +1042,7 @@ func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, im
 }
 
 func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node, data provisioner.ProvisionData) *nodeUpdater {
-	updater := updateOptsBuilder(p.log)
+	updater := updateOptsBuilder(p.debugLog)
 
 	hasCustomDeploy := data.CustomDeploy != nil && data.CustomDeploy.Method != ""
 	p.getImageUpdateOptsForNode(ironicNode, &data.Image, data.BootMode, hasCustomDeploy, updater)
@@ -1798,7 +1798,7 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) {
 		// Make sure we don't have a stale instance UUID
 		if ironicNode.InstanceUUID != "" {
 			p.log.Info("removing stale instance UUID before deletion", "instanceUUID", ironicNode.InstanceUUID)
-			updater := updateOptsBuilder(p.log)
+			updater := updateOptsBuilder(p.debugLog)
 			updater.SetTopLevelOpt("instance_uuid", nil, ironicNode.InstanceUUID)
 			_, success, result, err := p.tryUpdateNode(ironicNode, updater)
 			if !success {
diff --git a/pkg/provisioner/ironic/raid.go b/pkg/provisioner/ironic/raid.go
index d055af63..569057cc 100644
--- a/pkg/provisioner/ironic/raid.go
+++ b/pkg/provisioner/ironic/raid.go
@@ -43,7 +43,7 @@ func setTargetRAIDCfg(p *ironicProvisioner, raidInterface string, ironicNode *no
 		p.log.Info("rootDeviceHints is used, the first volume of raid will not be set to root")
 	}
 
-	updater := updateOptsBuilder(p.log)
+	updater := updateOptsBuilder(p.debugLog)
 	updater.SetTopLevelOpt("raid_interface", targetRaidInterface, ironicNode.RAIDInterface)
 	ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
 	if !success {
diff --git a/pkg/provisioner/ironic/update_opts.go b/pkg/provisioner/ironic/update_opts.go
index 99e3c89f..dad6e27a 100644
--- a/pkg/provisioner/ironic/update_opts.go
+++ b/pkg/provisioner/ironic/update_opts.go
@@ -100,13 +100,15 @@ func getUpdateOperation(name string, currentData map[string]interface{}, desired
 	desiredValue = deref(desiredValue)
 	if desiredValue != nil {
 		if !(present && optionValueEqual(deref(current), desiredValue)) {
-			if present {
-				log.Info("updating option data",
-					"value", sanitisedValue(desiredValue),
-					"oldValue", current)
-			} else {
-				log.Info("adding option data",
-					"value", sanitisedValue(desiredValue))
+			if log.GetSink() != nil {
+				if present {
+					log.Info("updating option data",
+						"value", sanitisedValue(desiredValue),
+						"old_value", current)
+				} else {
+					log.Info("adding option data",
+						"value", sanitisedValue(desiredValue))
+				}
 			}
 			return &nodes.UpdateOperation{
 				Op:    nodes.AddOp, // Add also does replace
@@ -116,7 +118,9 @@ func getUpdateOperation(name string, currentData map[string]interface{}, desired
 		}
 	} else {
 		if present {
-			log.Info("removing option data")
+			if log.GetSink() != nil {
+				log.Info("removing option data")
+			}
 			return &nodes.UpdateOperation{
 				Op:   nodes.RemoveOp,
 				Path: path,
@@ -138,6 +142,9 @@ func updateOptsBuilder(logger logr.Logger) *nodeUpdater {
 }
 
 func (nu *nodeUpdater) logger(basepath, option string) logr.Logger {
+	if nu.log.GetSink() == nil {
+		return logr.Logger{}
+	}
 	log := nu.log.WithValues("option", option)
 	if basepath != "" {
 		log = log.WithValues("section", basepath[1:])


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index e5dd5aa3..bad5ec27 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -392,7 +392,7 @@ func TestValidateCreate(t *testing.T) {
 					BMC: BMCDetails{
 						Address: "ipmi://-host.example.com.org"}}},
 			oldBMH:    nil,
-			wantedErr: "BMO validation: failed to parse BMC address information: BMC address hostname/IP : [-host.example.com.org] is invalid",
+			wantedErr: "failed to parse BMC address information: BMC address hostname/IP : [-host.example.com.org] is invalid",
 		},
 		{
 			name: "invalidDNSNameinvalidcharacter",
@@ -403,7 +403,7 @@ func TestValidateCreate(t *testing.T) {
 					BMC: BMCDetails{
 						Address: "ipmi://host+1.example.com.org"}}},
 			oldBMH:    nil,
-			wantedErr: "BMO validation: failed to parse BMC address information: BMC address hostname/IP : [host+1.example.com.org] is invalid",
+			wantedErr: "failed to parse BMC address information: BMC address hostname/IP : [host+1.example.com.org] is invalid",
 		},
 		{
 			name: "invalidDNSNameinvalidformat",
@@ -414,7 +414,7 @@ func TestValidateCreate(t *testing.T) {
 					BMC: BMCDetails{
 						Address: "[@]host.example.com"}}},
 			oldBMH:    nil,
-			wantedErr: "BMO validation: failed to parse BMC address information: parse \"ipmi://[@]host.example.com\": net/url: invalid userinfo",
+			wantedErr: "failed to parse BMC address information: parse \"ipmi://[@]host.example.com\": net/url: invalid userinfo",
 		},
 		{
 			name: "invalidDNSNameinvalidbmc",
@@ -436,7 +436,7 @@ func TestValidateCreate(t *testing.T) {
 					BMC: BMCDetails{
 						Address: "ipmi://[fe80::fc33:62ff:fe33:8xff]:6223"}}},
 			oldBMH:    nil,
-			wantedErr: "BMO validation: failed to parse BMC address information: BMC address hostname/IP : [fe80::fc33:62ff:fe33:8xff] is invalid",
+			wantedErr: "failed to parse BMC address information: BMC address hostname/IP : [fe80::fc33:62ff:fe33:8xff] is invalid",
 		},
 		{
 			name: "validRootDeviceHint",
@@ -478,7 +478,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "Device Name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"/dev/disk/by-uuid/cdaacd50-3a4c-421c-91c0-fe9ba7b8b2f1\"",
+			wantedErr: "device name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"/dev/disk/by-uuid/cdaacd50-3a4c-421c-91c0-fe9ba7b8b2f1\"",
 		},
 		{
 			name: "invalidRootDeviceHintNoPath",
@@ -492,7 +492,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "Device Name of root device hint must be a /dev/ path, not \"sda\"",
+			wantedErr: "device name of root device hint must be a /dev/ path, not \"sda\"",
 		},
 		{
 			name: "invalidImageURL",
@@ -510,7 +510,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "Image URL test1 is an invalid URL",
+			wantedErr: "image URL test1 is invalid: parse \"test1\": invalid URI for request",
 		},
 		{
 			name: "validStatusAnnotation",
@@ -540,7 +540,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "error decoding status annotation, error=json: unknown field \"InvalidField\"",
+			wantedErr: "error decoding status annotation: json: unknown field \"InvalidField\"",
 		},
 		{
 			name: "invalidOpstatusStatusAnnotation",
@@ -555,7 +555,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "invalid OperationalStatus='NotOK' in StatusAnnotation",
+			wantedErr: "invalid operationalStatus 'NotOK' in the baremetalhost.metal3.io/status annotation",
 		},
 		{
 			name: "invalidErrtypeStatusAnnotation",
@@ -570,7 +570,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "invalid ErrorType='No Error' in StatusAnnotation",
+			wantedErr: "invalid errorType 'No Error' in the baremetalhost.metal3.io/status annotation",
 		},
 		{
 			name: "invalidFormatStatusAnnotation",
@@ -585,7 +585,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "error decoding status annotation, error=unexpected EOF",
+			wantedErr: "error decoding status annotation: unexpected EOF",
 		},
 		{
 			name: "invalidValueRebootAnnotationPrefix",
@@ -600,7 +600,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "invalid RebootMode in RebootAnnotation",
+			wantedErr: "invalid mode in the reboot.metal3.io annotation, allowed are \"hard\", \"soft\" or \"\"",
 		},
 		{
 			name: "invalidValueRebootAnnotationWithKey",
@@ -615,7 +615,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "invalid RebootMode in RebootAnnotation",
+			wantedErr: "invalid mode in the reboot.metal3.io annotation, allowed are \"hard\", \"soft\" or \"\"",
 		},
 		{
 			name: "inspectionNotDisabledHardwareDetailsAnnotation",
@@ -630,7 +630,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "inspection has to be disabled for HardwareDetailsAnnotation, check if {'inspect.metal3.io' : 'disabled'}",
+			wantedErr: "when hardware details are provided, the inspect.metal3.io annotation must be set to disabled",
 		},
 		{
 			name: "invalidFieldHardwareDetailsAnnotation",
@@ -646,7 +646,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "error decoding hardware details annotation, error=json: unknown field \"INVALIDField\"",
+			wantedErr: "error decoding the inspect.metal3.io/hardwaredetails annotation: json: unknown field \"INVALIDField\"",
 		},
 		{
 			name: "invalidJsonHardwareDetailsAnnotation",
@@ -661,7 +661,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "error decoding hardware details annotation, error=unexpected EOF",
+			wantedErr: "error decoding the inspect.metal3.io/hardwaredetails annotation: unexpected EOF",
 		},
 		{
 			name: "invalidValueInspectAnnotation",
@@ -676,7 +676,7 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 			oldBMH:    nil,
-			wantedErr: "invalid value for Inspect Annotation",
+			wantedErr: "invalid value for the inspect.metal3.io annotation, allowed are \"disabled\" or \"\"",
 		},
 	}
 
diff --git a/apis/metal3.io/v1alpha1/bmceventsubscription_validation_test.go b/apis/metal3.io/v1alpha1/bmceventsubscription_validation_test.go
index 712f53d4..c10514b7 100644
--- a/apis/metal3.io/v1alpha1/bmceventsubscription_validation_test.go
+++ b/apis/metal3.io/v1alpha1/bmceventsubscription_validation_test.go
@@ -37,7 +37,7 @@ func TestBMCEventSubscriptionValidateCreate(t *testing.T) {
 				Spec:       BMCEventSubscriptionSpec{Destination: "http://localhost/abc/abc"},
 			},
 			oldS:      nil,
-			wantedErr: "HostName cannot be empty",
+			wantedErr: "hostName cannot be empty",
 		},
 		{
 			name: "missingDestination",
@@ -47,7 +47,7 @@ func TestBMCEventSubscriptionValidateCreate(t *testing.T) {
 				Spec:       BMCEventSubscriptionSpec{HostName: "worker-01"},
 			},
 			oldS:      nil,
-			wantedErr: "Destination cannot be empty",
+			wantedErr: "destination cannot be empty",
 		},
 		{
 			name: "destinationNotUrl",
@@ -57,7 +57,7 @@ func TestBMCEventSubscriptionValidateCreate(t *testing.T) {
 				Spec:       BMCEventSubscriptionSpec{HostName: "worker-01", Destination: "abc"},
 			},
 			oldS:      nil,
-			wantedErr: "Destination is an invalid URL: parse \"abc\": invalid URI for request",
+			wantedErr: "destination is invalid: parse \"abc\": invalid URI for request",
 		},
 		{
 			name: "destinationMissingTrailingSlash",
@@ -67,7 +67,7 @@ func TestBMCEventSubscriptionValidateCreate(t *testing.T) {
 				Spec:       BMCEventSubscriptionSpec{HostName: "worker-01", Destination: "http://localhost"},
 			},
 			oldS:      nil,
-			wantedErr: "Hostname-only destination must have a trailing slash",
+			wantedErr: "hostname-only destination must have a trailing slash",
 		},
 	}