🐛 Update object upon deletion #3098

bigkevmcd · 2025-01-30T07:43:32Z

If an object is deleted and the API responds with a resource, update the deleted object.

This causes the object to be updated with the deletion timestamp.

The fake Client was already behaving like this.

k8s-ci-robot · 2025-01-30T07:43:42Z

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

alvaroaleman · 2025-01-30T22:02:33Z

/ok-to-test

alvaroaleman · 2025-02-08T22:44:57Z

/ok-to-test

alvaroaleman

Thanks, this is great! We also need to update the fake client to match the behavior

pkg/client/client_test.go

alvaroaleman · 2025-02-08T22:47:19Z

pkg/client/client_test.go

+				nodeName := node.Name
+				err = cl.Delete(context.TODO(), node)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(node.ObjectMeta.DeletionTimestamp).NotTo(BeNil())


Can we also test this with unstructured.Unstructured and metav1.MetaObject, please?

Unstructured works... metav1.MetaObject will require a fair bit more work.

controller-runtime/pkg/client/metadata_client.go

Line 67 in c5bb1d4

return resInt.Delete(ctx, metadata.Name, *deleteOpts.AsDeleteOptions())

Not sure if this is relevant?

controller-runtime/pkg/client/metadata_client.go

Line 30 in c5bb1d4

// TODO(directxman12): we could rewrite this on top of the low-level REST

I think we should also implement this for the metadata client. It's already tricky that we change a nuance like this, but I think it's a lot worse if our clients behave inconsistently

bigkevmcd · 2025-02-09T21:12:23Z

Thanks, this is great! We also need to update the fake client to match the behavior

My reading of this test is that the fake client does actually work?

controller-runtime/pkg/client/fake/client_test.go

Line 817 in c5bb1d4

Expect(obj.DeletionTimestamp).NotTo(BeNil())

alvaroaleman · 2025-02-10T00:53:48Z

That example is after a Get call, it should be set after a Delete call

sbueringer · 2025-02-10T06:58:36Z

/assign

k8s-ci-robot · 2025-02-24T20:17:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bigkevmcd
Once this PR has been reviewed and has the lgtm label, please ask for approval from sbueringer. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

sbueringer · 2025-05-12T12:15:35Z

pkg/client/unstructured_client.go

 		Body(deleteOpts.AsDeleteOptions()).
 		Do(ctx).
-		Error()
+		Into(obj)


I ran this through the debugger and this object looks like this afterwards if I'm not mistaken:

(I assume this is the case when we don't have a finalizer on the object, but this is still bad?)

I think we also need a test case that when we delete via Unstructured without a finalizer the object keeps its data and does not become a Status object

Should we maybe just check if the returned body is a status and only if its not update the original object?

As far as I can tell there's no way to tell apart you got the object back or a metav1.Status unless you actually read the request body.

Which I think might be okay, the main overhead will be that we have to deserialize it twice if it is not a status

I think it would be a bad UX if after calling client.Delete() you only sometimes see the deletionTimestamp set :/

In general, it appears if it's gone from storage you'll get Status object in the response body. (You can try Pod deletion with grace period.) But I don't think there's a guarantee around any of this in the API machinery.

I think it would be a bad UX if after calling client.Delete() you only sometimes see the deletionTimestamp set :/

I mean what can we realistically put in there if the object is gone from storage? I guess we could put a non-nil fake deletiontimestamp in there but not sure if that is a good idea.

What is IMHO annoying about the current state is that you can not do a check of Did I already delete this

Since Delete() already doesn't do what we expect it to do, what if we changed its return signature to (deletedFromStorage bool, err error)?

I mean what can we realistically put in there if the object is gone from storage? I guess we could put a non-nil fake deletiontimestamp in there but not sure if that is a good idea.

Yup agree. I mostly just meant that if deletionTimestamp is only sometimes set (aka in some cases) this would be tricky to rely on

k8s-triage-robot · 2025-08-10T12:51:46Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2025-09-09T12:56:52Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

alvaroaleman · 2025-09-24T14:19:58Z

Ref #3059

sbueringer · 2025-09-24T15:21:21Z

Added it to the PR description

If an object is deleted and the API responds with a resource, update the deleted object. This causes the object to be updated with the deletion timestamp. Signed-off-by: Kevin McDermott <[email protected]>

Signed-off-by: Kevin McDermott <[email protected]>

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 30, 2025

k8s-ci-robot requested review from joelanford and vincepri January 30, 2025 07:43

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 Jan 30, 2025

bigkevmcd mentioned this pull request Jan 30, 2025

Object doesn't get updated after Client.Delete(object) #3059

Open

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 30, 2025

bigkevmcd force-pushed the delete-response branch 3 times, most recently from e2ca8b7 to 68cdf84 Compare February 4, 2025 06:12

bigkevmcd marked this pull request as ready for review February 4, 2025 06:45

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 Feb 4, 2025

k8s-ci-robot requested a review from troy0820 February 4, 2025 06:45

bigkevmcd force-pushed the delete-response branch 2 times, most recently from 8ed5e0b to 5b4fcfc Compare February 4, 2025 09:24

alvaroaleman reviewed Feb 8, 2025

View reviewed changes

k8s-ci-robot assigned sbueringer Feb 10, 2025

bigkevmcd force-pushed the delete-response branch from b48125a to 7c9b5e5 Compare February 24, 2025 20:17

bigkevmcd force-pushed the delete-response branch from 7c9b5e5 to 4c4e252 Compare April 23, 2025 19:11

sbueringer reviewed May 12, 2025

View reviewed changes

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 10, 2025

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 9, 2025

Update object upon deletion

00b465d

If an object is deleted and the API responds with a resource, update the deleted object. This causes the object to be updated with the deletion timestamp. Signed-off-by: Kevin McDermott <[email protected]>

bigkevmcd force-pushed the delete-response branch from 4c4e252 to f0d6c12 Compare September 27, 2025 06:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2025

Update the unstructured client to return the values.

4ac29a2

Signed-off-by: Kevin McDermott <[email protected]>

bigkevmcd force-pushed the delete-response branch from f0d6c12 to 4ac29a2 Compare September 27, 2025 10:13

🐛 Update object upon deletion #3098

Are you sure you want to change the base?

🐛 Update object upon deletion #3098

Conversation

bigkevmcd commented Jan 30, 2025 • edited by sbueringer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 30, 2025

Uh oh!

alvaroaleman commented Jan 30, 2025

Uh oh!

alvaroaleman commented Feb 8, 2025

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigkevmcd commented Feb 9, 2025

Uh oh!

alvaroaleman commented Feb 10, 2025

Uh oh!

sbueringer commented Feb 10, 2025

Uh oh!

k8s-ci-robot commented Feb 24, 2025

Uh oh!

sbueringer May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Aug 10, 2025

Uh oh!

k8s-triage-robot commented Sep 9, 2025

Uh oh!

alvaroaleman commented Sep 24, 2025

Uh oh!

sbueringer commented Sep 24, 2025

Uh oh!

Uh oh!

bigkevmcd commented Jan 30, 2025 •

edited by sbueringer

Loading

sbueringer May 12, 2025 •

edited

Loading

sbueringer May 12, 2025 •

edited

Loading

sbueringer Sep 15, 2025 •

edited

Loading