Repository: metal3-io/baremetal-operator
Pull request #591 (2020-07-14 20:59:41)

stop reconciling paused hosts
If a host is paused, there is no need to reconcile it
periodically. Any change that removes the pause annotation will
trigger normal reconciliation.

See comments on #589 for more context

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/591#" title="Author self-approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

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":[]} -->
dhellmann: /test-integration 
dhellmann: /test-integration
metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | c0b19087b302982f725d1f60c1c8f6294322043f | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/591/unit/1283386318960726020) | `/test unit`



<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/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/deploy/crds/metal3.io_baremetalhosts_crd.yaml b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
index 8f481807..8f1ba483 100644
--- a/deploy/crds/metal3.io_baremetalhosts_crd.yaml
+++ b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
@@ -90,13 +90,6 @@ spec:
                 types, but required for libvirt VMs driven by vbmc.
               pattern: '[0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5}'
               type: string
-            bootMode:
-              description: Select the method of initializing the hardware during boot
-                to override the value based on the BMC driver.
-              enum:
-              - UEFI
-              - legacy
-              type: string
             consumerRef:
               description: ConsumerRef can be used to store information about something
                 that is using a host. When it is not empty, the host is considered
@@ -593,13 +586,6 @@ spec:
                   description: The machine's UUID from the underlying provisioning
                     tool
                   type: string
-                bootMode:
-                  description: BootMode indicates the boot mode used to provision
-                    the node
-                  enum:
-                  - UEFI
-                  - legacy
-                  type: string
                 image:
                   description: Image holds the details of the last image successfully
                     provisioned to the host.
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index cc9e2bca..c4790362 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -73,16 +73,6 @@ type RootDeviceHints struct {
 	Rotational *bool `json:"rotational,omitempty"`
 }
 
-// BootMode is the boot mode of the system
-// +kubebuilder:validation:Enum=UEFI;legacy
-type BootMode string
-
-// Allowed boot mode from metal3
-const (
-	UEFI   BootMode = "UEFI"
-	Legacy BootMode = "legacy"
-)
-
 // OperationalStatus represents the state of the host
 type OperationalStatus string
 
@@ -226,11 +216,6 @@ type BareMetalHostSpec struct {
 	// being provisioned.
 	RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`
 
-	// Select the method of initializing the hardware during boot to
-	// override the value based on the BMC driver.
-	// +optional
-	BootMode BootMode `json:"bootMode,omitempty"`
-
 	// Which MAC address will PXE boot? This is optional for some
 	// types, but required for libvirt VMs driven by vbmc.
 	// +kubebuilder:validation:Pattern=`[0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5}`
@@ -560,9 +545,6 @@ type ProvisionStatus struct {
 
 	// The RootDevicehints set by the user
 	RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`
-
-	// BootMode indicates the boot mode used to provision the node
-	BootMode BootMode `json:"bootMode,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 2cf8e518..cc62ebda 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -41,7 +41,6 @@ import (
 
 const (
 	hostErrorRetryDelay    = time.Second * 10
-	pauseRetryDelay        = time.Second * 30
 	unmanagedRetryDelay    = time.Minute * 10
 	rebootAnnotationPrefix = "reboot.metal3.io"
 )
@@ -173,6 +172,10 @@ func (r *ReconcileBareMetalHost) Reconcile(request reconcile.Request) (result re
 		}
 	}()
 
+	reqLogger := log.WithValues("Request.Namespace",
+		request.Namespace, "Request.Name", request.Name)
+	reqLogger.Info("Reconciling BareMetalHost")
+
 	// Fetch the BareMetalHost
 	host := &metal3v1alpha1.BareMetalHost{}
 	err = r.client.Get(context.TODO(), request.NamespacedName, host)
@@ -192,14 +195,11 @@ func (r *ReconcileBareMetalHost) Reconcile(request reconcile.Request) (result re
 	annotations := host.GetAnnotations()
 	if annotations != nil {
 		if _, ok := annotations[metal3v1alpha1.PausedAnnotation]; ok {
-			return reconcile.Result{Requeue: true, RequeueAfter: pauseRetryDelay}, nil
+			reqLogger.Info("host is paused, no work to do")
+			return reconcile.Result{Requeue: false}, nil
 		}
 	}
 
-	reqLogger := log.WithValues("Request.Namespace",
-		request.Namespace, "Request.Name", request.Name)
-	reqLogger.Info("Reconciling BareMetalHost")
-
 	// Check if Status is empty and status annotation is present
 	// Manually restore data.
 	if !r.hostHasStatus(host) {


Test patch:
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 039038ce..9e56227b 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -290,8 +290,11 @@ func TestPause(t *testing.T) {
 
 	tryReconcile(t, r, host,
 		func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
-			if result.Requeue && result.RequeueAfter == pauseRetryDelay &&
-				len(host.Finalizers) == 0 {
+			// Because the host is created with the annotation, we
+			// expect it to never have any reconciling done at all, so
+			// it has no finalizer.
+			t.Logf("requeue: %v  finalizers: %v", result.Requeue, host.Finalizers)
+			if !result.Requeue && len(host.Finalizers) == 0 {
 				return true
 			}
 			return false