Refactors phases to functions

Author: zszabo-rh

api/v1alpha1/imageclusterinstall_types.go

@@ -26,9 +26,9 @@ import (
 
 const (
 	ConfigurationPendingReason = "ConfigurationPending"
-	ConfigurationFailedReason = "ConfigurationFailed"
+	ConfigurationFailedReason  = "ConfigurationFailed"
 
-	ImageCreationFailedReason = "ImageCreationFailed"
+	ImageCreationFailedReason  = "ImageCreationFailed"
 	ImageCreationPendingReason = "ImageCreationPending"
 
 	HostConfigurationPendingReason   = "HostConfigurationPending"

controllers/imageclusterinstall_controller.go

@@ -26,6 +26,7 @@ import (
 	_ "crypto/sha512"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net"
 	"net/url"
@@ -37,7 +38,7 @@ import (
 
 	"gopkg.in/yaml.v3"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	k8sapierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -161,7 +162,6 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 		return ctrl.Result{}, err
 	}
 
-	/////// Requirements not met yet: setting defaults //////
 	cond := hivev1.ClusterInstallCondition{
 		Type:    hivev1.ClusterInstallRequirementsMet,
 		Status:  corev1.ConditionFalse,
@@ -172,31 +172,101 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 		r.setRequirementsMetCondition(ctx, ici, cond.Status, cond.Reason, cond.Message)
 	}()
 
-	/////// Requirements not met yet: starting config (CD, BMH) validation //////
+	// 1. Config validation phase
+	// Possible reasons for not meeting requirements and exiting reconcile:
+	// - ConfigurationPending (default): it's either the user needs to complete the ImageClusterInstall definition, or some of
+	//   referenced resources (CD or BMH) are not available yet. In both cases the reconcile ends, and will be triggered again
+	//   when the problem is resolved.
+	// - ConfigurationFailed: sets this reason when AutomatedCleaningMode cannot be modified in BMH.
 	cond.Reason = v1alpha1.ConfigurationPendingReason
+	cd, bmh, err := r.validateConfiguration(ctx, ici, &cond, log)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// 2. Host validation phase
+	// Possible reasons for not meeting requirements and exiting reconcile:
+	// - HostValidationPending: if BMH provisioning or hardware inspection is not ready yet, reconcile is requeued for 30s later.
+	// - HostValidationFailed (default): in case of any errors or invalid BMH configuration the reconcile ends here.
+	// Default is HostValidationFailedReason but validateBMH() can change this to HostValidationPendingReason
+	cond.Reason = v1alpha1.HostValidationFailedReason
+	res, err := r.validateHost(ctx, ici, bmh, cd, &cond, log)
+	if !res.IsZero() || err != nil {
+		return res, err
+	}
+
+	if err := r.setClusterInstallMetadata(ctx, log, ici, cd); err != nil {
+		cond.Message = "failed to set ClusterMetaData in ImageClusterInstall"
+		log.Error(err)
+		return ctrl.Result{}, err
+	}
+
+	// 3. Image creation phase
+	// Possible reasons for not meeting requirements and exiting reconcile:
+	// - ImageCreationPending: when lock cannot be acquired, reconcile gets requeued for 5s later to try again.
+	// - ImageCreationFailed (default): any other unexpected error stops the reconcile loop with this reason.
+	cond.Reason = v1alpha1.ImageCreationFailedReason
+	imageUrl, res, err := r.createImage(ctx, ici, req, bmh, cd, &cond, log)
+	if !res.IsZero() || err != nil {
+		return res, err
+	}
+
+	r.labelReferencedObjectsForBackup(ctx, log, ici, cd)
+
+	// 4. Host configuration phase
+	// Possible reasons for not meeting requirements and exiting reconcile:
+	// - HostConfigurationPending: sets this reason in following scenarios:
+	//   > earlier DataImage instance is still being deleted for some reason (requeue after 30s)
+	//   > current DataImage was just created less than a second ago so BMO might not be notified yet (requeue after 1s)
+	//   > image-based-install-managed annotation is not set yet in BMH (no requeue)
+	// - HostConfigurationFailed (default): any unexpected errors during this phase will lead to this reason and finish reconcile.
+	cond.Reason = v1alpha1.HostConfigurationFailedReason
+	res, err = r.configureHost(ctx, ici, imageUrl, bmh, &cond, log)
+	if !res.IsZero() || err != nil {
+		return res, err
+	}
+
+	// Requirements met, host configured
+	cond.Status = corev1.ConditionTrue
+	cond.Reason = v1alpha1.HostConfigurationSucceededReason
+	cond.Message = "configuration image is attached to the referenced host"
+
+	return ctrl.Result{}, nil
+}
+
+func GetClusterConfigDir(namespacesDir, namespace, uid string) string {
+	return filepath.Join(namespacesDir, namespace, uid, FilesDir, ClusterConfigDir)
+}
+
+func (r *ImageClusterInstallReconciler) validateConfiguration(
+	ctx context.Context,
+	ici *v1alpha1.ImageClusterInstall,
+	cond *hivev1.ClusterInstallCondition,
+	log logrus.FieldLogger,
+) (*hivev1.ClusterDeployment, *bmh_v1alpha1.BareMetalHost, error) {
 
 	if ici.Spec.ClusterDeploymentRef == nil || ici.Spec.ClusterDeploymentRef.Name == "" {
 		cond.Message = "ClusterDeploymentRef is unset"
-		return ctrl.Result{}, nil
+		return nil, nil, errors.New(cond.Message)
 	}
 
-	clusterDeployment, err := r.getCD(ctx, ici)
+	cd, err := r.getCD(ctx, ici)
 	if err != nil {
 		cond.Message = fmt.Sprintf("failed to get ClusterDeployment %s/%s", ici.Namespace, ici.Spec.ClusterDeploymentRef.Name)
-		log.WithError(err).Error(cond.Message)
-		return ctrl.Result{}, err
+		log.Error(err)
+		return nil, nil, err
 	}
 
-	if ici.Spec.BareMetalHostRef == nil {
-		cond.Message = "no BareMetalHostRef set, nothing to do without provided bmh"
-		return ctrl.Result{}, nil
+	if ici.Spec.BareMetalHostRef == nil || ici.Spec.BareMetalHostRef.Name == "" {
+		cond.Message = "BareMetalHostRef is unset"
+		return nil, nil, errors.New(cond.Message)
 	}
 
 	bmh, err := r.getBMH(ctx, ici.Spec.BareMetalHostRef)
 	if err != nil {
 		cond.Message = fmt.Sprintf("failed to get BareMetalHost %s/%s", ici.Spec.BareMetalHostRef.Namespace, ici.Spec.BareMetalHostRef.Name)
-		log.WithError(err).Error(cond.Message)
-		return ctrl.Result{}, err
+		log.Error(err)
+		return nil, nil, err
 	}
 
 	// AutomatedCleaningMode is set at the beginning of this flow because we don't want ironic to format the disk
@@ -208,27 +278,50 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 			cond.Reason = v1alpha1.ConfigurationFailedReason
 			cond.Message = fmt.Sprintf("failed to disable automated cleaning mode for BareMetalHost %s/%s", bmh.Namespace, bmh.Name)
 			log.WithError(err).Error(cond.Message)
-			return ctrl.Result{}, err
+			return nil, nil, err
 		}
 	}
 
-	/////// Requirements not met yet: config validated, starting host validation //////
-	cond.Reason = v1alpha1.HostValidationFailedReason
+	return cd, bmh, nil
+}
 
-	if res, err := r.validateBMH(ctx, ici, bmh, &cond); !res.IsZero() || err != nil {
+func (r *ImageClusterInstallReconciler) validateHost(
+	ctx context.Context,
+	ici *v1alpha1.ImageClusterInstall,
+	bmh *bmh_v1alpha1.BareMetalHost,
+	cd *hivev1.ClusterDeployment,
+	cond *hivev1.ClusterInstallCondition,
+	log logrus.FieldLogger,
+) (ctrl.Result, error) {
+
+	if res, err := r.validateBMH(ici, bmh, cond); !res.IsZero() || err != nil {
 		return res, err
 	}
 
-	if err = r.setClusterInstallMetadata(ctx, log, ici, clusterDeployment); err != nil {
-		cond.Message = "failed to set ImageClusterInstall data"
-		log.WithError(err).Error(cond.Message)
-		return ctrl.Result{}, err
+	if !bmh.Spec.ExternallyProvisioned {
+		log.Infof("Setting BareMetalHost (%s/%s) ExternallyProvisioned spec", bmh.Namespace, bmh.Name)
+		patch := client.MergeFrom(bmh.DeepCopy())
+		bmh.Spec.ExternallyProvisioned = true
+		if err := r.Patch(ctx, bmh, patch); err != nil {
+			return ctrl.Result{}, err
+		}
+
 	}
 
-	/////// Requirements not met yet: host validated, starting image creation //////
-	cond.Reason = v1alpha1.ImageCreationFailedReason
+	return ctrl.Result{}, nil
+}
 
-	res, err := r.writeInputData(ctx, log, ici, clusterDeployment, bmh)
+func (r *ImageClusterInstallReconciler) createImage(
+	ctx context.Context,
+	ici *v1alpha1.ImageClusterInstall,
+	req ctrl.Request,
+	bmh *bmh_v1alpha1.BareMetalHost,
+	cd *hivev1.ClusterDeployment,
+	cond *hivev1.ClusterInstallCondition,
+	log logrus.FieldLogger,
+) (string, ctrl.Result, error) {
+
+	res, err := r.writeInputData(ctx, log, ici, cd, bmh)
 	if !res.IsZero() || err != nil {
 		cond.Reason = v1alpha1.ImageCreationPendingReason
 		cond.Message = "could not acquire lock for image data"
@@ -237,20 +330,26 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 			cond.Message = "failed to create image"
 			log.WithError(err).Error(cond.Message)
 		}
-		return res, err
+		return "", res, err
 	}
 
-	r.labelReferencedObjectsForBackup(ctx, log, ici, clusterDeployment)
-
 	imageUrl, err := url.JoinPath(r.BaseURL, "images", req.Namespace, fmt.Sprintf("%s.iso", ici.ObjectMeta.UID))
 	if err != nil {
 		cond.Message = "failed to create image url"
 		log.WithError(err).Error(cond.Message)
-		return ctrl.Result{}, err
+		return "", ctrl.Result{}, err
 	}
 
-	/////// Requirements not met yet: image created, starting host configuration //////
-	cond.Reason = v1alpha1.HostConfigurationFailedReason
+	return imageUrl, ctrl.Result{}, nil
+}
+func (r *ImageClusterInstallReconciler) configureHost(
+	ctx context.Context,
+	ici *v1alpha1.ImageClusterInstall,
+	imageUrl string,
+	bmh *bmh_v1alpha1.BareMetalHost,
+	cond *hivev1.ClusterInstallCondition,
+	log logrus.FieldLogger,
+) (ctrl.Result, error) {
 
 	dataImage, res, err := r.ensureBMHDataImage(ctx, log, bmh, imageUrl)
 	if err != nil {
@@ -267,8 +366,8 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 		// in case the dataImage was created less than a second ago requeue to allow BMO some time to get
 		// notified about the newly created DataImage before adding the reboot annotation in updateBMHProvisioningState
 		cond.Reason = v1alpha1.HostConfigurationPendingReason
-		cond.Message = "Waiting for BareMetalHost to get DataImage"
-		return ctrl.Result{RequeueAfter: r.Options.DataImageCoolDownPeriod}, err
+		cond.Message = "waiting for DataImage to cool down"
+		return ctrl.Result{RequeueAfter: r.Options.DataImageCoolDownPeriod}, errors.New(cond.Message)
 	}
 
 	if err := r.updateBMHProvisioningState(ctx, log, bmh, dataImage); err != nil {
@@ -279,9 +378,9 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 	if !annotationExists(&bmh.ObjectMeta, ibioManagedBMH) {
 		// TODO: consider replacing this condition with `dataDisk.Status.AttachedImage`
 		cond.Reason = v1alpha1.HostConfigurationPendingReason
-		cond.Message = fmt.Sprintf("Waiting for BMH to get %s annotation", ibioManagedBMH)
+		cond.Message = fmt.Sprintf("waiting for BMH provisioning state to be StateAvailable or StateExternallyProvisioned, current state is: %s", bmh.Status.Provisioning.State)
 		log.Info(cond.Message)
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, errors.New(cond.Message)
 	}
 
 	if ici.Status.BareMetalHostRef == nil {
@@ -298,20 +397,10 @@ func (r *ImageClusterInstallReconciler) Reconcile(ctx context.Context, req ctrl.
 		}
 	}
 
-	/////// Requirements met, host configured //////
-	cond.Status = corev1.ConditionTrue
-	cond.Reason = v1alpha1.HostConfigurationSucceededReason
-	cond.Message = "Configuration image is attached to the referenced host"
-
 	return ctrl.Result{}, nil
 }
 
-func GetClusterConfigDir(namespacesDir, namespace, uid string) string {
-	return filepath.Join(namespacesDir, namespace, uid, FilesDir, ClusterConfigDir)
-}
-
 func (r *ImageClusterInstallReconciler) validateBMH(
-	ctx context.Context,
 	ici *v1alpha1.ImageClusterInstall,
 	bmh *bmh_v1alpha1.BareMetalHost,
 	cond *hivev1.ClusterInstallCondition) (ctrl.Result, error) {
@@ -499,13 +588,7 @@ func (r *ImageClusterInstallReconciler) updateBMHProvisioningState(ctx context.C
 	if bmh.Status.Provisioning.State != bmh_v1alpha1.StateAvailable && bmh.Status.Provisioning.State != bmh_v1alpha1.StateExternallyProvisioned {
 		return nil
 	}
-	log.Infof("Updating BareMetalHost %s/%s provisioning state, current PoweredOn status is: %s", bmh.Namespace, bmh.Name, bmh.Status.PoweredOn)
-	if bmh.Status.Provisioning.State == bmh_v1alpha1.StateAvailable {
-		if !bmh.Spec.ExternallyProvisioned {
-			log.Infof("Setting BareMetalHost (%s/%s) ExternallyProvisioned spec", bmh.Namespace, bmh.Name)
-			bmh.Spec.ExternallyProvisioned = true
-		}
-	}
+	log.Infof("BareMetalHost %s/%s PoweredOn status is: %s", bmh.Namespace, bmh.Name, bmh.Status.PoweredOn)
 	if !bmh.Spec.Online {
 		bmh.Spec.Online = true
 		log.Infof("Setting BareMetalHost (%s/%s) spec.Online to true", bmh.Namespace, bmh.Name)
@@ -539,7 +622,7 @@ func (r *ImageClusterInstallReconciler) ensureBMHDataImage(
 		return dataImage, ctrl.Result{}, nil
 	}
 
-	if err != nil && !errors.IsNotFound(err) {
+	if err != nil && !k8sapierrors.IsNotFound(err) {
 		return dataImage, ctrl.Result{}, err
 	}
 	log.Infof("creating new dataImage for BareMetalHost (%s/%s)", bmh.Name, bmh.Namespace)
@@ -600,7 +683,7 @@ func (r *ImageClusterInstallReconciler) removeBMHDataImage(ctx context.Context,
 
 	bmh := &bmh_v1alpha1.BareMetalHost{}
 	if err := r.Get(ctx, bmhRef, bmh); err != nil {
-		if errors.IsNotFound(err) {
+		if k8sapierrors.IsNotFound(err) {
 			log.Warnf("Referenced BareMetalHost %s/%s does not exist, not waiting for dataImage deletion", bmhRef.Namespace, bmhRef.Name)
 			return nil, nil
 		} else {
@@ -634,7 +717,7 @@ func (r *ImageClusterInstallReconciler) deleteDataImage(ctx context.Context, log
 	dataImage := &bmh_v1alpha1.DataImage{}
 
 	if err := r.Get(ctx, dataImageRef, dataImage); err != nil {
-		if errors.IsNotFound(err) {
+		if k8sapierrors.IsNotFound(err) {
 			log.Infof("Can't find DataImage from BareMetalHost %s/%s, Nothing to remove", dataImageRef.Namespace, dataImageRef.Name)
 			return nil, nil
 		}

controllers/imageclusterinstall_controller_test.go

@@ -997,7 +997,8 @@ var _ = Describe("Reconcile", func() {
 		}
 		installerSuccess()
 		res, err := r.Reconcile(ctx, req)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("current state is: registering"))
 		Expect(res).To(Equal(ctrl.Result{}))
 
 		key := types.NamespacedName{
@@ -1295,15 +1296,15 @@ var _ = Describe("Reconcile", func() {
 		Expect(res).To(Equal(ctrl.Result{}))
 	})
 
-	It("doesn't error when ClusterDeploymentRef is unset", func() {
+	It("sets the ClusterInstallRequirementsMet condition to false when ClusterDeploymentRef is unset", func() {
 		clusterInstall.Spec.ClusterDeploymentRef = nil
 		Expect(c.Create(ctx, clusterInstall)).To(Succeed())
 		key := types.NamespacedName{
 			Namespace: clusterInstallNamespace,
 			Name:      clusterInstallName,
 		}
 		res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key})
-		Expect(err).NotTo(HaveOccurred())
+		Expect(err).To(HaveOccurred())
 		Expect(res).To(Equal(ctrl.Result{}))
 		Expect(c.Get(ctx, key, clusterInstall)).To(Succeed())
 		cond := findCondition(clusterInstall.Status.Conditions, hivev1.ClusterInstallRequirementsMet)
@@ -1323,15 +1324,15 @@ var _ = Describe("Reconcile", func() {
 			Name:      clusterInstallName,
 		}
 		res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key})
-		Expect(err).NotTo(HaveOccurred())
+		Expect(err).To(HaveOccurred())
 		Expect(res).To(Equal(ctrl.Result{}))
 
 		Expect(c.Get(ctx, key, clusterInstall)).To(Succeed())
 		cond := findCondition(clusterInstall.Status.Conditions, hivev1.ClusterInstallRequirementsMet)
 		Expect(cond).NotTo(BeNil())
 		Expect(cond.Status).To(Equal(corev1.ConditionFalse))
 		Expect(cond.Reason).To(Equal(v1alpha1.ConfigurationPendingReason))
-		Expect(cond.Message).To(Equal("no BareMetalHostRef set, nothing to do without provided bmh"))
+		Expect(cond.Message).To(Equal("BareMetalHostRef is unset"))
 	})
 
 	It("Set ClusterInstallRequirementsMet to false in case there is not actual bmh under the reference", func() {
@@ -1538,7 +1539,7 @@ var _ = Describe("Reconcile", func() {
 		Expect(err).To(HaveOccurred())
 	})
 
-	It("reque in case bmh has no hw details but after adding them it succeeds", func() {
+	It("requeue in case bmh has no hw details but after adding them it succeeds", func() {
 		bmh := bmhInState(bmh_v1alpha1.StateAvailable)
 		bmh.Status.HardwareDetails = nil
 		Expect(c.Create(ctx, bmh)).To(Succeed())
@@ -1959,7 +1960,8 @@ var _ = Describe("Reconcile with DataImageCoolDownPeriod set to 1 second", func(
 		}
 		installerSuccess()
 		res, err := r.Reconcile(ctx, req)
-		Expect(err).NotTo(HaveOccurred())
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("waiting for DataImage to cool down"))
 		Expect(res).To(Equal(ctrl.Result{RequeueAfter: time.Second}))
 		time.Sleep(time.Second)
 		res, err = r.Reconcile(ctx, req)