WIP - move ports out of instance

Author: EmilienM

api/v1alpha5/conversion.go

@@ -437,3 +437,14 @@ func Convert_v1alpha5_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(
 func Convert_v1alpha7_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in *infrav1.OpenStackMachineSpec, out *OpenStackMachineSpec, s conversion.Scope) error {
 	return autoConvert_v1alpha7_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in, out, s)
 }
+
+func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha5_OpenStackMachineStatus(in *infrav1.OpenStackMachineStatus, out *OpenStackMachineStatus, s conversion.Scope) error {
+	err := autoConvert_v1alpha7_OpenStackMachineStatus_To_v1alpha5_OpenStackMachineStatus(in, out, s)
+	if err != nil {
+		return err
+	}
+
+	in.PortsStatus = nil
+
+	return nil
+}

api/v1alpha5/zz_generated.conversion.go

@@ -659,3 +659,14 @@ func Convert_v1alpha6_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(
 func Convert_v1alpha7_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *infrav1.OpenStackMachineSpec, out *OpenStackMachineSpec, s apiconversion.Scope) error {
 	return autoConvert_v1alpha7_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in, out, s)
 }
+
+func Convert_v1alpha7_OpenStackMachineStatus_To_v1alpha6_OpenStackMachineStatus(in *infrav1.OpenStackMachineStatus, out *OpenStackMachineStatus, s apiconversion.Scope) error {
+	err := autoConvert_v1alpha7_OpenStackMachineStatus_To_v1alpha6_OpenStackMachineStatus(in, out, s)
+	if err != nil {
+		return err
+	}
+
+	in.PortsStatus = nil
+
+	return nil
+}

api/v1alpha6/conversion.go

@@ -22,6 +22,10 @@ const (
 	// InstanceReadyCondition reports on current status of the OpenStack instance. Ready indicates the instance is in a Running state.
 	InstanceReadyCondition clusterv1.ConditionType = "InstanceReady"
 
+	// PortsReadyCondition reports on the current status of the ports. Ready indicates that all ports have been created
+	// successfully and ready to be attached to the instance.
+	PortsReadyCondition clusterv1.ConditionType = "PortsReady"
+
 	// WaitingForClusterInfrastructureReason used when machine is waiting for cluster infrastructure to be ready before proceeding.
 	WaitingForClusterInfrastructureReason = "WaitingForClusterInfrastructure"
 	// WaitingForBootstrapDataReason used when machine is waiting for bootstrap data to be ready before proceeding.
@@ -42,6 +46,8 @@ const (
 	InstanceDeleteFailedReason = "InstanceDeleteFailed"
 	// OpenstackErrorReason used when there is an error communicating with OpenStack.
 	OpenStackErrorReason = "OpenStackError"
+	// PortsCreateFailedReason used when creating the ports failed.
+	PortsCreateFailedReason = "PortsCreateFailed"
 )
 
 const (

api/v1alpha6/zz_generated.conversion.go

@@ -113,6 +113,10 @@ type OpenStackMachineStatus struct {
 
 	FailureReason *errors.MachineStatusError `json:"failureReason,omitempty"`
 
+	// PortsStatus is the status of the ports associated with the machine.
+	// +optional
+	PortsStatus []PortStatus `json:"portsStatus,omitempty"`
+
 	// FailureMessage will be set in the event that there is a terminal problem
 	// reconciling the Machine and will contain a more verbose string suitable
 	// for logging and human consumption.

api/v1alpha7/conditions_consts.go

@@ -128,6 +128,12 @@ type PortOpts struct {
 	ValueSpecs []ValueSpec `json:"valueSpecs,omitempty"`
 }
 
+type PortStatus struct {
+	// ID is the unique identifier of the port.
+	// +optional
+	ID string `json:"id,omitempty"`
+}
+
 type BindingProfile struct {
 	// OVSHWOffload enables or disables the OVS hardware offload feature.
 	OVSHWOffload bool `json:"ovsHWOffload,omitempty"`

api/v1alpha7/openstackmachine_types.go

@@ -1603,6 +1603,16 @@ spec:
                 description: InstanceState is the state of the OpenStack instance
                   for this machine.
                 type: string
+              portsStatus:
+                description: PortsStatus is the status of the ports associated with
+                  the machine.
+                items:
+                  properties:
+                    id:
+                      description: ID is the unique identifier of the port.
+                      type: string
+                  type: object
+                type: array
               ready:
                 description: Ready is true when the provider resource is ready.
                 type: boolean

api/v1alpha7/types.go

@@ -349,7 +349,7 @@ func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCl
 		}
 	}
 
-	instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name)
+	instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name, []string{})
 	if err != nil {
 		return fmt.Errorf("failed to reconcile bastion: %w", err)
 	}

