refactor(agent): split install and join across separate Reconcile() Calls

agent/reconciler/heartbeat_test.go

@@ -137,13 +137,21 @@ func TestHostReconciler_HeartbeatSurvivesConcurrentConditionPatch(t *testing.T)
 		t.Fatal("Reconcile did not return in time")
 	}
 
-	updated := &infrastructurev1beta1.ByoHost{}
-	require.NoError(t, k8sClient.Get(t.Context(), key, updated))
+	afterInstall := &infrastructurev1beta1.ByoHost{}
+	require.NoError(t, k8sClient.Get(t.Context(), key, afterInstall))
 
-	installCond := conditions.Get(updated, infrastructurev1beta1.K8sComponentsInstallationSucceeded)
+	installCond := conditions.Get(afterInstall, infrastructurev1beta1.K8sComponentsInstallationSucceeded)
 	require.NotNil(t, installCond)
 	assert.Equal(t, corev1.ConditionTrue, installCond.Status)
 
+	// join lands on its own reconcile (see host_reconciler.go) -- one more, fast, call to reach it
+	joinResult, err := r.Reconcile(t.Context(), controllerruntime.Request{NamespacedName: key})
+	require.NoError(t, err)
+	assert.Equal(t, controllerruntime.Result{RequeueAfter: r.HeartbeatInterval}, joinResult)
+
+	updated := &infrastructurev1beta1.ByoHost{}
+	require.NoError(t, k8sClient.Get(t.Context(), key, updated))
+
 	bootstrapCond := conditions.Get(updated, infrastructurev1beta1.K8sNodeBootstrapSucceeded)
 	require.NotNil(t, bootstrapCond)
 	assert.Equal(t, corev1.ConditionTrue, bootstrapCond.Status)

agent/reconciler/host_reconciler.go

@@ -93,7 +93,10 @@ func (r *HostReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctr
 	}
 
 	result, err := r.reconcileNormal(ctx, byoHost)
-	if err == nil {
+	// Only apply the default heartbeat-driven requeue cadence if reconcileNormal
+	// didn't already ask for something more specific (e.g. an immediate
+	// requeue to continue a multi-step reconcile without waiting on it).
+	if err == nil && result.RequeueAfter == 0 && !result.Requeue {
 		result.RequeueAfter = r.HeartbeatInterval
 	}
 	return result, err
@@ -131,13 +134,7 @@ func (r *HostReconciler) reconcileNormal(ctx context.Context, byoHost *infrastru
 	}
 
 	if !conditions.IsTrue(byoHost, infrastructurev1beta1.K8sNodeBootstrapSucceeded) {
-		bootstrapScript, err := r.getBootstrapScript(ctx, byoHost.Spec.BootstrapSecret.Name, byoHost.Spec.BootstrapSecret.Namespace)
-		if err != nil {
-			logger.Error(err, "error getting bootstrap script")
-			r.Recorder.Eventf(byoHost, corev1.EventTypeWarning, "ReadBootstrapSecretFailed", "bootstrap secret %s not found", byoHost.Spec.BootstrapSecret.Name)
-			return ctrl.Result{}, err
-		}
-
+		// Both branches below fall through to the BootstrapSecret read further down within this same call: neither one runs anything slow first, so there's nothing that could go stale by the time they get there.
 		if r.SkipK8sInstallation {
 			logger.Info("Skipping installation of k8s components")
 		} else if !conditions.IsTrue(byoHost, infrastructurev1beta1.K8sComponentsInstallationSucceeded) {
@@ -146,16 +143,26 @@ func (r *HostReconciler) reconcileNormal(ctx context.Context, byoHost *infrastru
 				conditions.MarkFalse(byoHost, infrastructurev1beta1.K8sComponentsInstallationSucceeded, infrastructurev1beta1.K8sInstallationSecretUnavailableReason, clusterv1.ConditionSeverityInfo, "")
 				return ctrl.Result{}, nil
 			}
-			err = r.executeInstallerController(ctx, byoHost)
+			err := r.executeInstallerController(ctx, byoHost)
 			if err != nil {
 				return ctrl.Result{}, err
 			}
 			r.Recorder.Event(byoHost, corev1.EventTypeNormal, "InstallScriptExecutionSucceeded", "install script executed")
 			conditions.MarkTrue(byoHost, infrastructurev1beta1.K8sComponentsInstallationSucceeded)
+
+			// Stop here instead of reading BootstrapSecret and joining right away: install has no time bound, so the kubeadm join token it contains could be rotated while we wait. Requeue immediately (not tied to HeartbeatInterval) so the next reconcile reads it fresh right before joining, with nothing slow in between.
+			return ctrl.Result{Requeue: true}, nil
 		} else {
 			logger.Info("install script already executed")
 		}
 
+		bootstrapScript, err := r.getBootstrapScript(ctx, byoHost.Spec.BootstrapSecret.Name, byoHost.Spec.BootstrapSecret.Namespace)
+		if err != nil {
+			logger.Error(err, "error getting bootstrap script")
+			r.Recorder.Eventf(byoHost, corev1.EventTypeWarning, "ReadBootstrapSecretFailed", "bootstrap secret %s not found", byoHost.Spec.BootstrapSecret.Name)
+			return ctrl.Result{}, err
+		}
+
 		err = r.cleank8sdirectories(ctx)
 		if err != nil {
 			logger.Error(err, "error cleaning up k8s directories, please delete it manually for reconcile to proceed.")

agent/reconciler/reconciler_test.go

@@ -149,6 +149,8 @@ var _ = Describe("Byohost Agent Tests", func() {
 			})
 
 			It("should return an error if we fail to load the bootstrap secret", func() {
+				// host_reconciler.go returns before reading BootstrapSecret unless install is already marked done; mark it here since this test isn't exercising install.
+				conditions.MarkTrue(byoHost, infrastructurev1beta1.K8sComponentsInstallationSucceeded)
 				byoHost.Spec.BootstrapSecret = &corev1.ObjectReference{
 					Kind:      kindSecret,
 					Namespace: nonExistentName,
@@ -204,6 +206,10 @@ runCmd:
 					Expect(result).To(Equal(controllerruntime.Result{}))
 					Expect(reconcilerErr).ToNot(HaveOccurred())
 
+					// only the join step's runCmd/write_files ran -- no install call, and no second reconcile was needed to reach it
+					Expect(fakeCommandRunner.RunCmdCallCount()).To(Equal(1))
+					Expect(fakeFileWriter.WriteToFileCallCount()).To(Equal(1))
+
 					updatedByoHost := &infrastructurev1beta1.ByoHost{}
 					err := k8sClient.Get(ctx, byoHostLookupKey, updatedByoHost)
 					Expect(err).ToNot(HaveOccurred())
@@ -304,6 +310,11 @@ runCmd:
 					})
 
 					It("should set K8sNodeBootstrapSucceeded to True if the boostrap execution succeeds", func() {
+						// install lands on the first reconcile, join on the next (see host_reconciler.go)
+						_, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
+							NamespacedName: byoHostLookupKey,
+						})
+						Expect(reconcilerErr).ToNot(HaveOccurred())
 
 						result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
@@ -357,7 +368,13 @@ runCmd:
 							return nil
 						}
 
-						// first reconcile: install succeeds, join fails on the stale/expired token
+						// first reconcile: install only (see host_reconciler.go -- join lands on the next reconcile)
+						_, installErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
+							NamespacedName: byoHostLookupKey,
+						})
+						Expect(installErr).ToNot(HaveOccurred())
+
+						// second reconcile: join fails on the stale/expired token
 						_, firstErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
 						})
