From 053a8b2844eb3b1a6ca6c95cd8cc561b9dc964a5 Mon Sep 17 00:00:00 2001 From: Huy Mai Date: Wed, 15 May 2024 09:50:31 +0300 Subject: [PATCH] Fix sub-ports not deleted with trunks Signed-off-by: Huy Mai --- pkg/clients/mock/network.go | 29 +++++ pkg/clients/networking.go | 21 ++++ pkg/cloud/services/compute/instance_test.go | 1 + pkg/cloud/services/networking/port_test.go | 1 + pkg/cloud/services/networking/trunk.go | 57 +++++++++- test/e2e/suites/e2e/e2e_test.go | 119 ++++++++++++++++++-- 6 files changed, 217 insertions(+), 11 deletions(-) diff --git a/pkg/clients/mock/network.go b/pkg/clients/mock/network.go index 091a89f3b7..045a93d3bc 100644 --- a/pkg/clients/mock/network.go +++ b/pkg/clients/mock/network.go @@ -546,6 +546,21 @@ func (mr *MockNetworkClientMockRecorder) ListTrunk(arg0 interface{}) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListTrunk", reflect.TypeOf((*MockNetworkClient)(nil).ListTrunk), arg0) } +// ListTrunkSubports mocks base method. +func (m *MockNetworkClient) ListTrunkSubports(arg0 string) ([]trunks.Subport, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListTrunkSubports", arg0) + ret0, _ := ret[0].([]trunks.Subport) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListTrunkSubports indicates an expected call of ListTrunkSubports. +func (mr *MockNetworkClientMockRecorder) ListTrunkSubports(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListTrunkSubports", reflect.TypeOf((*MockNetworkClient)(nil).ListTrunkSubports), arg0) +} + // RemoveRouterInterface mocks base method. func (m *MockNetworkClient) RemoveRouterInterface(arg0 string, arg1 routers.RemoveInterfaceOptsBuilder) (*routers.InterfaceInfo, error) { m.ctrl.T.Helper() @@ -561,6 +576,20 @@ func (mr *MockNetworkClientMockRecorder) RemoveRouterInterface(arg0, arg1 interf return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveRouterInterface", reflect.TypeOf((*MockNetworkClient)(nil).RemoveRouterInterface), arg0, arg1) } +// RemoveSubports mocks base method. +func (m *MockNetworkClient) RemoveSubports(arg0 string, arg1 trunks.RemoveSubportsOpts) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveSubports", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveSubports indicates an expected call of RemoveSubports. +func (mr *MockNetworkClientMockRecorder) RemoveSubports(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveSubports", reflect.TypeOf((*MockNetworkClient)(nil).RemoveSubports), arg0, arg1) +} + // ReplaceAllAttributesTags mocks base method. func (m *MockNetworkClient) ReplaceAllAttributesTags(arg0, arg1 string, arg2 attributestags.ReplaceAllOptsBuilder) ([]string, error) { m.ctrl.T.Helper() diff --git a/pkg/clients/networking.go b/pkg/clients/networking.go index 23ad3c8fdf..8967c7b34e 100644 --- a/pkg/clients/networking.go +++ b/pkg/clients/networking.go @@ -53,6 +53,9 @@ type NetworkClient interface { CreateTrunk(opts trunks.CreateOptsBuilder) (*trunks.Trunk, error) DeleteTrunk(id string) error + ListTrunkSubports(trunkID string) ([]trunks.Subport, error) + RemoveSubports(id string, opts trunks.RemoveSubportsOpts) error + ListRouter(opts routers.ListOpts) ([]routers.Router, error) CreateRouter(opts routers.CreateOptsBuilder) (*routers.Router, error) DeleteRouter(id string) error @@ -238,6 +241,24 @@ func (c networkClient) DeleteTrunk(id string) error { return mc.ObserveRequestIgnoreNotFound(trunks.Delete(c.serviceClient, id).ExtractErr()) } +func (c networkClient) ListTrunkSubports(trunkID string) ([]trunks.Subport, error) { + mc := metrics.NewMetricPrometheusContext("trunk", "listsubports") + subports, err := trunks.GetSubports(c.serviceClient, trunkID).Extract() + if mc.ObserveRequest(err) != nil { + return nil, err + } + return subports, nil +} + +func (c networkClient) RemoveSubports(id string, opts trunks.RemoveSubportsOpts) error { + mc := metrics.NewMetricPrometheusContext("trunk", "deletesubports") + _, err := trunks.RemoveSubports(c.serviceClient, id, opts).Extract() + if mc.ObserveRequest(err) != nil { + return err + } + return nil +} + func (c networkClient) ListTrunk(opts trunks.ListOptsBuilder) ([]trunks.Trunk, error) { mc := metrics.NewMetricPrometheusContext("trunk", "list") allPages, err := trunks.List(c.serviceClient, opts).AllPages() diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index cafc9e3edc..74c4298361 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -953,6 +953,7 @@ func TestService_ReconcileInstance(t *testing.T) { // We should cleanup the first port and its trunk r.network.DeleteTrunk(trunkUUID).Return(nil) + r.network.ListTrunkSubports(trunkUUID).Return([]trunks.Subport{}, nil) r.network.DeletePort(portUUID).Return(nil) }, wantErr: true, diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index eea07d36fc..c0756296dc 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -625,6 +625,7 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) { }, }, nil) m.DeleteTrunk(trunkID1) + m.ListTrunkSubports(trunkID1).Return([]trunks.Subport{}, nil) m.DeletePort(portID1) }, portOpts: []infrav1.PortOpts{ diff --git a/pkg/cloud/services/networking/trunk.go b/pkg/cloud/services/networking/trunk.go index 6743478ffd..374bcb6eb7 100644 --- a/pkg/cloud/services/networking/trunk.go +++ b/pkg/cloud/services/networking/trunk.go @@ -31,8 +31,9 @@ import ( ) const ( - timeoutTrunkDelete = 3 * time.Minute - retryIntervalTrunkDelete = 5 * time.Second + timeoutTrunkDelete = 3 * time.Minute + retryIntervalTrunkDelete = 5 * time.Second + retryIntervalSubportDelete = 30 * time.Second ) func (s *Service) GetTrunkSupport() (bool, error) { @@ -77,6 +78,42 @@ func (s *Service) getOrCreateTrunk(eventObject runtime.Object, clusterName, trun return trunk, nil } +func (s *Service) RemoveTrunkSubports(trunkID string) error { + subports, err := s.client.ListTrunkSubports(trunkID) + if err != nil { + return err + } + + if len(subports) == 0 { + return nil + } + + portList := make([]trunks.RemoveSubport, len(subports)) + for i, subport := range subports { + portList[i] = trunks.RemoveSubport{ + PortID: subport.PortID, + } + } + + removeSubportsOpts := trunks.RemoveSubportsOpts{ + Subports: portList, + } + + err = s.client.RemoveSubports(trunkID, removeSubportsOpts) + if err != nil { + return err + } + + for _, subPort := range subports { + err := s.client.DeletePort(subPort.PortID) + if err != nil { + return err + } + } + + return nil +} + func (s *Service) DeleteTrunk(eventObject runtime.Object, portID string) error { listOpts := trunks.ListOpts{ PortID: portID, @@ -88,6 +125,22 @@ func (s *Service) DeleteTrunk(eventObject runtime.Object, portID string) error { if len(trunkInfo) != 1 { return nil } + // Delete sub-ports if trunk is associated with sub-ports + err = wait.PollUntilContextTimeout(context.TODO(), retryIntervalSubportDelete, timeoutTrunkDelete, true, func(_ context.Context) (bool, error) { + if err := s.RemoveTrunkSubports(trunkInfo[0].ID); err != nil { + if capoerrors.IsNotFound(err) || capoerrors.IsConflict(err) || capoerrors.IsRetryable(err) { + return false, nil + } + return false, err + } + return true, nil + }) + if err != nil { + record.Warnf(eventObject, "FailedRemoveTrunkSubports", "Failed to delete sub ports trunk %s with id %s: %v", trunkInfo[0].Name, trunkInfo[0].ID, err) + return err + } + + record.Eventf(eventObject, "SuccessfulRemoveTrunkSubports", "Removed trunk sub ports %s with id %s", trunkInfo[0].Name, trunkInfo[0].ID) err = wait.PollUntilContextTimeout(context.TODO(), retryIntervalTrunkDelete, timeoutTrunkDelete, true, func(_ context.Context) (bool, error) { if err := s.client.DeleteTrunk(trunkInfo[0].ID); err != nil { diff --git a/test/e2e/suites/e2e/e2e_test.go b/test/e2e/suites/e2e/e2e_test.go index c851054749..bb135a580f 100644 --- a/test/e2e/suites/e2e/e2e_test.go +++ b/test/e2e/suites/e2e/e2e_test.go @@ -258,9 +258,10 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { // Note that as the bootstrap config does not have cloud.conf, the node will not be added to the cluster. // We still expect the port for the machine to be created. + machineDeployment := makeMachineDeployment(namespace.Name, md3Name, clusterName, "", 1) framework.CreateMachineDeployment(ctx, framework.CreateMachineDeploymentInput{ Creator: e2eCtx.Environment.BootstrapClusterProxy.GetClient(), - MachineDeployment: makeMachineDeployment(namespace.Name, md3Name, clusterName, "", 1), + MachineDeployment: machineDeployment, BootstrapConfigTemplate: makeJoinBootstrapConfigTemplate(namespace.Name, md3Name), InfraMachineTemplate: makeOpenStackMachineTemplateWithPortOptions(namespace.Name, clusterName, md3Name, customPortOptions, machineTags), }) @@ -274,9 +275,9 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { return len(plist) }, e2eCtx.E2EConfig.GetIntervals(specName, "wait-worker-nodes")...).Should(Equal(1)) - port := plist[0] - Expect(port.Description).To(Equal("primary")) - Expect(port.Tags).To(ContainElement(testTag)) + primaryPort := plist[0] + Expect(primaryPort.Description).To(Equal("primary")) + Expect(primaryPort.Tags).To(ContainElement(testTag)) // assert trunked port is created. Eventually(func() int { @@ -284,23 +285,123 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { Expect(err).To(BeNil()) return len(plist) }, e2eCtx.E2EConfig.GetIntervals(specName, "wait-worker-nodes")...).Should(Equal(1)) - port = plist[0] - Expect(port.Description).To(Equal("trunked")) - Expect(port.Tags).To(ContainElement(testTag)) + trunkedPort := plist[0] + Expect(trunkedPort.Description).To(Equal("trunked")) + Expect(trunkedPort.Tags).To(ContainElement(testTag)) // assert trunk data. var trunk *trunks.Trunk Eventually(func() int { - trunk, err = shared.DumpOpenStackTrunks(e2eCtx, port.ID) + trunk, err = shared.DumpOpenStackTrunks(e2eCtx, trunkedPort.ID) Expect(err).To(BeNil()) Expect(trunk).NotTo(BeNil()) return 1 }, e2eCtx.E2EConfig.GetIntervals(specName, "wait-worker-nodes")...).Should(Equal(1)) - Expect(trunk.PortID).To(Equal(port.ID)) + Expect(trunk.PortID).To(Equal(trunkedPort.ID)) + // assert port level security group is created by name using SecurityGroupFilters + securityGroupsList, err := shared.DumpOpenStackSecurityGroups(e2eCtx, groups.ListOpts{Name: testSecurityGroupName}) Expect(err).NotTo(HaveOccurred()) Expect(securityGroupsList).To(HaveLen(1)) + + // Testing subports + shared.Logf("Create a new port and add it as a subport of the trunk") + + providerClient, clientOpts, _, err := shared.GetTenantProviderClient(e2eCtx) + Expect(err).To(BeNil(), "Cannot create providerClient") + + networkClient, err := openstack.NewNetworkV2(providerClient, gophercloud.EndpointOpts{ + Region: clientOpts.RegionName, + }) + Expect(err).To(BeNil(), "Cannot create network client") + + networksList, err := shared.DumpOpenStackNetworks( + e2eCtx, + networks.ListOpts{ + TenantID: securityGroupsList[0].TenantID, + }, + ) + Expect(err).To(BeNil(), "Cannot get network List") + + createOpts := ports.CreateOpts{ + Name: "subPort", + NetworkID: networksList[0].ID, + } + + subPort, err := ports.Create(networkClient, createOpts).Extract() + Expect(err).To(BeNil(), "Cannot create subPort") + + addSubportsOpts := trunks.AddSubportsOpts{ + Subports: []trunks.Subport{ + { + SegmentationID: 1, + SegmentationType: "vlan", + PortID: subPort.ID, + }, + }, + } + shared.Logf("Add subport to trunk") + _, err = trunks.AddSubports(networkClient, trunk.ID, addSubportsOpts).Extract() + Expect(err).To(BeNil(), "Cannot add subports") + + subports, err := trunks.GetSubports(networkClient, trunk.ID).Extract() + Expect(err).To(BeNil()) + Expect(subports).To(HaveLen(1)) + + shared.Logf("Get machine object from MachineDeployments") + c := e2eCtx.Environment.BootstrapClusterProxy.GetClient() + + machines := framework.GetMachinesByMachineDeployments(ctx, framework.GetMachinesByMachineDeploymentsInput{ + Lister: c, + ClusterName: clusterName, + Namespace: namespace.Name, + MachineDeployment: *machineDeployment, + }) + + Expect(machines).To(HaveLen(1)) + + machine := machines[0] + + shared.Logf("Fetching serverID") + allServers, err := shared.DumpOpenStackServers(e2eCtx, servers.ListOpts{Name: machine.Spec.InfrastructureRef.Name}) + Expect(err).To(BeNil()) + Expect(allServers).To(HaveLen(1)) + serverID := allServers[0].ID + Expect(err).To(BeNil()) + + shared.Logf("Deleting the machine deployment, which should trigger trunk deletion") + + err = c.Delete(ctx, machineDeployment) + Expect(err).To(BeNil()) + + shared.Logf("Waiting for the server to be cleaned") + + computeClient, err := openstack.NewComputeV2(providerClient, gophercloud.EndpointOpts{ + Region: clientOpts.RegionName, + }) + Expect(err).To(BeNil(), "Cannot create compute client") + + Eventually( + func() bool { + _, err := servers.Get(computeClient, serverID).Extract() + _, ok := err.(gophercloud.ErrDefault404) + return ok + }, e2eCtx.E2EConfig.GetIntervals(specName, "wait-delete-cluster")..., + ).Should(BeTrue()) + + // Wait here for some time, to make sure the reconciler fully cleans everything + time.Sleep(10 * time.Second) + + // Verify that the trunk is deleted + _, err = trunks.Get(networkClient, trunk.ID).Extract() + _, ok := err.(gophercloud.ErrDefault404) + Expect(ok).To(BeTrue()) + + // Verify that subPort is deleted + _, err = ports.Get(networkClient, subPort.ID).Extract() + _, ok = err.(gophercloud.ErrDefault404) + Expect(ok).To(BeTrue()) }) })