🐛 Fix Port Leaks #1648

spjmurray · 2023-08-21T09:21:54Z

What this PR does / why we need it:

When an instance fails to come up, there's a bug where the port doesn't get cleaned up, and eventually you will exhaust your quota. The code does attempt to garbage collect, but I suspect at some point in the past ports were suffixed with an index, but the garbage collection code wasn't made aware of this, so lists nothing. Replace the APi "filtering" that doesn't work with a manual filter that does prefix matching.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1404

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

k8s-ci-robot · 2023-08-21T09:22:03Z

Hi @spjmurray. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2023-08-21T09:22:23Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`b77e660`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64e86dd52fa12d0008ac1e58
😎 Deploy Preview	https://deploy-preview-1648--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

spjmurray · 2023-08-21T12:06:44Z

pkg/cloud/services/compute/instance.go

@@ -380,14 +380,6 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste
 	return createdInstance, nil
 }

-// getPortName appends a suffix to an instance name in order to try and get a unique name per port.


Why did I move this you will inevitably ask? Well, problem is I want to associate the test with the function that causes the issue in the first place. Problem is I cannot just export this and include it because of circular dependencies. That led me to going, well this should be a networking_test package anyway, which then led to to not being able to create the networking service due to the client field not being exported, which then became ugly because of a hacky NewWithClientForTestOnly() function in the production code. Moving it was by far the simplest solution.

spjmurray · 2023-08-21T12:09:00Z

pkg/cloud/services/networking/port_test.go

+	instanceName := "foo"
+	portID1 := "dc6e0ae3-dad6-4240-a9cb-e541916f20d3"
+	portID2 := "a38ab1cb-c2cc-4c1b-9d1d-696ec73356d2"
+	portName1 := GetPortName(instanceName, nil, 0)


RE: above comment, I wanted to be absolutely sure that things generated with GetPortName will get offed. By adding a tight coupling between the test and the call, you'll be less likely to change one in a broken way without being drawn to the test cases, so a win in my opinion,

pkg/cloud/services/networking/port.go

jichenjc · 2023-08-22T00:34:39Z

/ok-to-test

spjmurray · 2023-08-23T09:04:10Z

@dulek I've probably broken everything, but this may be a "better", definitely far more complex solution, but I can always revert back to the KISS version.

So I've copied instance creation by generating an InstanceSpec, propagated that down so we can generate []PortOpts then passed that down to the garbage collector so we only check for specific ports that should exist.

dulek

Honestly I like this version more, thank you! One comment inline.

dulek · 2023-08-24T15:02:25Z

pkg/cloud/services/networking/port.go

+		for _, p := range portList {
+			if err := s.DeletePort(eventObject, p.ID); err != nil {
+				return err
+			}
+		}


We should probably error out if there are multiple ports of that name? Unless we can get to such a case in a single cluster scenario too?

Agreed, update incoming. And now this has been philosophically reviewed, I guess I had better test it! Luckily I have a 0.8.0 branch... back in a bit.

Good news! It does actually work.

When an instance fails to come up, there's a bug where the port doesn't get cleaned up, and eventually you will exhaust your quota. The code does attempt to garbage collect, but I suspect at some point in the past ports were suffixed with an index, but the garbage collection code wasn't made aware of this, so lists nothing. Replace the APi "filtering" that doesn't work with a better algorithm that uses the instance creation code to generate the expected ports, then generate the names and delete if the port exists. Also protected with some nice unit tests.

dulek · 2023-08-25T12:20:59Z

/lgtm

mdbooth · 2023-08-30T12:00:32Z

pkg/cloud/services/networking/port.go

+		// TODO: whould be nice if gophercloud could be persuaded to accept multiple
+		// names as is allowed by the API in order to reduce API traffic.


ping @pierreprinetti

If you'd like to resurrect that effort? The use cases are mounting.

k8s-ci-robot · 2023-08-30T12:05:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth, spjmurray

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mdbooth]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spjmurray · 2023-08-30T13:03:56Z

@mdbooth thanks! Should I unhold this or is that your job? Speaking of which... do we have a rough estimate for 0.8.0 GA?

mdbooth · 2023-08-30T13:37:47Z

@spjmurray You get the pleasure of removing your hold :) Unless I get bored waiting for you, that is, but that takes a couple of weeks.

