Repository: metal3-io/baremetal-operator
Pull request #2336 (2025-03-21 09:49:09)

:sparkles: Use standard GetVirtualMedia API for checking dataImage status
**What this PR does / why we need it**:
Use the newly introduced [virtual media get api](https://github.com/gophercloud/gophercloud/pull/3303) to get the current dataImage status.


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 assign [rozzii](https://github.com/rozzii) for approval. For more information see [the 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":["rozzii"]} -->
metal3-io-bot: PR needs rebase.

<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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
iurygregory: We need to update gophercloud, so the changes in go.mod and go.sum are valid.
/lgtm 
metal3-io-bot: New changes are detected. LGTM label has been removed.
metal3-io-bot: @hroyrh: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gomod | 3c905c9e6ae11d2c2643fcef237f6c369c6401f7 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/2336/gomod/1907800004890726400) | true | `/test gomod`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=2336). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tuminoid: > We need to update gophercloud, so the changes in go.mod and go.sum are valid. /lgtm

2.7.0 is now out, which has been updated here, nice.
elfosardo: /lgtm
thanks!
metal3-io-bot: New changes are detected. LGTM label has been removed.
metal3-io-bot: PR needs rebase.

<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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
tuminoid: Are we planning to take this in 0.10 or can we leave it after branching? 
dtantsur: > Are we planning to take this in 0.10 or can we leave it after branching?

