🐛 Fix sub-ports not deleted with trunks #2081

mquhuy · 2024-05-15T06:53:00Z

What this PR does / why we need it:
Sometimes, when a trunk is associated with sub ports, deletion of the trunk will not lead to deletion of the subports, which is not expected. This PR adds the check to ensure the deletion of the subports in such cases.

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 · 2024-05-15T06:53:09Z

Hi @mquhuy. 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-sigs/prow repository.

netlify · 2024-05-15T06:53:19Z

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

Name	Link
🔨 Latest commit	`dabce29`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/664ef05dec2d4d0009e3c25e
😎 Deploy Preview	https://deploy-preview-2081--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.

lentzi90 · 2024-05-15T07:42:32Z

/ok-to-test

lentzi90

Thanks for this, it looks pretty good!
Please make lint-update to fix the linting error.

pkg/clients/networking.go

mquhuy · 2024-05-15T09:33:56Z

/retest

mquhuy · 2024-05-15T11:47:52Z

/cc @mdbooth

mdbooth

Thanks, looks good. Just a request to move some code for consistency.

pkg/clients/networking.go

pkg/cloud/services/networking/trunk.go

mquhuy · 2024-05-15T13:50:39Z

Thank you @mdbooth! I have to go now, but I'll make the change asap tomorrow.

mdbooth

I'm happy to merge this, but I recommend considering how best to test it. Some unit tests covering error cases might be one way. I think (but I haven't checked) we create a port with a trunk in one of the e2e tests. Perhaps we could also add a subport to it, which would cause the test to fail if it isn't cleaned up properly. Just some ideas.

mdbooth · 2024-05-21T08:04:57Z

pkg/cloud/services/networking/trunk.go

@@ -77,6 +79,44 @@ func (s *Service) getOrCreateTrunkForPort(eventObject runtime.Object, port *port
 	return trunk, nil
 }

+func (s *Service) RemoveTrunkSubports(trunkID string) error {
+	mc := metrics.NewMetricPrometheusContext("trunk", "deletesubports")


We really need a metrics strategy. I'd probably leave this out, tbh, but in absence of any related policy I'm not going to object.

It looks to me like this is a duplicate. We are already getting the metrics in networking.go above.
Can you remove this @mquhuy ?

Yes, sure. Coming up

k8s-ci-robot · 2024-05-21T08:22:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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

lentzi90

This looks good, except the duplicate metrics

lentzi90 · 2024-05-22T09:10:04Z

pkg/cloud/services/networking/trunk.go

@@ -77,6 +79,44 @@ func (s *Service) getOrCreateTrunkForPort(eventObject runtime.Object, port *port
 	return trunk, nil
 }

+func (s *Service) RemoveTrunkSubports(trunkID string) error {
+	mc := metrics.NewMetricPrometheusContext("trunk", "deletesubports")


It looks to me like this is a duplicate. We are already getting the metrics in networking.go above.
Can you remove this @mquhuy ?

mquhuy · 2024-05-23T07:18:12Z

I'm happy to merge this, but I recommend considering how best to test it. Some unit tests covering error cases might be one way. I think (but I haven't checked) we create a port with a trunk in one of the e2e tests. Perhaps we could also add a subport to it, which would cause the test to fail if it isn't cleaned up properly. Just some ideas.

Thank you. I'm wondering if this is the one you were thinking of? https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/pkg/cloud/services/networking/port_test.go#L309

I'm not sure if I understand it correctly, but it seems to me the test is only checking if the port could be created. Do you think we should also test if it can be cleaned? If it turned out to be in a bigger scope, I'd be happy to do it in another PR.

Signed-off-by: Huy Mai <[email protected]>

lentzi90

/lgtm

mquhuy · 2024-05-23T08:06:42Z

/unhold

mdbooth · 2024-05-23T10:03:05Z

I'm happy to merge this, but I recommend considering how best to test it. Some unit tests covering error cases might be one way. I think (but I haven't checked) we create a port with a trunk in one of the e2e tests. Perhaps we could also add a subport to it, which would cause the test to fail if it isn't cleaned up properly. Just some ideas.

Thank you. I'm wondering if this is the one you were thinking of? https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/pkg/cloud/services/networking/port_test.go#L309

I'm not sure if I understand it correctly, but it seems to me the test is only checking if the port could be created. Do you think we should also test if it can be cleaned? If it turned out to be in a bigger scope, I'd be happy to do it in another PR.

Looks like that's only testing port creation. You might want a new unit test for the delete flow. But something similar to that.

For the e2e test I was thinking of this:

cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_test.go

Lines 447 to 455 in 046d6b7

    
           // assert trunked port is created. 
        
           Eventually(func() int { 
        
           	plist, err = shared.DumpOpenStackPorts(e2eCtx, ports.ListOpts{Description: "trunked", Tags: testTag}) 
        
           	Expect(err).To(BeNil()) 
        
           	return len(plist) 
        
           }, e2eCtx.E2EConfig.GetIntervals(specName, "wait-worker-nodes")...).Should(Equal(1)) 
        
           port = plist[0] 
        
           Expect(port.Description).To(Equal("trunked")) 
        
           Expect(port.Tags).To(ContainElement(testTag))

