search

kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
289 stars 253 forks source link

:bug: Fix sub-ports not deleted with trunks #2081

Closed mquhuy closed 4 months ago

mquhuy commented 4 months ago

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:

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

TODOs:

/hold

k8s-ci-robot commented 4 months ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
netlify[bot] commented 4 months ago

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

Name Link
Latest commit dabce293a11d9d12b65bf7a6f25241e2cdddd7f3
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...

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 commented 4 months ago

/ok-to-test

mquhuy commented 4 months ago

/retest

mquhuy commented 4 months ago

/cc @mdbooth

mquhuy commented 4 months ago

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

mdbooth commented 4 months ago

e2e test failures look like an infra-flake. test failures are most likely missing mock calls for the new delete subports flow.

Incidentally, I didn't check but I was assuming that because this is specifically in the Trunk delete flow we've already checked that trunks are expected, and therefore we're not making unnecessary OpenStack API calls for the majority of folks who aren't using trunks.

If this is the case, and the failing tests don't really need trunks, it might be simpler to update the test input object to disable trunks rather than adding trunk calls to the expected mocks.

mdbooth commented 4 months ago

I used this PR as a test for junit formatting if you want to look at something a bit prettier: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/2095/pull-cluster-api-provider-openstack-test/1792498916650913792

k8s-ci-robot commented 4 months ago

[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](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/OWNERS)~~ [mdbooth] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mquhuy commented 4 months ago

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.

mquhuy commented 4 months ago

/unhold

mdbooth commented 4 months ago

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: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/046d6b7c9c7df3f6a514dd452d65922ea7156c92/test/e2e/suites/e2e/e2e_test.go#L447-L455 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 commented 1 month ago

/cherrypick release-0.9 /cherrypick release-0.10

k8s-infra-cherrypick-robot commented 1 month ago

@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](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/2081#issuecomment-2302175257): >/cherrypick release-0.9 >/cherrypick release-0.10 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.