diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index b34f0f4201..64488cfc48 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -234,7 +234,10 @@ func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1. return true, nil } - changed, err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(openStackCluster.Name)) + changed, err = compute.AdoptDependentMachineResources(scope, + bastionName(openStackCluster.Name), + &openStackCluster.Status.Bastion.ReferencedResources, + &openStackCluster.Status.Bastion.DependentResources) if err != nil { return false, err } diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 97f234b6ad..91c06ed0b9 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -163,8 +163,8 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req return reconcile.Result{}, nil } - // Resolve and store dependent resources - changed, err = compute.ResolveDependentMachineResources(scope, openStackMachine) + // Adopt any existing dependent resources + changed, err = compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources) if err != nil { return reconcile.Result{}, err } diff --git a/pkg/cloud/services/compute/dependent_resources.go b/pkg/cloud/services/compute/dependent_resources.go index 7a7d4e5b65..a9b1c49dc8 100644 --- a/pkg/cloud/services/compute/dependent_resources.go +++ b/pkg/cloud/services/compute/dependent_resources.go @@ -22,24 +22,11 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func ResolveDependentMachineResources(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine) (changed bool, err error) { - changed = false - - networkingService, err := networking.NewService(scope) - if err != nil { - return changed, err - } - - return networkingService.AdoptMachinePorts(scope, openStackMachine, openStackMachine.Status.ReferencedResources.Ports) -} - -func ResolveDependentBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) (changed bool, err error) { - changed = false - +func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) (bool, error) { networkingService, err := networking.NewService(scope) if err != nil { - return changed, err + return false, err } - return networkingService.AdoptBastionPorts(scope, openStackCluster, bastionName) + return networkingService.AdoptPorts(scope, baseName, referencedResources.Ports, dependentResources) } diff --git a/pkg/cloud/services/compute/dependent_resources_test.go b/pkg/cloud/services/compute/dependent_resources_test.go deleted file mode 100644 index e25caba053..0000000000 --- a/pkg/cloud/services/compute/dependent_resources_test.go +++ /dev/null @@ -1,219 +0,0 @@ -/* -Copyright 2024 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package compute - -import ( - "testing" - - "github.com/go-logr/logr/testr" - "github.com/golang/mock/gomock" - "github.com/google/go-cmp/cmp" - . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" - "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" -) - -func Test_ResolveDependentMachineResources(t *testing.T) { - const networkID = "5e8e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" - const portID = "78e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" - - tests := []struct { - testName string - openStackCluster *infrav1.OpenStackCluster - openStackMachineStatus infrav1.OpenStackMachineStatus - want *infrav1.DependentMachineResources - wantErr bool - }{ - { - testName: "no Network ID yet and no ports in status", - openStackCluster: &infrav1.OpenStackCluster{}, - want: &infrav1.DependentMachineResources{}, - wantErr: false, - }, - { - testName: "Network ID set but no ports in status", - openStackCluster: &infrav1.OpenStackCluster{ - Status: infrav1.OpenStackClusterStatus{ - Network: &infrav1.NetworkStatusWithSubnets{ - NetworkStatus: infrav1.NetworkStatus{ - ID: networkID, - }, - }, - }, - }, - want: &infrav1.DependentMachineResources{}, - wantErr: false, - }, - { - testName: "Network ID set and ports in status", - openStackCluster: &infrav1.OpenStackCluster{ - Status: infrav1.OpenStackClusterStatus{ - Network: &infrav1.NetworkStatusWithSubnets{ - NetworkStatus: infrav1.NetworkStatus{ - ID: networkID, - }, - }, - }, - }, - openStackMachineStatus: infrav1.OpenStackMachineStatus{ - DependentResources: infrav1.DependentMachineResources{ - Ports: []infrav1.PortStatus{ - { - ID: portID, - }, - }, - }, - }, - want: &infrav1.DependentMachineResources{ - Ports: []infrav1.PortStatus{ - { - ID: portID, - }, - }, - }, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.testName, func(t *testing.T) { - g := NewWithT(t) - log := testr.New(t) - mockCtrl := gomock.NewController(t) - mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") - - defaultOpenStackMachine := &infrav1.OpenStackMachine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Status: tt.openStackMachineStatus, - } - - _, 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)) - }) - } -} - -func TestResolveDependentBastionResources(t *testing.T) { - const networkID = "5e8e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" - const portID = "78e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" - const bastionName = "bastion" - - tests := []struct { - testName string - openStackCluster *infrav1.OpenStackCluster - want *infrav1.DependentMachineResources - wantErr bool - }{ - { - testName: "no Network ID yet and no ports in status", - openStackCluster: &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Bastion: &infrav1.Bastion{ - Enabled: true, - }, - }, - }, - want: &infrav1.DependentMachineResources{}, - wantErr: false, - }, - { - testName: "Network ID set but no ports in status", - openStackCluster: &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Bastion: &infrav1.Bastion{ - Enabled: true, - }, - }, - Status: infrav1.OpenStackClusterStatus{ - Network: &infrav1.NetworkStatusWithSubnets{ - NetworkStatus: infrav1.NetworkStatus{ - ID: networkID, - }, - }, - }, - }, - want: &infrav1.DependentMachineResources{}, - }, - { - testName: "Network ID set and ports in status", - openStackCluster: &infrav1.OpenStackCluster{ - Spec: infrav1.OpenStackClusterSpec{ - Bastion: &infrav1.Bastion{ - Enabled: true, - }, - }, - Status: infrav1.OpenStackClusterStatus{ - Bastion: &infrav1.BastionStatus{ - DependentResources: infrav1.DependentMachineResources{ - Ports: []infrav1.PortStatus{ - { - ID: portID, - }, - }, - }, - }, - Network: &infrav1.NetworkStatusWithSubnets{ - NetworkStatus: infrav1.NetworkStatus{ - ID: networkID, - }, - }, - }, - }, - want: &infrav1.DependentMachineResources{ - Ports: []infrav1.PortStatus{ - { - ID: portID, - }, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.testName, func(t *testing.T) { - g := NewWithT(t) - log := testr.New(t) - mockCtrl := gomock.NewController(t) - mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") - - _, err := ResolveDependentBastionResources(scope.NewWithLogger(mockScopeFactory, log), tt.openStackCluster, bastionName) - if tt.wantErr { - g.Expect(err).Error() - return - } - - defaultOpenStackCluster := &infrav1.OpenStackCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Spec: tt.openStackCluster.Spec, - Status: tt.openStackCluster.Status, - } - - if tt.openStackCluster.Status.Bastion != nil { - g.Expect(&defaultOpenStackCluster.Status.Bastion.DependentResources).To(Equal(tt.want), cmp.Diff(&defaultOpenStackCluster.Status.Bastion.DependentResources, tt.want)) - } - }) - } -} diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index b5d5aee5a1..bdd01b0ef9 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -569,104 +569,36 @@ func (s *Service) IsTrunkExtSupported() (trunknSupported bool, err error) { return true, nil } -// AdoptMachinePorts checks if the ports are in ready condition. If not, it'll try to adopt them -// 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) { +// AdoptPorts looks for ports in desiredPorts which were previously created, and adds them to dependentResources.Ports. +// A port matches if it has the same name and network ID as the desired port. +func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) (changed bool, err error) { changed = false - // 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.Ports) == len(desiredPorts) { - scope.Logger().V(5).Info("OpenStackMachine is ready, skipping the adoption of ports") - return changed, nil - } - - scope.Logger().Info("Adopting ports for OpenStackMachine", "name", openStackMachine.Name) - - // We create ports in order and adopt them in order in PortsStatus. - // This means that if port N doesn't exist we know that ports >N don't exist. - // We can therefore stop searching for ports once we find one that doesn't exist. - for i, port := range desiredPorts { - // check if the port is in status first and if it is, skip it - if i < len(openStackMachine.Status.DependentResources.Ports) { - scope.Logger().V(5).Info("Port already in status, skipping it", "port index", i) - continue - } - - portOpts := &desiredPorts[i] - portName := GetPortName(openStackMachine.Name, portOpts, i) - ports, err := s.client.ListPort(ports.ListOpts{ - Name: portName, - NetworkID: port.Network.ID, - }) - if err != nil { - return changed, 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 - } - if len(ports) > 1 { - return changed, 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.Ports = append(openStackMachine.Status.DependentResources.Ports, infrav1.PortStatus{ID: ports[0].ID}) - changed = true - } - - return changed, 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 - - if openStackCluster.Status.Network == nil { - scope.Logger().V(5).Info("Network status is nil, skipping the adoption of ports") - return changed, nil - } - - if openStackCluster.Status.Bastion == nil { - scope.Logger().V(5).Info("Bastion status is nil, initializing it") - openStackCluster.Status.Bastion = &infrav1.BastionStatus{} - } - - desiredPorts := openStackCluster.Status.Bastion.ReferencedResources.Ports - // We can skip adoption if the ports are already in the status - if len(desiredPorts) == len(openStackCluster.Status.Bastion.DependentResources.Ports) { + if len(desiredPorts) == len(dependentResources.Ports) { return changed, nil } - scope.Logger().Info("Adopting bastion ports for OpenStackCluster", "name", openStackCluster.Name) + scope.Logger().V(5).Info("Adopting ports") // We create ports in order and adopt them in order in PortsStatus. // This means that if port N doesn't exist we know that ports >N don't exist. // We can therefore stop searching for ports once we find one that doesn't exist. for i, port := range desiredPorts { // check if the port is in status first and if it is, skip it - if i < len(openStackCluster.Status.Bastion.DependentResources.Ports) { + if i < len(dependentResources.Ports) { scope.Logger().V(5).Info("Port already in status, skipping it", "port index", i) continue } portOpts := &desiredPorts[i] - portName := GetPortName(bastionName, portOpts, i) + portName := GetPortName(baseName, portOpts, i) ports, err := s.client.ListPort(ports.ListOpts{ Name: portName, NetworkID: port.Network.ID, }) if err != nil { - return changed, fmt.Errorf("searching for existing port for bastion %s: %v", bastionName, err) + return changed, fmt.Errorf("searching for existing port %s in network %s: %v", portName, port.Network.ID, 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 @@ -679,8 +611,9 @@ func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *i } // 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.Ports = append(openStackCluster.Status.Bastion.DependentResources.Ports, infrav1.PortStatus{ID: ports[0].ID}) + portID := ports[0].ID + scope.Logger().Info("Adopted previously created port which was not in status", "port index", i, "portID", portID) + dependentResources.Ports = append(dependentResources.Ports, infrav1.PortStatus{ID: portID}) changed = true } diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index c6bb99c95d..e281125689 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -38,9 +38,6 @@ import ( ) func Test_CreatePort(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - // Arbitrary GUIDs used in the tests netID := "7fd24ceb-788a-441f-ad0a-d8e2f5d31a1d" subnetID1 := "d9c88a6d-0b8c-48ff-8f0e-8d85a078c194" @@ -412,6 +409,9 @@ func Test_CreatePort(t *testing.T) { for i := range tests { tt := tests[i] t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + g := NewWithT(t) mockClient := mock.NewMockNetworkClient(mockCtrl) tt.expect(mockClient.EXPECT()) @@ -437,9 +437,6 @@ func Test_CreatePort(t *testing.T) { } func TestService_normalizePorts(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - const ( defaultNetworkID = "3c66f3ca-2d26-4d9d-ae3b-568f54129773" defaultSubnetID = "d8dbba89-8c39-4192-a571-e702fca35bac" @@ -775,6 +772,8 @@ func TestService_normalizePorts(t *testing.T) { g := NewWithT(t) log := testr.New(t) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() mockClient := mock.NewMockNetworkClient(mockCtrl) if tt.expectNetwork != nil { tt.expectNetwork(mockClient.EXPECT()) @@ -837,3 +836,150 @@ func Test_getPortName(t *testing.T) { }) } } + +func Test_AdoptPorts(t *testing.T) { + const ( + networkID1 = "5e8e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" + networkID2 = "0a4ff38e-1e03-4b4e-994c-c8ae38a2915e" + networkID3 = "bd22ea65-53de-4585-bb6f-b0a84d0085d1" + portID1 = "78e0d3b-7f3d-4f3e-8b3f-3e3e3e3e3e3e" + portID2 = "a838209b-389a-47a0-9161-3d6919891074" + ) + + tests := []struct { + testName string + desiredPorts []infrav1.PortOpts + dependentResources infrav1.DependentMachineResources + expect func(*mock.MockNetworkClientMockRecorder) + want infrav1.DependentMachineResources + wantErr bool + }{ + { + testName: "No desired ports", + }, + { + testName: "desired port already in status: no-op", + desiredPorts: []infrav1.PortOpts{ + {Network: &infrav1.NetworkFilter{ID: networkID1}}, + }, + dependentResources: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + want: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + }, + { + testName: "desired port not in status, exists: adopt", + desiredPorts: []infrav1.PortOpts{ + {Network: &infrav1.NetworkFilter{ID: networkID1}}, + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { + m.ListPort(ports.ListOpts{Name: "test-machine-0", NetworkID: networkID1}). + Return([]ports.Port{{ID: portID1}}, nil) + }, + want: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + }, + { + testName: "desired port not in status, does not exist: ignore", + desiredPorts: []infrav1.PortOpts{ + {Network: &infrav1.NetworkFilter{ID: networkID1}}, + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { + m.ListPort(ports.ListOpts{Name: "test-machine-0", NetworkID: networkID1}). + Return(nil, nil) + }, + want: infrav1.DependentMachineResources{}, + }, + { + testName: "2 desired ports, first in status, second exists: adopt second", + desiredPorts: []infrav1.PortOpts{ + {Network: &infrav1.NetworkFilter{ID: networkID1}}, + {Network: &infrav1.NetworkFilter{ID: networkID2}}, + }, + dependentResources: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { + m.ListPort(ports.ListOpts{Name: "test-machine-1", NetworkID: networkID2}). + Return([]ports.Port{{ID: portID2}}, nil) + }, + want: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + {ID: portID1}, + {ID: portID2}, + }, + }, + }, + { + testName: "3 desired ports, first in status, second does not exist: ignore, do no look for third", + desiredPorts: []infrav1.PortOpts{ + {Network: &infrav1.NetworkFilter{ID: networkID1}}, + {Network: &infrav1.NetworkFilter{ID: networkID2}}, + {Network: &infrav1.NetworkFilter{ID: networkID3}}, + }, + dependentResources: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + { + ID: portID1, + }, + }, + }, + expect: func(m *mock.MockNetworkClientMockRecorder) { + m.ListPort(ports.ListOpts{Name: "test-machine-1", NetworkID: networkID2}). + Return(nil, nil) + }, + want: infrav1.DependentMachineResources{ + Ports: []infrav1.PortStatus{ + {ID: portID1}, + }, + }, + }, + } + for i := range tests { + tt := &tests[i] + t.Run(tt.testName, func(t *testing.T) { + g := NewWithT(t) + log := testr.New(t) + + mockCtrl := gomock.NewController(t) + mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "") + mockClient := mock.NewMockNetworkClient(mockCtrl) + if tt.expect != nil { + tt.expect(mockClient.EXPECT()) + } + + s := Service{ + client: mockClient, + } + + _, err := s.AdoptPorts(scope.NewWithLogger(mockScopeFactory, log), + "test-machine", + tt.desiredPorts, &tt.dependentResources) + if tt.wantErr { + g.Expect(err).Error() + return + } + + g.Expect(tt.dependentResources).To(Equal(tt.want), cmp.Diff(&tt.dependentResources, tt.want)) + }) + } +}