From 591861ae157fff2fd2ec8d3fdc2473a3aa266fdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Wed, 19 Nov 2025 16:22:23 +0100 Subject: [PATCH 1/2] Fix instance deletion order Delete the OpenStack instance before deleting its network ports. This prevents "Policy doesn't allow compute:detach primary port" errors that occur when attempting to delete a primary NIC while the instance still exists. Deleting the instance automatically detaches all interfaces, so the explicit detach call is no longer needed. --- pkg/cloud/services/compute/instance.go | 33 +++++---------------- pkg/cloud/services/compute/instance_test.go | 7 ++--- 2 files changed, 10 insertions(+), 30 deletions(-) diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index 9583c42f73..25d58e6228 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/pkg/cloud/services/compute/instance.go @@ -657,12 +657,13 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve return err } - // get and delete trunks - for _, port := range instanceInterfaces { - if err = s.deleteAttachInterface(eventObject, instanceStatus.InstanceIdentifier(), port.PortID); err != nil { - return err - } + // Delete the instance before ports, since openstack likely prevents the deletion of the primary NIC. + // Deleting the instance also automatically detach the interfaces. + if err := s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier()); err != nil { + return err + } + for _, port := range instanceInterfaces { if trunkSupported { if err = networkingService.DeleteTrunk(eventObject, port.PortID); err != nil { return err @@ -685,7 +686,7 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve } } - return s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier()) + return nil } func (s *Service) deleteVolumes(instanceSpec *InstanceSpec) error { @@ -740,26 +741,6 @@ func (s *Service) deletePorts(eventObject runtime.Object, nets []servers.Network return nil } -func (s *Service) deleteAttachInterface(eventObject runtime.Object, instance *InstanceIdentifier, portID string) error { - err := s.getComputeClient().DeleteAttachedInterface(instance.ID, portID) - if err != nil { - if capoerrors.IsNotFound(err) { - record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Attach interface did not exist: instance %s, port %s", instance.ID, portID) - return nil - } - if capoerrors.IsConflict(err) { - // we don't want to block deletion because of Conflict - // due to instance must be paused/active/shutoff in order to detach interface - return nil - } - record.Warnf(eventObject, "FailedDeleteAttachInterface", "Failed to delete attach interface: instance %s, port %s: %v", instance.ID, portID, err) - return err - } - - record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Deleted attach interface: instance %s, port %s", instance.ID, portID) - return nil -} - func (s *Service) deleteInstance(eventObject runtime.Object, instance *InstanceIdentifier) error { err := s.getComputeClient().DeleteServer(instance.ID) if err != nil { diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index 74c4298361..320624972a 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -1032,13 +1032,12 @@ func TestService_DeleteInstance(t *testing.T) { Alias: "trunk", }, }}, nil) - r.compute.DeleteAttachedInterface(instanceUUID, portUUID).Return(nil) + r.compute.DeleteServer(instanceUUID).Return(nil) + r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{}) + // FIXME: Why we are looking for a trunk when we know the port is not trunked? r.network.ListTrunk(trunks.ListOpts{PortID: portUUID}).Return([]trunks.Trunk{}, nil) r.network.DeletePort(portUUID).Return(nil) - - r.compute.DeleteServer(instanceUUID).Return(nil) - r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{}) }, wantErr: false, }, From 29cec45108cee5d7685d4d78d52c4a4f7e455be8 Mon Sep 17 00:00:00 2001 From: Dan Childers Date: Tue, 2 Jun 2026 17:32:30 -0400 Subject: [PATCH 2/2] fix: check for standard-attr-tag extension before tagging ports Before setting tags on ports or trunks, check whether the neutron standard-attr-tag extension is available. If the extension is absent: - Return an error if port-specific tags were explicitly requested - Skip tagging if instance tags are present, logging a warning Adds getStdAttrTagSupport() helper to list neutron extensions and check for the standard-attr-tag alias, with unit tests covering both supported and unsupported extension scenarios. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: Dan Childers --- pkg/cloud/services/networking/port.go | 29 +++++- pkg/cloud/services/networking/port_test.go | 100 +++++++++++++++++++++ pkg/cloud/services/networking/service.go | 15 ++++ 3 files changed, 140 insertions(+), 4 deletions(-) diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index dd65f30333..e2e061df47 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -169,7 +169,25 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string var tags []string tags = append(tags, instanceTags...) tags = append(tags, portOpts.Tags...) - if len(tags) > 0 { + + tagsAreSupported, err := s.getStdAttrTagSupport() + + if err != nil { + record.Warnf(eventObject, "FailedGetTagSupport", "Failed to verify neutron support for the standard-attr-tag extension, skipping tags: %v", err) + } + + // Return error if user sets port tags; Skip over but warn for instance tags + if !tagsAreSupported { + if len(portOpts.Tags) > 0 { + err = fmt.Errorf("cannot tag port %s using port tags; tags are configured but neutron does not support the standard-attr-tag extension", portName) + record.Warnf(eventObject, "FailedTagPort", "Failed to tag port %s: %v", portName, err) + return nil, err + } + + record.Warnf(eventObject, "FailedTagPort", "Neutron does not support the standard-attr-tag extension, skipping instance tags for port %s", portName) + } + + if len(tags) > 0 && tagsAreSupported { 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 @@ -182,9 +200,12 @@ func (s *Service) GetOrCreatePort(eventObject runtime.Object, clusterName string 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 + + if tagsAreSupported { + 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 + } } } diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index c0756296dc..0f0bf0acf8 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -17,9 +17,12 @@ limitations under the License. package networking import ( + "errors" "testing" "github.com/golang/mock/gomock" + common "github.com/gophercloud/gophercloud/openstack/common/extensions" + "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsbinding" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity" @@ -149,6 +152,7 @@ func Test_GetOrCreatePort(t *testing.T) { AllowedAddressPairs: []ports.AddressPair{}, }, }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) }, &ports.Port{ID: portID1}, false, @@ -232,6 +236,7 @@ func Test_GetOrCreatePort(t *testing.T) { Return(&ports.Port{ ID: portID1, }, nil) + m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil) m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-port-tag"}}).Return([]string{"my-port-tag"}, nil) m. ListSubnet(subnets.ListOpts{ @@ -323,6 +328,7 @@ func Test_GetOrCreatePort(t *testing.T) { }, }, ).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) }, &ports.Port{ID: portID1}, false, @@ -352,6 +358,7 @@ func Test_GetOrCreatePort(t *testing.T) { AllowedAddressPairs: []ports.AddressPair{}, }, }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil) m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag"}}).Return([]string{"my-instance-tag"}, nil) }, &ports.Port{ID: portID1}, @@ -383,6 +390,7 @@ func Test_GetOrCreatePort(t *testing.T) { AllowedAddressPairs: []ports.AddressPair{}, }, }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil) m. ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-instance-tag", "my-port-tag"}}). Return([]string{"my-instance-tag", "my-port-tag"}, nil) @@ -429,12 +437,102 @@ func Test_GetOrCreatePort(t *testing.T) { Description: "Created by cluster-api-provider-openstack cluster test-cluster", }).Return(&trunks.Trunk{ID: trunkID}, nil) + m.ListExtensions().Return([]extensions.Extension{{Extension: common.Extension{Alias: "standard-attr-tag"}}}, nil) m.ReplaceAllAttributesTags("ports", portID1, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil) m.ReplaceAllAttributesTags("trunks", trunkID, attributestags.ReplaceAllOpts{Tags: []string{"my-tag"}}).Return([]string{"my-tag"}, nil) }, &ports.Port{Name: "foo-port-1", ID: portID1}, false, }, + { + "skips instance tags when standard-attr-tag extension is not supported", + "foo-port-1", + infrav1.PortOpts{ + Network: &infrav1.NetworkFilter{ + ID: netID, + }, + }, + nil, + []string{"my-instance-tag"}, + func(m *mock.MockNetworkClientMockRecorder) { + m. + ListPort(ports.ListOpts{ + Name: "foo-port-1", + NetworkID: netID, + }).Return([]ports.Port{}, nil) + m.CreatePort(portsbinding.CreateOptsExt{ + CreateOptsBuilder: ports.CreateOpts{ + Name: "foo-port-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: netID, + AllowedAddressPairs: []ports.AddressPair{}, + }, + }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) + }, + &ports.Port{ID: portID1}, + false, + }, + { + "errors when port tags are configured but standard-attr-tag extension is not supported", + "foo-port-1", + infrav1.PortOpts{ + Network: &infrav1.NetworkFilter{ + ID: netID, + }, + Tags: []string{"my-port-tag"}, + }, + nil, + nil, + func(m *mock.MockNetworkClientMockRecorder) { + m. + ListPort(ports.ListOpts{ + Name: "foo-port-1", + NetworkID: netID, + }).Return([]ports.Port{}, nil) + m.CreatePort(portsbinding.CreateOptsExt{ + CreateOptsBuilder: ports.CreateOpts{ + Name: "foo-port-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: netID, + AllowedAddressPairs: []ports.AddressPair{}, + }, + }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) + }, + nil, + true, + }, + { + "errors when port tags are configured and ListExtensions fails", + "foo-port-1", + infrav1.PortOpts{ + Network: &infrav1.NetworkFilter{ + ID: netID, + }, + Tags: []string{"my-port-tag"}, + }, + nil, + nil, + func(m *mock.MockNetworkClientMockRecorder) { + m. + ListPort(ports.ListOpts{ + Name: "foo-port-1", + NetworkID: netID, + }).Return([]ports.Port{}, nil) + m.CreatePort(portsbinding.CreateOptsExt{ + CreateOptsBuilder: ports.CreateOpts{ + Name: "foo-port-1", + Description: "Created by cluster-api-provider-openstack cluster test-cluster", + NetworkID: netID, + AllowedAddressPairs: []ports.AddressPair{}, + }, + }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return(nil, errors.New("connection refused")) + }, + nil, + true, + }, { "creates port with value_specs", "foo-port-1", @@ -469,6 +567,7 @@ func Test_GetOrCreatePort(t *testing.T) { ValueSpecs: &valueSpecs, }, }).Return(&ports.Port{ID: portID1}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) }, &ports.Port{ID: portID1}, false, @@ -502,6 +601,7 @@ func Test_GetOrCreatePort(t *testing.T) { PropagateUplinkStatus: pointerToTrue, }, }).Return(&ports.Port{ID: portID1, PropagateUplinkStatus: *pointerToTrue}, nil) + m.ListExtensions().Return([]extensions.Extension{}, nil) }, &ports.Port{ID: portID1, PropagateUplinkStatus: *pointerToTrue}, false, diff --git a/pkg/cloud/services/networking/service.go b/pkg/cloud/services/networking/service.go index b0a9b8da17..fc27c6bb07 100644 --- a/pkg/cloud/services/networking/service.go +++ b/pkg/cloud/services/networking/service.go @@ -90,3 +90,18 @@ func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, resourceT record.Eventf(eventObject, "SuccessfulReplaceAllAttributeTags", "Replaced all attributestags for %s with tags %s", resourceID, uniqueTags) return nil } + +// Checks if neutron supports Standard Attributes Tag Extension +func (s *Service) getStdAttrTagSupport() (bool, error) { + allExts, err := s.client.ListExtensions() + if err != nil { + return false, err + } + + for _, ext := range allExts { + if ext.Alias == "standard-attr-tag" { + return true, nil + } + } + return false, nil +}