@@ -381,7 +398,7 @@ runCmd:
 						latest.Data["value"] = []byte(freshSecretData)
 						Expect(k8sClient.Update(ctx, latest)).To(Succeed())
 
-						// second reconcile: install is skipped, so this attempt reads the secret fresh with no gap
+						// third reconcile: install is already done, so this attempt reads the secret fresh with no gap
 						_, secondErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
 						})
@@ -502,7 +519,8 @@ runCmd:
 						result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
 						})
-						Expect(result).To(Equal(controllerruntime.Result{}))
+						// requeues immediately after install rather than waiting on HeartbeatInterval -- see the Reconcile wrapper
+						Expect(result).To(Equal(controllerruntime.Result{Requeue: true}))
 						Expect(reconcilerErr).NotTo(HaveOccurred())
 
 						updatedByoHost := &infrastructurev1beta1.ByoHost{}
@@ -516,7 +534,8 @@ runCmd:
 						result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
 						})
-						Expect(result).To(Equal(controllerruntime.Result{}))
+						// requeues immediately rather than waiting on HeartbeatInterval -- see the Reconcile wrapper
+						Expect(result).To(Equal(controllerruntime.Result{Requeue: true}))
 						Expect(reconcilerErr).ToNot(HaveOccurred())
 
 						updatedByoHost := &infrastructurev1beta1.ByoHost{}
@@ -528,16 +547,31 @@ runCmd:
 							Type:   infrastructurev1beta1.K8sComponentsInstallationSucceeded,
 							Status: corev1.ConditionTrue,
 						}))
+					})
 
-						// assert events
+					It("should requeue immediately after install regardless of HeartbeatInterval", func() {
+						hostReconciler.HeartbeatInterval = time.Hour // deliberately huge -- the post-install requeue must not inherit this
+
+						result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
+							NamespacedName: byoHostLookupKey,
+						})
+						Expect(reconcilerErr).ToNot(HaveOccurred())
+						Expect(result).To(Equal(controllerruntime.Result{Requeue: true}))
+
+						// assert events -- join lands on the next reconcile (see host_reconciler.go), so only the install event has fired so far
 						events := eventutils.CollectEvents(recorder.Events)
 						Expect(events).Should(ConsistOf([]string{
 							eventInstallScriptExecutionSucceeded,
-							eventBootstrapK8sNodeSucceeded,
 						}))
 					})
 
 					It("should set K8sNodeBootstrapSucceeded to True if the boostrap execution succeeds", func() {
+						// install lands on the first reconcile, join on the next (see host_reconciler.go)
+						_, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
+							NamespacedName: byoHostLookupKey,
+						})
+						Expect(reconcilerErr).ToNot(HaveOccurred())
+
 						result, reconcilerErr := hostReconciler.Reconcile(ctx, controllerruntime.Request{
 							NamespacedName: byoHostLookupKey,
 						})