Fix sub-ports not deleted with trunks

Signed-off-by: Huy Mai <huy.mai@est.tech>

Author: Huy Mai

controllers/openstackmachine_controller_test.go

@@ -265,6 +265,7 @@ func Test_reconcileDelete(t *testing.T) {
 		trunkExtension.Alias = "trunk"
 		r.network.ListExtensions().Return([]extensions.Extension{trunkExtension}, nil)
 		r.network.ListTrunk(trunks.ListOpts{PortID: portUUID}).Return([]trunks.Trunk{{ID: trunkUUID}}, nil)
+		r.network.ListTrunkSubports(trunkUUID).Return([]trunks.Subport{}, nil)
 		r.network.DeleteTrunk(trunkUUID).Return(nil)
 		r.network.DeletePort(portUUID).Return(nil)
 	}

pkg/clients/mock/network.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()

pkg/clients/networking.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) getOrCreateTrunkForPort(eventObject runtime.Object, port *port
 	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 {

pkg/cloud/services/networking/trunk.go

@@ -28,6 +28,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/gophercloud/gophercloud"
+	"github.com/gophercloud/gophercloud/openstack"
 	"github.com/gophercloud/gophercloud/openstack/blockstorage/v3/volumes"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/routers"
@@ -424,9 +426,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),
 			})
@@ -440,33 +443,134 @@ 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 {
 				plist, err = shared.DumpOpenStackPorts(e2eCtx, ports.ListOpts{Description: "trunked", Tags: testTag})
 				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())
 		})
 	})