This feature is not on the default path, it shouldn't be a large regression risk if that's what worries you.

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 05414d0c..33310bf7 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -1664,21 +1664,15 @@ func (r *BareMetalHostReconciler) handleDataImageActions(prov provisioner.Provis
 	// given constant ?
 	dataImageRetryBackoff := max(dataImageUpdateDelay, calculateBackoff(dataImage.Status.Error.Count))
 
-	// Check if any attach/detach action is pending or failed to attach
-	// We are assuming that the action will have completed by the time
-	// this reconcile is called ( after the delay specified in the previous
-	// action)
-	// TODO(hroyrh) : update this once vmedia.get api is available
-	isNodeBusy, nodeError := prov.IsDataImageReady()
-	if isNodeBusy {
-		info.log.Info("Node is busy, requeuing")
-
-		// In case the last node error was not nil for dataimage,
-		// update message and counter
-		if nodeError != nil {
-			info.log.Info("DataImage action failed", "Error", nodeError.Error())
-
-			dataImage.Status.Error.Message = nodeError.Error()
+	// Check if dataImage is attached to the node or not
+	// Given that this is a synchronous call to Ironic, should we add
+	// a longer wait ?
+	isImageAttached, getVmediaError := prov.GetDataImageStatus()
+	if getVmediaError != nil {
+		info.log.Error(getVmediaError, "Error fetching Virtual Media details")
+
+		if !errors.Is(getVmediaError, provisioner.ErrNodeIsBusy) {
+			dataImage.Status.Error.Message = getVmediaError.Error()
 			dataImage.Status.Error.Count++
 
 			if err := r.Status().Update(info.ctx, dataImage); err != nil {
@@ -1689,29 +1683,31 @@ func (r *BareMetalHostReconciler) handleDataImageActions(prov provisioner.Provis
 		return actionContinue{dataImageRetryBackoff}
 	}
 
-	// Is the current dataImage status valid
-	dirty := false
-
-	// In case the last node error was not nil for dataimage,
-	// update message and counter
-	if nodeError != nil {
-		info.log.Info("DataImage not ready", "Error", nodeError.Error())
-
-		dataImage.Status.Error.Message = nodeError.Error()
-		dataImage.Status.Error.Count++
-		dirty = true
-	}
-
 	deleteDataImage := !dataImage.DeletionTimestamp.IsZero()
 
 	requestedURL := dataImage.Spec.URL
 
+	// We will assume that the attached url is correct
+	// In case of failed attach, this variable will be overridden
 	attachedURL := dataImage.Status.AttachedImage.URL
 
+	// If there is no attached image, we will override the attachedURL
+	// so that further actions are handled accordingly
+	if !isImageAttached && attachedURL != "" {
+		dataImage.Status.AttachedImage.URL = ""
+
+		// Update DataImage Status
+		if err := r.Status().Update(info.ctx, dataImage); err != nil {
+			return actionError{fmt.Errorf("failed to update DataImage status, %w", err)}
+		}
+
+		return actionContinue{}
+	}
+
 	if deleteDataImage {
 		info.log.Info("DataImage requested for deletion")
-		if attachedURL != "" || dirty {
-			info.log.Info("Detaching DataImage as it was deleted")
+		if isImageAttached {
+			info.log.Info("Detaching DataImage as its deletion has been requested")
 			err := r.detachDataImage(prov, info, dataImage)
 			if err != nil {
 				return actionError{fmt.Errorf("failed to detach, %w", err)}
@@ -1726,7 +1722,7 @@ func (r *BareMetalHostReconciler) handleDataImageActions(prov provisioner.Provis
 		return nil
 	}
 
-	if requestedURL != attachedURL || dirty {
+	if requestedURL != attachedURL {
 		info.log.Info("DataImage change detected")
 		if attachedURL != "" {
 			info.log.Info("Detaching DataImage")
@@ -1754,21 +1750,15 @@ func (r *BareMetalHostReconciler) handleDataImageActions(prov provisioner.Provis
 		}
 	}
 
-	// Clear dataImage errors if nodeError is nil
-	if !dirty {
-		dataImage.Status.Error.Message = ""
-		dataImage.Status.Error.Count = 0
-	}
+	// Clear dataImage errors as relevant error cases were already handled
+	dataImage.Status.Error.Message = ""
+	dataImage.Status.Error.Count = 0
 
 	if err := r.Status().Update(info.ctx, dataImage); err != nil {
 		return actionError{fmt.Errorf("failed to update DataImage status, %w", err)}
 	}
 
-	info.log.Info("Updated DataImage Status after handling attachment/detachment")
-
-	if dirty {
-		return actionContinue{dataImageRetryBackoff}
-	}
+	info.log.Info("Updated DataImage Status triggered after handling attachment/detachment")
 
 	return nil
 }
@@ -1801,9 +1791,13 @@ func (r *BareMetalHostReconciler) attachDataImage(prov provisioner.Provisioner,
 		return fmt.Errorf("failed to attach dataImage, %w", err)
 	}
 
-	// Update attached.URL here, we will mark it dirty in case any node errors
-	// are encountered
+	// Update the attached URL here assuming this operation will succeed
+	// In case of failure, this will be overridden in next reconcile
+	// We have to do this, as there is no other way to make sure the
+	// attached image url since the virtual media get api always returns
+	// the same standard url
 	dataImage.Status.AttachedImage.URL = dataImage.Spec.URL
+
 	// Error updating DataImage Status
 	if err := r.Status().Update(info.ctx, dataImage); err != nil {
 		return fmt.Errorf("failed to update DataImage status, %w", err)
@@ -1827,14 +1821,6 @@ func (r *BareMetalHostReconciler) detachDataImage(prov provisioner.Provisioner,
 		return fmt.Errorf("failed to detach dataImage, %w", err)
 	}
 
-	// Update attached.URL here, we will mark it dirty in case any node errors
-	// are encountered
-	dataImage.Status.AttachedImage.URL = ""
-	// Error updating DataImage Status
-	if err := r.Status().Update(info.ctx, dataImage); err != nil {
-		return fmt.Errorf("failed to update DataImage status, %w", err)
-	}
-
 	return nil
 }
 
diff --git a/controllers/metal3.io/dataimage_controller.go b/controllers/metal3.io/dataimage_controller.go
index ad1791eb..f505405f 100644
--- a/controllers/metal3.io/dataimage_controller.go
+++ b/controllers/metal3.io/dataimage_controller.go
@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -187,26 +188,34 @@ func (r *DataImageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	// Check if any attach/detach action is pending or failed to attach
-	_, nodeError := prov.IsDataImageReady()
+	isImageAttached, vmediaGetError := prov.GetDataImageStatus()
 
-	// Is the current dataImage status valid
-	dirty := false
-	// In case the last node error was not nil for dataimage,
+	// In case there was an error fetching vmedia details
 	// upadate message and counter
-	if nodeError != nil {
-		di.Status.Error.Message = nodeError.Error()
-		di.Status.Error.Count++
-		dirty = true
+	if vmediaGetError != nil {
+		reqLogger.Error(vmediaGetError, "failed to fetch Virtual Media details")
+
+		if !errors.Is(vmediaGetError, provisioner.ErrNodeIsBusy) {
+			di.Status.Error.Message = vmediaGetError.Error()
+			di.Status.Error.Count++
+
+			// Update dataImage status and requeue
+			if err := r.updateStatus(info); err != nil {
+				return ctrl.Result{Requeue: true, RequeueAfter: dataImageRetryDelay}, fmt.Errorf("failed to update resource status, %w", err)
+			}
+		}
+
+		return ctrl.Result{Requeue: true, RequeueAfter: dataImageUpdateDelay}, nil
 	}
+	di.Status.Error.Message = ""
+	di.Status.Error.Count = 0
 
 	// Remove finalizer if DataImage has been requested for deletion and
 	// there is no attached image, else wait for the detachment.
 	if !di.DeletionTimestamp.IsZero() {
 		reqLogger.Info("cleaning up deleted dataImage resource")
 
-		dataImageAttachedURL := di.Status.AttachedImage.URL
-
-		if dataImageAttachedURL != "" || dirty {
+		if isImageAttached {
 			reqLogger.Info("Wait for DataImage to detach before removing finalizer, requeueing")
 			return ctrl.Result{Requeue: true, RequeueAfter: dataImageRetryDelay}, nil
 		}
diff --git a/go.mod b/go.mod
index 17d008fe..3cc26dfd 100644
--- a/go.mod
+++ b/go.mod
@@ -6,7 +6,7 @@ require (
 	github.com/go-logr/logr v1.4.2
 	github.com/google/safetext v0.0.0-20230106111101-7156a760e523
 	github.com/google/uuid v1.6.0
-	github.com/gophercloud/gophercloud/v2 v2.6.0
+	github.com/gophercloud/gophercloud/v2 v2.7.0
 	github.com/metal3-io/baremetal-operator/apis v0.5.1
 	github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.5.1
 	github.com/onsi/gomega v1.37.0
diff --git a/go.sum b/go.sum
index fc2c6fb2..7f3d5e5c 100644
--- a/go.sum
+++ b/go.sum
@@ -69,8 +69,8 @@ github.com/google/safetext v0.0.0-20230106111101-7156a760e523 h1:i4NsbmB9pD5+Ggp
 github.com/google/safetext v0.0.0-20230106111101-7156a760e523/go.mod h1:mJNEy0r5YPHC7ChQffpOszlGB4L1iqjXWpIEKcFpr9s=
 github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
 github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
-github.com/gophercloud/gophercloud/v2 v2.6.0 h1:XJKQ0in3iHOZHVAFMXq/OhjCuvvG+BKR0unOqRfG1EI=
-github.com/gophercloud/gophercloud/v2 v2.6.0/go.mod h1:Ki/ILhYZr/5EPebrPL9Ej+tUg4lqx71/YH2JWVeU+Qk=
+github.com/gophercloud/gophercloud/v2 v2.7.0 h1:o0m4kgVcPgHlcXiWAjoVxGd8QCmvM5VU+YM71pFbn0E=
+github.com/gophercloud/gophercloud/v2 v2.7.0/go.mod h1:Ki/ILhYZr/5EPebrPL9Ej+tUg4lqx71/YH2JWVeU+Qk=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.20.0 h1:bkypFPDjIYGfCYD5mRBvpqxfYX1YCS1PXdKYWi8FsN0=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.20.0/go.mod h1:P+Lt/0by1T8bfcF3z737NnSbmxQAppXMRziHUxPOC8k=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 4f436cb5..dc764df9 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -327,7 +327,7 @@ func (p *demoProvisioner) GetFirmwareComponents() (components []metal3api.Firmwa
 	return components, nil
 }
 
-func (p *demoProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
+func (p *demoProvisioner) GetDataImageStatus() (isImageAttached bool, err error) {
 	return false, nil
 }
 
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index f9bac981..beb0f45a 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -400,7 +400,7 @@ func (p *fixtureProvisioner) GetFirmwareComponents() (components []metal3api.Fir
 	return p.state.HostFirmwareComponents.Components, nil
 }
 
-func (p *fixtureProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
+func (p *fixtureProvisioner) GetDataImageStatus() (isImageAttached bool, err error) {
 	return false, nil
 }
 
diff --git a/pkg/provisioner/ironic/clients/features.go b/pkg/provisioner/ironic/clients/features.go
index 982e5bfc..db415345 100644
--- a/pkg/provisioner/ironic/clients/features.go
+++ b/pkg/provisioner/ironic/clients/features.go
@@ -58,6 +58,10 @@ func (af AvailableFeatures) HasDataImage() bool {
 	return af.MaxVersion >= 89
 }
 
+func (af AvailableFeatures) HasVirtualMediaGetAPI() bool {
+	return af.MaxVersion >= 93
+}
+
 func (af AvailableFeatures) HasDisablePowerOff() bool {
 	return af.MaxVersion >= 95
 }
@@ -67,6 +71,10 @@ func (af AvailableFeatures) ChooseMicroversion() string {
 		return "1.95"
 	}
 
+	if af.HasVirtualMediaGetAPI() {
+		return "1.93"
+	}
+
 	if af.HasDataImage() {
 		return "1.89"
 	}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 454dd28a..9a4b4589 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -101,6 +101,14 @@ type ironicProvisioner struct {
 	ctx context.Context
 }
 
+// FIXME(hroyrh) : move this to gophercloud when implementing
+// VirtualMediaGetResult Extract function.
+type VirtualMedia struct {
+	Image      string   `json:"image"`
+	Inserted   bool     `json:"inserted"`
+	MediaTypes []string `json:"media_types"`
+}
+
 func (p *ironicProvisioner) bmcAccess() (bmc.AccessDetails, error) {
 	bmcAccess, err := bmc.NewAccessDetails(p.bmcAddress, p.disableCertVerification)
 	if err != nil {
@@ -1758,18 +1766,16 @@ func (p *ironicProvisioner) RemoveBMCEventSubscriptionForNode(subscription metal
 	return operationComplete()
 }
 
-// TODO(hroyrh) : Replace with GetDataImageStatus function once the virtua_media.get
-// api is available.
-// Checks if the last VirtualMedia action(attach/detach) to a BareMetalHost was
-// successful of not.
-func (p *ironicProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
-	// TODO(hroyrh)
-	// Get BareMetalHost VirtualMedia details using a GET api to vmedia
-
-	// Check if Ironic API version supports DataImage API
-	// Needs version >= 1.89
-	if !p.availableFeatures.HasDataImage() {
-		return true, fmt.Errorf("ironic version=%d doesn't support DataImage API, needs version>=1.89", p.availableFeatures.MaxVersion)
+// Uses the Ironic Virtual Media Get API which synchronously fetches the
+// virtual media details for the given node.
+// We return only the bool isImageAttached because the url in the response
+// body is always the same - based on the node uuid, so it is not useful
+// for any comparison purpose.
+func (p *ironicProvisioner) GetDataImageStatus() (isImageAttached bool, err error) {
+	// Check if Ironic API version supports Virtual Media Get API
+	// Needs version >= 1.93
+	if !p.availableFeatures.HasVirtualMediaGetAPI() {
+		return false, fmt.Errorf("ironic version=%d doesn't support Virtual Media Get API, needs version>=1.93", p.availableFeatures.MaxVersion)
 	}
 
 	node, err := p.getNode()
@@ -1777,15 +1783,39 @@ func (p *ironicProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error
 		return true, err
 	}
 
-	isNodeBusy = node.Reservation != ""
+	// If the node is busy, return error, since the get request will anyways
+	// fail
+	// TODO(hroyrh): is there a better way to do this ?
+	isNodeBusy := node.Reservation != ""
+	if isNodeBusy {
+		p.log.Info("node is busy, retry GetVirtualMedia request in some time", "node", node.UUID)
+		return false, provisioner.ErrNodeIsBusy
+	}
+
+	result := nodes.GetVirtualMedia(p.ctx, p.client, p.nodeID)
+	err = result.Err
+	if err != nil {
+		return false, err
+	}
+
+	// FIXME (hroyrh) : implement Extract function for VirtualMediaGetResult
+	// in gophercloud
+	var vmediaList []VirtualMedia
+	err = result.ExtractInto(&vmediaList)
+	if err != nil {
+		p.log.Info("GetDataImage, failed to extract result body", "Error", err)
+		return false, err
+	}
 
-	// In case the error node encountered was related to something else
-	// TODO(hroyrh) : Is this check valid ?
-	if !strings.Contains(node.LastError, "attach") && !strings.Contains(node.LastError, "detach") {
-		return isNodeBusy, nil
+	for _, vmedia := range vmediaList {
+		if vmedia.Inserted {
+			p.log.Info("GetDataImage, vmedia attached", "URL", vmedia.Image)
+			return true, nil
+		}
 	}
 
-	return isNodeBusy, fmt.Errorf("last dataImage action failed, %s", node.LastError)
+	p.log.Info("GetDataImage, no vmedia is attached")
+	return false, nil
 }
 
 func (p *ironicProvisioner) AttachDataImage(url string) (err error) {
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 313e58d7..faddd0fd 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -216,8 +216,8 @@ type Provisioner interface {
 	// GetFirmwareComponents gets all firmware components available from a note
 	GetFirmwareComponents() (components []metal3api.FirmwareComponentStatus, err error)
 
-	// Check if DataImage attach/detach was successful
-	IsDataImageReady() (isNodeBusy bool, nodeError error)
+	// Get DataImage
+	GetDataImageStatus() (isImageAttached bool, err error)
 
 	// Attach DataImage
 	AttachDataImage(URL string) (err error)
@@ -254,3 +254,7 @@ var ErrNeedsPreprovisioningImage = errors.New("no suitable Preprovisioning image
 
 // ErrFirmwareUpdateUnsupported is returned if the host can't execute firmware updates.
 var ErrFirmwareUpdateUnsupported = errors.New("host does not support Firmware Updates")
+
+// ErrNodeIsBusy is returned when the node is busy due to being reserved for another
+// task.
+var ErrNodeIsBusy = errors.New("node is busy")


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index da8fc70e..e6204016 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1403,7 +1403,7 @@ func (p *mockProvisioner) GetFirmwareComponents() (components []metal3api.Firmwa
 	return components, nil
 }
 
-func (m *mockProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
+func (p *mockProvisioner) GetDataImageStatus() (isImageAttached bool, err error) {
 	return false, nil
 }