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

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

controllers/metal3.io/baremetalhost_controller.go

@@ -1471,7 +1471,6 @@ func (r *BareMetalHostReconciler) handleDataImageActions(prov provisioner.Provis
 	// 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)
 
 	// Check if any attach/detach action is pending or failed to attach
 	// We are assuming that the action will have completed by the time
@@ -1598,7 +1597,7 @@ func ownerReferenceExists(owner metav1.Object, resource metav1.Object) bool {
 // 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())
+		info.log.Info("Error while attaching DataImage", "URL", dataImage.Spec.URL, "Error", err.Error())
 
 		dataImage.Status.Error.Count++
 		dataImage.Status.Error.Message = err.Error()
@@ -1610,8 +1609,6 @@ func (r *BareMetalHostReconciler) attachDataImage(prov provisioner.Provisioner,
 		return fmt.Errorf("failed to attach dataImage, %w", err)
 	}
 
-	info.log.Info("Attach dataImage initiated", "DataImage", dataImage.Name)
-
 	// Update attached.URL here, we will mark it dirty in case any node errors
 	// are encountered
 	dataImage.Status.AttachedImage.URL = dataImage.Spec.URL
@@ -1638,8 +1635,6 @@ func (r *BareMetalHostReconciler) detachDataImage(prov provisioner.Provisioner,
 		return fmt.Errorf("failed to detach dataImage, %w", err)
 	}
 
-	info.log.Info("Detach dataImage initiated", "DataImage", dataImage.Name)
-
 	// Update attached.URL here, we will mark it dirty in case any node errors
 	// are encountered
 	dataImage.Status.AttachedImage.URL = ""

controllers/metal3.io/dataimage_controller.go

@@ -93,12 +93,9 @@ func (info *rdiInfo) publishEvent(reason, message string) {
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // 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
 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")
+	reqLogger.Info("start dataImage reconciliation")
 
 	di := &metal3api.DataImage{}
 	if err := r.Get(ctx, req.NamespacedName, di); err != nil {
@@ -132,11 +129,22 @@ func (r *DataImageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	// If the reconciliation is paused, requeue
 	annotations := bmh.GetAnnotations()
-	if annotations != nil {
-		if _, ok := annotations[metal3api.PausedAnnotation]; ok {
-			reqLogger.Info("host is paused, no work to do")
-			return ctrl.Result{Requeue: false}, nil
+	if _, ok := annotations[metal3api.PausedAnnotation]; ok {
+		reqLogger.Info("host is paused, no work to do")
+		return ctrl.Result{Requeue: false}, nil
+	}
+
+	// Add finalizer for newly created DataImage
+	if di.DeletionTimestamp.IsZero() && !utils.StringInList(di.Finalizers, metal3api.DataImageFinalizer) {
+		reqLogger.Info("adding finalizer")
+		di.Finalizers = append(di.Finalizers, metal3api.DataImageFinalizer)
+
+		// Update dataImage after adding finalizer, requeue in case of failure
+		err := r.Update(ctx, di)
+		if err != nil {
+			return ctrl.Result{RequeueAfter: dataImageUpdateDelay}, fmt.Errorf("failed to update resource after add finalizer, %w", err)
 		}
+		return ctrl.Result{Requeue: true}, nil
 	}
 
 	// Create a provisioner that can access Ironic API
@@ -157,19 +165,6 @@ func (r *DataImageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil
 	}
 
-	// Add finalizer for newly created DataImage
-	if di.DeletionTimestamp.IsZero() && !utils.StringInList(di.Finalizers, metal3api.DataImageFinalizer) {
-		reqLogger.Info("adding finalizer")
-		di.Finalizers = append(di.Finalizers, metal3api.DataImageFinalizer)
-
-		// Update dataImage after adding finalizer, requeue in case of failure
-		err := r.Update(ctx, di)
-		if err != nil {
-			return ctrl.Result{RequeueAfter: dataImageUpdateDelay}, fmt.Errorf("failed to update resource after add finalizer, %w", err)
-		}
-		return ctrl.Result{Requeue: true}, nil
-	}
-
 	// Check if any attach/detach action is pending or failed to attach
 	_, nodeError := prov.IsDataImageReady()
 

main.go

@@ -177,7 +177,7 @@ func main() {
 		logOpts.Development = true
 		logOpts.TimeEncoder = zapcore.ISO8601TimeEncoder
 	} else {
-		logOpts.TimeEncoder = zapcore.ISO8601TimeEncoder // zapcore.EpochTimeEncoder
+		logOpts.TimeEncoder = zapcore.EpochTimeEncoder
 	}
 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&logOpts)))
 

pkg/provisioner/ironic/ironic.go

@@ -2201,7 +2201,13 @@ func (p *ironicProvisioner) RemoveBMCEventSubscriptionForNode(subscription metal
 // successful of not.
 func (p *ironicProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
 	// TODO(hroyrh)
-	// Get BareMetalHost VirtualMedia details and handle errors
+	// 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)
+	}
 
 	node, err := p.getNode()
 	if err != nil {
@@ -2220,6 +2226,12 @@ func (p *ironicProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error
 }
 
 func (p *ironicProvisioner) AttachDataImage(url string) (err error) {
+	// Check if Ironic API version supports DataImage API
+	// Needs version >= 1.89
+	if !p.availableFeatures.HasDataImage() {
+		return fmt.Errorf("ironic version=%d doesn't support DataImage API, needs version>=1.89", p.availableFeatures.MaxVersion)
+	}
+
 	err = nodes.AttachVirtualMedia(p.ctx, p.client, p.nodeID, nodes.AttachVirtualMediaOpts{
 		DeviceType: nodes.VirtualMediaCD,
 		ImageURL:   url,
@@ -2232,6 +2244,12 @@ func (p *ironicProvisioner) AttachDataImage(url string) (err error) {
 }
 
 func (p *ironicProvisioner) DetachDataImage() (err error) {
+	// Check if Ironic API version supports DataImage API
+	// Needs version >= 1.89
+	if !p.availableFeatures.HasDataImage() {
+		return fmt.Errorf("ironic version=%d doesn't support DataImage API, needs version>=1.89", p.availableFeatures.MaxVersion)
+	}
+
 	err = nodes.DetachVirtualMedia(p.ctx, p.client, p.nodeID, nodes.DetachVirtualMediaOpts{
 		DeviceTypes: []nodes.VirtualMediaDeviceType{nodes.VirtualMediaCD},
 	}).ExtractErr()