From 2758a6d27d46202ff8cff16374de8a2cc728993a Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 21 Mar 2024 16:04:59 +0000 Subject: [PATCH] Resolved and Resources become pointers in machine status --- api/v1alpha5/conversion_test.go | 2 +- api/v1alpha6/openstackcluster_conversion.go | 76 ++++++++++--------- api/v1alpha6/openstackmachine_conversion.go | 4 +- api/v1alpha7/openstackcluster_conversion.go | 21 +++-- api/v1alpha7/openstackmachine_conversion.go | 4 +- api/v1beta1/openstackmachine_types.go | 6 +- api/v1beta1/types.go | 23 ++++-- api/v1beta1/zz_generated.deepcopy.go | 24 +++++- ...re.cluster.x-k8s.io_openstackclusters.yaml | 7 +- controllers/openstackcluster_controller.go | 36 ++++++--- .../openstackcluster_controller_test.go | 30 ++++---- controllers/openstackmachine_controller.go | 62 +++++++++++---- .../openstackmachine_controller_test.go | 4 +- docs/book/src/api/v1beta1/api.md | 7 ++ .../services/compute/machine_resources.go | 4 +- .../compute/referenced_resources_test.go | 2 +- 16 files changed, 207 insertions(+), 105 deletions(-) diff --git a/api/v1alpha5/conversion_test.go b/api/v1alpha5/conversion_test.go index 82afcd82e8..7fbf79bda6 100644 --- a/api/v1alpha5/conversion_test.go +++ b/api/v1alpha5/conversion_test.go @@ -87,7 +87,7 @@ func TestConvertFrom(t *testing.T) { Spec: OpenStackMachineSpec{}, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"flavor\":\"\",\"image\":{}},\"status\":{\"ready\":false,\"resolved\":{},\"resources\":{}}}", + "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"flavor\":\"\",\"image\":{}},\"status\":{\"ready\":false}}", }, }, }, diff --git a/api/v1alpha6/openstackcluster_conversion.go b/api/v1alpha6/openstackcluster_conversion.go index 95131f6c3f..92fe7df622 100644 --- a/api/v1alpha6/openstackcluster_conversion.go +++ b/api/v1alpha6/openstackcluster_conversion.go @@ -367,12 +367,7 @@ func restorev1beta1ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst * dst.WorkerSecurityGroup = previous.WorkerSecurityGroup dst.BastionSecurityGroup = previous.BastionSecurityGroup - if previous.Bastion != nil { - dst.Bastion.Resolved = previous.Bastion.Resolved - } - if previous.Bastion != nil && previous.Bastion.Resources.Ports != nil { - dst.Bastion.Resources.Ports = previous.Bastion.Resources.Ports - } + restorev1beta1BastionStatus(previous.Bastion, dst.Bastion) } func Convert_v1beta1_OpenStackClusterStatus_To_v1alpha6_OpenStackClusterStatus(in *infrav1.OpenStackClusterStatus, out *OpenStackClusterStatus, s apiconversion.Scope) error { @@ -412,36 +407,15 @@ func Convert_v1alpha6_OpenStackClusterStatus_To_v1beta1_OpenStackClusterStatus(i /* Bastion */ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) { - if *previous != nil { - if *dst != nil && (*previous).Spec != nil && (*dst).Spec != nil { - restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec) - } - - optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP) - optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone) + if previous == nil || dst == nil || *previous == nil || *dst == nil { + return + } + if *dst != nil && (*previous).Spec != nil && (*dst).Spec != nil { + restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec) } -} - -func Convert_v1alpha6_Instance_To_v1beta1_BastionStatus(in *Instance, out *infrav1.BastionStatus, _ apiconversion.Scope) error { - // BastionStatus is the same as Spec with unused fields removed - out.ID = in.ID - out.Name = in.Name - out.SSHKeyName = in.SSHKeyName - out.State = infrav1.InstanceState(in.State) - out.IP = in.IP - out.FloatingIP = in.FloatingIP - return nil -} -func Convert_v1beta1_BastionStatus_To_v1alpha6_Instance(in *infrav1.BastionStatus, out *Instance, _ apiconversion.Scope) error { - // BastionStatus is the same as Spec with unused fields removed - out.ID = in.ID - out.Name = in.Name - out.SSHKeyName = in.SSHKeyName - out.State = InstanceState(in.State) - out.IP = in.IP - out.FloatingIP = in.FloatingIP - return nil + optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP) + optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone) } func Convert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error { @@ -496,3 +470,37 @@ func Convert_v1beta1_Bastion_To_v1alpha6_Bastion(in *infrav1.Bastion, out *Basti return optional.Convert_optional_String_To_string(&in.FloatingIP, &out.Instance.FloatingIP, s) } + +/* Bastion status */ + +func restorev1beta1BastionStatus(previous *infrav1.BastionStatus, dst *infrav1.BastionStatus) { + if previous == nil || dst == nil { + return + } + + // Resolved and resources have no equivalents + dst.Resolved = previous.Resolved + dst.Resources = previous.Resources +} + +func Convert_v1alpha6_Instance_To_v1beta1_BastionStatus(in *Instance, out *infrav1.BastionStatus, _ apiconversion.Scope) error { + // BastionStatus is the same as Spec with unused fields removed + out.ID = in.ID + out.Name = in.Name + out.SSHKeyName = in.SSHKeyName + out.State = infrav1.InstanceState(in.State) + out.IP = in.IP + out.FloatingIP = in.FloatingIP + return nil +} + +func Convert_v1beta1_BastionStatus_To_v1alpha6_Instance(in *infrav1.BastionStatus, out *Instance, _ apiconversion.Scope) error { + // BastionStatus is the same as Spec with unused fields removed + out.ID = in.ID + out.Name = in.Name + out.SSHKeyName = in.SSHKeyName + out.State = InstanceState(in.State) + out.IP = in.IP + out.FloatingIP = in.FloatingIP + return nil +} diff --git a/api/v1alpha6/openstackmachine_conversion.go b/api/v1alpha6/openstackmachine_conversion.go index d0f01cac1b..ad88c7afc0 100644 --- a/api/v1alpha6/openstackmachine_conversion.go +++ b/api/v1alpha6/openstackmachine_conversion.go @@ -90,13 +90,13 @@ var v1beta1OpenStackMachineRestorer = conversion.RestorerFor[*infrav1.OpenStackM restorev1beta1MachineSpec, ), "depresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.MachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.MachineResources { return &c.Status.Resources }, ), // No equivalent in v1alpha6 "refresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.ResolvedMachineSpec { + func(c *infrav1.OpenStackMachine) **infrav1.ResolvedMachineSpec { return &c.Status.Resolved }, ), diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index 6cbd9a5118..3defae462a 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/api/v1alpha7/openstackcluster_conversion.go @@ -348,14 +348,11 @@ func restorev1alpha7ClusterStatus(previous *OpenStackClusterStatus, dst *OpenSta } func restorev1beta1ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst *infrav1.OpenStackClusterStatus) { - // ReferencedResources have no equivalent in v1alpha7 - if previous.Bastion != nil { - dst.Bastion.Resolved = previous.Bastion.Resolved + if previous == nil || dst == nil { + return } - if previous.Bastion != nil && previous.Bastion.Resources.Ports != nil { - dst.Bastion.Resources.Ports = previous.Bastion.Resources.Ports - } + restorev1beta1BastionStatus(previous.Bastion, dst.Bastion) } func Convert_v1beta1_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(in *infrav1.OpenStackClusterStatus, out *OpenStackClusterStatus, s apiconversion.Scope) error { @@ -434,6 +431,18 @@ func Convert_v1beta1_Bastion_To_v1alpha7_Bastion(in *infrav1.Bastion, out *Basti return optional.Convert_optional_String_To_string(&in.FloatingIP, &out.Instance.FloatingIP, s) } +/* Bastion status */ + +func restorev1beta1BastionStatus(previous *infrav1.BastionStatus, dst *infrav1.BastionStatus) { + if previous == nil || dst == nil { + return + } + + // Resolved and resources have no equivalents + dst.Resolved = previous.Resolved + dst.Resources = previous.Resources +} + func Convert_v1beta1_BastionStatus_To_v1alpha7_BastionStatus(in *infrav1.BastionStatus, out *BastionStatus, s apiconversion.Scope) error { // ReferencedResources have no equivalent in v1alpha7 return autoConvert_v1beta1_BastionStatus_To_v1alpha7_BastionStatus(in, out, s) diff --git a/api/v1alpha7/openstackmachine_conversion.go b/api/v1alpha7/openstackmachine_conversion.go index 08ef6f94ce..fa0655288c 100644 --- a/api/v1alpha7/openstackmachine_conversion.go +++ b/api/v1alpha7/openstackmachine_conversion.go @@ -77,13 +77,13 @@ var v1beta1OpenStackMachineRestorer = conversion.RestorerFor[*infrav1.OpenStackM restorev1beta1MachineSpec, ), "depresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.MachineResources { + func(c *infrav1.OpenStackMachine) **infrav1.MachineResources { return &c.Status.Resources }, ), // No equivalent in v1alpha7 "refresources": conversion.UnconditionalFieldRestorer( - func(c *infrav1.OpenStackMachine) *infrav1.ResolvedMachineSpec { + func(c *infrav1.OpenStackMachine) **infrav1.ResolvedMachineSpec { return &c.Status.Resolved }, ), diff --git a/api/v1beta1/openstackmachine_types.go b/api/v1beta1/openstackmachine_types.go index 95e5edfe88..f222f71761 100644 --- a/api/v1beta1/openstackmachine_types.go +++ b/api/v1beta1/openstackmachine_types.go @@ -128,10 +128,12 @@ type OpenStackMachineStatus struct { // Resolved contains parts of the machine spec with all external // references fully resolved. - Resolved ResolvedMachineSpec `json:"resolved,omitempty"` + // +optional + Resolved *ResolvedMachineSpec `json:"resolved,omitempty"` // Resources contains references to OpenStack resources created for the machine. - Resources MachineResources `json:"resources,omitempty"` + // +optional + Resources *MachineResources `json:"resources,omitempty"` FailureReason *errors.MachineStatusError `json:"failureReason,omitempty"` diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index a8a9288393..d2d90d8f6e 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -358,14 +358,21 @@ 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"` - Resolved ResolvedMachineSpec `json:"resolved,omitempty"` - Resources MachineResources `json:"resources,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"` + + // Resolved contains parts of the bastion's machine spec with all + // external references fully resolved. + // +optional + Resolved *ResolvedMachineSpec `json:"resolved,omitempty"` + + // Resources contains references to OpenStack resources created for the bastion. + // +optional + Resources *MachineResources `json:"resources,omitempty"` } type RootVolume struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 892e950daf..b5d80bafff 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -146,8 +146,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.Resolved.DeepCopyInto(&out.Resolved) - in.Resources.DeepCopyInto(&out.Resources) + if in.Resolved != nil { + in, out := &in.Resolved, &out.Resolved + *out = new(ResolvedMachineSpec) + (*in).DeepCopyInto(*out) + } + if in.Resources != nil { + in, out := &in.Resources, &out.Resources + *out = new(MachineResources) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new BastionStatus. @@ -964,8 +972,16 @@ func (in *OpenStackMachineStatus) DeepCopyInto(out *OpenStackMachineStatus) { *out = new(InstanceState) **out = **in } - in.Resolved.DeepCopyInto(&out.Resolved) - in.Resources.DeepCopyInto(&out.Resources) + if in.Resolved != nil { + in, out := &in.Resolved, &out.Resolved + *out = new(ResolvedMachineSpec) + (*in).DeepCopyInto(*out) + } + if in.Resources != nil { + in, out := &in.Resources, &out.Resources + *out = new(MachineResources) + (*in).DeepCopyInto(*out) + } if in.FailureReason != nil { in, out := &in.FailureReason, &out.FailureReason *out = new(errors.MachineStatusError) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml index abdbf1c5da..4aa7ca14b0 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -6251,8 +6251,9 @@ spec: name: type: string resolved: - description: ResolvedMachineSpec contains resolved references - to resources required by the machine. + description: |- + Resolved contains parts of the bastion's machine spec with all + external references fully resolved. properties: imageID: description: ImageID is the ID of the image to use for the @@ -6435,6 +6436,8 @@ spec: type: string type: object resources: + description: Resources contains references to OpenStack resources + created for the bastion. properties: ports: description: Ports is the status of the ports created for diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index f1ab14b09c..1af03280f6 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -227,8 +227,13 @@ func resolveBastionResources(scope *scope.WithLogger, clusterResourceName string if openStackCluster.Spec.Bastion.Spec == nil { return false, fmt.Errorf("bastion spec is nil when bastion is enabled, this shouldn't happen") } + resolved := openStackCluster.Status.Bastion.Resolved + if resolved == nil { + resolved = &infrav1.ResolvedMachineSpec{} + openStackCluster.Status.Bastion.Resolved = resolved + } changed, err := compute.ResolveMachineSpec(scope, - openStackCluster.Spec.Bastion.Spec, &openStackCluster.Status.Bastion.Resolved, + openStackCluster.Spec.Bastion.Spec, resolved, clusterResourceName, bastionName(clusterResourceName), openStackCluster, getBastionSecurityGroupID(openStackCluster)) if err != nil { @@ -238,10 +243,13 @@ func resolveBastionResources(scope *scope.WithLogger, clusterResourceName string // If the resolved machine spec changed we need to restart the reconcile to avoid inconsistencies between reconciles. return true, nil } + resources := openStackCluster.Status.Bastion.Resources + if resources == nil { + resources = &infrav1.MachineResources{} + openStackCluster.Status.Bastion.Resources = resources + } - err = compute.AdoptMachineResources(scope, - &openStackCluster.Status.Bastion.Resolved, - &openStackCluster.Status.Bastion.Resources) + err = compute.AdoptMachineResources(scope, resolved, resources) if err != nil { return false, err } @@ -268,8 +276,10 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac } } + bastionStatus := openStackCluster.Status.Bastion + var instanceStatus *compute.InstanceStatus - if openStackCluster.Status.Bastion != nil && openStackCluster.Status.Bastion.ID != "" { + if bastionStatus != nil && bastionStatus.ID != "" { instanceStatus, err = computeService.GetInstanceStatus(openStackCluster.Status.Bastion.ID) if err != nil { return err @@ -308,18 +318,18 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac } } - if openStackCluster.Status.Bastion != nil && len(openStackCluster.Status.Bastion.Resources.Ports) > 0 { + if bastionStatus != nil && bastionStatus.Resources != nil { trunkSupported, err := networkingService.IsTrunkExtSupported() if err != nil { return err } - for _, port := range openStackCluster.Status.Bastion.Resources.Ports { + for _, port := range bastionStatus.Resources.Ports { if err := networkingService.DeleteInstanceTrunkAndPort(openStackCluster, port, trunkSupported); err != nil { handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete port: %w", err)) return fmt.Errorf("failed to delete port: %w", err) } } - openStackCluster.Status.Bastion.Resources.Ports = nil + bastionStatus.Resources.Ports = nil } scope.Logger().Info("Deleted Bastion") @@ -542,7 +552,10 @@ func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, cluster * // v1beta1 API validations prevent this from happening in normal circumstances. bastion.Spec = &infrav1.OpenStackMachineSpec{} } - resolved := &openStackCluster.Status.Bastion.Resolved + resolved := openStackCluster.Status.Bastion.Resolved + if resolved == nil { + return nil, errors.New("bastion resolved is nil") + } machineSpec := bastion.Spec instanceSpec := &compute.InstanceSpec{ @@ -579,7 +592,10 @@ func getBastionSecurityGroupID(openStackCluster *infrav1.OpenStackCluster) *stri func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service) error { desiredPorts := openStackCluster.Status.Bastion.Resolved.Ports - resources := &openStackCluster.Status.Bastion.Resources + resources := openStackCluster.Status.Bastion.Resources + if resources == nil { + return errors.New("bastion resources are nil") + } if len(desiredPorts) == len(resources.Ports) { return nil diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index b738cafe47..29cfd40a21 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/controllers/openstackcluster_controller_test.go @@ -237,7 +237,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(err).To(BeNil()) testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -245,7 +245,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -286,7 +286,7 @@ var _ = Describe("OpenStackCluster controller", func() { expectedStatus := &infrav1.BastionStatus{ ID: "adopted-bastion-uuid", State: "ACTIVE", - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -294,7 +294,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -327,7 +327,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, Bastion: &infrav1.BastionStatus{ ID: "adopted-fip-bastion-uuid", - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -335,7 +335,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -369,7 +369,7 @@ var _ = Describe("OpenStackCluster controller", func() { ID: "adopted-fip-bastion-uuid", FloatingIP: "1.2.3.4", State: "ACTIVE", - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -377,7 +377,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -409,7 +409,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, Bastion: &infrav1.BastionStatus{ ID: "requeue-bastion-uuid", - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -417,7 +417,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -445,7 +445,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(testCluster.Status.Bastion).To(Equal(&infrav1.BastionStatus{ ID: "requeue-bastion-uuid", State: "BUILD", - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", Ports: []infrav1.ResolvedPortSpec{ { @@ -453,7 +453,7 @@ var _ = Describe("OpenStackCluster controller", func() { }, }, }, - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "portID1", @@ -475,7 +475,7 @@ var _ = Describe("OpenStackCluster controller", func() { Expect(err).To(BeNil()) testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: "imageID", }, }, @@ -528,7 +528,7 @@ var _ = Describe("OpenStackCluster controller", func() { } testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "port-id", @@ -613,7 +613,7 @@ var _ = Describe("OpenStackCluster controller", func() { } testCluster.Status = infrav1.OpenStackClusterStatus{ Bastion: &infrav1.BastionStatus{ - Resources: infrav1.MachineResources{ + Resources: &infrav1.MachineResources{ Ports: []infrav1.PortStatus{ { ID: "port-id", diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 246cf2bfc3..969ec5b61a 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -92,6 +92,7 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req } log = log.WithValues("openStackMachine", openStackMachine.Name) + log.V(4).Info("Reconciling OpenStackMachine") // Fetch the Machine. machine, err := util.GetOwnerMachine(ctx, r.Client, openStackMachine.ObjectMeta) @@ -162,9 +163,14 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req } func resolveMachineResources(scope *scope.WithLogger, clusterResourceName string, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine) (bool, error) { + resolved := openStackMachine.Status.Resolved + if resolved == nil { + resolved = &infrav1.ResolvedMachineSpec{} + openStackMachine.Status.Resolved = resolved + } // Resolve and store resources changed, err := compute.ResolveMachineSpec(scope, - &openStackMachine.Spec, &openStackMachine.Status.Resolved, + &openStackMachine.Spec, resolved, clusterResourceName, openStackMachine.Name, openStackCluster, getManagedSecurityGroup(openStackCluster, machine)) if err != nil { @@ -175,8 +181,14 @@ func resolveMachineResources(scope *scope.WithLogger, clusterResourceName string return true, nil } + resources := openStackMachine.Status.Resources + if resources == nil { + resources = &infrav1.MachineResources{} + openStackMachine.Status.Resources = resources + } + // Adopt any existing resources - return false, compute.AdoptMachineResources(scope, &openStackMachine.Status.Resolved, &openStackMachine.Status.Resources) + return false, compute.AdoptMachineResources(scope, resolved, resources) } func patchMachine(ctx context.Context, patchHelper *patch.Helper, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine, options ...patch.Option) error { @@ -296,7 +308,10 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl } } - instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "") + instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "") + if err != nil { + return ctrl.Result{}, err + } if err := computeService.DeleteInstance(openStackMachine, instanceStatus, instanceSpec); err != nil { conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err) @@ -308,10 +323,12 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl return ctrl.Result{}, err } - portsStatus := openStackMachine.Status.Resources.Ports - 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.Resources != nil { + portsStatus := openStackMachine.Status.Resources.Ports + 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) + } } } @@ -485,6 +502,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope } if changed, err := resolveMachineResources(scope, clusterResourceName, openStackCluster, openStackMachine, machine); changed || err != nil { + scope.Logger().V(6).Info("Machine resources updated, requeuing") return ctrl.Result{}, err } @@ -658,8 +676,15 @@ func (r *OpenStackMachineReconciler) reconcileAPIServerLoadBalancer(scope *scope } func getOrCreateMachinePorts(openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service) error { - desiredPorts := openStackMachine.Status.Resolved.Ports - resources := &openStackMachine.Status.Resources + resolved := openStackMachine.Status.Resolved + if resolved == nil { + return errors.New("machine resolved is nil") + } + resources := openStackMachine.Status.Resources + if resources == nil { + return errors.New("machine resources is nil") + } + desiredPorts := resolved.Ports if len(desiredPorts) == len(resources.Ports) { return nil @@ -696,7 +721,11 @@ func (r *OpenStackMachineReconciler) getOrCreateInstance(logger logr.Logger, ope return nil, nil } } - instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData) + instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData) + if err != nil { + return nil, err + } + logger.Info("Machine does not exist, creating Machine", "name", openStackMachine.Name) instanceStatus, err = computeService.CreateInstance(openStackMachine, instanceSpec, portIDs) if err != nil { @@ -707,7 +736,12 @@ func (r *OpenStackMachineReconciler) getOrCreateInstance(logger logr.Logger, ope return instanceStatus, nil } -func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) *compute.InstanceSpec { +func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) (*compute.InstanceSpec, error) { + resolved := openStackMachine.Status.Resolved + if resolved == nil { + return nil, errors.New("machine resolved is nil") + } + serverMetadata := make(map[string]string, len(openStackMachine.Spec.ServerMetadata)) for i := range openStackMachine.Spec.ServerMetadata { key := openStackMachine.Spec.ServerMetadata[i].Key @@ -717,7 +751,7 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine * instanceSpec := compute.InstanceSpec{ Name: openStackMachine.Name, - ImageID: openStackMachine.Status.Resolved.ImageID, + ImageID: resolved.ImageID, Flavor: openStackMachine.Spec.Flavor, SSHKeyName: openStackMachine.Spec.SSHKeyName, UserData: userData, @@ -725,7 +759,7 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine * ConfigDrive: openStackMachine.Spec.ConfigDrive != nil && *openStackMachine.Spec.ConfigDrive, RootVolume: openStackMachine.Spec.RootVolume, AdditionalBlockDevices: openStackMachine.Spec.AdditionalBlockDevices, - ServerGroupID: openStackMachine.Status.Resolved.ServerGroupID, + ServerGroupID: resolved.ServerGroupID, Trunk: openStackMachine.Spec.Trunk, } @@ -736,7 +770,7 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine * instanceSpec.Tags = compute.InstanceTags(&openStackMachine.Spec, openStackCluster) - return &instanceSpec + return &instanceSpec, nil } // getManagedSecurityGroup returns the ID of the security group managed by the diff --git a/controllers/openstackmachine_controller_test.go b/controllers/openstackmachine_controller_test.go index bc2b4897f3..c941f11d04 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{ - Resolved: infrav1.ResolvedMachineSpec{ + Resolved: &infrav1.ResolvedMachineSpec{ ImageID: imageUUID, ServerGroupID: serverGroupUUID, }, @@ -160,7 +160,7 @@ func Test_machineToInstanceSpec(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data") + got, _ := machineToInstanceSpec(tt.openStackCluster(), tt.machine(), tt.openStackMachine(), "user-data") wanted := tt.wantInstanceSpec() g.Expect(got).To(Equal(wanted), cmp.Diff(got, wanted)) diff --git a/docs/book/src/api/v1beta1/api.md b/docs/book/src/api/v1beta1/api.md index fc84797f7a..c60863aaf2 100644 --- a/docs/book/src/api/v1beta1/api.md +++ b/docs/book/src/api/v1beta1/api.md @@ -1220,6 +1220,9 @@ ResolvedMachineSpec +(Optional) +

Resolved contains parts of the bastion’s machine spec with all +external references fully resolved.

@@ -1232,6 +1235,8 @@ MachineResources +(Optional) +

Resources contains references to OpenStack resources created for the bastion.

@@ -3294,6 +3299,7 @@ ResolvedMachineSpec +(Optional)

Resolved contains parts of the machine spec with all external references fully resolved.

@@ -3308,6 +3314,7 @@ MachineResources +(Optional)

Resources contains references to OpenStack resources created for the machine.

diff --git a/pkg/cloud/services/compute/machine_resources.go b/pkg/cloud/services/compute/machine_resources.go index ced3faff76..6619d2ea72 100644 --- a/pkg/cloud/services/compute/machine_resources.go +++ b/pkg/cloud/services/compute/machine_resources.go @@ -22,11 +22,11 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func AdoptMachineResources(scope *scope.WithLogger, referencedResources *infrav1.ResolvedMachineSpec, resources *infrav1.MachineResources) error { +func AdoptMachineResources(scope *scope.WithLogger, resolved *infrav1.ResolvedMachineSpec, resources *infrav1.MachineResources) error { networkingService, err := networking.NewService(scope) if err != nil { return err } - return networkingService.AdoptPorts(scope, referencedResources.Ports, resources) + return networkingService.AdoptPorts(scope, resolved.Ports, resources) } diff --git a/pkg/cloud/services/compute/referenced_resources_test.go b/pkg/cloud/services/compute/referenced_resources_test.go index 1f13ce5427..d2456918dc 100644 --- a/pkg/cloud/services/compute/referenced_resources_test.go +++ b/pkg/cloud/services/compute/referenced_resources_test.go @@ -33,7 +33,7 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func Test_ResolveReferencedMachineResources(t *testing.T) { +func Test_ResolveMachineSpec(t *testing.T) { const ( serverGroupID1 = "ce96e584-7ebc-46d6-9e55-987d72e3806c" imageID1 = "de96e584-7ebc-46d6-9e55-987d72e3806c"