From fd992baba7b5644e3c173d2743d7894348c23dcd Mon Sep 17 00:00:00 2001 From: Huy Mai Date: Fri, 15 Nov 2024 12:37:43 +0200 Subject: [PATCH] Ensure that existing ports also have correct tags and trunks Signed-off-by: Huy Mai --- controllers/openstackserver_controller.go | 6 +- .../openstackserver_controller_test.go | 2 + pkg/cloud/services/networking/port.go | 78 ++++++++++++------- pkg/cloud/services/networking/port_test.go | 32 +++++++- test/e2e/data/e2e_conf.yaml | 2 +- 5 files changed, 83 insertions(+), 37 deletions(-) diff --git a/controllers/openstackserver_controller.go b/controllers/openstackserver_controller.go index c7a1ca1b1c..35cf706a68 100644 --- a/controllers/openstackserver_controller.go +++ b/controllers/openstackserver_controller.go @@ -418,11 +418,7 @@ func getOrCreateServerPorts(openStackServer *infrav1alpha1.OpenStackServer, netw } desiredPorts := resolved.Ports - if len(desiredPorts) == len(resources.Ports) { - return nil - } - - if err := networkingService.CreatePorts(openStackServer, desiredPorts, resources); err != nil { + if err := networkingService.EnsurePorts(openStackServer, desiredPorts, resources); err != nil { return fmt.Errorf("creating ports: %w", err) } diff --git a/controllers/openstackserver_controller_test.go b/controllers/openstackserver_controller_test.go index ce0eb8e34d..4c98dd48c5 100644 --- a/controllers/openstackserver_controller_test.go +++ b/controllers/openstackserver_controller_test.go @@ -479,6 +479,7 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) { listDefaultPortsNotFound(r) createDefaultPort(r) listDefaultServerNotFound(r) + listDefaultPortsNotFound(r) createDefaultServer(r) }, }, @@ -499,6 +500,7 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) { }, }, expect: func(r *recorders) { + listDefaultPorts(r) listDefaultPorts(r) listDefaultServerFound(r) }, diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index 66cd94f167..aafe18fb4b 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -124,7 +124,49 @@ func (s *Service) GetPortForExternalNetwork(instanceID string, externalNetworkID return nil, nil } -func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) (*ports.Port, error) { +// ensurePortTagsAndTrunk ensures that the provided port has the tags and trunk defined in portSpec. +func (s *Service) ensurePortTagsAndTrunk(port *ports.Port, eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) error { + if len(portSpec.Tags) > 0 { + if err := s.replaceAllAttributesTags(eventObject, portResource, port.ID, portSpec.Tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portSpec.Name, err) + return err + } + } + if ptr.Deref(portSpec.Trunk, false) { + trunk, err := s.getOrCreateTrunkForPort(eventObject, port) + if err != nil { + record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", port.Name, err) + return err + } + if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, portSpec.Tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err) + return err + } + } + return nil +} + +// EnsurePort ensure that a port defined with portSpec Name and NetworkID exists, +// and that the port has suitable tags and trunk. +func (s *Service) EnsurePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec) (*ports.Port, error) { + existingPorts, err := s.client.ListPort(ports.ListOpts{ + Name: portSpec.Name, + NetworkID: portSpec.NetworkID, + }) + if err != nil { + return nil, fmt.Errorf("searching for existing port for server: %v", err) + } + if len(existingPorts) > 1 { + return nil, fmt.Errorf("multiple ports found with name \"%s\"", portSpec.Name) + } + + if len(existingPorts) == 1 { + port := &existingPorts[0] + if err = s.ensurePortTagsAndTrunk(port, eventObject, portSpec); err != nil { + return nil, err + } + return port, nil + } var addressPairs []ports.AddressPair if !ptr.Deref(portSpec.DisablePortSecurity, false) { for _, ap := range portSpec.AllowedAddressPairs { @@ -200,24 +242,10 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol return nil, err } - if len(portSpec.Tags) > 0 { - if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, portSpec.Tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portSpec.Name, err) - return nil, err - } + if err = s.ensurePortTagsAndTrunk(port, eventObject, portSpec); err != nil { + return nil, err } record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID) - if ptr.Deref(portSpec.Trunk, false) { - trunk, err := s.getOrCreateTrunkForPort(eventObject, port) - if err != nil { - record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", port.Name, err) - return nil, err - } - if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, portSpec.Tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err) - return nil, err - } - } return port, nil } @@ -328,16 +356,12 @@ func getPortName(baseName string, portSpec *infrav1.PortOpts, netIndex int) stri return fmt.Sprintf("%s-%d", baseName, netIndex) } -func (s *Service) CreatePorts(eventObject runtime.Object, desiredPorts []infrav1.ResolvedPortSpec, resources *infrav1alpha1.ServerResources) error { - for i := range desiredPorts { - // Skip creation of ports which already exist - if i < len(resources.Ports) { - continue - } - - portSpec := &desiredPorts[i] - // Events are recorded in CreatePort - port, err := s.CreatePort(eventObject, portSpec) +// EnsurePorts ensures that every one of desiredPorts is created and has +// expected trunk and tags. +func (s *Service) EnsurePorts(eventObject runtime.Object, desiredPorts []infrav1.ResolvedPortSpec, resources *infrav1alpha1.ServerResources) error { + for _, portSpec := range desiredPorts { + // Events are recorded in EnsurePort + port, err := s.EnsurePort(eventObject, &portSpec) if err != nil { return err } diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index 4a247e89af..aef4d6b6cf 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -41,7 +41,7 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) -func Test_CreatePort(t *testing.T) { +func Test_EnsurePort(t *testing.T) { // Arbitrary values used in the tests const ( netID = "7fd24ceb-788a-441f-ad0a-d8e2f5d31a1d" @@ -60,8 +60,8 @@ func Test_CreatePort(t *testing.T) { name string port infrav1.ResolvedPortSpec expect func(m *mock.MockNetworkClientMockRecorder, g Gomega) - // Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or CreatePort to return. - // Mostly in this test suite, we're checking that CreatePort is called with the expected port opts. + // Note the 'wanted' port isn't so important, since it will be whatever we tell ListPort or EnsurePort to return. + // Mostly in this test suite, we're checking that EnsurePort is called with the expected port opts. want *ports.Port wantErr bool }{ @@ -157,6 +157,10 @@ func Test_CreatePort(t *testing.T) { }, } + m.ListPort(ports.ListOpts{ + Name: "foo-port-1", + NetworkID: netID, + }).Return(nil, nil) // The following allows us to use gomega to // compare the argument instead of gomock. // Gomock's output in the case of a mismatch is @@ -184,6 +188,10 @@ func Test_CreatePort(t *testing.T) { expectedCreateOpts = portsbinding.CreateOptsExt{ CreateOptsBuilder: expectedCreateOpts, } + m.ListPort(ports.ListOpts{ + Name: "test-port", + NetworkID: netID, + }).Return(nil, nil) m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { gotCreateOpts := builder.(portsbinding.CreateOptsExt) g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) @@ -203,6 +211,10 @@ func Test_CreatePort(t *testing.T) { SecurityGroups: []string{portSecurityGroupID}, }, expect: func(m *mock.MockNetworkClientMockRecorder, _ Gomega) { + m.ListPort(ports.ListOpts{ + Name: "test-port", + NetworkID: netID, + }).Return(nil, nil) m.CreatePort(gomock.Any()).Times(0) }, wantErr: true, @@ -235,6 +247,10 @@ func Test_CreatePort(t *testing.T) { expectedCreateOpts = portsbinding.CreateOptsExt{ CreateOptsBuilder: expectedCreateOpts, } + m.ListPort(ports.ListOpts{ + Name: "test-port", + NetworkID: netID, + }).Return(nil, nil) m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { gotCreateOpts := builder.(portsbinding.CreateOptsExt) g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) @@ -277,6 +293,10 @@ func Test_CreatePort(t *testing.T) { expectedCreateOpts = portsbinding.CreateOptsExt{ CreateOptsBuilder: expectedCreateOpts, } + m.ListPort(ports.ListOpts{ + Name: "test-port", + NetworkID: netID, + }).Return(nil, nil) m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { gotCreateOpts := builder.(portsbinding.CreateOptsExt) g.Expect(gotCreateOpts).To(Equal(expectedCreateOpts), cmp.Diff(gotCreateOpts, expectedCreateOpts)) @@ -303,6 +323,10 @@ func Test_CreatePort(t *testing.T) { CreateOptsBuilder: expectedCreateOpts, } + m.ListPort(ports.ListOpts{ + Name: "test-port", + NetworkID: netID, + }).Return(nil, nil) // Create the port m.CreatePort(gomock.Any()).DoAndReturn(func(builder ports.CreateOptsBuilder) (*ports.Port, error) { gotCreateOpts := builder.(portsbinding.CreateOptsExt) @@ -349,7 +373,7 @@ func Test_CreatePort(t *testing.T) { s := Service{ client: mockClient, } - got, err := s.CreatePort( + got, err := s.EnsurePort( eventObject, &tt.port, ) diff --git a/test/e2e/data/e2e_conf.yaml b/test/e2e/data/e2e_conf.yaml index 8b4db452d2..694fb2006c 100644 --- a/test/e2e/data/e2e_conf.yaml +++ b/test/e2e/data/e2e_conf.yaml @@ -198,7 +198,7 @@ intervals: default/wait-cluster: ["20m", "10s"] default/wait-control-plane: ["30m", "10s"] default/wait-worker-nodes: ["30m", "10s"] - default/wait-delete-cluster: ["5m", "10s"] + default/wait-delete-cluster: ["10m", "10s"] default/wait-alt-az: ["20m", "30s"] default/wait-machine-upgrade: ["30m", "10s"] default/wait-nodes-ready: ["15m", "10s"]