Repository: metal3-io/baremetal-operator
Pull request #796 (2021-02-15 16:41:42)

Add inspect.metal3.io/hardwaredetails annotation
As described in https://github.com/metal3-io/metal3-docs/pull/155

This enables addition of only the hardware part of the status,
unlike the status annotation which can only be set on the first
reconcile, since it allows all fields of the status to be modified.

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/796#" title="Author self-approved">hardys</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/master/OWNERS)~~ [hardys]

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":[]} -->
hardys: /test-integration
hardys: /test-integration
hardys: /test-integration
metal3-io-bot: @hardys: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gomod | 5b47acb6151fb6135c2193ef35ff76d415b37f63 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/796/gomod/1366440284870676486) | `/test gomod`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @hardys: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gomod | 9da2adff6c6580f90e1aae924f5564a85eac481c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/796/gomod/1366481308821229568) | `/test gomod`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
hardys: /test-integration
hardys: /hold I found an issue while testing
hardys: /hold cancel
hardys: /test-integration
hardys: Thanks for the feedback @andfasano I've tested this locally and it now seems to be working OK, so ready for another review when you get time - also perhaps @dhellmann you could take a look please?  Thanks!
hardys: @dhellmann @andfasano @sadasu thanks for the reviews - I added the events discussed, another test for the case where existing HardwareDetails can be overwritten (when inspection is disabled), and some docs, so I think this is ready for another view pass when you get a moment :)
hardys: /test-integration
andfasano: Changes looks fine to me, leaving final comments to the others
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/796#pullrequestreview-604556069" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/796#" title="Author self-approved">hardys</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/master/OWNERS)~~ [dhellmann,hardys]

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":[]} -->
hardys: /test-integration
hardys: @andfasano @sadasu @dhellmann I think this should be good to go now, PTAL, thanks!
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/796#pullrequestreview-607311971" title="Approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/796#pullrequestreview-604556069" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/796#" title="Author self-approved">hardys</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/master/OWNERS)~~ [dhellmann,hardys]

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":[]} -->
hardys: /test-integration
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/796#pullrequestreview-607311971" title="Approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/796#issuecomment-794162812" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/796#" title="Author self-approved">hardys</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/master/OWNERS)~~ [dhellmann,hardys]

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/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index e959bdf4..712c2937 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -52,6 +52,7 @@ const (
 	provisionerNotReadyRetryDelay = time.Second * 30
 	rebootAnnotationPrefix        = "reboot.metal3.io"
 	inspectAnnotationPrefix       = "inspect.metal3.io"
+	hardwareDetailsAnnotation     = inspectAnnotationPrefix + "/hardwaredetails"
 )
 
 // BareMetalHostReconciler reconciles a BareMetalHost object
@@ -154,6 +155,14 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 		}
 	}
 
+	// Consume hardwaredetails from annotation if present
+	hwdUpdated, err := r.updateHardwareDetails(request, host)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrap(err, "Could not update Hardware Details")
+	} else if hwdUpdated {
+		return ctrl.Result{Requeue: true}, nil
+	}
+
 	// NOTE(dhellmann): Handle a few steps outside of the phase
 	// structure because they require extra data lookup (like the
 	// credential checks) or have to be done "first" (like delete
@@ -259,6 +268,42 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 	return
 }
 
