Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Ensure that existing ports also have correct tags and trunks #2256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛 Ensure that existing ports also have correct tags and trunks #2256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions controllers/openstackserver_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 2 additions & 0 deletions controllers/openstackserver_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,7 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) {
listDefaultPortsNotFound(r)
createDefaultPort(r)
listDefaultServerNotFound(r)
listDefaultPortsNotFound(r)
createDefaultServer(r)
},
},
Expand All @@ -499,6 +500,7 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) {
},
},
expect: func(r *recorders) {
listDefaultPorts(r)
lentzi90 marked this conversation as resolved.
Show resolved Hide resolved
listDefaultPorts(r)
listDefaultServerFound(r)
},
Expand Down
78 changes: 51 additions & 27 deletions pkg/cloud/services/networking/port.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Comment on lines +129 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this going to call replaceAllAttributesTags on every reconcile, regardless of whether they're already set?

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,
})
Comment on lines +152 to +155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the port IDs of ports we've already created. Can we fetch by port ID? We should fall back to fetch by name in case we created a port but failed to write it to the status, but ideally fetch by ID would be the default here.

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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
32 changes: 28 additions & 4 deletions pkg/cloud/services/networking/port_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand All @@ -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,
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -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)
Expand Down Expand Up @@ -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,
)
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/data/e2e_conf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
lentzi90 marked this conversation as resolved.
Show resolved Hide resolved
default/wait-alt-az: ["20m", "30s"]
default/wait-machine-upgrade: ["30m", "10s"]
default/wait-nodes-ready: ["15m", "10s"]
Expand Down