Use get instead of list to retrieve servers

Under certain circumstances, when they are non-responsive cells, nova
may return a partial result instead of an error.

(this is controlled by list_records_by_skipping_down_cells parameter
that is true by default - no error will be thrown)

When it faces this issue, capo controller will try to create a new
server for the machine, resulting in deleting ports of existing one.

In order avoid this issue, use GET instead of LIST with server uuid stored
in machine spec when it is created, and store server ID in machine spec
immediately.

For that purpose server status polling that used to take place in
CreateMachine has to be moved to main reconcile loop. ReconcileBastion
also needed to be revisited to properly support requeuing.

Author: zioc

api/v1alpha5/types.go

@@ -273,8 +273,8 @@ func (r SecurityGroupRule) Equal(x SecurityGroupRule) bool {
 type InstanceState string
 
 var (
-	// InstanceStateBuilding is the string representing an instance in a building state.
-	InstanceStateBuilding = InstanceState("BUILDING")
+	// InstanceStateBuild is the string representing an instance in a build state.
+	InstanceStateBuild = InstanceState("BUILD")
 
 	// InstanceStateActive is the string representing an instance in an active state.
 	InstanceStateActive = InstanceState("ACTIVE")
@@ -290,6 +290,9 @@ var (
 
 	// InstanceStateDeleted is the string representing an instance in a deleted state.
 	InstanceStateDeleted = InstanceState("DELETED")
+
+	// InstanceStateUndefined is the string representing an undefined instance state.
+	InstanceStateUndefined = InstanceState("")
 )
 
 // Bastion represents basic information about the bastion node.

api/v1alpha6/types.go

@@ -285,8 +285,8 @@ func (r SecurityGroupRule) Equal(x SecurityGroupRule) bool {
 type InstanceState string
 
 var (
-	// InstanceStateBuilding is the string representing an instance in a building state.
-	InstanceStateBuilding = InstanceState("BUILDING")
+	// InstanceStateBuild is the string representing an instance in a build state.
+	InstanceStateBuild = InstanceState("BUILD")
 
 	// InstanceStateActive is the string representing an instance in an active state.
 	InstanceStateActive = InstanceState("ACTIVE")
@@ -302,6 +302,9 @@ var (
 
 	// InstanceStateDeleted is the string representing an instance in a deleted state.
 	InstanceStateDeleted = InstanceState("DELETED")
+
+	// InstanceStateUndefined is the string representing an undefined instance state.
+	InstanceStateUndefined = InstanceState("")
 )
 
 // Bastion represents basic information about the bastion node.

api/v1alpha7/types.go

@@ -315,8 +315,8 @@ func (r SecurityGroupRule) Equal(x SecurityGroupRule) bool {
 type InstanceState string
 
 var (
-	// InstanceStateBuilding is the string representing an instance in a building state.
-	InstanceStateBuilding = InstanceState("BUILDING")
+	// InstanceStateBuild is the string representing an instance in a build state.
+	InstanceStateBuild = InstanceState("BUILD")
 
 	// InstanceStateActive is the string representing an instance in an active state.
 	InstanceStateActive = InstanceState("ACTIVE")
@@ -332,6 +332,9 @@ var (
 
 	// InstanceStateDeleted is the string representing an instance in a deleted state.
 	InstanceStateDeleted = InstanceState("DELETED")
+
+	// InstanceStateUndefined is the string representing an undefined instance state.
+	InstanceStateUndefined = InstanceState("")
 )
 
 // Bastion represents basic information about the bastion node.

api/v1alpha8/types.go

@@ -321,8 +321,8 @@ func (r SecurityGroupRule) Equal(x SecurityGroupRule) bool {
 type InstanceState string
 
 var (
-	// InstanceStateBuilding is the string representing an instance in a building state.
-	InstanceStateBuilding = InstanceState("BUILDING")
+	// InstanceStateBuild is the string representing an instance in a build state.
+	InstanceStateBuild = InstanceState("BUILD")
 
 	// InstanceStateActive is the string representing an instance in an active state.
 	InstanceStateActive = InstanceState("ACTIVE")
@@ -338,6 +338,9 @@ var (
 
 	// InstanceStateDeleted is the string representing an instance in a deleted state.
 	InstanceStateDeleted = InstanceState("DELETED")
+
+	// InstanceStateUndefined is the string representing an undefined instance state.
+	InstanceStateUndefined = InstanceState("")
 )
 
 // Bastion represents basic information about the bastion node.

controllers/openstackcluster_controller.go

@@ -215,10 +215,24 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust
 		return err
 	}
 
-	instanceName := fmt.Sprintf("%s-bastion", cluster.Name)
-	instanceStatus, err := computeService.GetInstanceStatusByName(openStackCluster, instanceName)
-	if err != nil {
-		return err
+	if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.FloatingIP != "" {
+		if err = networkingService.DeleteFloatingIP(openStackCluster, openStackCluster.Status.Bastion.FloatingIP); err != nil {
+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete floating IP: %w", err))
+			return fmt.Errorf("failed to delete floating IP: %w", err)
+		}
+	}
+
+	var instanceStatus *compute.InstanceStatus
+	if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.ID != "" {
+		instanceStatus, err = computeService.GetInstanceStatus(openStackCluster.Status.Bastion.ID)
+		if err != nil {
+			return err
+		}
+	} else {
+		instanceStatus, err = computeService.GetInstanceStatusByName(openStackCluster, bastionName(cluster.Name))
+		if err != nil {
+			return err
+		}
 	}
 
 	if instanceStatus != nil {
@@ -230,6 +244,7 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust
 
 		for _, address := range addresses {
 			if address.Type == corev1.NodeExternalIP {
+				// Floating IP may not have properly saved in bastion status (thus not deleted above), delete any remaining floating IP
 				if err = networkingService.DeleteFloatingIP(openStackCluster, address.Address); err != nil {
 					handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete floating IP: %w", err))
 					return fmt.Errorf("failed to delete floating IP: %w", err)
@@ -277,8 +292,9 @@ func reconcileNormal(scope scope.Scope, cluster *clusterv1.Cluster, openStackClu
 		return reconcile.Result{}, err
 	}
 
-	if err = reconcileBastion(scope, cluster, openStackCluster); err != nil {
-		return reconcile.Result{}, err
+	result, err := reconcileBastion(scope, cluster, openStackCluster)
+	if err != nil || !reflect.DeepEqual(result, reconcile.Result{}) {
+		return result, err
 	}
 
 	availabilityZones, err := computeService.GetAvailabilityZones()
@@ -308,88 +324,112 @@ func reconcileNormal(scope scope.Scope, cluster *clusterv1.Cluster, openStackClu
 	return reconcile.Result{}, nil
 }
 
-func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) error {
+func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) (ctrl.Result, error) {
 	scope.Logger().Info("Reconciling Bastion")
 
 	if openStackCluster.Spec.Bastion == nil || !openStackCluster.Spec.Bastion.Enabled {
-		return deleteBastion(scope, cluster, openStackCluster)
+		return reconcile.Result{}, deleteBastion(scope, cluster, openStackCluster)
 	}
 
 	computeService, err := compute.NewService(scope)
 	if err != nil {
-		return err
+		return reconcile.Result{}, err
 	}
 
 	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
 	bastionHash, err := compute.HashInstanceSpec(instanceSpec)
 	if err != nil {
-		return fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
+		return reconcile.Result{}, fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
+	}
+	if bastionHashHasChanged(bastionHash, openStackCluster.ObjectMeta.Annotations) {
+		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
 
-	instanceStatus, err := computeService.GetInstanceStatusByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))
-	if err != nil {
-		return err
+	var instanceStatus *compute.InstanceStatus
+	if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.ID != "" {
+		if instanceStatus, err = computeService.GetInstanceStatus(openStackCluster.Status.Bastion.ID); err != nil {
+			return reconcile.Result{}, err
+		}
 	}
-	if instanceStatus != nil {
-		if !bastionHashHasChanged(bastionHash, openStackCluster.ObjectMeta.Annotations) {
-			bastion, err := instanceStatus.BastionStatus(openStackCluster)
-			if err != nil {
-				return err
-			}
-			// Add the current hash if no annotation is set.
-			if _, ok := openStackCluster.ObjectMeta.Annotations[BastionInstanceHashAnnotation]; !ok {
-				annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
-			}
-			openStackCluster.Status.Bastion = bastion
-			return nil
+	if instanceStatus == nil {
+		// Check if there is an existing instance with bastion name, in case where bastion ID would not have been properly stored in cluster status
+		if instanceStatus, err = computeService.GetInstanceStatusByName(openStackCluster, instanceSpec.Name); err != nil {
+			return reconcile.Result{}, err
 		}
-
-		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
-			return err
+	}
+	if instanceStatus == nil {
+		instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name)
+		if err != nil {
+			return reconcile.Result{}, fmt.Errorf("failed to create bastion: %w", err)
 		}
 	}
 
-	instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name)
-	if err != nil {
-		return fmt.Errorf("failed to reconcile bastion: %w", err)
+	// Save hash & status as soon as we know we have an instance
+	instanceStatus.UpdateBastionStatus(openStackCluster)
+	annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
+
+	// Make sure that bastion instance has a valid state
+	switch instanceStatus.State() {
+	case infrav1.InstanceStateError:
+		return ctrl.Result{}, fmt.Errorf("failed to reconcile bastion, instance state is ERROR")
+	case infrav1.InstanceStateBuild, infrav1.InstanceStateUndefined:
+		scope.Logger().Info("Waiting for bastion instance to become ACTIVE", "id", instanceStatus.ID(), "status", instanceStatus.State())
+		return ctrl.Result{RequeueAfter: waitForBuildingInstanceToReconcile}, nil
+	case infrav1.InstanceStateDeleted:
+		// This should normally be handled by deleteBastion
+		openStackCluster.Status.Bastion = nil
+		return ctrl.Result{}, nil
 	}
 
 	networkingService, err := networking.NewService(scope)
 	if err != nil {
-		return err
-	}
-	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
-	fp, err := networkingService.GetOrCreateFloatingIP(openStackCluster, openStackCluster, clusterName, openStackCluster.Spec.Bastion.Instance.FloatingIP)
-	if err != nil {
-		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create floating IP for bastion: %w", err))
-		return fmt.Errorf("failed to get or create floating IP for bastion: %w", err)
+		return ctrl.Result{}, err
 	}
 	port, err := computeService.GetManagementPort(openStackCluster, instanceStatus)
 	if err != nil {
 		err = fmt.Errorf("getting management port for bastion: %w", err)
 		handleUpdateOSCError(openStackCluster, err)
-		return err
+		return ctrl.Result{}, err
 	}
-	err = networkingService.AssociateFloatingIP(openStackCluster, fp, port.ID)
+	fp, err := networkingService.GetFloatingIPByPortID(port.ID)
 	if err != nil {
-		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to associate floating IP with bastion: %w", err))
-		return fmt.Errorf("failed to associate floating IP with bastion: %w", err)
+		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create floating IP for bastion: %w", err))
+		return ctrl.Result{}, fmt.Errorf("failed to get floating IP for bastion port: %w", err)
+	}
+	if fp != nil {
+		// Floating IP is already attached to bastion, no need to proceed
+		openStackCluster.Status.Bastion.FloatingIP = fp.FloatingIP
+		return ctrl.Result{}, nil
 	}
 
-	bastion, err := instanceStatus.BastionStatus(openStackCluster)
+	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
+	floatingIP := openStackCluster.Spec.Bastion.Instance.FloatingIP
+	if openStackCluster.Status.Bastion.FloatingIP != "" {
+		// Some floating IP has already been created for this bastion, make sure we re-use it
+		floatingIP = openStackCluster.Status.Bastion.FloatingIP
+	}
+	// Check if there is an existing floating IP attached to bastion, in case where FloatingIP would not yet have been stored in cluster status
+	fp, err = networkingService.GetOrCreateFloatingIP(openStackCluster, openStackCluster, clusterName, floatingIP)
 	if err != nil {
-		return err
+		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create floating IP for bastion: %w", err))
+		return ctrl.Result{}, fmt.Errorf("failed to get or create floating IP for bastion: %w", err)
 	}
-	bastion.FloatingIP = fp.FloatingIP
-	openStackCluster.Status.Bastion = bastion
-	annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
-	return nil
+	openStackCluster.Status.Bastion.FloatingIP = fp.FloatingIP
+
+	err = networkingService.AssociateFloatingIP(openStackCluster, fp, port.ID)
+	if err != nil {
+		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to associate floating IP with bastion: %w", err))
+		return ctrl.Result{}, fmt.Errorf("failed to associate floating IP with bastion: %w", err)
+	}
+
+	return ctrl.Result{}, nil
 }
 
 func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string) *compute.InstanceSpec {
-	name := fmt.Sprintf("%s-bastion", clusterName)
 	instanceSpec := &compute.InstanceSpec{
-		Name:          name,
+		Name:          bastionName(clusterName),
 		Flavor:        openStackCluster.Spec.Bastion.Instance.Flavor,
 		SSHKeyName:    openStackCluster.Spec.Bastion.Instance.SSHKeyName,
 		Image:         openStackCluster.Spec.Bastion.Instance.Image,
@@ -412,6 +452,10 @@ func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterNa
 	return instanceSpec
 }
 
+func bastionName(clusterName string) string {
+	return fmt.Sprintf("%s-bastion", clusterName)
+}
+
 // bastionHashHasChanged returns a boolean whether if the latest bastion hash, built from the instance spec, has changed or not.
 func bastionHashHasChanged(computeHash string, clusterAnnotations map[string]string) bool {
 	latestHash, ok := clusterAnnotations[BastionInstanceHashAnnotation]