🌱 Ensure E2E cleanup #2005

mquhuy · 2024-04-08T12:31:14Z

What this PR does / why we need it:

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 #1801

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:

[x ] squashed commits

k8s-ci-robot · 2024-04-08T12:31:24Z

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/test-infra repository.

netlify · 2024-04-08T12:31:30Z

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

Name	Link
🔨 Latest commit	`48c952f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6619338f50e1850008b5779c
😎 Deploy Preview	https://deploy-preview-2005--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.

mquhuy · 2024-04-09T09:35:21Z

/test ?

k8s-ci-robot · 2024-04-09T09:35:36Z

@mquhuy: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test ?

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.

lentzi90 · 2024-04-09T11:33:03Z

/ok-to-test

mdbooth · 2024-04-09T16:51:15Z

I like this! I'm slightly nervous of what this is going to find when it's working 😬

Looks like it's failing because e2eCtx hasn't been initialized trying to dump the initial set of resources:

> Enter [SynchronizedBeforeSuite] TOP-LEVEL - /home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_suite_test.go:66 @ 04/09/24 15:18:52.03
[PANICKED] Test Panicked
In [SynchronizedBeforeSuite] at: /usr/local/go/src/runtime/panic.go:261 @ 04/09/24 15:18:52.031

runtime error: invalid memory address or nil pointer dereference

Full Stack Trace
  sigs.k8s.io/cluster-api/test/framework/clusterctl.(*E2EConfig).GetVariable(0x0, {0x267ba00, 0xf})
  	/root/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/clusterctl/e2e_config.go:580 +0x3b
  sigs.k8s.io/cluster-api-provider-openstack/test/e2e/shared.GetTenantProviderClient(0xc00015e160)
  	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/shared/openstack.go:626 +0x2b
  sigs.k8s.io/cluster-api-provider-openstack/test/e2e/shared.DumpOpenStackServers(_, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, ...}, ...})
  	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/shared/openstack.go:252 +0x25
  sigs.k8s.io/cluster-api-provider-openstack/test/e2e/suites/e2e.init.func5({0xc000839000, 0x2ebb, 0x2ebb})
  	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_suite_test.go:70 +0x5e
  reflect.Value.call({0x2136120?, 0x27b51e8?, 0x13?}, {0x266c731, 0x4}, {0xc0007f00f0, 0x1, 0x1?})
  	/usr/local/go/src/reflect/value.go:596 +0xca6
  reflect.Value.Call({0x2136120?, 0x27b51e8?, 0x2ebb?}, {0xc0007f00f0?, 0x2666c80?, 0x0?})
  	/usr/local/go/src/reflect/value.go:380 +0xb9
< Exit [SynchronizedBeforeSuite] TOP-LEVEL - /home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_suite_test.go:66 @ 04/09/24 15:18:52.031 (0s)

mdbooth · 2024-04-09T16:55:04Z

test/e2e/suites/e2e/e2e_suite_test.go

+	initialLoadBalancers, err = shared.DumpOpenStackLoadBalancers(e2eCtx, loadbalancers.ListOpts{})
+	Expect(err).NotTo(HaveOccurred())
+	initialVolumes, err = shared.DumpOpenStackVolumes(e2eCtx, volumes.ListOpts{})
+	Expect(err).NotTo(HaveOccurred())
 	shared.AllNodesBeforeSuite(e2eCtx, data)


I think you need to move this call to AllNodesBeforeSuite up to the top.

mdbooth · 2024-04-09T18:55:13Z

I tested my suggested fix in #2007, and it nearly passes! In fact, it looks like we probably just need a custom matcher. It looks like the reported difference in networks isn't relevant. I can see at a glance that the subnets are in the opposite order. There may be other differences.

I wonder if we just want to compare the list of UUIDs?

mdbooth · 2024-04-09T21:37:23Z

I wrote a custom matcher over in #2007. You can see an example test failure in https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/2007/pull-cluster-api-provider-openstack-e2e-test/1777791653365420032

I think it looks... ok. This would probably fly, tbh, although ideally we'd have some way to dump all the match failures after a run rather than stopping on the first one.

Feel free to take that code if you want. I am not planning to merge #2007, it was just for playing with your PR.

mquhuy · 2024-04-10T06:40:39Z

Hi @mdbooth . Thank you a lot! I'll have a look and come back asap xD

mdbooth · 2024-04-10T08:44:05Z

I added the 'test everything and report after' too. You can see the results here: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/2007/pull-cluster-api-provider-openstack-e2e-test/1777826551497232384

With this change the reported failure is simply:

{Not all resources were cleaned up failed [FAILED] Not all resources were cleaned up
In [SynchronizedAfterSuite] at: /home/prow/go/src/sigs.k8s.io/cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_suite_test.go:124 @ 04/09/24 23:29:56.707
}

And the actual leaked resources are in the logs. Honestly this wasn't my intention, but I think I like it?

mdbooth · 2024-04-10T08:49:09Z

Incidentally, the advantage of using a custom matcher on whole objects rather than just matching on UUIDs is that you get the whole object in the failure message, e.g.:

the extra elements were
    <[]groups.SecGroup | len:1, cap:1>: [
        {
            ID: "7f1d446f-3762-4f6a-9a18-2e6704e60d03",
            Name: "testSecGroup",
            Description: "Test security group",
            Rules: [
                {
                    ID: "6e0a5a67-4249-4f40-be46-9df343761043",
                    Direction: "egress",
                    Description: "",
                    EtherType: "IPv4",
                    SecGroupID: "7f1d446f-3762-4f6a-9a18-2e6704e60d03",
                    PortRangeMin: 0,
                    PortRangeMax: 0,
                    Protocol: "",
                    RemoteGroupID: "",
                    RemoteIPPrefix: "",
                    TenantID: "5e83f484071a448dabc9570ec37c44de",
                    ProjectID: "5e83f484071a448dabc9570ec37c44de",
                },
                {
                    ID: "71158c32-b1ca-45b5-a9f9-8fbdffc5b31d",
                    Direction: "egress",
                    Description: "",
                    EtherType: "IPv6",
                    SecGroupID: "7f1d446f-3762-4f6a-9a18-2e6704e60d03",
                    PortRangeMin: 0,
                    PortRangeMax: 0,
                    Protocol: "",
                    RemoteGroupID: "",
                    RemoteIPPrefix: "",
                    TenantID: "5e83f484071a448dabc9570ec37c44de",
                    ProjectID: "5e83f484071a448dabc9570ec37c44de",
                },
            ],
            TenantID: "5e83f484071a448dabc9570ec37c44de",
            UpdatedAt: 2024-04-09T23:11:17Z,
            CreatedAt: 2024-04-09T23:11:17Z,
            ProjectID: "5e83f484071a448dabc9570ec37c44de",
            Tags: [],
        },
    ]

mquhuy · 2024-04-11T10:53:22Z

Hi @mdbooth . Thank you for all the work! I've stolen all of the implementation and updated the PR here.

test/e2e/suites/e2e/e2e_suite_test.go

test/e2e/suites/e2e/idmatcher_test.go

lentzi90

/approve

k8s-ci-robot · 2024-04-12T12:43:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

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~~ [lentzi90]

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

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

mdbooth · 2024-04-12T19:08:47Z

/lgtm

mquhuy · 2024-04-15T05:15:23Z

/unhold

k8s-ci-robot requested review from EmilienM and jichenjc April 8, 2024 12:31

mquhuy marked this pull request as draft April 8, 2024 12:31

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2024

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch from b58a276 to 2fa50b8 Compare April 9, 2024 09:34

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 Apr 9, 2024

mquhuy marked this pull request as ready for review April 9, 2024 09:36

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 9, 2024

mquhuy changed the title ~~🌱 WIP Ensure E2E cleanup~~ 🌱 Ensure E2E cleanup Apr 9, 2024

k8s-ci-robot requested a review from mdbooth April 9, 2024 09:36

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 Apr 9, 2024

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch 2 times, most recently from f028cc0 to 0de2853 Compare April 9, 2024 14:47

mdbooth reviewed Apr 9, 2024

View reviewed changes

mdbooth mentioned this pull request Apr 9, 2024

[DNM] mdbooth copy of PR 2005 #2007

Closed

mdbooth mentioned this pull request Apr 10, 2024

🌱 Cleanup security group created by e2e test #2010

Merged

1 task

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch from 0de2853 to 4d3cc33 Compare April 11, 2024 10:52

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 Apr 11, 2024

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch 3 times, most recently from 950154d to 926bbaa Compare April 11, 2024 11:59

lentzi90 reviewed Apr 12, 2024

View reviewed changes

test/e2e/suites/e2e/e2e_suite_test.go Show resolved Hide resolved

test/e2e/suites/e2e/idmatcher_test.go Outdated Show resolved Hide resolved

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch 2 times, most recently from adbbd79 to fbb826a Compare April 12, 2024 12:13

lentzi90 reviewed Apr 12, 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 Apr 12, 2024

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch from fbb826a to 7e28d96 Compare April 12, 2024 12:55

Ensure E2E cleanup

48c952f

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

mquhuy force-pushed the mquhuy/ensure-cleanup-e2e-tests branch from 7e28d96 to 48c952f Compare April 12, 2024 13:13

k8s-ci-robot assigned mdbooth Apr 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 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 Apr 15, 2024

k8s-ci-robot merged commit 62dda69 into kubernetes-sigs:main Apr 15, 2024

mquhuy deleted the mquhuy/ensure-cleanup-e2e-tests branch April 15, 2024 06:54

🌱 Ensure E2E cleanup #2005

🌱 Ensure E2E cleanup #2005

Uh oh!

Conversation

mquhuy commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 8, 2024

Uh oh!

netlify bot commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

mquhuy commented Apr 9, 2024

Uh oh!

k8s-ci-robot commented Apr 9, 2024

Uh oh!

lentzi90 commented Apr 9, 2024

Uh oh!

mdbooth commented Apr 9, 2024

Uh oh!

mdbooth Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Apr 9, 2024

Uh oh!

mdbooth commented Apr 9, 2024

Uh oh!

mquhuy commented Apr 10, 2024

Uh oh!

mdbooth commented Apr 10, 2024

Uh oh!

mdbooth commented Apr 10, 2024

Uh oh!

mquhuy commented Apr 11, 2024

Uh oh!

Uh oh!

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 12, 2024

Uh oh!

mdbooth commented Apr 12, 2024

Uh oh!

mquhuy commented Apr 15, 2024

Uh oh!

Uh oh!

mquhuy commented Apr 8, 2024 •

edited

Loading

netlify bot commented Apr 8, 2024 •

edited

Loading