Skip to content

🐛 Update object upon deletion #3098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

🐛 Update object upon deletion #3098

wants to merge 2 commits into from

Conversation

bigkevmcd
Copy link
Contributor

@bigkevmcd bigkevmcd commented Jan 30, 2025 •
edited
Loading

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 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
Copy link
Contributor

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.

@k8s-ci-robot 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 bigkevmcd mentioned this pull request Jan 30, 2025
Open
@alvaroaleman
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 bigkevmcd force-pushed the delete-response branch 3 times, most recently from e2ca8b7 to 68cdf84 Compare February 4, 2025 06:12
@bigkevmcd bigkevmcd marked this pull request as ready for review February 4, 2025 06:45
@k8s-ci-robot 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 k8s-ci-robot requested a review from troy0820 February 4, 2025 06:45
@bigkevmcd bigkevmcd force-pushed the delete-response branch 2 times, most recently from 8ed5e0b to 5b4fcfc Compare February 4, 2025 09:24
@alvaroaleman
Copy link
Member

/ok-to-test

alvaroaleman
alvaroaleman reviewed Feb 8, 2025
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/client/client_test.go
nodeName := node.Name
err = cl.Delete(context.TODO(), node)
Expect(err).NotTo(HaveOccurred())
Expect(node.ObjectMeta.DeletionTimestamp).NotTo(BeNil())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

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

@sbueringer
Copy link
Member

/assign

@k8s-ci-robot
Copy link
Contributor

[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:

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

@bigkevmcd
Update object upon deletion
7fa47ee 
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 <bigkevmcd@gmail.com>
@bigkevmcd
Update the unstructured client to return the values.
4c4e252 
Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>
sbueringer
sbueringer reviewed May 12, 2025
View reviewed changes
pkg/client/unstructured_client.go
@@ -111,7 +111,7 @@ func (uc *unstructuredClient) Delete(ctx context.Context, obj Object, opts ...De
Name(o.GetName()).
Body(deleteOpts.AsDeleteOptions()).
Do(ctx).
Error()
Into(obj)
Copy link
Member

@sbueringer sbueringer May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

Copy link
Member

@sbueringer sbueringer May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@alvaroaleman alvaroaleman alvaroaleman left review comments

@joelanford joelanford Awaiting requested review from joelanford

@vincepri vincepri Awaiting requested review from vincepri

@troy0820 troy0820 Awaiting requested review from troy0820

Assignees

@sbueringer sbueringer

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bigkevmcd @k8s-ci-robot @alvaroaleman @sbueringer