✨ Attach non-bootable iso to BareMetalHost #1594

hroyrh · 2024-03-05T07:50:08Z

What this PR does / why we need it:
Add new DataImage CRD which allows attachment/detachment of non-bootable iso to BareMetalHost, based on this proposal.

dtantsur · 2024-03-13T15:16:23Z

controllers/metal3.io/baremetalhost_controller.go

+			return nil
+		}
+		// Error reading the object - requeue the request.
+		return actionError{errors.Wrap(err, "could not load dataImage")}


nit: avoid deprecated errors.Wrap (use fmt.Errorf with %w)

(same below)

dtantsur · 2024-03-13T15:17:07Z

controllers/metal3.io/baremetalhost_controller.go

+		if err := r.Update(info.ctx, dataImage); err != nil {
+			return actionError{errors.Wrap(err, "failure creating dataImage resource")}
+		}
+		// Should we requeue at this point


I'm never sure myself...

dtantsur · 2024-03-13T15:17:43Z

controllers/metal3.io/baremetalhost_controller.go

+
+	// Fetch the latest status of DataImage from node
+	if err := prov.GetDataImageStatus(dataImage); err != nil {
+		info.log.Info("Failed to get current status : ", "ErrorIs", err)


nit: Please remove " : ", there is no guarantee how exactly the fields are logged (also s/ErrorIs/Error/ for the same reason)

dtantsur · 2024-03-13T15:19:02Z

controllers/metal3.io/baremetalhost_controller.go

+		deleteDataImage = true
+	}
+
+	diSpecURL := dataImage.Spec.URL


nit: the "di" prefix is not very useful in the context of this function, I'd rather do something like requestedURL vs attachedURL

controllers/metal3.io/baremetalhost_controller.go

dtantsur · 2024-03-13T15:20:42Z

controllers/metal3.io/baremetalhost_controller.go

