Merge pull request #405 from jacobweinstock/iso-mounting

Use network and ISO booting capabilities in latest Workflow spec:

## Description

<!--- Please describe what this PR is going to change -->
Add `bootOptions` to TinkerbellMachine spec. Also, update to the latest Workflow spec. This latest spec
will handle network and iso booting. It will also toggle the allowPXE field in the Hardware object.

Remove using the `allowPXE` field to track if Hardware is ready. Hardware is ready when the Workflow is successful and/or the Hardware has the label `v1alpha1.tinkerbell.org/inUse` equal to "true".

## Why is this needed

<!--- Link to issue you have raised -->

Fixes: #

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->


## How are existing users impacted? What migration steps/scripts do we need?

<!--- Fixes a bug, unblocks installation, removes a component of the stack etc -->
<!--- Requires a DB migration script, etc. -->


## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade

api/v1beta1/tinkerbellmachine_types.go

@@ -28,6 +28,9 @@ const (
 	MachineFinalizer = "tinkerbellmachine.infrastructure.cluster.x-k8s.io"
 )
 
+// BootMode defines the type of booting that will be done. i.e. netboot, iso, etc.
+type BootMode string
+
 // TinkerbellMachineSpec defines the desired state of TinkerbellMachine.
 type TinkerbellMachineSpec struct {
 	// ImageLookupFormat is the URL naming format to use for machine images when
@@ -69,12 +72,32 @@ type TinkerbellMachineSpec struct {
 	// +optional
 	HardwareAffinity *HardwareAffinity `json:"hardwareAffinity,omitempty"`
 
+	// BootOptions are options that control the booting of Hardware.
+	// +optional
+	BootOptions BootOptions `json:"bootOptions,omitempty"`
+
 	// Those fields are set programmatically, but they cannot be re-constructed from "state of the world", so
 	// we put them in spec instead of status.
 	HardwareName string `json:"hardwareName,omitempty"`
 	ProviderID   string `json:"providerID,omitempty"`
 }
 
+// BootOptions are options that control the booting of Hardware.
+type BootOptions struct {
+	// ISOURL is the URL of the ISO that will be one-time booted.
+	// When this field is set, the controller will create a job.bmc.tinkerbell.org object
+	// for getting the associated hardware into a CDROM booting state.
+	// A HardwareRef that contains a spec.BmcRef must be provided.
+	// +optional
+	// +kubebuilder:validation:Format=url
+	ISOURL string `json:"isoURL,omitempty"`
+
+	// BootMode is the type of booting that will be done.
+	// +optional
+	// +kubebuilder:validation:Enum=none;netboot;iso
+	BootMode BootMode `json:"bootMode,omitempty"`
+}
+
 // HardwareAffinity defines the required and preferred hardware affinities.
 type HardwareAffinity struct {
 	// Required are the required hardware affinity terms.  The terms are OR'd together, hardware must match one term to

config/crd/bases/infrastructure.cluster.x-k8s.io_tinkerbellmachines.yaml

@@ -62,6 +62,25 @@ spec:
           spec:
             description: TinkerbellMachineSpec defines the desired state of TinkerbellMachine.
             properties:
+              bootOptions:
+                description: BootOptions are options that control the booting of Hardware.
+                properties:
+                  bootMode:
+                    description: BootMode is the type of booting that will be done.
+                    enum:
+                    - none
+                    - netboot
+                    - iso
+                    type: string
+                  isoURL:
+                    description: |-
+                      ISOURL is the URL of the ISO that will be one-time booted.
+                      When this field is set, the controller will create a job.bmc.tinkerbell.org object
+                      for getting the associated hardware into a CDROM booting state.
+                      A HardwareRef that contains a spec.BmcRef must be provided.
+                    format: url
+                    type: string
+                type: object
               hardwareAffinity:
                 description: HardwareAffinity allows filtering for hardware.
                 properties:

config/crd/bases/infrastructure.cluster.x-k8s.io_tinkerbellmachinetemplates.yaml

@@ -51,6 +51,27 @@ spec:
                     description: Spec is the specification of the desired behavior
                       of the machine.
                     properties:
+                      bootOptions:
+                        description: BootOptions are options that control the booting
+                          of Hardware.
+                        properties:
+                          bootMode:
+                            description: BootMode is the type of booting that will
+                              be done.
+                            enum:
+                            - none
+                            - netboot
+                            - iso
+                            type: string
+                          isoURL:
+                            description: |-
+                              ISOURL is the URL of the ISO that will be one-time booted.
+                              When this field is set, the controller will create a job.bmc.tinkerbell.org object
+                              for getting the associated hardware into a CDROM booting state.
+                              A HardwareRef that contains a spec.BmcRef must be provided.
+                            format: url
+                            type: string
+                        type: object
                       hardwareAffinity:
                         description: HardwareAffinity allows filtering for hardware.
                         properties:

controller/machine/bmc.go

@@ -10,102 +10,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
-// ensureHardwareProvisionJob ensures the hardware is ready to be provisioned.
-// Uses the BMCRef from the hardware to create a BMCJob.
-// The BMCJob is responsible for getting the machine to desired state for provisioning.
-// If a BMCJob is already found and has failed, we error.
-func (scope *machineReconcileScope) ensureHardwareProvisionJob(hw *tinkv1.Hardware) error {
-	if hw.Spec.BMCRef == nil {
-		scope.log.Info("Hardware BMC reference not present; skipping BMCJob creation",
-			"BMCRef", hw.Spec.BMCRef, "Hardware", hw.Name)
-
-		return nil
-	}
-
-	bmcJob := &rufiov1.Job{}
-	jobName := fmt.Sprintf("%s-provision", scope.tinkerbellMachine.Name)
-
-	err := scope.getBMCJob(jobName, bmcJob)
-	if err != nil {
-		if apierrors.IsNotFound(err) {
-			// Create a BMCJob for hardware provisioning
-			return scope.createHardwareProvisionJob(hw, jobName)
-		}
-
-		return err
-	}
-
-	if bmcJob.HasCondition(rufiov1.JobFailed, rufiov1.ConditionTrue) {
-		return fmt.Errorf("bmc job %s/%s failed", bmcJob.Namespace, bmcJob.Name) //nolint:goerr113
-	}
-
-	return nil
-}
-
-// getBMCJob fetches the BMCJob with name JName.
-func (scope *machineReconcileScope) getBMCJob(jName string, bmj *rufiov1.Job) error {
-	namespacedName := types.NamespacedName{
-		Name:      jName,
-		Namespace: scope.tinkerbellMachine.Namespace,
-	}
-
-	if err := scope.client.Get(scope.ctx, namespacedName, bmj); err != nil {
-		return fmt.Errorf("GET BMCJob: %w", err)
-	}
-
-	return nil
-}
-
-// createHardwareProvisionJob creates a BMCJob object with the required tasks for hardware provisioning.
-func (scope *machineReconcileScope) createHardwareProvisionJob(hw *tinkv1.Hardware, name string) error {
-	job := &rufiov1.Job{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: scope.tinkerbellMachine.Namespace,
-			OwnerReferences: []metav1.OwnerReference{
-				{
-					APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
-					Kind:       "TinkerbellMachine",
-					Name:       scope.tinkerbellMachine.Name,
-					UID:        scope.tinkerbellMachine.ObjectMeta.UID,
-				},
-			},
-		},
-		Spec: rufiov1.JobSpec{
-			MachineRef: rufiov1.MachineRef{
-				Name:      hw.Spec.BMCRef.Name,
-				Namespace: scope.tinkerbellMachine.Namespace,
-			},
-			Tasks: []rufiov1.Action{
-				{
-					PowerAction: rufiov1.PowerHardOff.Ptr(),
-				},
-				{
-					OneTimeBootDeviceAction: &rufiov1.OneTimeBootDeviceAction{
-						Devices: []rufiov1.BootDevice{
-							rufiov1.PXE,
-						},
-						EFIBoot: hw.Spec.Interfaces[0].DHCP.UEFI,
-					},
-				},
-				{
-					PowerAction: rufiov1.PowerOn.Ptr(),
-				},
-			},
-		},
-	}
-
-	if err := scope.client.Create(scope.ctx, job); err != nil {
-		return fmt.Errorf("creating job: %w", err)
-	}
-
-	scope.log.Info("Created BMCJob to get hardware ready for provisioning",
-		"Name", job.Name,
-		"Namespace", job.Namespace)
-
-	return nil
-}
-
 // createPowerOffJob creates a BMCJob object with the required tasks for hardware power off.
 func (scope *machineReconcileScope) createPowerOffJob(hw *tinkv1.Hardware) error {
 	controller := true

controller/machine/hardware.go

@@ -9,7 +9,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/utils/ptr"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -25,6 +24,9 @@ const (
 	// HardwareOwnerNamespaceLabel is a label set by either CAPT controllers or Tinkerbell controller to indicate
 	// that given hardware takes part of at least one workflow.
 	HardwareOwnerNamespaceLabel = "v1alpha1.tinkerbell.org/ownerNamespace"
+
+	// HardwareInUseLabel signifies that the Hardware with this label has be provisioned by CAPT.
+	HardwareInUseLabel = "v1alpha1.tinkerbell.org/inUse"
 )
 
 var (
@@ -71,34 +73,15 @@ func hardwareIP(hardware *tinkv1.Hardware) (string, error) {
 	return hardware.Spec.Interfaces[0].DHCP.IP.Address, nil
 }
 
-func isHardwareReady(hw *tinkv1.Hardware) bool {
-	// if allowpxe false for all interface, hardware ready
-	if len(hw.Spec.Interfaces) == 0 {
-		return false
-	}
-
-	for _, ifc := range hw.Spec.Interfaces {
-		if ifc.Netboot != nil {
-			if *ifc.Netboot.AllowPXE {
-				return false
-			}
-		}
-	}
-
-	return true
-}
-
 // patchHardwareStates patches a hardware's metadata and instance states.
-func (scope *machineReconcileScope) patchHardwareStates(hw *tinkv1.Hardware, allowpxe bool) error {
+func (scope *machineReconcileScope) patchHardwareLabel(hw *tinkv1.Hardware, labels map[string]string) error {
 	patchHelper, err := patch.NewHelper(hw, scope.client)
 	if err != nil {
 		return fmt.Errorf("initializing patch helper for selected hardware: %w", err)
 	}
 
-	for _, ifc := range hw.Spec.Interfaces {
-		if ifc.Netboot != nil {
-			ifc.Netboot.AllowPXE = ptr.To(allowpxe)
-		}
+	for k, v := range labels {
+		hw.ObjectMeta.Labels[k] = v
 	}
 
 	if err := patchHelper.Patch(scope.ctx, hw); err != nil {
@@ -291,16 +274,19 @@ func (scope *machineReconcileScope) releaseHardware(hw *tinkv1.Hardware) error {
 
 	delete(hw.ObjectMeta.Labels, HardwareOwnerNameLabel)
 	delete(hw.ObjectMeta.Labels, HardwareOwnerNamespaceLabel)
-	// setting the AllowPXE=true indicates to Smee that this hardware should be allowed
-	// to netboot. FYI, this is not authoritative.
-	// Other hardware values can be set to prohibit netbooting of a machine.
-	// See this Boots function for the logic around this:
-	// https://github.com/tinkerbell/smee/blob/main/internal/ipxe/script/ipxe.go#L112
-	for _, ifc := range hw.Spec.Interfaces {
-		if ifc.Netboot != nil {
-			ifc.Netboot.AllowPXE = ptr.To(true)
+	delete(hw.ObjectMeta.Labels, HardwareInUseLabel)
+	/*
+		// setting the AllowPXE=true indicates to Smee that this hardware should be allowed
+		// to netboot. FYI, this is not authoritative.
+		// Other hardware values can be set to prohibit netbooting of a machine.
+		// See this Boots function for the logic around this:
+		// https://github.com/tinkerbell/smee/blob/main/internal/ipxe/script/ipxe.go#L112
+		for _, ifc := range hw.Spec.Interfaces {
+			if ifc.Netboot != nil {
+				ifc.Netboot.AllowPXE = ptr.To(true)
+			}
 		}
-	}
+	*/
 
 	controllerutil.RemoveFinalizer(hw, infrastructurev1.MachineFinalizer)
 

controller/machine/scope.go

@@ -126,15 +126,10 @@ func (scope *machineReconcileScope) Reconcile() error {
 }
 
 func (scope *machineReconcileScope) reconcile(hw *tinkv1.Hardware) error {
-	if isHardwareReady(hw) {
+	// If the workflow has completed the TinkerbellMachine is ready.
+	if v, found := hw.ObjectMeta.Labels[HardwareInUseLabel]; found && v == "true" {
 		scope.log.Info("Marking TinkerbellMachine as Ready")
 		scope.tinkerbellMachine.Status.Ready = true
-
-		return nil
-	}
-
-	if ensureJobErr := scope.ensureHardwareProvisionJob(hw); ensureJobErr != nil {
-		return fmt.Errorf("failed to ensure hardware ready for provisioning: %w", ensureJobErr)
 	}
 
 	wf, err := scope.ensureTemplateAndWorkflow(hw)
@@ -151,17 +146,17 @@ func (scope *machineReconcileScope) reconcile(hw *tinkv1.Hardware) error {
 		return errWorkflowFailed
 	}
 
-	if !lastActionStarted(wf) {
+	if wf.Status.State != tinkv1.WorkflowStateSuccess {
 		return nil
 	}
 
-	if err := scope.patchHardwareStates(hw, false); err != nil {
-		return fmt.Errorf("failed to patch hardware: %w", err)
-	}
-
 	scope.log.Info("Marking TinkerbellMachine as Ready")
 	scope.tinkerbellMachine.Status.Ready = true
 
+	if err := scope.patchHardwareLabel(hw, map[string]string{HardwareInUseLabel: "true"}); err != nil {
+		return fmt.Errorf("failed to patch hardware: %w", err)
+	}
+
 	return nil
 }
 

controller/machine/tinkerbellmachine.go

@@ -68,6 +68,7 @@ func (r *TinkerbellMachineReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	}
 
 	log := ctrl.LoggerFrom(ctx)
+	log.Info("starting reconcile")
 
 	scope := &machineReconcileScope{
 		log:               log,