diff --git a/api/v1alpha5/conversion_test.go b/api/v1alpha5/conversion_test.go index 6e0ab71e74..a563c0aa4d 100644 --- a/api/v1alpha5/conversion_test.go +++ b/api/v1alpha5/conversion_test.go @@ -79,7 +79,7 @@ func TestConvertFrom(t *testing.T) { Spec: OpenStackMachineSpec{}, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\",\"image\":{}},\"status\":{\"dependentResources\":{},\"ready\":false,\"referencedResources\":{}}}", + "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\",\"image\":{}},\"status\":{\"ready\":false}}", }, }, }, diff --git a/api/v1alpha6/conversion.go b/api/v1alpha6/conversion.go index bb055369d1..22cd39720f 100644 --- a/api/v1alpha6/conversion.go +++ b/api/v1alpha6/conversion.go @@ -129,11 +129,11 @@ func restorev1beta1ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst * dst.WorkerSecurityGroup = previous.WorkerSecurityGroup dst.BastionSecurityGroup = previous.BastionSecurityGroup - if previous.Bastion != nil { + if previous.Bastion != nil && previous.Bastion.ReferencedResources != nil { dst.Bastion.ReferencedResources = previous.Bastion.ReferencedResources } - if previous.Bastion != nil && previous.Bastion.DependentResources.PortsStatus != nil { - dst.Bastion.DependentResources.PortsStatus = previous.Bastion.DependentResources.PortsStatus + if previous.Bastion != nil && previous.Bastion.DependentResources != nil { + dst.Bastion.DependentResources = previous.Bastion.DependentResources } } @@ -389,13 +389,13 @@ var v1beta1OpenStackMachineRestorer = conversion.RestorerFor[*infrav1.OpenStackM restorev1beta1MachineSpec, ), "depresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.DependentMachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.DependentMachineResources { return &c.Status.DependentResources }, ), // No equivalent in v1alpha6 "refresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.ReferencedMachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.ReferencedMachineResources { return &c.Status.ReferencedResources }, ), diff --git a/api/v1alpha7/conversion.go b/api/v1alpha7/conversion.go index ce307ea407..8e97712071 100644 --- a/api/v1alpha7/conversion.go +++ b/api/v1alpha7/conversion.go @@ -121,12 +121,12 @@ func restorev1beta1ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst * restorev1beta1SecurityGroupStatus(previous.BastionSecurityGroup, dst.BastionSecurityGroup) // ReferencedResources have no equivalent in v1alpha7 - if previous.Bastion != nil { + if previous.Bastion != nil && previous.Bastion.ReferencedResources != nil { dst.Bastion.ReferencedResources = previous.Bastion.ReferencedResources } - if previous.Bastion != nil && previous.Bastion.DependentResources.PortsStatus != nil { - dst.Bastion.DependentResources.PortsStatus = previous.Bastion.DependentResources.PortsStatus + if previous.Bastion != nil && previous.Bastion.DependentResources != nil { + dst.Bastion.DependentResources = previous.Bastion.DependentResources } } @@ -354,14 +354,14 @@ var v1beta1OpenStackMachineRestorer = conversion.RestorerFor[*infrav1.OpenStackM restorev1beta1MachineSpec, ), "depresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.DependentMachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.DependentMachineResources { return &c.Status.DependentResources }, ), // No equivalent in v1alpha7 "refresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.ReferencedMachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.ReferencedMachineResources { return &c.Status.ReferencedResources }, ), diff --git a/api/v1beta1/openstackmachine_types.go b/api/v1beta1/openstackmachine_types.go index c3f5ea367b..30c10dca97 100644 --- a/api/v1beta1/openstackmachine_types.go +++ b/api/v1beta1/openstackmachine_types.go @@ -117,10 +117,10 @@ type OpenStackMachineStatus struct { InstanceState *InstanceState `json:"instanceState,omitempty"` // ReferencedResources contains resolved references to resources that the machine depends on. - ReferencedResources ReferencedMachineResources `json:"referencedResources,omitempty"` + ReferencedResources *ReferencedMachineResources `json:"referencedResources,omitempty"` // DependentResources contains resolved dependent resources that were created by the machine. - DependentResources DependentMachineResources `json:"dependentResources,omitempty"` + DependentResources *DependentMachineResources `json:"dependentResources,omitempty"` FailureReason *errors.MachineStatusError `json:"failureReason,omitempty"` diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index c96995cd31..a980788b49 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -190,14 +190,14 @@ type AddressPair struct { } type BastionStatus struct { - ID string `json:"id,omitempty"` - Name string `json:"name,omitempty"` - SSHKeyName string `json:"sshKeyName,omitempty"` - State InstanceState `json:"state,omitempty"` - IP string `json:"ip,omitempty"` - FloatingIP string `json:"floatingIP,omitempty"` - ReferencedResources ReferencedMachineResources `json:"referencedResources,omitempty"` - DependentResources DependentMachineResources `json:"dependentResources,omitempty"` + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` + SSHKeyName string `json:"sshKeyName,omitempty"` + State InstanceState `json:"state,omitempty"` + IP string `json:"ip,omitempty"` + FloatingIP string `json:"floatingIP,omitempty"` + ReferencedResources *ReferencedMachineResources `json:"referencedResources,omitempty"` + DependentResources *DependentMachineResources `json:"dependentResources,omitempty"` } type RootVolume struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index f9105944fc..81192f7c68 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -117,8 +117,16 @@ func (in *Bastion) DeepCopy() *Bastion { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BastionStatus) DeepCopyInto(out *BastionStatus) { *out = *in - in.ReferencedResources.DeepCopyInto(&out.ReferencedResources) - in.DependentResources.DeepCopyInto(&out.DependentResources) + if in.ReferencedResources != nil { + in, out := &in.ReferencedResources, &out.ReferencedResources + *out = new(ReferencedMachineResources) + (*in).DeepCopyInto(*out) + } + if in.DependentResources != nil { + in, out := &in.DependentResources, &out.DependentResources + *out = new(DependentMachineResources) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new BastionStatus. @@ -811,8 +819,16 @@ func (in *OpenStackMachineStatus) DeepCopyInto(out *OpenStackMachineStatus) { *out = new(InstanceState) **out = **in } - in.ReferencedResources.DeepCopyInto(&out.ReferencedResources) - in.DependentResources.DeepCopyInto(&out.DependentResources) + if in.ReferencedResources != nil { + in, out := &in.ReferencedResources, &out.ReferencedResources + *out = new(ReferencedMachineResources) + (*in).DeepCopyInto(*out) + } + if in.DependentResources != nil { + in, out := &in.DependentResources, &out.DependentResources + *out = new(DependentMachineResources) + (*in).DeepCopyInto(*out) + } if in.FailureReason != nil { in, out := &in.FailureReason, &out.FailureReason *out = new(errors.MachineStatusError) diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 34e40adce9..4bd985806b 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -123,24 +123,28 @@ func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Req scope := scope.NewWithLogger(clientScope, log) // Resolve and store referenced & dependent resources for the bastion - if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled { + // Note: the bastion won't be created before networking is done, which is why we only adopt resources once we have a network in status. + if openStackCluster.Spec.Bastion != nil && openStackCluster.Spec.Bastion.Enabled && openStackCluster.Status.Network != nil { if openStackCluster.Status.Bastion == nil { openStackCluster.Status.Bastion = &infrav1.BastionStatus{} } - changed, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, &openStackCluster.Status.Bastion.ReferencedResources) - if err != nil { - return reconcile.Result{}, err - } - if changed { + if openStackCluster.Status.Bastion.ReferencedResources == nil { + openStackCluster.Status.Bastion.ReferencedResources = &infrav1.ReferencedMachineResources{} + referencedResources, err := compute.ResolveReferencedMachineResources(scope, openStackCluster, &openStackCluster.Spec.Bastion.Instance, openStackCluster.Status.Bastion.ReferencedResources) + if err != nil { + return reconcile.Result{}, err + } + openStackCluster.Status.Bastion.ReferencedResources = referencedResources // If the referenced resources have changed, we need to update the OpenStackCluster status now. return reconcile.Result{}, nil } - changed, err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(cluster.Name)) - if err != nil { - return reconcile.Result{}, err - } - if changed { + if openStackCluster.Status.Bastion.DependentResources == nil { + openStackCluster.Status.Bastion.DependentResources = &infrav1.DependentMachineResources{} + err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(cluster.Name)) + if err != nil { + return reconcile.Result{}, err + } // If the dependent resources have changed, we need to update the OpenStackCluster status now. return reconcile.Result{}, nil } @@ -292,7 +296,7 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac } } - if openStackCluster.Status.Bastion != nil && len(openStackCluster.Status.Bastion.DependentResources.PortsStatus) > 0 { + if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.DependentResources != nil && len(openStackCluster.Status.Bastion.DependentResources.PortsStatus) > 0 { trunkSupported, err := networkingService.IsTrunkExtSupported() if err != nil { return err @@ -333,6 +337,11 @@ func reconcileNormal(scope *scope.WithLogger, cluster *clusterv1.Cluster, openSt return reconcile.Result{}, err } + // If the network is not yet written in status, we should requeue. + if openStackCluster.Status.Network == nil { + return reconcile.Result{}, nil + } + result, err := reconcileBastion(scope, cluster, openStackCluster) if err != nil || !reflect.DeepEqual(result, reconcile.Result{}) { return result, err @@ -403,7 +412,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS } } - err = getOrCreateBastionPorts(scope, cluster, openStackCluster, networkingService, cluster.Name) + err = getOrCreateBastionPorts(cluster, openStackCluster, networkingService, cluster.Name) if err != nil { handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create ports for bastion: %w", err)) return ctrl.Result{}, fmt.Errorf("failed to get or create ports for bastion: %w", err) @@ -543,7 +552,7 @@ func getBastionSecurityGroups(openStackCluster *infrav1.OpenStackCluster) []infr return instanceSpecSecurityGroups } -func getOrCreateBastionPorts(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error { +func getOrCreateBastionPorts(cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error { if openStackCluster.Status.Bastion == nil { openStackCluster.Status.Bastion = &infrav1.BastionStatus{} } diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index 971d0308d2..a2fcbfc99c 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/controllers/openstackcluster_controller_test.go @@ -231,7 +231,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(err).To(BeNil()) testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -241,7 +241,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -282,7 +282,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(testCluster.Status.Bastion).To(Equal(&infrav1.BastionStatus{ ID: "adopted-bastion-uuid", State: "ACTIVE", - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -292,7 +292,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -323,7 +323,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, Bastion: &infrav1.BastionStatus{ ID: "adopted-fip-bastion-uuid", - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -333,7 +333,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -367,7 +367,7 @@ var _ = Describe("OpenStackCluster controller", func() { ID: "adopted-fip-bastion-uuid", FloatingIP: "1.2.3.4", State: "ACTIVE", - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -377,7 +377,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -408,7 +408,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, Bastion: &infrav1.BastionStatus{ ID: "requeue-bastion-uuid", - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -418,7 +418,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -446,7 +446,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(testCluster.Status.Bastion).To(Equal(&infrav1.BastionStatus{ ID: "requeue-bastion-uuid", State: "BUILD", - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", PortsOpts: []infrav1.PortOpts{ { @@ -456,7 +456,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "portID1", @@ -478,7 +478,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(err).To(BeNil()) testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: "imageID", }, }, @@ -530,7 +530,7 @@ var _ = Describe("OpenStackCluster controller", func() { } testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "port-id", @@ -613,7 +613,7 @@ var _ = Describe("OpenStackCluster controller", func() { } testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: "port-id", diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index fb0155cc2a..2c0b67b14e 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -148,22 +148,23 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req } scope := scope.NewWithLogger(clientScope, log) - // Resolve and store referenced resources - changed, err := compute.ResolveReferencedMachineResources(scope, infraCluster, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources) - if err != nil { - return reconcile.Result{}, err - } - if changed { + if openStackMachine.Status.ReferencedResources == nil { + openStackMachine.Status.ReferencedResources = &infrav1.ReferencedMachineResources{} + referencedResources, err := compute.ResolveReferencedMachineResources(scope, infraCluster, &openStackMachine.Spec, openStackMachine.Status.ReferencedResources) + if err != nil { + return reconcile.Result{}, err + } + openStackMachine.Status.ReferencedResources = referencedResources // If the referenced resources have changed, we need to update the OpenStackMachine status now. return reconcile.Result{}, nil } - // Resolve and store dependent resources - changed, err = compute.ResolveDependentMachineResources(scope, openStackMachine) - if err != nil { - return reconcile.Result{}, err - } - if changed { + if openStackMachine.Status.DependentResources == nil { + openStackMachine.Status.DependentResources = &infrav1.DependentMachineResources{} + err = compute.ResolveDependentMachineResources(scope, openStackMachine) + if err != nil { + return reconcile.Result{}, err + } // If the dependent resources have changed, we need to update the OpenStackMachine status now. return reconcile.Result{}, nil } @@ -313,10 +314,12 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl return ctrl.Result{}, err } - portsStatus := openStackMachine.Status.DependentResources.PortsStatus - for _, port := range portsStatus { - if err := networkingService.DeleteInstanceTrunkAndPort(openStackMachine, port, trunkSupported); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to delete port %q: %w", port.ID, err) + if openStackMachine.Status.DependentResources != nil { + portsStatus := openStackMachine.Status.DependentResources.PortsStatus + for _, port := range portsStatus { + if err := networkingService.DeleteInstanceTrunkAndPort(openStackMachine, port, trunkSupported); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to delete port %q: %w", port.ID, err) + } } } @@ -379,7 +382,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope return ctrl.Result{}, err } - err = getOrCreateMachinePorts(scope, openStackCluster, machine, openStackMachine, networkingService, clusterName) + err = getOrCreateMachinePorts(openStackCluster, machine, openStackMachine, networkingService, clusterName) if err != nil { return ctrl.Result{}, err } @@ -490,7 +493,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope return ctrl.Result{}, nil } -func getOrCreateMachinePorts(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error { +func getOrCreateMachinePorts(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error { var machinePortsStatus []infrav1.PortStatus var err error diff --git a/controllers/openstackmachine_controller_test.go b/controllers/openstackmachine_controller_test.go index a541795528..2d41d91501 100644 --- a/controllers/openstackmachine_controller_test.go +++ b/controllers/openstackmachine_controller_test.go @@ -95,7 +95,7 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine { ServerGroup: &infrav1.ServerGroupFilter{ID: serverGroupUUID}, }, Status: infrav1.OpenStackMachineStatus{ - ReferencedResources: infrav1.ReferencedMachineResources{ + ReferencedResources: &infrav1.ReferencedMachineResources{ ImageID: imageUUID, ServerGroupID: serverGroupUUID, }, diff --git a/pkg/cloud/services/compute/dependent_resources.go b/pkg/cloud/services/compute/dependent_resources.go index ab8bf67547..2de48e2376 100644 --- a/pkg/cloud/services/compute/dependent_resources.go +++ b/pkg/cloud/services/compute/dependent_resources.go @@ -22,23 +22,31 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func ResolveDependentMachineResources(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine) (changed bool, err error) { - changed = false +func ResolveDependentMachineResources(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine) error { + if openStackMachine.Status.ReferencedResources == nil { + return nil + } networkingService, err := networking.NewService(scope) if err != nil { - return changed, err + return err } return networkingService.AdoptMachinePorts(scope, openStackMachine, openStackMachine.Status.ReferencedResources.PortsOpts) } -func ResolveDependentBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) (changed bool, err error) { - changed = false +func ResolveDependentBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) error { + if openStackCluster.Status.Bastion == nil { + return nil + } + + if openStackCluster.Status.Bastion.DependentResources == nil { + return nil + } networkingService, err := networking.NewService(scope) if err != nil { - return changed, err + return err } return networkingService.AdoptBastionPorts(scope, openStackCluster, bastionName) diff --git a/pkg/cloud/services/compute/dependent_resources_test.go b/pkg/cloud/services/compute/dependent_resources_test.go index e1e91f27f3..65b513ae9b 100644 --- a/pkg/cloud/services/compute/dependent_resources_test.go +++ b/pkg/cloud/services/compute/dependent_resources_test.go @@ -41,10 +41,16 @@ func Test_ResolveDependentMachineResources(t *testing.T) { wantErr bool }{ { - testName: "no Network ID yet and no ports in status", - openStackCluster: &infrav1.OpenStackCluster{}, - want: &infrav1.DependentMachineResources{}, - wantErr: false, + testName: "no Network ID yet and no ports in status", + openStackCluster: &infrav1.OpenStackCluster{ + Status: infrav1.OpenStackClusterStatus{ + Bastion: &infrav1.BastionStatus{ + DependentResources: &infrav1.DependentMachineResources{}, + }, + }, + }, + want: nil, + wantErr: false, }, { testName: "Network ID set but no ports in status", @@ -57,7 +63,7 @@ func Test_ResolveDependentMachineResources(t *testing.T) { }, }, }, - want: &infrav1.DependentMachineResources{}, + want: nil, wantErr: false, }, { @@ -72,7 +78,7 @@ func Test_ResolveDependentMachineResources(t *testing.T) { }, }, openStackMachineStatus: infrav1.OpenStackMachineStatus{ - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: portID, @@ -104,13 +110,13 @@ func Test_ResolveDependentMachineResources(t *testing.T) { Status: tt.openStackMachineStatus, } - _, err := ResolveDependentMachineResources(scope.NewWithLogger(mockScopeFactory, log), defaultOpenStackMachine) + err := ResolveDependentMachineResources(scope.NewWithLogger(mockScopeFactory, log), defaultOpenStackMachine) if tt.wantErr { g.Expect(err).Error() return } - g.Expect(&defaultOpenStackMachine.Status.DependentResources).To(Equal(tt.want), cmp.Diff(&defaultOpenStackMachine.Status.DependentResources, tt.want)) + g.Expect(defaultOpenStackMachine.Status.DependentResources).To(Equal(tt.want), cmp.Diff(defaultOpenStackMachine.Status.DependentResources, tt.want)) }) } } @@ -134,6 +140,11 @@ func TestResolveDependentBastionResources(t *testing.T) { Enabled: true, }, }, + Status: infrav1.OpenStackClusterStatus{ + Bastion: &infrav1.BastionStatus{ + DependentResources: &infrav1.DependentMachineResources{}, + }, + }, }, want: &infrav1.DependentMachineResources{}, wantErr: false, @@ -152,6 +163,9 @@ func TestResolveDependentBastionResources(t *testing.T) { ID: networkID, }, }, + Bastion: &infrav1.BastionStatus{ + DependentResources: &infrav1.DependentMachineResources{}, + }, }, }, want: &infrav1.DependentMachineResources{}, @@ -166,7 +180,7 @@ func TestResolveDependentBastionResources(t *testing.T) { }, Status: infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - DependentResources: infrav1.DependentMachineResources{ + DependentResources: &infrav1.DependentMachineResources{ PortsStatus: []infrav1.PortStatus{ { ID: portID, @@ -197,7 +211,7 @@ func TestResolveDependentBastionResources(t *testing.T) { mockCtrl := gomock.NewController(t) mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") - _, err := ResolveDependentBastionResources(scope.NewWithLogger(mockScopeFactory, log), tt.openStackCluster, bastionName) + err := ResolveDependentBastionResources(scope.NewWithLogger(mockScopeFactory, log), tt.openStackCluster, bastionName) if tt.wantErr { g.Expect(err).Error() return @@ -212,7 +226,7 @@ func TestResolveDependentBastionResources(t *testing.T) { } if tt.openStackCluster.Status.Bastion != nil { - g.Expect(&defaultOpenStackCluster.Status.Bastion.DependentResources).To(Equal(tt.want), cmp.Diff(&defaultOpenStackCluster.Status.Bastion.DependentResources, tt.want)) + g.Expect(defaultOpenStackCluster.Status.Bastion.DependentResources).To(Equal(tt.want), cmp.Diff(defaultOpenStackCluster.Status.Bastion.DependentResources, tt.want)) } }) } diff --git a/pkg/cloud/services/compute/referenced_resources.go b/pkg/cloud/services/compute/referenced_resources.go index 657a14f93a..3678e34df2 100644 --- a/pkg/cloud/services/compute/referenced_resources.go +++ b/pkg/cloud/services/compute/referenced_resources.go @@ -28,37 +28,33 @@ import ( // Note that we only set the fields in ReferencedMachineResources that are not set yet. This is ok because: // - OpenStackMachine is immutable, so we can't change the spec after the machine is created. // - the bastion is mutable, but we delete the bastion when the spec changes, so the bastion status will be empty. -func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, spec *infrav1.OpenStackMachineSpec, resources *infrav1.ReferencedMachineResources) (changed bool, err error) { - changed = false - +func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, spec *infrav1.OpenStackMachineSpec, resources *infrav1.ReferencedMachineResources) (*infrav1.ReferencedMachineResources, error) { computeService, err := NewService(scope) if err != nil { - return changed, err + return resources, err } networkingService, err := networking.NewService(scope) if err != nil { - return changed, err + return resources, err } // ServerGroup is optional, so we only need to resolve it if it's set in the spec and not in ReferencedMachineResources yet. if spec.ServerGroup != nil && resources.ServerGroupID == "" { serverGroupID, err := computeService.GetServerGroupID(spec.ServerGroup) if err != nil { - return changed, err + return resources, err } resources.ServerGroupID = serverGroupID - changed = true } // Image is required, so we need to resolve it if it's not set in ReferencedMachineResources yet. if resources.ImageID == "" { imageID, err := computeService.GetImageID(spec.Image) if err != nil { - return changed, err + return resources, err } resources.ImageID = imageID - changed = true } // Network resources are required in order to get ports options. @@ -67,15 +63,14 @@ func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster // support at any time, so we should probably check this on every reconcile. trunkSupported, err := networkingService.IsTrunkExtSupported() if err != nil { - return changed, err + return resources, err } portsOpts, err := networkingService.ConstructPorts(openStackCluster, spec.Ports, spec.Trunk, trunkSupported) if err != nil { - return changed, err + return resources, err } resources.PortsOpts = portsOpts - changed = true } - return changed, nil + return resources, nil } diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index 1ecf4beaca..26578ff0bb 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -518,14 +518,12 @@ func (s *Service) IsTrunkExtSupported() (trunknSupported bool, err error) { // by checking if they exist and if they do, it'll add them to the OpenStackMachine status. // A port is searched by name and network ID and has to be unique. // If the port is not found, it'll be ignored because it'll be created after the adoption. -func (s *Service) AdoptMachinePorts(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine, desiredPorts []infrav1.PortOpts) (changed bool, err error) { - changed = false - +func (s *Service) AdoptMachinePorts(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine, desiredPorts []infrav1.PortOpts) (err error) { // We can skip adoption if the instance is ready because OpenStackMachine is immutable once ready // or if the ports are already in the status if openStackMachine.Status.Ready && len(openStackMachine.Status.DependentResources.PortsStatus) == len(desiredPorts) { scope.Logger().V(5).Info("OpenStackMachine is ready, skipping the adoption of ports") - return changed, nil + return nil } scope.Logger().Info("Adopting ports for OpenStackMachine", "name", openStackMachine.Name) @@ -547,37 +545,34 @@ func (s *Service) AdoptMachinePorts(scope *scope.WithLogger, openStackMachine *i NetworkID: port.Network.ID, }) if err != nil { - return changed, fmt.Errorf("searching for existing port for machine %s: %v", openStackMachine.Name, err) + return fmt.Errorf("searching for existing port for machine %s: %v", openStackMachine.Name, err) } // if the port is not found, we stop the adoption of ports since the rest of the ports will not be found either // and will be created after the adoption if len(ports) == 0 { scope.Logger().V(5).Info("Port not found, stopping the adoption of ports", "port index", i) - return changed, nil + return nil } if len(ports) > 1 { - return changed, fmt.Errorf("found multiple ports with name %s", portName) + return fmt.Errorf("found multiple ports with name %s", portName) } // The desired port was found, so we add it to the status scope.Logger().V(5).Info("Port found, adding it to the status", "port index", i) openStackMachine.Status.DependentResources.PortsStatus = append(openStackMachine.Status.DependentResources.PortsStatus, infrav1.PortStatus{ID: ports[0].ID}) - changed = true } - return changed, nil + return nil } // AdopteBastionPorts tries to adopt the ports for the bastion instance by checking if they exist and if they do, // it'll add them to the OpenStackCluster status. // A port is searched by name and network ID and has to be unique. // If the port is not found, it'll be ignored because it'll be created after the adoption. -func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) (changed bool, err error) { - changed = false - +func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) error { if openStackCluster.Status.Network == nil { scope.Logger().V(5).Info("Network status is nil, skipping the adoption of ports") - return changed, nil + return nil } if openStackCluster.Status.Bastion == nil { @@ -585,11 +580,16 @@ func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *i openStackCluster.Status.Bastion = &infrav1.BastionStatus{} } + if openStackCluster.Status.Bastion.ReferencedResources == nil { + scope.Logger().V(5).Info("ReferencedResources status is nil, initializing it") + openStackCluster.Status.Bastion.ReferencedResources = &infrav1.ReferencedMachineResources{} + } + desiredPorts := openStackCluster.Status.Bastion.ReferencedResources.PortsOpts // We can skip adoption if the ports are already in the status if len(desiredPorts) == len(openStackCluster.Status.Bastion.DependentResources.PortsStatus) { - return changed, nil + return nil } scope.Logger().Info("Adopting bastion ports for OpenStackCluster", "name", openStackCluster.Name) @@ -611,25 +611,24 @@ func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *i NetworkID: port.Network.ID, }) if err != nil { - return changed, fmt.Errorf("searching for existing port for bastion %s: %v", bastionName, err) + return fmt.Errorf("searching for existing port for bastion %s: %v", bastionName, err) } // if the port is not found, we stop the adoption of ports since the rest of the ports will not be found either // and will be created after the adoption if len(ports) == 0 { scope.Logger().V(5).Info("Port not found, stopping the adoption of ports", "port index", i) - return changed, nil + return nil } if len(ports) > 1 { - return changed, fmt.Errorf("found multiple ports with name %s", portName) + return fmt.Errorf("found multiple ports with name %s", portName) } // The desired port was found, so we add it to the status scope.Logger().V(5).Info("Port found, adding it to the status", "port index", i) openStackCluster.Status.Bastion.DependentResources.PortsStatus = append(openStackCluster.Status.Bastion.DependentResources.PortsStatus, infrav1.PortStatus{ID: ports[0].ID}) - changed = true } - return changed, nil + return nil } // MissingPorts returns the ports that are not in the ports status but are desired ports which should be created.