From a0063ed1e7d9365689b7e686a8ab901fde101a51 Mon Sep 17 00:00:00 2001 From: Huy Mai Date: Wed, 13 Nov 2024 13:29:32 +0200 Subject: [PATCH] Fix port fetch should check for missing trunk Signed-off-by: Huy Mai --- pkg/cloud/services/compute/instance_test.go | 14 +++++- pkg/cloud/services/networking/port.go | 53 +++++++++++++-------- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index 74c4298361..7b418ea2b9 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -366,7 +366,6 @@ func TestService_ReconcileInstance(t *testing.T) { Description: portOpts["description"].(string), }, nil }) - networkRecorder.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) } // Expected calls if we delete the network port @@ -460,6 +459,7 @@ func TestService_ReconcileInstance(t *testing.T) { expectDefaultImageAndFlavor(r.compute, r.image) expectCreateServer(r.compute, getDefaultServerMap(), false) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectServerPollSuccess(r.compute) }, wantErr: false, @@ -472,6 +472,7 @@ func TestService_ReconcileInstance(t *testing.T) { expectDefaultImageAndFlavor(r.compute, r.image) expectCreateServer(r.compute, getDefaultServerMap(), true) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) // Make sure we delete ports expectCleanupDefaultPort(r.network) @@ -491,6 +492,7 @@ func TestService_ReconcileInstance(t *testing.T) { expect: func(r *recorders) { expectDefaultImageAndFlavor(r.compute, r.image) expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) // Looking up the second port fails r.network.ListPort(ports.ListOpts{ @@ -511,6 +513,7 @@ func TestService_ReconcileInstance(t *testing.T) { expectDefaultImageAndFlavor(r.compute, r.image) expectCreateServer(r.compute, getDefaultServerMap(), false) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectServerPoll(r.compute, []string{"BUILDING", "ACTIVE"}) }, wantErr: false, @@ -523,6 +526,7 @@ func TestService_ReconcileInstance(t *testing.T) { expectDefaultImageAndFlavor(r.compute, r.image) expectCreateServer(r.compute, getDefaultServerMap(), false) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectServerPoll(r.compute, []string{"BUILDING", "ERROR"}) // Don't delete ports because the server is created: DeleteInstance will do it @@ -567,6 +571,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, } expectCreateServer(r.compute, createMap, false) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectServerPollSuccess(r.compute) // Don't delete ports because the server is created: DeleteInstance will do it @@ -614,6 +619,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, } expectCreateServer(r.compute, createMap, false) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectServerPollSuccess(r.compute) // Don't delete ports because the server is created: DeleteInstance will do it @@ -631,6 +637,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectDefaultImageAndFlavor(r.compute, r.image) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). @@ -679,6 +686,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectDefaultImageAndFlavor(r.compute, r.image) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). @@ -767,6 +775,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectDefaultImageAndFlavor(r.compute, r.image) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-etcd", openStackMachineName)}). @@ -836,6 +845,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectDefaultImageAndFlavor(r.compute, r.image) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-etcd", openStackMachineName)}). @@ -893,6 +903,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) expectDefaultImageAndFlavor(r.compute, r.image) // Make sure we delete ports @@ -918,6 +929,7 @@ func TestService_ReconcileInstance(t *testing.T) { r.network.ListExtensions().Return(extensions, nil) expectCreatePort(r.network, portName, networkUUID) + r.network.ReplaceAllAttributesTags("ports", portUUID, attributestags.ReplaceAllOpts{Tags: []string{"test-tag"}}).Return(nil, nil) // Check for existing trunk r.network.ListTrunk(newGomegaMockMatcher( diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index dd65f30333..86e30b1008 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -54,6 +54,32 @@ func (s *Service) GetPortFromInstanceIP(instanceID string, ip string) ([]ports.P return s.client.ListPort(portOpts) } +// ensurePortTagsAndTrunk tags the specified port and checks if a trunk is needed. +// If required, it creates and tags the trunk. +func (s *Service) ensurePortTagsAndTrunk(eventObject runtime.Object, clusterName string, port *ports.Port, portOpts *infrav1.PortOpts, instanceTags []string) error { + var tags []string + tags = append(tags, instanceTags...) + tags = append(tags, portOpts.Tags...) + if len(tags) > 0 { + if err := s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", port.Name, err) + return err + } + } + if portOpts.Trunk != nil && *portOpts.Trunk { + trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID) + 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, tags); err != nil { + record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", port.Name, err) + return err + } + } + return nil +} + func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, instanceSecurityGroups []string, instanceTags []string) (*ports.Port, error) { networkID := portOpts.Network.ID @@ -66,7 +92,11 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string } if len(existingPorts) == 1 { - return &existingPorts[0], nil + port := &existingPorts[0] + if err := s.ensurePortTagsAndTrunk(eventObject, clusterName, port, portOpts, instanceTags); err != nil { + return nil, err + } + return port, nil } if len(existingPorts) > 1 { @@ -166,27 +196,10 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string return nil, err } - var tags []string - tags = append(tags, instanceTags...) - tags = append(tags, portOpts.Tags...) - if len(tags) > 0 { - if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace port tags %s: %v", portName, err) - return nil, err - } + if err := s.ensurePortTagsAndTrunk(eventObject, clusterName, port, portOpts, instanceTags); err != nil { + return nil, err } record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID) - if portOpts.Trunk != nil && *portOpts.Trunk { - trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID) - if err != nil { - record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err) - return nil, err - } - if err = s.replaceAllAttributesTags(eventObject, trunkResource, trunk.ID, tags); err != nil { - record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err) - return nil, err - } - } return port, nil }