🌱 test: Ensure finalizers are re-reconciled #2626

adityabhatia · 2024-01-17T14:17:59Z

What this PR does / why we need it:
This PR introduces finalizer resiliency checks - covering both initial presence and upon deletion cases using the CAPI e2e test framework.

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

k8s-ci-robot · 2024-01-17T14:18:09Z

Hi @adityabhatia. 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.

adityabhatia · 2024-01-18T17:39:54Z

test/e2e/ownerreference_test.go

@@ -177,7 +177,7 @@ var (

 // cleanupVSphereObjects deletes the Secret, VSphereClusterIdentity, and VSphereDeploymentZone created for this test.
 // The VSphereFailureDomain, and the Secret for the VSphereClusterIdentity should be deleted as a result of the above.
-func cleanupVSphereObjects(ctx context.Context, bootstrapClusterProxy framework.ClusterProxy) bool {


Removed the boolean return value since linter was complaining, also I do not see any use.

chrischdi · 2024-01-18T18:36:17Z

test/e2e/finalizers_test.go

+				// This check ensures that finalizers are resilient - i.e. correctly re-reconciled, when removed.
+				framework.ValidateFinalizersResilience(ctx, proxy, namespace, clusterName,
+					framework.CoreFinalizersAssertion,
+					framework.KubeadmControlPlaneFinalizersAssertion,
+					framework.ExpFinalizersAssertion,
+					VSphereFinalizers,
+				)


Looks like the ownerreference test already has what we need (from object perspective).

Could we combine these two tests maybe to combine ownerreferences and finalizers in one test?
If I got it right its only about having all the objects and running framework.ValidateFinalizersResilience as PostMachinesProvisioned.
This would reduce the time needed for CI because bringing up cluster takes time and costs resources :-)

Or do you see points why we should not combine them?

Yes combining these tests will surely make it more efficient, should also work (looks good locally).

I have purposely made a second commit to see the notable differences, before and after test combination and also renamed the flavor ownerreferences -> ownerrefs-finalizers which IMO reflects the purpose of the combined test. 42a1cff

Once these changes are reviewed I will squash my commits.

zhanggbj · 2024-01-19T03:12:40Z

/ok-to-test

codecov · 2024-01-19T03:17:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a938dbd) 64.84% compared to head (4f1352f) 64.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2626      +/-   ##
==========================================
- Coverage   64.84%   64.56%   -0.28%     
==========================================
  Files         118      118              
  Lines        8580     8580              
==========================================
- Hits         5564     5540      -24     
- Misses       2588     2605      +17     
- Partials      428      435       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chrischdi · 2024-01-19T11:30:55Z

test/e2e/data/infrastructure-vsphere/main/finalizers/delete-cluster-secret.yaml

@@ -0,0 +1,8 @@
+---


these can get removed right?

yes this will also go away, it was just kept for reference. If you think the combined tests should be ok, I can push a squashed commit

yes, please drop them 👍

(and feel free to squash)

chrischdi · 2024-01-22T08:38:08Z

/test help

k8s-ci-robot · 2024-01-22T08:38:11Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-conformance-ci-latest-main
/test pull-cluster-api-provider-vsphere-conformance-main
/test pull-cluster-api-provider-vsphere-e2e-full-main
/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-test-integration-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-e2e-main
pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

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.

chrischdi · 2024-01-22T08:38:18Z

/test pull-cluster-api-provider-vsphere-e2e-full-main

test/e2e/ownerrefs_finalizers_test.go

sbueringer · 2024-01-22T13:40:21Z

/lgtm

/test pull-cluster-api-provider-vsphere-e2e-full-main

k8s-ci-robot · 2024-01-22T13:40:28Z

LGTM label has been added.

Git tree hash: 1dfa4428807feef34e3fa73b51a57d22cec8ec70

sbueringer · 2024-01-25T18:18:44Z

/assign @chrischdi

sbueringer · 2024-01-25T18:18:52Z

@adityabhatia Can you please rebase?

sbueringer · 2024-01-26T07:24:42Z

Thx!
/lgtm

/assign @chrischdi

k8s-ci-robot · 2024-01-26T07:24:47Z

LGTM label has been added.

Git tree hash: e1ba044833255598a86fcca143dc347523a4ca17

chrischdi · 2024-01-26T10:51:08Z

/approve

sbueringer · 2024-01-26T11:23:39Z

/approve

k8s-ci-robot · 2024-01-26T11:23:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, sbueringer

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~~ [chrischdi,sbueringer]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 17, 2024

k8s-ci-robot requested review from killianmuldoon and zhanggbj January 17, 2024 14:18

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 17, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 17, 2024

adityabhatia force-pushed the ensureFinalisersResiliancy branch 2 times, most recently from 1c696df to 2f118bb Compare January 18, 2024 17:38

adityabhatia changed the title ~~[WIP] 🌱 test: Ensure finalizers are re-reconciled~~ 🌱 test: Ensure finalizers are re-reconciled Jan 18, 2024

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 Jan 18, 2024

adityabhatia commented Jan 18, 2024

View reviewed changes

chrischdi reviewed Jan 18, 2024

View reviewed changes

adityabhatia force-pushed the ensureFinalisersResiliancy branch from 867f6f7 to 42a1cff Compare January 18, 2024 21:39

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 Jan 19, 2024

chrischdi reviewed Jan 19, 2024

View reviewed changes

adityabhatia force-pushed the ensureFinalisersResiliancy branch from 42a1cff to 1ed21bf Compare January 19, 2024 17:00

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 Jan 19, 2024

adityabhatia force-pushed the ensureFinalisersResiliancy branch from 1ed21bf to 55c9e2b Compare January 19, 2024 17:12

sbueringer reviewed Jan 22, 2024

View reviewed changes

test/e2e/ownerrefs_finalizers_test.go Outdated Show resolved Hide resolved

adityabhatia force-pushed the ensureFinalisersResiliancy branch from 55c9e2b to 033a3b8 Compare January 22, 2024 13:11

k8s-ci-robot assigned sbueringer Jan 22, 2024

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

k8s-ci-robot assigned chrischdi Jan 25, 2024

test: Ensure finalizers are re-reconciled

4f1352f

adityabhatia force-pushed the ensureFinalisersResiliancy branch from 033a3b8 to 4f1352f Compare January 25, 2024 22:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024

k8s-ci-robot requested review from chrischdi and sbueringer January 25, 2024 22:17

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

adityabhatia mentioned this pull request Jan 26, 2024

REQUEST: New membership for adityabhatia kubernetes/org#4724

Closed

9 tasks

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

k8s-ci-robot merged commit 54a227f into kubernetes-sigs:main Jan 26, 2024
18 checks passed

k8s-ci-robot added this to the v1.9 milestone Jan 26, 2024

chrischdi mentioned this pull request Apr 8, 2024

Ensure finalizers are re-reconciled in CAPV #2357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 test: Ensure finalizers are re-reconciled #2626

🌱 test: Ensure finalizers are re-reconciled #2626

adityabhatia commented Jan 17, 2024 •

edited

Loading

k8s-ci-robot commented Jan 17, 2024

adityabhatia Jan 18, 2024

chrischdi Jan 18, 2024

adityabhatia Jan 18, 2024

zhanggbj commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

chrischdi Jan 19, 2024

adityabhatia Jan 19, 2024

chrischdi Jan 19, 2024

chrischdi Jan 19, 2024

chrischdi commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

chrischdi commented Jan 22, 2024

sbueringer commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

sbueringer commented Jan 25, 2024

sbueringer commented Jan 25, 2024

sbueringer commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

chrischdi commented Jan 26, 2024

sbueringer commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

🌱 test: Ensure finalizers are re-reconciled #2626

🌱 test: Ensure finalizers are re-reconciled #2626

Conversation

adityabhatia commented Jan 17, 2024 • edited Loading

k8s-ci-robot commented Jan 17, 2024

adityabhatia Jan 18, 2024

Choose a reason for hiding this comment

chrischdi Jan 18, 2024

Choose a reason for hiding this comment

adityabhatia Jan 18, 2024

Choose a reason for hiding this comment

zhanggbj commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

chrischdi Jan 19, 2024

Choose a reason for hiding this comment

adityabhatia Jan 19, 2024

Choose a reason for hiding this comment

chrischdi Jan 19, 2024

Choose a reason for hiding this comment

chrischdi Jan 19, 2024

Choose a reason for hiding this comment

chrischdi commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

chrischdi commented Jan 22, 2024

sbueringer commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

sbueringer commented Jan 25, 2024

sbueringer commented Jan 25, 2024

sbueringer commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

chrischdi commented Jan 26, 2024

sbueringer commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

adityabhatia commented Jan 17, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading