Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ports follow-up #1910

Closed
wants to merge 2 commits into from
Closed

Ports follow-up #1910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8425da
Remove useless logging for port loop
EmilienM Feb 27, 2024
9d31afd
API changes for Dependent & Referenced resources
EmilienM Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v1alpha5/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}",
},
},
},
Expand Down
10 changes: 5 additions & 5 deletions api/v1alpha6/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
},
),
Expand Down
10 changes: 5 additions & 5 deletions api/v1alpha7/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
},
),
Expand Down
4 changes: 2 additions & 2 deletions api/v1beta1/openstackmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

Expand Down
16 changes: 8 additions & 8 deletions api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 20 additions & 4 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 24 additions & 17 deletions controllers/openstackcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
EmilienM marked this conversation as resolved.
Show resolved Hide resolved
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DependentResources is different to ReferencedResources because it's not immutable: we can successively create or adopt dependent resources over multiple reconciles. We must always do adoption here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but how would you know it has changed and you must leave the reconcile now to write the object?
If you remove the if, we're in infinite loop where we'll always adopt...

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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -374,7 +383,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS

// If ports options aren't in the status, we'll re-trigger the reconcile to get them
// via adopting the referenced resources.
if len(openStackCluster.Status.Bastion.ReferencedResources.PortsOpts) == 0 {
if openStackCluster.Status.Bastion == nil || openStackCluster.Status.Bastion.ReferencedResources == nil || len(openStackCluster.Status.Bastion.ReferencedResources.PortsOpts) == 0 {
return reconcile.Result{}, nil
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -543,9 +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 {
scope.Logger().Info("Reconciling ports for bastion", "bastion", bastionName(openStackCluster.Name))

func getOrCreateBastionPorts(cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error {
if openStackCluster.Status.Bastion == nil {
openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
}
Expand Down
30 changes: 15 additions & 15 deletions controllers/openstackcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
{
Expand All @@ -241,7 +241,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand Down Expand Up @@ -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{
{
Expand All @@ -292,7 +292,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand Down Expand Up @@ -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{
{
Expand All @@ -333,7 +333,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand Down Expand Up @@ -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{
{
Expand All @@ -377,7 +377,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand Down Expand Up @@ -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{
{
Expand All @@ -418,7 +418,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand Down Expand Up @@ -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{
{
Expand All @@ -456,7 +456,7 @@ var _ = Describe("OpenStackCluster controller", func() {
},
},
},
DependentResources: infrav1.DependentMachineResources{
DependentResources: &infrav1.DependentMachineResources{
PortsStatus: []infrav1.PortStatus{
{
ID: "portID1",
Expand All @@ -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",
},
},
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Loading