spjmurray · 2023-08-30T15:32:00Z

/unhold

spjmurray · 2023-08-30T15:32:48Z

Not mine it's part of the PR template.

spjmurray · 2023-08-30T15:33:03Z

/pony

k8s-ci-robot · 2023-08-30T15:33:10Z

@spjmurray:

In response to this:

/pony

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pierreprinetti · 2023-08-31T10:28:31Z

Here's a proposal for "or" junctions in Gophercloud List queries: gophercloud/utils#190
Would this match your use-case?

I would also really appreciate candid feedback on how ergonomic my proposal would be. There are many different ways to add this capability to Gophercloud and this is just one of them.

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2023

k8s-ci-robot requested review from mdbooth and tobiasgiese August 21, 2023 09:22

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 21, 2023

spjmurray force-pushed the fix_port_leak branch from ada0c5b to 13d3d5c Compare August 21, 2023 11:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2023

spjmurray force-pushed the fix_port_leak branch from 13d3d5c to 512e572 Compare August 21, 2023 12:01

spjmurray commented Aug 21, 2023

View reviewed changes

dulek suggested changes Aug 21, 2023

View reviewed changes

pkg/cloud/services/networking/port.go Outdated Show resolved Hide resolved

spjmurray force-pushed the fix_port_leak branch from 3619d72 to e6a1dc4 Compare August 23, 2023 09:06

dulek suggested changes Aug 24, 2023

View reviewed changes

spjmurray force-pushed the fix_port_leak branch from e6a1dc4 to b77e660 Compare August 25, 2023 09:01

k8s-ci-robot assigned dulek Aug 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2023

mdbooth approved these changes Aug 30, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023

k8s-ci-robot merged commit 1384d61 into kubernetes-sigs:main Aug 30, 2023
4 checks passed

spjmurray deleted the fix_port_leak branch August 30, 2023 17:25

mdbooth mentioned this pull request Sep 6, 2023

Observed panic deleting cluster which was still initialising #1654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix Port Leaks #1648

🐛 Fix Port Leaks #1648

spjmurray commented Aug 21, 2023

k8s-ci-robot commented Aug 21, 2023

netlify bot commented Aug 21, 2023 •

edited

Loading

spjmurray Aug 21, 2023 •

edited

Loading

spjmurray Aug 21, 2023

jichenjc commented Aug 22, 2023

spjmurray commented Aug 23, 2023

dulek left a comment

dulek Aug 24, 2023

spjmurray Aug 25, 2023

spjmurray Aug 25, 2023

dulek commented Aug 25, 2023

mdbooth Aug 30, 2023

k8s-ci-robot commented Aug 30, 2023

spjmurray commented Aug 30, 2023

mdbooth commented Aug 30, 2023

spjmurray commented Aug 30, 2023

spjmurray commented Aug 30, 2023

spjmurray commented Aug 30, 2023

k8s-ci-robot commented Aug 30, 2023

pierreprinetti commented Aug 31, 2023

		// TODO: whould be nice if gophercloud could be persuaded to accept multiple
		// names as is allowed by the API in order to reduce API traffic.

🐛 Fix Port Leaks #1648

🐛 Fix Port Leaks #1648

Conversation

spjmurray commented Aug 21, 2023

k8s-ci-robot commented Aug 21, 2023

netlify bot commented Aug 21, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

spjmurray Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

spjmurray Aug 21, 2023

Choose a reason for hiding this comment

jichenjc commented Aug 22, 2023

spjmurray commented Aug 23, 2023

dulek left a comment

Choose a reason for hiding this comment

dulek Aug 24, 2023

Choose a reason for hiding this comment

spjmurray Aug 25, 2023

Choose a reason for hiding this comment

spjmurray Aug 25, 2023

Choose a reason for hiding this comment

dulek commented Aug 25, 2023

mdbooth Aug 30, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 30, 2023

spjmurray commented Aug 30, 2023

mdbooth commented Aug 30, 2023

spjmurray commented Aug 30, 2023

spjmurray commented Aug 30, 2023

spjmurray commented Aug 30, 2023

k8s-ci-robot commented Aug 30, 2023

pierreprinetti commented Aug 31, 2023

netlify bot commented Aug 21, 2023 •

edited

Loading

spjmurray Aug 21, 2023 •

edited

Loading