If we updated the test to add a subport to that trunk we'd presumably get an error if the test didn't clean it up properly.

mquhuy · 2024-08-21T14:21:01Z

/cherrypick release-0.9
/cherrypick release-0.10

k8s-infra-cherrypick-robot · 2024-08-21T14:21:10Z

@mquhuy: only kubernetes-sigs org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherrypick release-0.9
/cherrypick release-0.10

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-sigs/prow repository.

…ase-0.9 🌱 Backport #2081 and #2141 to release-0.9

…ase-0.10 🌱 Backport #2081 and #2141 to release-0.10

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. labels May 15, 2024

k8s-ci-robot requested review from dulek and lentzi90 May 15, 2024 06:53

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 15, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2024

lentzi90 reviewed May 15, 2024

View reviewed changes

pkg/clients/networking.go Outdated Show resolved Hide resolved

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch 3 times, most recently from 86c9d6e to 796af8e Compare May 15, 2024 08:44

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from 796af8e to 2d230c5 Compare May 15, 2024 10:10

mquhuy requested a review from lentzi90 May 15, 2024 11:40

k8s-ci-robot requested a review from mdbooth May 15, 2024 11:47

mdbooth requested changes May 15, 2024

View reviewed changes

pkg/clients/networking.go Outdated Show resolved Hide resolved

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

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from 2d230c5 to 4e979cc Compare May 16, 2024 05:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2024

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from 4e979cc to c23bf37 Compare May 16, 2024 05:24

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2024

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from c23bf37 to c64a89d Compare May 16, 2024 05:55

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2024

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch 6 times, most recently from 5f01e18 to 34406b5 Compare May 20, 2024 14:23

mquhuy requested a review from mdbooth May 20, 2024 14:41

mdbooth approved these changes May 21, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2024

lentzi90 reviewed May 22, 2024

View reviewed changes

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from 34406b5 to 4a91e03 Compare May 23, 2024 07:19

Fix sub-ports not deleted with trunks

dabce29

Signed-off-by: Huy Mai <[email protected]>

mquhuy force-pushed the mquhuy/ensure-trunkport-deletion branch from 4a91e03 to dabce29 Compare May 23, 2024 07:29

lentzi90 reviewed May 23, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 May 23, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2024

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

k8s-ci-robot merged commit 7a19fb6 into kubernetes-sigs:main May 23, 2024
9 checks passed

mquhuy mentioned this pull request Jul 11, 2024

🌱 E2E: Add subports to trunked port to test subports deletion #2141

Merged

This was referenced Aug 21, 2024

🌱 Backport #2081 and #2141 to release-0.9 #2164

Merged

🌱 Backport #2081 and #2141 to release-0.10 #2165

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 27, 2024

Merge pull request #2164 from Nordix/mquhuy/fix-subport-deletion-rele…

80473c6

…ase-0.9 🌱 Backport #2081 and #2141 to release-0.9

k8s-ci-robot added a commit that referenced this pull request Aug 27, 2024

Merge pull request #2165 from Nordix/mquhuy/fix-subport-deletion-rele…

3601912

…ase-0.10 🌱 Backport #2081 and #2141 to release-0.10

lentzi90 deleted the mquhuy/ensure-trunkport-deletion branch November 26, 2024 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix sub-ports not deleted with trunks #2081

🐛 Fix sub-ports not deleted with trunks #2081

mquhuy commented May 15, 2024

k8s-ci-robot commented May 15, 2024

netlify bot commented May 15, 2024 •

edited

Loading

lentzi90 commented May 15, 2024

lentzi90 left a comment

mquhuy commented May 15, 2024

mquhuy commented May 15, 2024

mdbooth left a comment

mquhuy commented May 15, 2024

mdbooth left a comment

mdbooth May 21, 2024

lentzi90 May 22, 2024

mquhuy May 23, 2024

k8s-ci-robot commented May 21, 2024

lentzi90 left a comment

lentzi90 May 22, 2024

mquhuy commented May 23, 2024

lentzi90 left a comment

mquhuy commented May 23, 2024

mdbooth commented May 23, 2024

mquhuy commented Aug 21, 2024

k8s-infra-cherrypick-robot commented Aug 21, 2024

🐛 Fix sub-ports not deleted with trunks #2081

🐛 Fix sub-ports not deleted with trunks #2081

Conversation

mquhuy commented May 15, 2024

k8s-ci-robot commented May 15, 2024

netlify bot commented May 15, 2024 • edited Loading

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

lentzi90 commented May 15, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

mquhuy commented May 15, 2024

mquhuy commented May 15, 2024

mdbooth left a comment

Choose a reason for hiding this comment

mquhuy commented May 15, 2024

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth May 21, 2024

Choose a reason for hiding this comment

lentzi90 May 22, 2024

Choose a reason for hiding this comment

mquhuy May 23, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented May 21, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

lentzi90 May 22, 2024

Choose a reason for hiding this comment

mquhuy commented May 23, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

mquhuy commented May 23, 2024

mdbooth commented May 23, 2024

mquhuy commented Aug 21, 2024

k8s-infra-cherrypick-robot commented Aug 21, 2024

netlify bot commented May 15, 2024 •

edited

Loading