Address comments

test/infrastructure/inmemory/api/v1alpha1/inmemorymachine_types.go

@@ -29,7 +29,7 @@ const (
 )
 
 const (
-	// VMProvisionedCondition documents the status of the provisioning VM implementing to the InMemoryMachine.
+	// VMProvisionedCondition documents the status of the provisioning VM implementing the InMemoryMachine.
 	VMProvisionedCondition clusterv1.ConditionType = "VMProvisioned"
 
 	// WaitingForClusterInfrastructureReason (Severity=Info) documents an InMemoryMachine VM waiting for the cluster
@@ -44,32 +44,32 @@ const (
 	// data to be ready before starting to create the CloudMachine/VM.
 	WaitingForBootstrapDataReason = "WaitingForBootstrapData"
 
-	// VMProvisioningReason (Severity=Info) documents a InMemoryMachine VM provisioning.
-	VMProvisioningReason = "WaitingForStartupTimeout"
+	// VMWaitingForStartupTimeoutReason (Severity=Info) documents a InMemoryMachine VM provisioning.
+	VMWaitingForStartupTimeoutReason = "WaitingForStartupTimeout"
 )
 
 const (
 	// NodeProvisionedCondition documents the status of the provisioning of the node hosted on the InMemoryMachine.
 	NodeProvisionedCondition clusterv1.ConditionType = "NodeProvisioned"
 
-	// NodeProvisioningReason (Severity=Info) documents a InMemoryMachine Node provisioning.
-	NodeProvisioningReason = "WaitingForStartupTimeout"
+	// NodeWaitingForStartupTimeoutReason (Severity=Info) documents a InMemoryMachine Node provisioning.
+	NodeWaitingForStartupTimeoutReason = "WaitingForStartupTimeout"
 )
 
 const (
 	// EtcdProvisionedCondition documents the status of the provisioning of the etcd member hosted on the InMemoryMachine.
 	EtcdProvisionedCondition clusterv1.ConditionType = "EtcdProvisioned"
 
-	// EtcdProvisioningReason (Severity=Info) documents a InMemoryMachine etcd pod provisioning.
-	EtcdProvisioningReason = "WaitingForStartupTimeout"
+	// EtcdWaitingForStartupTimeoutReason (Severity=Info) documents a InMemoryMachine etcd pod provisioning.
+	EtcdWaitingForStartupTimeoutReason = "WaitingForStartupTimeout"
 )
 
 const (
 	// APIServerProvisionedCondition documents the status of the provisioning of the APIServer instance hosted on the InMemoryMachine.
 	APIServerProvisionedCondition clusterv1.ConditionType = "APIServerProvisioned"
 
-	// APIServerProvisioningReason (Severity=Info) documents a InMemoryMachine API server pod provisioning.
-	APIServerProvisioningReason = "WaitingForStartupTimeout"
+	// APIServerWaitingForStartupTimeoutReason (Severity=Info) documents a InMemoryMachine API server pod provisioning.
+	APIServerWaitingForStartupTimeoutReason = "WaitingForStartupTimeout"
 )
 
 // InMemoryMachineSpec defines the desired state of InMemoryMachine.
@@ -85,7 +85,7 @@ type InMemoryMachineSpec struct {
 
 // InMemoryMachineBehaviour defines the behaviour of the InMemoryMachine.
 type InMemoryMachineBehaviour struct {
-	// VM defines the behaviour of the VM implementing to InMemoryMachine.
+	// VM defines the behaviour of the VM implementing the InMemoryMachine.
 	VM *InMemoryVMBehaviour `json:"vm,omitempty"`
 
 	// Node defines the behaviour of the Node (the kubelet) hosted on the InMemoryMachine.
@@ -98,9 +98,9 @@ type InMemoryMachineBehaviour struct {
 	Etcd *InMemoryEtcdBehaviour `json:"etcd,omitempty"`
 }
 
-// InMemoryVMBehaviour defines the behaviour of the VM implementing to InMemoryMachine.
+// InMemoryVMBehaviour defines the behaviour of the VM implementing the InMemoryMachine.
 type InMemoryVMBehaviour struct {
-	// Provisioning defines variables influencing how the VM implementing to InMemoryMachine is going to be provisioned.
+	// Provisioning defines variables influencing how the VM implementing the InMemoryMachine is going to be provisioned.
 	// NOTE: VM provisioning includes all the steps from creation to power-on.
 	Provisioning CommonProvisioningSettings `json:"provisioning,omitempty"`
 }
@@ -122,7 +122,7 @@ type InMemoryAPIServerBehaviour struct {
 // InMemoryEtcdBehaviour defines the behaviour of the etcd member hosted on the InMemoryMachine.
 type InMemoryEtcdBehaviour struct {
 	// Provisioning defines variables influencing how the etcd member hosted on the InMemoryMachine is going to be provisioned.
-	// NOTE: Etcd provisioning includes all the steps from starting the static to the Pod become ready and being registered in K8s.
+	// NOTE: Etcd provisioning includes all the steps from starting the static Pod to the Pod become ready and being registered in K8s.
 	Provisioning CommonProvisioningSettings `json:"provisioning,omitempty"`
 }
 
@@ -131,7 +131,7 @@ type CommonProvisioningSettings struct {
 	// StartupDuration defines the duration of the object provisioning phase.
 	StartupDuration metav1.Duration `json:"startupDuration"`
 
-	// StartupJitter adds some randomness on StartupSeconds; the actual duration will be StartupDuration plus an additional
+	// StartupJitter adds some randomness on StartupDuration; the actual duration will be StartupDuration plus an additional
 	// amount chosen uniformly at random from the interval between zero and `StartupJitter*StartupDuration`.
 	// NOTE: this is modeled as string because the usage of float is highly discouraged, as support for them varies across languages.
 	StartupJitter string `json:"startupJitter,omitempty"`

test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go

@@ -263,7 +263,10 @@ func (r *InMemoryMachineReconciler) reconcileNormalCloudMachine(ctx context.Cont
 		x := inMemoryMachine.Spec.Behaviour.VM.Provisioning
 
 		provisioningDuration = x.StartupDuration.Duration
-		jitter, _ := strconv.ParseFloat(x.StartupJitter, 64)
+		jitter, err := strconv.ParseFloat(x.StartupJitter, 64)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to parse VM's StartupJitter")
+		}
 		if jitter > 0.0 {
 			provisioningDuration += time.Duration(rand.Float64() * jitter * float64(provisioningDuration)) //nolint:gosec // Intentionally using a weak random number generator here.
 		}
@@ -272,7 +275,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalCloudMachine(ctx context.Cont
 	start := cloudMachine.CreationTimestamp
 	now := time.Now()
 	if now.Before(start.Add(provisioningDuration)) {
-		conditions.MarkFalse(inMemoryMachine, infrav1.VMProvisionedCondition, infrav1.VMProvisioningReason, clusterv1.ConditionSeverityInfo, "")
+		conditions.MarkFalse(inMemoryMachine, infrav1.VMProvisionedCondition, infrav1.VMWaitingForStartupTimeoutReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{RequeueAfter: start.Add(provisioningDuration).Sub(now)}, nil
 	}
 
@@ -294,7 +297,10 @@ func (r *InMemoryMachineReconciler) reconcileNormalNode(ctx context.Context, clu
 		x := inMemoryMachine.Spec.Behaviour.Node.Provisioning
 
 		provisioningDuration = x.StartupDuration.Duration
-		jitter, _ := strconv.ParseFloat(x.StartupJitter, 64)
+		jitter, err := strconv.ParseFloat(x.StartupJitter, 64)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to parse node's StartupJitter")
+		}
 		if jitter > 0.0 {
 			provisioningDuration += time.Duration(rand.Float64() * jitter * float64(provisioningDuration)) //nolint:gosec // Intentionally using a weak random number generator here.
 		}
@@ -303,7 +309,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalNode(ctx context.Context, clu
 	start := conditions.Get(inMemoryMachine, infrav1.VMProvisionedCondition).LastTransitionTime
 	now := time.Now()
 	if now.Before(start.Add(provisioningDuration)) {
-		conditions.MarkFalse(inMemoryMachine, infrav1.NodeProvisionedCondition, infrav1.NodeProvisioningReason, clusterv1.ConditionSeverityInfo, "")
+		conditions.MarkFalse(inMemoryMachine, infrav1.NodeProvisionedCondition, infrav1.NodeWaitingForStartupTimeoutReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{RequeueAfter: start.Add(provisioningDuration).Sub(now)}, nil
 	}
 
@@ -339,7 +345,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalNode(ctx context.Context, clu
 
 	if err := cloudClient.Get(ctx, client.ObjectKeyFromObject(node), node); err != nil {
 		if !apierrors.IsNotFound(err) {
-			return ctrl.Result{}, errors.Wrapf(err, "failed to get etcd Pod")
+			return ctrl.Result{}, errors.Wrapf(err, "failed to get node")
 		}
 
 		// NOTE: for the first control plane machine we might create the node before etcd and API server pod are running
@@ -373,7 +379,10 @@ func (r *InMemoryMachineReconciler) reconcileNormalETCD(ctx context.Context, clu
 		x := inMemoryMachine.Spec.Behaviour.Etcd.Provisioning
 
 		provisioningDuration = x.StartupDuration.Duration
-		jitter, _ := strconv.ParseFloat(x.StartupJitter, 64)
+		jitter, err := strconv.ParseFloat(x.StartupJitter, 64)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to parse etcd's StartupJitter")
+		}
 		if jitter > 0.0 {
 			provisioningDuration += time.Duration(rand.Float64() * jitter * float64(provisioningDuration)) //nolint:gosec // Intentionally using a weak random number generator here.
 		}
@@ -382,7 +391,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalETCD(ctx context.Context, clu
 	start := conditions.Get(inMemoryMachine, infrav1.NodeProvisionedCondition).LastTransitionTime
 	now := time.Now()
 	if now.Before(start.Add(provisioningDuration)) {
-		conditions.MarkFalse(inMemoryMachine, infrav1.EtcdProvisionedCondition, infrav1.EtcdProvisioningReason, clusterv1.ConditionSeverityInfo, "")
+		conditions.MarkFalse(inMemoryMachine, infrav1.EtcdProvisionedCondition, infrav1.EtcdWaitingForStartupTimeoutReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{RequeueAfter: start.Add(provisioningDuration).Sub(now)}, nil
 	}
 
@@ -426,7 +435,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalETCD(ctx context.Context, clu
 		// NOTE: for the first control plane machine we might create the etcd pod before the API server pod is running
 		// but this is not an issue, because it won't be visible to CAPI until the API server start serving requests.
 		if err := cloudClient.Create(ctx, etcdPod); err != nil && !apierrors.IsAlreadyExists(err) {
-			return ctrl.Result{}, errors.Wrapf(err, "failed to create etcdPod Pod")
+			return ctrl.Result{}, errors.Wrapf(err, "failed to create Pod")
 		}
 	}
 
@@ -483,7 +492,10 @@ func (r *InMemoryMachineReconciler) reconcileNormalAPIServer(ctx context.Context
 		x := inMemoryMachine.Spec.Behaviour.APIServer.Provisioning
 
 		provisioningDuration = x.StartupDuration.Duration
-		jitter, _ := strconv.ParseFloat(x.StartupJitter, 64)
+		jitter, err := strconv.ParseFloat(x.StartupJitter, 64)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to parse API server's StartupJitter")
+		}
 		if jitter > 0.0 {
 			provisioningDuration += time.Duration(rand.Float64() * jitter * float64(provisioningDuration)) //nolint:gosec // Intentionally using a weak random number generator here.
 		}
@@ -492,7 +504,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalAPIServer(ctx context.Context
 	start := conditions.Get(inMemoryMachine, infrav1.NodeProvisionedCondition).LastTransitionTime
 	now := time.Now()
 	if now.Before(start.Add(provisioningDuration)) {
-		conditions.MarkFalse(inMemoryMachine, infrav1.APIServerProvisionedCondition, infrav1.APIServerProvisioningReason, clusterv1.ConditionSeverityInfo, "")
+		conditions.MarkFalse(inMemoryMachine, infrav1.APIServerProvisionedCondition, infrav1.APIServerWaitingForStartupTimeoutReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{RequeueAfter: start.Add(provisioningDuration).Sub(now)}, nil
 	}
 
@@ -501,7 +513,7 @@ func (r *InMemoryMachineReconciler) reconcileNormalAPIServer(ctx context.Context
 	resourceGroup := klog.KObj(cluster).String()
 	cloudClient := r.CloudManager.GetResourceGroup(resourceGroup).GetClient()
 
-	// Create the etcd pod
+	// Create the apiserver pod
 	// TODO: consider if to handle an additional setting adding a delay in between create pod and pod ready
 	apiServer := fmt.Sprintf("kube-apiserver-%s", inMemoryMachine.Name)
 
@@ -752,7 +764,7 @@ func (r *InMemoryMachineReconciler) reconcileDelete(ctx context.Context, cluster
 		}
 		res = util.LowestNonZeroResult(res, phaseResult)
 	}
-	if res.IsZero() {
+	if res.IsZero() && len(errs) == 0 {
 		controllerutil.RemoveFinalizer(inMemoryMachine, infrav1.MachineFinalizer)
 	}
 	return res, kerrors.NewAggregate(errs)
@@ -815,7 +827,7 @@ func (r *InMemoryMachineReconciler) reconcileDeleteETCD(ctx context.Context, clu
 		},
 	}
 	if err := cloudClient.Delete(ctx, etcdPod); err != nil && !apierrors.IsNotFound(err) {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to etcd Pod")
+		return ctrl.Result{}, errors.Wrapf(err, "failed to delete etcd Pod")
 	}
 	if err := r.APIServerMux.DeleteEtcdMember(resourceGroup, etcdMember); err != nil {
 		return ctrl.Result{}, err
@@ -848,7 +860,7 @@ func (r *InMemoryMachineReconciler) reconcileDeleteAPIServer(ctx context.Context
 		},
 	}
 	if err := cloudClient.Delete(ctx, apiServerPod); err != nil && !apierrors.IsNotFound(err) {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to apiServer Pod")
+		return ctrl.Result{}, errors.Wrapf(err, "failed to delete apiServer Pod")
 	}
 	if err := r.APIServerMux.DeleteAPIServer(resourceGroup, apiServer); err != nil {
 		return ctrl.Result{}, err

test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller_test.go

@@ -69,7 +69,7 @@ var (
 
 	workerMachine = &clusterv1.Machine{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "bar",
+			Name: "baz",
 		},
 	}
 )