Only replace tags when needed

Signed-off-by: Lennart Jern <[email protected]>

Author: lentzi90

controllers/openstackserver_controller_test.go

@@ -114,6 +114,18 @@ var listDefaultPorts = func(r *recorders) {
 	}, nil)
 }
 
+var listDefaultPortsWithID = func(r *recorders) {
+	r.network.ListPort(ports.ListOpts{
+		Name:      openStackServerName + "-0",
+		ID:        portUUID,
+		NetworkID: networkUUID,
+	}).Return([]ports.Port{
+		{
+			ID: portUUID,
+		},
+	}, nil)
+}
+
 var listDefaultPortsNotFound = func(r *recorders) {
 	r.network.ListPort(ports.ListOpts{
 		Name:      openStackServerName + "-0",
@@ -501,7 +513,7 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) {
 			},
 			expect: func(r *recorders) {
 				listDefaultPorts(r)
-				listDefaultPorts(r)
+				listDefaultPortsWithID(r)
 				listDefaultServerFound(r)
 			},
 		},

pkg/cloud/services/networking/port.go

@@ -126,9 +126,12 @@ func (s *Service) GetPortForExternalNetwork(instanceID string, externalNetworkID
 
 // 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)
+	wantedTags := uniqueSortedTags(portSpec.Tags)
+	actualTags := uniqueSortedTags(port.Tags)
+	// Only replace tags if there is a difference
+	if !slices.Equal(wantedTags, actualTags) && len(wantedTags) > 0 {
+		if err := s.replaceAllAttributesTags(eventObject, portResource, port.ID, wantedTags); err != nil {
+			record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", port.Name, err)
 			return err
 		}
 	}
@@ -138,21 +141,30 @@ func (s *Service) ensurePortTagsAndTrunk(port *ports.Port, eventObject runtime.O
 			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
+
+		if !slices.Equal(wantedTags, trunk.Tags) {
+			if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, wantedTags); 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{
+// and that the port has suitable tags and trunk. If the PortStatus is already known,
+// use the ID when filtering for existing ports.
+func (s *Service) EnsurePort(eventObject runtime.Object, portSpec *infrav1.ResolvedPortSpec, portStatus infrav1.PortStatus) (*ports.Port, error) {
+	opts := ports.ListOpts{
 		Name:      portSpec.Name,
 		NetworkID: portSpec.NetworkID,
-	})
+	}
+	if portStatus.ID != "" {
+		opts.ID = portStatus.ID
+	}
+
+	existingPorts, err := s.client.ListPort(opts)
 	if err != nil {
 		return nil, fmt.Errorf("searching for existing port for server: %v", err)
 	}
@@ -359,16 +371,27 @@ func getPortName(baseName string, portSpec *infrav1.PortOpts, netIndex int) stri
 // 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 {
+	for i := range desiredPorts {
+		// If we already created the port, make use of the status
+		portStatus := infrav1.PortStatus{}
+		if i < len(resources.Ports) {
+			portStatus = resources.Ports[i]
+		}
 		// Events are recorded in EnsurePort
-		port, err := s.EnsurePort(eventObject, &portSpec)
+		port, err := s.EnsurePort(eventObject, &desiredPorts[i], portStatus)
 		if err != nil {
 			return err
 		}
 
-		resources.Ports = append(resources.Ports, infrav1.PortStatus{
-			ID: port.ID,
-		})
+		// If we already have the status, replace it,
+		// otherwise append it.
+		if i < len(resources.Ports) {
+			resources.Ports[i] = portStatus
+		} else {
+			resources.Ports = append(resources.Ports, infrav1.PortStatus{
+				ID: port.ID,
+			})
+		}
 	}
 
 	return nil
@@ -628,3 +651,19 @@ func (s *Service) AdoptPortsServer(scope *scope.WithLogger, desiredPorts []infra
 
 	return nil
 }
+
+// uniqueSortedTags returns a new, sorted slice where any duplicates have been removed.
+func uniqueSortedTags(tags []string) []string {
+	// remove duplicate values from tags
+	tagsMap := make(map[string]string)
+	for _, t := range tags {
+		tagsMap[t] = t
+	}
+
+	uniqueTags := []string{}
+	for k := range tagsMap {
+		uniqueTags = append(uniqueTags, k)
+	}
+	slices.Sort(uniqueTags)
+	return uniqueTags
+}

pkg/cloud/services/networking/port_test.go

@@ -306,7 +306,7 @@ func Test_EnsurePort(t *testing.T) {
 			want: &ports.Port{ID: portID},
 		},
 		{
-			name: "tags and trunk",
+			name: "create port with tags and trunk",
 			port: infrav1.ResolvedPortSpec{
 				Name:      "test-port",
 				NetworkID: netID,
@@ -358,6 +358,87 @@ func Test_EnsurePort(t *testing.T) {
 			},
 			want: &ports.Port{ID: portID, Name: "test-port"},
 		},
+		{
+			name: "port with tags and trunk already exists",
+			port: infrav1.ResolvedPortSpec{
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+				Trunk:     ptr.To(true),
+			},
+			expect: func(m *mock.MockNetworkClientMockRecorder, _ types.Gomega) {
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return([]ports.Port{{
+					ID:        portID,
+					Name:      "test-port",
+					NetworkID: netID,
+					Tags:      []string{"tag1", "tag2"},
+				}}, nil)
+
+				// Look for existing trunk
+				m.ListTrunk(trunks.ListOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return([]trunks.Trunk{{
+					ID:   trunkID,
+					Tags: []string{"tag1", "tag2"},
+				}}, nil)
+			},
+			want: &ports.Port{
+				ID:        portID,
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+			},
+		},
+		{
+			name: "partial port missing tags and trunk",
+			port: infrav1.ResolvedPortSpec{
+				Name:      "test-port",
+				NetworkID: netID,
+				Tags:      []string{"tag1", "tag2"},
+				Trunk:     ptr.To(true),
+			},
+			expect: func(m *mock.MockNetworkClientMockRecorder, _ types.Gomega) {
+				m.ListPort(ports.ListOpts{
+					Name:      "test-port",
+					NetworkID: netID,
+				}).Return([]ports.Port{{
+					ID:        portID,
+					Name:      "test-port",
+					NetworkID: netID,
+				}}, nil)
+
+				// Tag the port
+				m.ReplaceAllAttributesTags("ports", portID, attributestags.ReplaceAllOpts{
+					Tags: []string{"tag1", "tag2"},
+				})
+
+				// Look for existing trunk
+				m.ListTrunk(trunks.ListOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return([]trunks.Trunk{}, nil)
+
+				// Create the trunk
+				m.CreateTrunk(trunks.CreateOpts{
+					PortID: portID,
+					Name:   "test-port",
+				}).Return(&trunks.Trunk{ID: trunkID}, nil)
+
+				// Tag the trunk
+				m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{
+					Tags: []string{"tag1", "tag2"},
+				})
+			},
+			want: &ports.Port{
+				ID:        portID,
+				Name:      "test-port",
+				NetworkID: netID,
+			},
+		},
 	}
 
 	eventObject := &infrav1.OpenStackMachine{}
@@ -376,6 +457,7 @@ func Test_EnsurePort(t *testing.T) {
 			got, err := s.EnsurePort(
 				eventObject,
 				&tt.port,
+				infrav1.PortStatus{},
 			)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())

pkg/cloud/services/networking/service.go

@@ -18,7 +18,6 @@ package networking
 
 import (
 	"fmt"
-	"sort"
 
 	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -65,28 +64,15 @@ func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, resourceT
 		record.Warnf(eventObject, "FailedReplaceAllAttributesTags", "Invalid resourceType argument in function call")
 		panic(fmt.Errorf("invalid argument: resourceType, %s, does not match allowed arguments: %s or %s", resourceType, trunkResource, portResource))
 	}
-	// remove duplicate values from tags
-	tagsMap := make(map[string]string)
-	for _, t := range tags {
-		tagsMap[t] = t
-	}
-
-	uniqueTags := []string{}
-	for k := range tagsMap {
-		uniqueTags = append(uniqueTags, k)
-	}
-
-	// Sort the tags so that we always get fixed order of tags to make UT easier
-	sort.Strings(uniqueTags)
 
 	_, err := s.client.ReplaceAllAttributesTags(resourceType, resourceID, attributestags.ReplaceAllOpts{
-		Tags: uniqueTags,
+		Tags: tags,
 	})
 	if err != nil {
 		record.Warnf(eventObject, "FailedReplaceAllAttributesTags", "Failed to replace all attributestags, %s: %v", resourceID, err)
 		return err
 	}
 
-	record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, uniqueTags)
+	record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, tags)
 	return nil
 }

test/e2e/data/e2e_conf.yaml

@@ -1,4 +1,3 @@
----
 # E2E test scenario using local dev images and manifests built from the source tree for following providers:
 # - openstack
 
@@ -188,7 +187,7 @@ intervals:
   default/wait-cluster: ["20m", "10s"]
   default/wait-control-plane: ["30m", "10s"]
   default/wait-worker-nodes: ["30m", "10s"]
-  default/wait-delete-cluster: ["10m", "10s"]
+  default/wait-delete-cluster: ["5m", "10s"]
   default/wait-alt-az: ["20m", "30s"]
   default/wait-machine-upgrade: ["30m", "10s"]
   default/wait-nodes-ready: ["15m", "10s"]