Skip to content
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.

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

Workaround for gson-syntax-exception upon deletion even if the deletion succeeds #1445

Conversation

yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Dec 16, 2020

Fixes: #86

this pull will be suppressing the GsonSyntaxException on deletion by adapting non-status resources into status during deserialization.

@brendandburns i think we should get this into 11.0.x releases.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 16, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2020
@yue9944882 yue9944882 force-pushed the fixes-deletion-gson-exception branch from 9faeb60 to b7234c3 Compare December 16, 2020 07:42
@yue9944882 yue9944882 force-pushed the fixes-deletion-gson-exception branch from b7234c3 to 0302618 Compare December 16, 2020 08:11
@brendandburns
Copy link
Contributor

Do we really want to do this? Doesn't this make every delete look like it's not done yet?

@yue9944882
Copy link
Member Author

yue9944882 commented Dec 17, 2020

Doesn't this make every delete look like it's not done yet?

@brendandburns upon deletion calls, the kube-apiserver can return either a status object or the deleted state of the target object. however the method generated from openapi is only returning the status object. that's why we had this issue in the past. now it seems feasible to workaround by the extension provided by gson-fire library.

the only change this pull will bring to the project is making gson suppressing the runtime exception when deserializing a non-status object (e.g. deployment, pod) into a V1Status which will be happening when we successfully did a background deletion. this is actually done by casting/transforming the deleted object into a status object by removing "status" fields in order to solve the gson exception --- there's still information loss but it's way better than throwing a gson runtime exception surprisingly even if the deletion succeeded.. the failing behavior will not be changed anyway.

@brendandburns
Copy link
Contributor

Ok, I went and dug up the actual code in the apiserver:

https://github.com/kubernetes/apiserver/blob/93ffe41e75d9d1c6cfe6d9224f8f5f474606b5b2/pkg/endpoints/handlers/delete.go#L144

I had thought that it returned V1Status when the delete wasn't completed, and the object when the delete was completed, but it looks like it's actually dependent on the deleter.

Given that, I think that this change is ok, even though it is a little lossy.

Let's file an issue to fix this better via oneOf instead of this work-around, so that we don't forget about the problem.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, yue9944882

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 [brendandburns,yue9944882]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting exception from deleteNamespacedStatefulSet() call
3 participants