Re: [kubernetes/kubernetes] [WIP] Implement opt-in graceful delete extension for custom resources (#63162)
Dr. Stefan Schimanski
/cc @kubernetes/sig-api-machinery-api-reviews
@deads2k @lavalamp @liggitt @caesarxuchao please take a look at this proposal. Has there been any discussion about finalizers vs. graceful deletion anywhere? The later predates the former by quite some time. Finalizers could simulate graceful deletion.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Daniel Smith
Chao Xu
I commented in the issue: #56567 (comment)
Davanum Srinivas
/ok-to-test
k8s-ci-robot
@rmohr: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-typecheck | 067899f | link | /test pull-kubernetes-typecheck |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
k8s-ci-robot
k8s-ci-robot
@rmohr: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-typecheck | 067899f | link | /test pull-kubernetes-typecheck |
| pull-kubernetes-integration | 067899f | link | /test pull-kubernetes-integration |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
k8s-ci-robot
@rmohr: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-typecheck | 067899f | link | /test pull-kubernetes-typecheck |
| pull-kubernetes-integration | 067899f | link | /test pull-kubernetes-integration |
| pull-kubernetes-bazel-test | 067899f | link | /test pull-kubernetes-bazel-test |
k8s-ci-robot
@rmohr: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-typecheck | 067899f | link | /test pull-kubernetes-typecheck |
| pull-kubernetes-integration | 067899f | link | /test pull-kubernetes-integration |
| pull-kubernetes-node-e2e | 067899f | link | /test pull-kubernetes-node-e2e |
| pull-kubernetes-bazel-test | 067899f | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-verify | 067899f | link | /test pull-kubernetes-verify |
Nikhita Raghunath
/sig api-machinery
/area custom-resources
/kind feature
Daniel Smith
/hold
We talked about this on the SIG. Let's have some more discussion (in the issue) before we commit ourselves to supporting this concept for every resource for all of eternity.
k8s-ci-robot
@rmohr: PR needs rebase.
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.
—
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
Roman Mohr
/remove-lifecycle stale
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
Roman Mohr
/remove lifecycle-rotten
Roman Mohr
/remove-lifecycle rotten
Kubernetes Prow Robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: rmohr
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lavalamp
If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready.
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:- staging/src/k8s.io/apiextensions-apiserver/OWNERS
- staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS
- staging/src/k8s.io/apiserver/OWNERS
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Roman Mohr
/cc @sttts
Updated everything. I would want to give this another try. @sttts I however have issues to understand under which circumstances the rest API should return a Status with success and when the object. I could not really make out the pattern when comparing the deletion of custom resources and pods.
Roman Mohr
@rmohr commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go:
> @@ -835,7 +835,7 @@ func (t *Tester) testDeleteNoGraceful(obj runtime.Object, createFn CreateFunc, g } defer t.delete(ctx, foo) objectMeta := t.getObjectMetaOrFail(foo) - opts := metav1.NewDeleteOptions(10) + opts := metav1.NewDeleteOptions(0)
It looked strange that the dryrun call should delete non-graceful although a grace period is set in the options ... What should be the correct behaviour for Pods and CRs?
Roman Mohr
@rmohr commented on this pull request.
In staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go:
> @@ -1082,13 +1082,15 @@ func (t *Tester) testDeleteGracefulImmediate(obj runtime.Object, createFn Create
if !errors.IsNotFound(err) {
t.Errorf("unexpected error, object should be deleted immediately: %v", err)
}
- objectMeta = t.getObjectMetaOrFail(out)
- // the second delete shouldn't update the object, so the objectMeta.GetDeletionGracePeriodSeconds() should eqaul to the value set in the first delete.
- if objectMeta.GetDeletionTimestamp() == nil || objectMeta.GetDeletionGracePeriodSeconds() == nil || *objectMeta.GetDeletionGracePeriodSeconds() != 0 {
- t.Errorf("unexpected deleted meta: %#v", objectMeta)
- }
- if generation >= objectMeta.GetGeneration() {
- t.Error("Generation wasn't bumped when deletion timestamp was set")
+ if _, ok := out.(*metav1.Status); !ok {
It is not clear to me why a successful real delete should return for Pods the pod and for CRs the status. Maybe I do something wrong somewhere or miss some connecting dots?
Kubernetes Prow Robot
@rmohr: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-verify | 5f5201f | link | /test pull-kubernetes-verify |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
—
Kubernetes Prow Robot
@rmohr: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce | 067899f | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-verify | 5f5201f | link | /test pull-kubernetes-verify |
| pull-kubernetes-e2e-kops-aws | 5f5201f | link | /test pull-kubernetes-e2e-kops-aws |
fejta-bot
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
fejta-bot
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
Kubernetes Prow Robot
Closed #63162.
Kubernetes Prow Robot
@fejta-bot: Closed this PR.
In response to this:
Rotten issues close after 30d of inactivity.
Reopen the issue with/reopen.
Mark the issue as fresh with/remove-lifecycle rotten.Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close
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.
—