+			}
+		}
+		if diSpecURL != "" {
+			info.log.Info("Attaching DataImage")


nit: log the image URL

probably worth log that and more info in case of failure below

dtantsur · 2024-03-13T15:21:18Z

controllers/metal3.io/baremetalhost_controller.go

+				return actionError{errors.Wrap(err, "Failed to detach")}
+			}
+
+			if err := r.Status().Update(info.ctx, dataImage); err != nil {


What are you trying to update here? Did you mean to remove the finalizer first?

Yes, I wanted to update the attachedURL here so that the dataimage reconciler can remove the finalizer. But given your previous comment ( requeuing after detach ) I think the more appropriate option will be to update the status here in case detach return an error, and then requeue.

dtantsur · 2024-03-13T15:22:30Z

controllers/metal3.io/baremetalhost_controller.go

+// Attach the DataImage to the BareMetalHost.
+func (r *BareMetalHostReconciler) attachDataImage(prov provisioner.Provisioner, info *reconcileInfo, dataImage *metal3api.DataImage) error {
+	if err := prov.AttachDataImage(dataImage.Spec.URL); err != nil {
+		info.log.Info("Called the IronicProvisioner.AttachDataImage function")


Redundant in the final version of the patch (same for detach)

controllers/metal3.io/baremetalhost_controller.go

dtantsur · 2024-03-13T15:23:45Z

controllers/metal3.io/dataimage_controller.go

+	"time"
+
+	"github.com/go-logr/logr"
+	"github.com/pkg/errors"


Please avoid the deprecated package (use errors.New or fmt.Errorf with %w)

dtantsur · 2024-03-13T15:26:34Z

pkg/provisioner/ironic/ironic.go

+
+// Get DataImage Details
+// Fetches the VirtualMedia details of the BareMetalHost and updates DataImage.
+func (p *ironicProvisioner) GetDataImageStatus(dataImage *metal3api.DataImage) (err error) {


Provisioners should not handle API objects directly, it's the responsibility of controllers.

pkg/provisioner/ironic/ironic.go

dtantsur · 2024-03-18T14:25:36Z

apis/metal3.io/v1alpha1/dataimage_types.go

+
+// DataImageSpec defines the desired state of DataImage.
+type DataImageSpec struct {
+	// Important: Run "make" to regenerate code after modifying this file


nit: can be removed

dtantsur · 2024-03-18T14:26:14Z

apis/metal3.io/v1alpha1/dataimage_types.go

+	// imageUrl is the URL from which the built image can be downloaded.
+	AttachedImage *AttachedImageReference `json:"attachedImage,omitempty"`
+
+	Error *DataImageError `json:"error,omitempty"`


nit: documentation

dtantsur · 2024-03-18T14:26:39Z

apis/metal3.io/v1alpha1/dataimage_types.go

+}
+
+//+kubebuilder:object:root=true
+//+kubebuilder:resource:shortName=dimg


It's going to be a very rarely used resource, let's maybe not have a short name for it?

Signed-off-by: Himanshu Roy <hroy@redhat.com>

… for BMH and DataImage controllers Signed-off-by: Himanshu Roy <hroy@redhat.com>

Signed-off-by: Himanshu Roy <hroy@redhat.com>

…, minor fixes Signed-off-by: Himanshu Roy <hroy@redhat.com>

…ling Signed-off-by: Himanshu Roy <hroy@redhat.com>

lastReconciled status field Signed-off-by: Himanshu Roy <hroy@redhat.com>

dtantsur · 2024-04-24T11:19:56Z

controllers/metal3.io/baremetalhost_controller.go

+	// TODO(hroyrh) : Should we fail after the error count exceeds a
+	// given constant ?
+	dataImageRetryBackoff := max(dataImageUpdateDelay, calculateBackoff(dataImage.Status.Error.Count))
+	info.log.Info("Current dataImage reconcile delay", "dataImageRetryBackoff", dataImageRetryBackoff)


nit: this is not very useful IMO

dtantsur · 2024-04-24T11:22:37Z

controllers/metal3.io/baremetalhost_controller.go

+		return fmt.Errorf("failed to attach dataImage, %w", err)
+	}
+
+	info.log.Info("Attach dataImage initiated", "DataImage", dataImage.Name)


nit: this is probably excessive: you have logging on line 1553

dtantsur · 2024-04-24T11:23:38Z

controllers/metal3.io/baremetalhost_controller.go

+		return fmt.Errorf("failed to detach dataImage, %w", err)
+	}
+
+	info.log.Info("Detach dataImage initiated", "DataImage", dataImage.Name)


dtantsur · 2024-04-24T11:24:01Z

controllers/metal3.io/baremetalhost_controller.go

+// Attach the DataImage to the BareMetalHost.
+func (r *BareMetalHostReconciler) attachDataImage(prov provisioner.Provisioner, info *reconcileInfo, dataImage *metal3api.DataImage) error {
+	if err := prov.AttachDataImage(dataImage.Spec.URL); err != nil {
+		info.log.Info("Error while attaching DataImage", "DataImage", dataImage.Name, "Error", err.Error())


nit: data image name matches the host name, better log the URL

dtantsur · 2024-04-24T11:25:54Z

controllers/metal3.io/dataimage_controller.go

+// move the current state of the cluster closer to the desired state.
+//
+// For more details, check Reconcile and its Result here:
+// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.4/pkg/reconcile


nit: boilerplate, can be removed

dtantsur · 2024-04-24T11:26:07Z

controllers/metal3.io/dataimage_controller.go

+// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.4/pkg/reconcile
+func (r *DataImageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	reqLogger := r.Log.WithValues("dataimage", req.NamespacedName)
+	reqLogger.Info("start dataImage reconciliation V1")


nit: not sure what V1 means here

dtantsur · 2024-04-24T11:26:34Z

controllers/metal3.io/dataimage_controller.go

+
+	// If the reconciliation is paused, requeue
+	annotations := bmh.GetAnnotations()
+	if annotations != nil {


nit: not needed, you can read from a nil map

dtantsur · 2024-04-24T11:27:05Z

controllers/metal3.io/dataimage_controller.go

+	}
+
+	// Add finalizer for newly created DataImage
+	if di.DeletionTimestamp.IsZero() && !utils.StringInList(di.Finalizers, metal3api.DataImageFinalizer) {


nit: this should happen before initializing the provisioner

dtantsur · 2024-04-24T11:28:12Z

main.go

@@ -177,7 +177,7 @@ func main() {
 		logOpts.Development = true
 		logOpts.TimeEncoder = zapcore.ISO8601TimeEncoder
 	} else {
-		logOpts.TimeEncoder = zapcore.EpochTimeEncoder
+		logOpts.TimeEncoder = zapcore.ISO8601TimeEncoder // zapcore.EpochTimeEncoder


Is it intended? I think we used different encoders on purpose.

dtantsur · 2024-04-24T11:30:02Z

controllers/metal3.io/dataimage_controller.go

+		}
+		return ctrl.Result{Requeue: true}, nil
+	}
+


nit: here you can use the newly introduced HasDataImage to provide a better error message when Ironic is too old

Signed-off-by: Himanshu Roy <hroy@redhat.com>

hroyrh · 2024-04-24T13:11:32Z

/test metal3-bmo-e2e-test-pull metal3-centos-e2e-integration-test-main

dtantsur · 2024-04-24T13:15:01Z

/approve

metal3-io-bot · 2024-04-24T13:15:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dtantsur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

elfosardo · 2024-04-24T13:59:11Z

/lgtm

dtantsur · 2024-04-24T14:11:21Z

/override Approve on ok-to-test

metal3-io-bot · 2024-04-24T14:11:25Z

@dtantsur: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

Approve
ok-to-test
on

Only the following failed contexts/checkruns were expected:

generate
gomod
manifestlint
metal3-bmo-e2e-test-pull
metal3-centos-e2e-integration-test-main
tide
unit

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override Approve on ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dtantsur · 2024-04-24T14:11:44Z

/override "Approve on ok-to-test"

metal3-io-bot · 2024-04-24T14:11:49Z

@dtantsur: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

Approve on ok-to-test

Only the following failed contexts/checkruns were expected:

generate
gomod
manifestlint
metal3-bmo-e2e-test-pull
metal3-centos-e2e-integration-test-main
tide
unit

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override "Approve on ok-to-test"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 5, 2024

hroyrh changed the title ~~✨ Attach non-bootable iso to BareMetalHost~~ ✨ [WIP] Attach non-bootable iso to BareMetalHost Mar 5, 2024

hroyrh force-pushed the non-bootable-iso branch from 8c7a54b to b2bb2cb Compare March 12, 2024 12:47

dtantsur reviewed Mar 13, 2024

View reviewed changes

controllers/metal3.io/baremetalhost_controller.go Show resolved Hide resolved

dtantsur reviewed Mar 13, 2024

View reviewed changes

controllers/metal3.io/baremetalhost_controller.go Show resolved Hide resolved

dtantsur reviewed Mar 13, 2024

View reviewed changes

pkg/provisioner/ironic/ironic.go Show resolved Hide resolved

hroyrh mentioned this pull request Mar 15, 2024

✨ Add nonbootable iso api #1623

Merged

dtantsur reviewed Mar 18, 2024

View reviewed changes

hroyrh force-pushed the non-bootable-iso branch from b2bb2cb to 21d18b6 Compare March 25, 2024 09:04

metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 25, 2024

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2024

hroyrh force-pushed the non-bootable-iso branch from 21d18b6 to f6afc45 Compare April 15, 2024 11:32

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 15, 2024

hroyrh force-pushed the non-bootable-iso branch from c53f0c8 to cd843d0 Compare April 15, 2024 12:08

iurygregory mentioned this pull request Apr 16, 2024

METAL-979: Add RBAC for newer CRDs hostfirmwarecomponents and dataimages openshift/cluster-baremetal-operator#412

Merged

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024

hroyrh added 7 commits April 24, 2024 15:43

update function signatures, fix minor issues

aa75ae1

Signed-off-by: Himanshu Roy <hroy@redhat.com>

update getDataImageStatus on provisioner side and corresponding logic…

45724e0

… for BMH and DataImage controllers Signed-off-by: Himanshu Roy <hroy@redhat.com>

fix dataimage status update issue, minor fixes

1b9b9e8

Signed-off-by: Himanshu Roy <hroy@redhat.com>

use non-pointer fields in dataimage api to avoid initializing them

10a1932

Signed-off-by: Himanshu Roy <hroy@redhat.com>

update getDataImageStatus to isDataImageReady, handle errors properly…

2a8abdc

…, minor fixes Signed-off-by: Himanshu Roy <hroy@redhat.com>

add paused annotation handling, fix node error and status update hand…

11c27fc

…ling Signed-off-by: Himanshu Roy <hroy@redhat.com>

add exponential backoff delay to dataImage error handling, update

75e0c18

lastReconciled status field Signed-off-by: Himanshu Roy <hroy@redhat.com>

dtantsur reviewed Apr 24, 2024

View reviewed changes

remove extra logs, check ironic api version for dataImage, other fixes

397bc8b

Signed-off-by: Himanshu Roy <hroy@redhat.com>

hroyrh force-pushed the non-bootable-iso branch from 19d0515 to 397bc8b Compare April 24, 2024 13:10

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2024

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2024

dtantsur merged commit 98c7c1a into metal3-io:main Apr 24, 2024
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Attach non-bootable iso to BareMetalHost #1594

✨ Attach non-bootable iso to BareMetalHost #1594

hroyrh commented Mar 5, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

elfosardo Mar 18, 2024

dtantsur Mar 13, 2024

hroyrh Mar 15, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 13, 2024

dtantsur Mar 18, 2024

dtantsur Mar 18, 2024

dtantsur Mar 18, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

dtantsur Apr 24, 2024

hroyrh commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024

elfosardo commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024

✨ Attach non-bootable iso to BareMetalHost #1594

✨ Attach non-bootable iso to BareMetalHost #1594

Conversation

hroyrh commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hroyrh commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024

elfosardo commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024

dtantsur commented Apr 24, 2024

metal3-io-bot commented Apr 24, 2024