+// Consume inspect.metal3.io/hardwaredetails when either
+// inspect.metal3.io=disabled or there are no existing HardwareDetails
+func (r *BareMetalHostReconciler) updateHardwareDetails(request ctrl.Request, host *metal3v1alpha1.BareMetalHost) (bool, error) {
+	updated := false
+	if host.Status.HardwareDetails == nil || inspectionDisabled(host) {
+		objHardwareDetails, err := r.getHardwareDetailsFromAnnotation(host)
+		if err != nil {
+			return updated, errors.Wrap(err, "Error getting HardwareDetails from annotation")
+		}
+		if objHardwareDetails != nil {
+			host.Status.HardwareDetails = objHardwareDetails
+			err = r.saveHostStatus(host)
+			if err != nil {
+				return updated, errors.Wrap(err, "Could not update hardwaredetails from annotation")
+			}
+			r.publishEvent(request, host.NewEvent("UpdateHardwareDetails", "Set HardwareDetails from annotation"))
+			updated = true
+		}
+	}
+	// We either just processed the annotation, or the status is already set
+	// so we remove it
+	annotations := host.GetAnnotations()
+	if _, present := annotations[hardwareDetailsAnnotation]; present {
+		delete(host.Annotations, hardwareDetailsAnnotation)
+		err := r.Update(context.TODO(), host)
+		if err != nil {
+			return updated, errors.Wrap(err, "Could not update removing hardwaredetails annotation")
+		}
+		// In the case where the value was not just consumed, generate an event
+		if updated != true {
+			r.publishEvent(request, host.NewEvent("RemoveAnnotation", "HardwareDetails annotation ignored, status already set and inspection is not disabled"))
+		}
+	}
+	return updated, nil
+}
+
 func logResult(info *reconcileInfo, result ctrl.Result) {
 	if result.Requeue || result.RequeueAfter != 0 ||
 		!utils.StringInList(info.host.Finalizers,
@@ -988,6 +1033,20 @@ func (r *BareMetalHostReconciler) getHostStatusFromAnnotation(host *metal3v1alph
 	return objStatus, nil
 }
 
+// extract HardwareDetails from annotation if present
+func (r *BareMetalHostReconciler) getHardwareDetailsFromAnnotation(host *metal3v1alpha1.BareMetalHost) (*metal3v1alpha1.HardwareDetails, error) {
+	annotations := host.GetAnnotations()
+	if annotations[hardwareDetailsAnnotation] == "" {
+		return nil, nil
+	}
+	content := []byte(annotations[hardwareDetailsAnnotation])
+	objHardwareDetails := &metal3v1alpha1.HardwareDetails{}
+	if err := json.Unmarshal(content, objHardwareDetails); err != nil {
+		return nil, err
+	}
+	return objHardwareDetails, nil
+}
+
 func (r *BareMetalHostReconciler) setErrorCondition(request ctrl.Request, host *metal3v1alpha1.BareMetalHost, errType metal3v1alpha1.ErrorType, message string) (err error) {
 	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
 
diff --git a/docs/inspectAnnotation.md b/docs/inspectAnnotation.md
new file mode 100644
index 00000000..e07a902b
--- /dev/null
+++ b/docs/inspectAnnotation.md
@@ -0,0 +1,35 @@
+# Inspect Annotation
+
+Baremetalhost's(BMH) _Status_ sub-resource contains a _hardware_ key
+which contains the result of introspection which is carried out during
+BMH registration.
+
+In some circumstances it may be desirable to disable this inspection process,
+and provide data from external source.  The _Inspect Annotation_ provides some
+interfaces to enable this.
+
+Note the `inspect.metal3.io/hardwaredetails` annotation is consumed:
+
+* At any time when `inspect.metal3.io: disabled` is specified
+* When there is no existing HardwareDetails data in the Status
+
+The `inspect.metal3.io/hardwaredetails` annotation will be removed when
+successfully processed or when the status is already set, generating an
+event in each case.
+
+The structure of the annotation's value should match the hardware status
+field schema, or a subset of that schema, for example:
+
+```yaml
+inspect.metal3.io: disabled
+inspect.metal3.io/hardwaredetails: '{"systemVendor":{"manufacturer":"QEMU",
+"productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},
+"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,
+"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6",
+"ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true}],
+"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,
+"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0",
+"hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64",
+"model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,
+"flags":["foo"],"count":4},"hostname":"hwdAnnotation-0"}'
+```
diff --git a/docs/statusAnnotation.md b/docs/statusAnnotation.md
index 4b18a22e..c64acd10 100644
--- a/docs/statusAnnotation.md
+++ b/docs/statusAnnotation.md
@@ -26,6 +26,9 @@ reconciliation of BMH starts from the point it was before pivot. This
 essentially ensures all the critical data residing in BMH _Status_ sub-resource
 is retained and BMH does not suffer any accidental introspection.
 
+Note that in the case where only the hardware field requires update, the
+[inspect annotation](inspectAnnotation.md) may also be used.
+
 Here is an example of a _Status annotation_:
 
 ```yaml


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index f8f4d50f..3f8f411f 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -32,6 +32,7 @@ const (
 	namespace         string = "test-namespace"
 	defaultSecretName string = "bmc-creds-valid" // #nosec
 	statusAnnotation  string = `{"operationalStatus":"OK","lastUpdated":"2020-04-15T15:00:50Z","hardwareProfile":"StatusProfile","hardware":{"systemVendor":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true},{"name":"eth1","model":"0x1af4  0x0001","mac":"00:b7:8b:bb:3d:f8","ip":"192.168.111.20","speedGbps":0,"vlanId":0,"pxe":false}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["aes","apic","arat","avx","clflush","cmov","constant_tsc","cx16","cx8","de","eagerfpu","ept","erms","f16c","flexpriority","fpu","fsgsbase","fxsr","hypervisor","lahf_lm","lm","mca","mce","mmx","msr","mtrr","nopl","nx","pae","pat","pclmulqdq","pge","pni","popcnt","pse","pse36","rdrand","rdtscp","rep_good","sep","smep","sse","sse2","sse4_1","sse4_2","ssse3","syscall","tpr_shadow","tsc","tsc_adjust","tsc_deadline_timer","vme","vmx","vnmi","vpid","x2apic","xsave","xsaveopt","xtopology"],"count":4},"hostname":"node-0"},"provisioning":{"state":"provisioned","ID":"8a0ede17-7b87-44ac-9293-5b7d50b94b08","image":{"url":"bar","checksum":""}},"goodCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"triedCredentials":{"credentials":{"name":"node-0-bmc-secret","namespace":"metal3"},"credentialsVersion":"879"},"errorMessage":"","poweredOn":true,"operationHistory":{"register":{"start":"2020-04-15T12:06:26Z","end":"2020-04-15T12:07:12Z"},"inspect":{"start":"2020-04-15T12:07:12Z","end":"2020-04-15T12:09:29Z"},"provision":{"start":null,"end":null},"deprovision":{"start":null,"end":null}}}`
+	hwdAnnotation     string = `{"systemVendor":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["foo"],"count":4},"hostname":"hwdAnnotation-0"}`
 )
 
 func newSecret(name string, data map[string]string) *corev1.Secret {
@@ -192,6 +193,82 @@ func waitForProvisioningState(t *testing.T, r *BareMetalHostReconciler, host *me
 	)
 }
 
+// TestHardwareDetails_EmptyStatus ensures that hardware details in
+// the status field are populated when the hardwaredetails annotation
+// is present and no existing HarwareDetails are present
+func TestHardwareDetails_EmptyStatus(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Annotations = map[string]string{
+		hardwareDetailsAnnotation: hwdAnnotation,
+	}
+
+	r := newTestReconciler(host)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			_, found := host.Annotations[hardwareDetailsAnnotation]
+			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "hwdAnnotation-0" && !found {
+				return true
+			}
+			return false
+		},
+	)
+}
+
+// TestHardwareDetails_StatusPresent ensures that hardware details in
+// the hardwaredetails annotation is ignored with existing Status
+func TestHardwareDetails_StatusPresent(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Annotations = map[string]string{
+		hardwareDetailsAnnotation: hwdAnnotation,
+	}
+	time := metav1.Now()
+	host.Status.LastUpdated = &time
+	hwd := metal3v1alpha1.HardwareDetails{}
+	hwd.Hostname = "existinghost"
+	host.Status.HardwareDetails = &hwd
+
+	r := newTestReconciler(host)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			_, found := host.Annotations[hardwareDetailsAnnotation]
+			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "existinghost" && !found {
+				return true
+			}
+			return false
+		},
+	)
+}
+
+// TestHardwareDetails_StatusPresentInspectDisabled ensures that
+// hardware details in the hardwaredetails annotation are consumed
+// even when existing status exists, when inspection is disabled
+func TestHardwareDetails_StatusPresentInspectDisabled(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Annotations = map[string]string{
+		inspectAnnotationPrefix:   "disabled",
+		hardwareDetailsAnnotation: hwdAnnotation,
+	}
+	time := metav1.Now()
+	host.Status.LastUpdated = &time
+	hwd := metal3v1alpha1.HardwareDetails{}
+	hwd.Hostname = "existinghost"
+	host.Status.HardwareDetails = &hwd
+
+	r := newTestReconciler(host)
+
+	tryReconcile(t, r, host,
+		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+			_, found := host.Annotations[hardwareDetailsAnnotation]
+			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "hwdAnnotation-0" && !found {
+				return true
+			}
+			return false
+		},
+	)
+}
+
 // TestStatusAnnotation_EmptyStatus ensures that status is manually populated
 // when status annotation is present and status field is empty.
 func TestStatusAnnotation_EmptyStatus(t *testing.T) {