🌱 Refactor Hcloud Provisioning, use Status.BootState (#1650)

Co-authored-by: Dhairya Arora <[email protected]>

Author: guettli

api/v1beta1/hcloudmachine_types.go

@@ -96,6 +96,19 @@ type HCloudMachineStatus struct {
 	// Conditions define the current service state of the HCloudMachine.
 	// +optional
 	Conditions clusterv1.Conditions `json:"conditions,omitempty"`
+
+	// BootState indicates the current state during provisioning.
+	//
+	// The states will be:
+	// "" -> BootToRealOS -> OperatingSystemRunning
+	//
+	// +optional
+	BootState HCloudBootState `json:"bootState"`
+
+	// BootStateSince is the timestamp of the last change to BootState. It is used to timeout
+	// provisioning if a state takes too long.
+	// +optional
+	BootStateSince metav1.Time `json:"bootStateSince,omitzero"`
 }
 
 // HCloudMachine is the Schema for the hcloudmachines API.
@@ -128,6 +141,15 @@ func (r *HCloudMachine) SetConditions(conditions clusterv1.Conditions) {
 	r.Status.Conditions = conditions
 }
 
+// SetBootState sets Status.BootStates and updates Status.BootStateSince.
+func (r *HCloudMachine) SetBootState(bootState HCloudBootState) {
+	if r.Status.BootState == bootState {
+		return
+	}
+	r.Status.BootState = bootState
+	r.Status.BootStateSince = metav1.Now()
+}
+
 //+kubebuilder:object:root=true
 
 // HCloudMachineList contains a list of HCloudMachine.

api/v1beta1/hcloudmachine_validation.go

@@ -22,7 +22,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-func validateHCloudMachineSpec(oldSpec, newSpec HCloudMachineSpec) field.ErrorList {
+func validateHCloudMachineSpecUpdate(oldSpec, newSpec HCloudMachineSpec) field.ErrorList {
 	var allErrs field.ErrorList
 	// Type is immutable
 	if !reflect.DeepEqual(oldSpec.Type, newSpec.Type) {

api/v1beta1/hcloudmachine_validation_test.go

@@ -28,7 +28,7 @@ type args struct {
 	newSpec HCloudMachineSpec
 }
 
-func TestValidateHCloudMachineSpec(t *testing.T) {
+func TestValidateHCloudMachineSpecUpdate(t *testing.T) {
 	tests := []struct {
 		name string
 		args args
@@ -38,10 +38,12 @@ func TestValidateHCloudMachineSpec(t *testing.T) {
 			name: "Immutable Type",
 			args: args{
 				oldSpec: HCloudMachineSpec{
-					Type: "cpx11",
+					ImageName: "ubuntu-24.04",
+					Type:      "cpx11",
 				},
 				newSpec: HCloudMachineSpec{
-					Type: "cx21",
+					ImageName: "ubuntu-24.04",
+					Type:      "cx21",
 				},
 			},
 			want: field.Invalid(field.NewPath("spec", "type"), "cx21", "field is immutable"),
@@ -62,6 +64,7 @@ func TestValidateHCloudMachineSpec(t *testing.T) {
 			name: "Immutable SSHKeys",
 			args: args{
 				oldSpec: HCloudMachineSpec{
+					ImageName: "ubuntu-24.04",
 					SSHKeys: []SSHKey{
 						{
 							Name:        "ssh-key-1",
@@ -70,6 +73,7 @@ func TestValidateHCloudMachineSpec(t *testing.T) {
 					},
 				},
 				newSpec: HCloudMachineSpec{
+					ImageName: "ubuntu-24.04",
 					SSHKeys: []SSHKey{
 						{
 							Name:        "ssh-key-1",
@@ -97,9 +101,11 @@ func TestValidateHCloudMachineSpec(t *testing.T) {
 			name: "Immutable PlacementGroupName",
 			args: args{
 				oldSpec: HCloudMachineSpec{
+					ImageName:          "ubuntu-24.04",
 					PlacementGroupName: createPlacementGroupName("placement-group-1"),
 				},
 				newSpec: HCloudMachineSpec{
+					ImageName:          "ubuntu-24.04",
 					PlacementGroupName: createPlacementGroupName("placement-group-2"),
 				},
 			},
@@ -124,24 +130,21 @@ func TestValidateHCloudMachineSpec(t *testing.T) {
 			want: nil,
 		},
 	}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := validateHCloudMachineSpec(tt.args.oldSpec, tt.args.newSpec)
+			got := validateHCloudMachineSpecUpdate(tt.args.oldSpec, tt.args.newSpec)
 
-			if len(got) == 0 {
+			if tt.want == nil {
 				assert.Empty(t, got)
+				return
 			}
 
-			if len(got) > 1 {
-				t.Errorf("got length: %d greater than 1", len(got))
-			}
+			assert.Equal(t, len(got), 1, "got length: %d greater than 1: %+v", len(got), got)
 
-			// assert if length of got is 1
-			if len(got) == 1 {
-				assert.Equal(t, tt.want.Type, got[0].Type)
-				assert.Equal(t, tt.want.Field, got[0].Field)
-				assert.Equal(t, tt.want.Detail, got[0].Detail)
-			}
+			assert.Equal(t, tt.want.Type, got[0].Type)
+			assert.Equal(t, tt.want.Field, got[0].Field)
+			assert.Equal(t, tt.want.Detail, got[0].Detail)
 		})
 	}
 }

api/v1beta1/hcloudmachine_webhook.go

@@ -81,7 +81,7 @@ func (r *HCloudMachine) ValidateUpdate(old runtime.Object) (admission.Warnings,
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an HCloudMachine but got a %T", old))
 	}
 
-	allErrs := validateHCloudMachineSpec(oldM.Spec, r.Spec)
+	allErrs := validateHCloudMachineSpecUpdate(oldM.Spec, r.Spec)
 
 	return nil, aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
 }

api/v1beta1/types.go

@@ -263,3 +263,17 @@ func (s *HCloudNetworkSpec) IsZero() bool {
 	}
 	return true
 }
+
+// HCloudBootState defines the boot state of an HCloud server.
+type HCloudBootState string
+
+const (
+	// HCloudBootStateUnset is the initial state when the boot state has not been set yet.
+	HCloudBootStateUnset HCloudBootState = ""
+
+	// HCloudBootStateBootToRealOS indicates that the server is booting the operating system.
+	HCloudBootStateBootToRealOS HCloudBootState = "BootToRealOS"
+
+	// HCloudBootStateOperatingSystemRunning indicates that the server is successfully running.
+	HCloudBootStateOperatingSystemRunning HCloudBootState = "OperatingSystemRunning"
+)

api/v1beta1/zz_generated.deepcopy.go

@@ -181,6 +181,20 @@ spec:
                   - type
                   type: object
                 type: array
+              bootState:
+                description: |-
+                  BootState indicates the current state during provisioning.
+
+
+                  The states will be:
+                  "" -> BootToRealOS -> OperatingSystemRunning
+                type: string
+              bootStateSince:
+                description: |-
+                  BootStateSince is the timestamp of the last change to BootState. It is used to timeout
+                  provisioning if a state takes too long.
+                format: date-time
+                type: string
               conditions:
                 description: Conditions define the current service state of the HCloudMachine.
                 items:

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

@@ -25,7 +25,9 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
+	"github.com/google/go-cmp/cmp"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog/v2"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
@@ -145,6 +147,8 @@ func (r *HCloudMachineReconciler) Reconcile(ctx context.Context, req reconcile.R
 		return reconcile.Result{}, fmt.Errorf("failed to create scope: %+v", err)
 	}
 
+	initialHCloudMachine := hcloudMachine.DeepCopy()
+	startReconcile := time.Now()
 	// Always close the scope when exiting this function so we can persist any HCloudMachine changes.
 	defer func() {
 		if reterr != nil && errors.Is(reterr, hcloudclient.ErrUnauthorized) {
@@ -153,10 +157,66 @@ func (r *HCloudMachineReconciler) Reconcile(ctx context.Context, req reconcile.R
 			conditions.MarkTrue(hcloudMachine, infrav1.HCloudTokenAvailableCondition)
 		}
 
+		// the Close() will use PatchHelper to store the changes.
 		if err := machineScope.Close(ctx); err != nil {
 			res = reconcile.Result{}
 			reterr = errors.Join(reterr, err)
 		}
+
+		if !cmp.Equal(initialHCloudMachine, hcloudMachine) {
+			// The hcloudMachine was changed. Wait until the local cache contains the revision
+			// which was created by above machineScope.Close().
+			// We want to read our own writes.
+			err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (done bool, err error) {
+				// new resource, read from local cache
+				latest := &infrav1.HCloudMachine{}
+				getErr := r.Get(ctx, client.ObjectKeyFromObject(machineScope.HCloudMachine), latest)
+				if apierrors.IsNotFound(getErr) {
+					// the object was deleted. All is fine.
+					return true, nil
+				}
+				if getErr != nil {
+					return false, getErr
+				}
+				// When the ResourceVersion has changed, then it is very likely that the local
+				// cache has the new version.
+				return latest.ResourceVersion != hcloudMachine.ResourceVersion, nil
+			})
+			if err != nil {
+				log.Error(err, "cache sync failed after BootState change")
+			}
+		}
+
+		readyReason := conditions.GetReason(machineScope.HCloudMachine, clusterv1.ReadyCondition)
+		readyMessage := conditions.GetMessage(machineScope.HCloudMachine, clusterv1.ReadyCondition)
+
+		duration := time.Since(startReconcile)
+
+		if duration > 5*time.Second {
+			log.Info("Reconcile took too long",
+				"reconcileDuration", duration,
+				"res", res,
+				"reterr", reterr,
+				"oldState", initialHCloudMachine.Status.BootState,
+				"newState", machineScope.HCloudMachine.Status.BootState,
+				"readyReason", readyReason,
+				"readyMessage", readyMessage,
+			)
+		}
+
+		if initialHCloudMachine.Status.BootState != machineScope.HCloudMachine.Status.BootState {
+			startBootState := initialHCloudMachine.Status.BootStateSince
+			if startBootState.IsZero() {
+				startBootState = initialHCloudMachine.CreationTimestamp
+			}
+			log.Info("BootState changed",
+				"oldState", initialHCloudMachine.Status.BootState,
+				"newState", machineScope.HCloudMachine.Status.BootState,
+				"durationInState", machineScope.HCloudMachine.Status.BootStateSince.Time.Sub(startBootState.Time),
+				"readyReason", readyReason,
+				"readyMessage", readyMessage,
+			)
+		}
 	}()
 
 	// Check whether rate limit has been reached and if so, then wait.
@@ -168,6 +228,10 @@ func (r *HCloudMachineReconciler) Reconcile(ctx context.Context, req reconcile.R
 		return r.reconcileDelete(ctx, machineScope)
 	}
 
+	if hcloudMachine.Status.FailureReason != nil {
+		// This machine will be removed.
+		return reconcile.Result{}, nil
+	}
 	return r.reconcileNormal(ctx, machineScope)
 }
 
@@ -460,6 +524,13 @@ func IgnoreInsignificantHCloudMachineStatusUpdates(logger logr.Logger) predicate
 			oldHCloudMachine.ResourceVersion = ""
 			newHCloudMachine.ResourceVersion = ""
 
+			// The ProviderID is set by the controller. Do not react if that changes.
+			// Otherwise the next Reconcile is likely to read outdated data, because
+			// the Status was not updated yet. PatchHelper updates three times in this order:
+			// Status.Conditions, Resource, Status.
+			oldHCloudMachine.Spec.ProviderID = nil
+			newHCloudMachine.Spec.ProviderID = nil
+
 			oldHCloudMachine.Status = infrav1.HCloudMachineStatus{}
 			newHCloudMachine.Status = infrav1.HCloudMachineStatus{}
 

controllers/hcloudmachine_controller.go

@@ -365,7 +365,7 @@ var _ = Describe("HCloudMachineReconciler", func() {
 				}, timeout).Should(BeTrue())
 			})
 
-			It("creates the HCloud machine in Hetzner 1", func() {
+			It("creates the HCloud machine in Hetzner 1 (flaky)", func() {
 				By("checking that no servers exist")
 
 				Eventually(func() bool {
@@ -433,6 +433,24 @@ var _ = Describe("HCloudMachineReconciler", func() {
 				Eventually(func() bool {
 					return isPresentAndTrue(key, hcloudMachine, infrav1.ServerAvailableCondition)
 				}, timeout, interval).Should(BeTrue())
+
+				By("checking if the BootState is now OperatingSystemRunning")
+				Eventually(func() bool {
+					if err = testEnv.Get(ctx, key, hcloudMachine); err != nil {
+						return false
+					}
+
+					return hcloudMachine.Status.BootState == infrav1.HCloudBootStateOperatingSystemRunning && !hcloudMachine.Status.BootStateSince.IsZero()
+				}, timeout, interval).Should(BeTrue())
+
+				By("checking if the ssh keys are set in the status")
+				Eventually(func() bool {
+					if err = testEnv.Get(ctx, key, hcloudMachine); err != nil {
+						return false
+					}
+
+					return len(hcloudMachine.Status.SSHKeys) == 1 && hcloudMachine.Status.SSHKeys[0].Name == "testsshkey"
+				}, timeout, interval).Should(BeTrue())
 			})
 		})
 

controllers/hcloudmachine_controller_test.go

@@ -7,6 +7,7 @@ require (
 	github.com/bramvdbogaerde/go-scp v1.5.0
 	github.com/go-logr/logr v1.4.2
 	github.com/go-logr/zapr v1.3.0
+	github.com/google/go-cmp v0.7.0
 	github.com/guettli/check-conditions v0.0.20
 	github.com/hetznercloud/hcloud-go/v2 v2.19.1
 	github.com/mitchellh/copystructure v1.2.0
@@ -72,7 +73,6 @@ require (
 	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/google/cel-go v0.17.8 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.7.0 // indirect
 	github.com/google/go-github/v53 v53.2.0 // indirect
 	github.com/google/go-querystring v1.1.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect