⚠ Handle finalizers in the fake client #1399

tjamet · 2021-02-26T08:04:01Z

The fake client differs significantly from the real implementation.
With the real client implementation, upon the deletion of a resource,
when finalizers are set the DeletionTimestamp is filled with a non-null
value.
This triggers a number of reconciling loops for all controllers that then
can take the opportunity to inspect the resource being deleted, perform
the required cleaning actions and then update the resource by removing
their finalizers as the cleaning job have been done.

In the current implementation, the Delete function implements a straight
delete and hence triggers a false positive or negative test when trying
to run tests at the client level, without interacting with the objects.

For example:

client.CreateObject()
controller.Reconcile()
Expect(Something).To(BeDone())
client.DeleteObject()
controller.Reconcile()
Expect(SomethingElse).To(BeDone())

In the real life, SomethingElse would actually be done as the controller
would still have access to the resource. In the current fake implementation,
this is not the case as the controller does not have access to the
resource any longer.

k8s-ci-robot · 2021-02-26T08:04:09Z

Welcome @tjamet!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-02-26T08:04:09Z

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

The fake client differs significantly from the real implementation. With the real client implementation, upon the deletion of a resource, when finalizers are set the DeletionTimestamp is filled with a non-null value. This triggers a number of reconciling loops for all controllers that then can take the opportunity to inspect the resource being deleted, perform the required cleaning actions and then update the resource by removing their finalizers as the cleaning job have been done. In the current implementation, the Delete function implements a straight delete and hence triggers a false positive or negative test when trying to run tests at the client level, without interacting with the objects. For example: ``` client.CreateObject() controller.Reconcile() Expect(Something).To(BeDone()) client.DeleteObject() controller.Reconcile() Expect(SomethingElse).To(BeDone()) ``` In the real life, SomethingElse would actually be done as the controller would still have access to the resource. In the current fake implementation, this is not the case as the controller does not have access to the resource any longer.

tjamet · 2021-02-26T08:14:20Z

I think the PR is ready on its own. I didn't implement yet the feature in DeleteAllOf which would probably also be required

alvaroaleman

/ok-to-test

This is a breaking change and needs to be marked as such

tjamet · 2021-02-27T23:35:59Z

/retest

tjamet · 2021-02-27T23:36:23Z

This is a breaking change and needs to be marked as such

Sure! shall I use ⚠ then?

alvaroaleman · 2021-03-01T14:15:00Z

Sure! shall I use then?

Yes, please.

I think the PR is ready on its own. I didn't implement yet the feature in DeleteAllOf which would probably also be required

Can you implement that as well, please? Not having it there will surprise users.

tjamet · 2021-03-01T15:07:44Z

Can you implement that as well, please? Not having it there will surprise users

Sure! I will push it soon

alvaroaleman

/label tide/merge-method-squash
/lgtm
/approve

thank you!

k8s-ci-robot · 2021-03-02T22:57:25Z

@alvaroaleman: The label(s) /label tide/merge-method-squash cannot be applied. These labels are supported: api-review

In response to this:

/label tide/merge-method-squash
/lgtm
/approve

thank you!

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.

k8s-ci-robot · 2021-03-02T22:57:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, tjamet

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

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

alvaroaleman · 2021-03-02T22:58:03Z

/hold

alvaroaleman · 2021-03-02T23:05:14Z

/hold cancel
Labeler gets fixed in kubernetes/test-infra#21119

Update tests for controller-runtime changes from 0.8.2 to 0.9.2(+). Most of these changes react to kubernetes-sigs/controller-runtime#1399, which made the fake client's Delete behave more like the real thing: - If the object has finalizers, set `DeletionTimestamp` but don't actually delete. - If there are no finalizers, actually delete. - Updates that remove the last finalizer from an object with a `DeletionTimestamp` actually delete the object. Then there's kubernetes-sigs/controller-runtime#1390 which deduplicates objects in the reconcile queue for Update actions.

* Handle finalizers in the fake client The fake client differs significantly from the real implementation. With the real client implementation, upon the deletion of a resource, when finalizers are set the DeletionTimestamp is filled with a non-null value. This triggers a number of reconciling loops for all controllers that then can take the opportunity to inspect the resource being deleted, perform the required cleaning actions and then update the resource by removing their finalizers as the cleaning job have been done. In the current implementation, the Delete function implements a straight delete and hence triggers a false positive or negative test when trying to run tests at the client level, without interacting with the objects. For example: ``` client.CreateObject() controller.Reconcile() Expect(Something).To(BeDone()) client.DeleteObject() controller.Reconcile() Expect(SomethingElse).To(BeDone()) ``` In the real life, SomethingElse would actually be done as the controller would still have access to the resource. In the current fake implementation, this is not the case as the controller does not have access to the resource any longer. * Handle finalizers when deleting collections

Fake client now respects finalizers, if any are present delete functionality will simulate real cluster resource delete More info: kubernetes-sigs/controller-runtime#1399 Signed-off-by: ehila <[email protected]>

Signed-off-by: ehila <[email protected]> fix: unit test for cleanup stale state Fake client now respects finalizers, if any are present delete functionality will simulate real cluster resource delete More info: kubernetes-sigs/controller-runtime#1399 Signed-off-by: ehila <[email protected]> fix: updated unit tests mock object with max call Signed-off-by: ehila <[email protected]>

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 26, 2021

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 Feb 26, 2021

k8s-ci-robot requested review from alenkacz and mengqiy February 26, 2021 08:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2021

tjamet changed the title ~~Handle finalizers in the fake client~~ ✨ Handle finalizers in the fake client Feb 26, 2021

tjamet changed the title ~~✨ Handle finalizers in the fake client~~ ✨ Handle finalizers in the fake client Feb 26, 2021

tjamet force-pushed the fake/client/finalizers branch from 755c845 to 4f23760 Compare February 26, 2021 08:11

alvaroaleman reviewed Feb 27, 2021

View reviewed changes

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 Feb 27, 2021

tjamet changed the title ~~✨ Handle finalizers in the fake client~~ ⚠ Handle finalizers in the fake client Mar 1, 2021

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 Mar 2, 2021

Handle finalizers when deleting collections

286a287

tjamet force-pushed the fake/client/finalizers branch from b53c144 to 286a287 Compare March 2, 2021 15:12

alvaroaleman approved these changes Mar 2, 2021

View reviewed changes

k8s-ci-robot assigned alvaroaleman Mar 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2021

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 2, 2021

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

k8s-ci-robot merged commit 854c5e8 into kubernetes-sigs:master Mar 2, 2021

andfasano mentioned this pull request Jun 8, 2021

Cache only required secrets metal3-io/baremetal-operator#903

Merged

alvaroaleman mentioned this pull request Jun 8, 2021

Update Kube dependencies to v1.21 level kubernetes/test-infra#22477

Merged

varshaprasad96 mentioned this pull request Jul 13, 2021

Bump k8s dependencies to 1.21+ operator-framework/operator-sdk#5057

Merged

2 tasks

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

ece312a

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

f22a5dc

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

143e055

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

f702342

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

e9ccdd6

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

519ef97

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 19, 2023

Fix unit tests and make sure to run them in CI

e689fd4

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 20, 2023

Fix unit tests and make sure to run them in CI

b0d89ad

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

akalenyu added a commit to akalenyu/controller-lifecycle-operator-sdk that referenced this pull request Mar 20, 2023

Fix unit tests and make sure to run them in CI

70980cc

Broke after we bumped and got kubernetes-sigs/controller-runtime#1399 Signed-off-by: Alex Kalenyuk <[email protected]>

ranakan19 mentioned this pull request Mar 25, 2024

SANDBOX-559 K8s 1 27 member codeready-toolchain/member-operator#551

Merged

ranakan19 mentioned this pull request Jul 5, 2024

SANDBOX-560 update controller-runtime to v0.15 codeready-toolchain/host-operator#1057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

⚠ Handle finalizers in the fake client #1399

⚠ Handle finalizers in the fake client #1399

Uh oh!

tjamet commented Feb 26, 2021 •

edited

Loading

Uh oh!

k8s-ci-robot commented Feb 26, 2021

Uh oh!

k8s-ci-robot commented Feb 26, 2021

Uh oh!

tjamet commented Feb 26, 2021

Uh oh!

alvaroaleman left a comment

Uh oh!

tjamet commented Feb 27, 2021

Uh oh!

tjamet commented Feb 27, 2021

Uh oh!

alvaroaleman commented Mar 1, 2021

Uh oh!

tjamet commented Mar 1, 2021

Uh oh!

alvaroaleman left a comment

Uh oh!

k8s-ci-robot commented Mar 2, 2021

Uh oh!

k8s-ci-robot commented Mar 2, 2021

Uh oh!

alvaroaleman commented Mar 2, 2021

Uh oh!

alvaroaleman commented Mar 2, 2021

Uh oh!

Uh oh!

⚠ Handle finalizers in the fake client #1399

⚠ Handle finalizers in the fake client #1399

Uh oh!

Conversation

tjamet commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 26, 2021

Uh oh!

k8s-ci-robot commented Feb 26, 2021

Uh oh!

tjamet commented Feb 26, 2021

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

tjamet commented Feb 27, 2021

Uh oh!

tjamet commented Feb 27, 2021

Uh oh!

alvaroaleman commented Mar 1, 2021

Uh oh!

tjamet commented Mar 1, 2021

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 2, 2021

Uh oh!

k8s-ci-robot commented Mar 2, 2021

Uh oh!

alvaroaleman commented Mar 2, 2021

Uh oh!

alvaroaleman commented Mar 2, 2021

Uh oh!

Uh oh!

tjamet commented Feb 26, 2021 •

edited

Loading