api/v1alpha7/zz_generated.deepcopy.go

@@ -328,9 +328,24 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
-	instanceStatus, err := r.getOrCreate(scope.Logger(), cluster, openStackCluster, machine, openStackMachine, computeService, userData)
+	managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
+	instanceTags := getInstanceTags(openStackMachine, openStackCluster)
+	portsStatus, err := r.getOrCreatePorts(scope.Logger(), clusterName, openStackCluster, openStackMachine.Spec.Ports, openStackMachine.Spec.Trunk, managedSecurityGroups, instanceTags, openStackMachine.Name, openStackMachine, networkingService)
 	if err != nil {
-		// Conditions set in getOrCreate
+		// Conditions set in getOrCreatePorts
+		return ctrl.Result{}, err
+	}
+	// TODO(emilien) do we want to deal with Ports Status like we do for the nova instance? Might be expensive...
+	// In the meantime, we consider that ports are ready if they are created.
+	conditions.MarkTrue(openStackMachine, infrav1.PortsReadyCondition)
+	portIDs := make([]string, len(*portsStatus))
+	for i, portStatus := range *portsStatus {
+		portIDs[i] = portStatus.ID
+	}
+
+	instanceStatus, err := r.getOrCreateInstance(scope.Logger(), cluster, openStackCluster, machine, openStackMachine, computeService, userData, portIDs)
+	if err != nil {
+		// Conditions set in getOrCreateInstance
 		return ctrl.Result{}, err
 	}
 
@@ -430,7 +445,33 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	return ctrl.Result{}, nil
 }
 
-func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, computeService *compute.Service, userData string) (*compute.InstanceStatus, error) {
+func (r *OpenStackMachineReconciler) getOrCreatePorts(logger logr.Logger, clusterName string, openStackCluster *infrav1.OpenStackCluster, machinePorts []infrav1.PortOpts, trunkEnabled bool, securityGroups []infrav1.SecurityGroupFilter, instanceTags []string, instanceName string, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service) (*[]infrav1.PortStatus, error) {
+	// TODO(emilien) implement portsStatus where we check if the machine has ports created and part of the OpenStackMachine.Status
+	// Check `GetInstanceStatusByName` as it's done for instances. For each port found in Status, we might want get the port name and verify there is no duplicate.
+	// If Status is empty, we create the ports and add them to the Status.
+	// If Status is not empty, we try to adopt the existing ports by checking if the ports are still there and if not, we create them and add them to the Status.
+
+	// For now, we create the ports.
+	var portsStatus *[]infrav1.PortStatus
+	var err error
+
+	if openStackMachine.Status.PortsStatus == nil {
+		logger.Info("Creating ports for OpenStackMachine", "name", openStackMachine.Name)
+		portsStatus, err = networkingService.CreatePorts(openStackMachine, clusterName, openStackCluster, machinePorts, trunkEnabled, securityGroups, instanceTags, instanceName)
+		if err != nil {
+			// Add Conditions
+			conditions.MarkFalse(openStackMachine, infrav1.PortsReadyCondition, infrav1.PortsCreateFailedReason, clusterv1.ConditionSeverityError, err.Error())
+			return nil, fmt.Errorf("create ports: %w", err)
+		}
+	}
+
+	// TODO(emilien) maybe we want to set PortsStatus in OpenStackMachineStatus here instead of in port.go?
+	// Major difference of doing it in port.go is that we can add ports one by one into the status instead of all at once in the controller.
+
+	return portsStatus, nil
+}
+
+func (r *OpenStackMachineReconciler) getOrCreateInstance(logger logr.Logger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, computeService *compute.Service, userData string, portIDs []string) (*compute.InstanceStatus, error) {
 	instanceStatus, err := computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name)
 	if err != nil {
 		logger.Info("Unable to get OpenStack instance", "name", openStackMachine.Name)
@@ -441,7 +482,7 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl
 	if instanceStatus == nil {
 		instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData)
 		logger.Info("Machine does not exist, creating Machine", "name", openStackMachine.Name)
-		instanceStatus, err = computeService.CreateInstance(openStackMachine, openStackCluster, instanceSpec, cluster.Name)
+		instanceStatus, err = computeService.CreateInstance(openStackMachine, openStackCluster, instanceSpec, cluster.Name, portIDs)
 		if err != nil {
 			conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceCreateFailedReason, clusterv1.ConditionSeverityError, err.Error())
 			return nil, fmt.Errorf("create OpenStack instance: %w", err)
@@ -472,6 +513,19 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
 		instanceSpec.FailureDomain = *machine.Spec.FailureDomain
 	}
 
+	instanceSpec.Tags = getInstanceTags(openStackMachine, openStackCluster)
+
+	instanceSpec.SecurityGroups = getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
+
+	instanceSpec.Ports = openStackMachine.Spec.Ports
+
+	return &instanceSpec
+}
+
+// getInstanceTags returns the tags that should be applied to the instance.
+// The tags are a combination of the tags specified on the OpenStackMachine and
+// the ones specified on the OpenStackCluster.
+func getInstanceTags(openStackMachine *infrav1.OpenStackMachine, openStackCluster *infrav1.OpenStackCluster) []string {
 	machineTags := []string{}
 
 	// Append machine specific tags
@@ -494,31 +548,31 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
 	}
 	machineTags = deduplicate(machineTags)
 
-	instanceSpec.Tags = machineTags
+	return machineTags
+}
 
-	instanceSpec.SecurityGroups = openStackMachine.Spec.SecurityGroups
-	if openStackCluster.Spec.ManagedSecurityGroups {
-		var managedSecurityGroup string
-		if util.IsControlPlaneMachine(machine) {
-			if openStackCluster.Status.ControlPlaneSecurityGroup != nil {
-				managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
-			}
-		} else {
-			if openStackCluster.Status.WorkerSecurityGroup != nil {
-				managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
-			}
+// getManagedSecurityGroups returns a combination of OpenStackMachine.Spec.SecurityGroups
+// and the security group managed by the OpenStackCluster whether it's a control plane or a worker machine.
+func getManagedSecurityGroups(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) []infrav1.SecurityGroupFilter {
+	machineSpecSecurityGroups := openStackMachine.Spec.SecurityGroups
+	var managedSecurityGroup string
+	if util.IsControlPlaneMachine(machine) {
+		if openStackCluster.Status.ControlPlaneSecurityGroup != nil {
+			managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
 		}
-
-		if managedSecurityGroup != "" {
-			instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupFilter{
-				ID: managedSecurityGroup,
-			})
+	} else {
+		if openStackCluster.Status.WorkerSecurityGroup != nil {
+			managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
 		}
 	}
 
-	instanceSpec.Ports = openStackMachine.Spec.Ports
+	if managedSecurityGroup != "" {
+		machineSpecSecurityGroups = append(machineSpecSecurityGroups, infrav1.SecurityGroupFilter{
+			ID: managedSecurityGroup,
+		})
+	}
 
-	return &instanceSpec
+	return machineSpecSecurityGroups
 }
 
 func (r *OpenStackMachineReconciler) reconcileLoadBalancerMember(scope scope.Scope, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine, instanceNS *compute.InstanceNetworkStatus, clusterName string) error {

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

@@ -151,7 +151,7 @@ var _ = Describe("When calling getOrCreate", func() {
 		openStackMachine := &infrav1.OpenStackMachine{}
 
 		mockScopeFactory.ComputeClient.EXPECT().ListServers(gomock.Any()).Return(nil, errors.New("Test error when listing servers"))
-		instanceStatus, err := reconsiler.getOrCreate(logger, cluster, openStackCluster, machine, openStackMachine, computeService, "")
+		instanceStatus, err := reconsiler.getOrCreateInstance(logger, cluster, openStackCluster, machine, openStackMachine, computeService, "", []string{})
 		Expect(err).To(HaveOccurred())
 		Expect(instanceStatus).To(BeNil())
 		conditions := openStackMachine.GetConditions()

controllers/openstackcluster_controller.go

@@ -36,6 +36,11 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/metrics"
 )
 
+// PortExt is the base gophercloud Port with extensions used by PortStatus.
+type PortExt struct {
+	ports.Port
+}
+
 type NetworkClient interface {
 	ListFloatingIP(opts floatingips.ListOptsBuilder) ([]floatingips.FloatingIP, error)
 	CreateFloatingIP(opts floatingips.CreateOptsBuilder) (*floatingips.FloatingIP, error)