-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
🐛 Allow building a fake client with graceful deleting Pods #2361
Conversation
Welcome @arkbriar! |
Hi @arkbriar. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
@vincepri: GitHub didn't allow me to assign the following users: lleshchi. Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
Hi @vincepri, thanks for unblocking the test. It seems that @lleshchi can not be assigned, could you help assign another folk who's willing to review? Besides, can you help review the changes? @alvaroaleman Much appreciate. |
Looks like it was caused by a flaky test. Let me rerun it. /retest |
Hi @FillZpp, thanks for your review. I have revised the code as you suggested. Would you mind taking a another look?f |
/lgtm |
/assign alvaroaleman |
pkg/client/fake/client.go
Outdated
return fmt.Errorf("refusing to create obj %s with metadata.deletionTimestamp but no finalizers", accessor.GetName()) | ||
} | ||
} else { | ||
if accessor.GetDeletionTimestamp() != nil && pointer.Int64Deref(accessor.GetDeletionGracePeriodSeconds(), 0) <= 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be invalid to have DeletionGracePeriodSeconds: 0
? The only check I see in the upstream code is this: https://github.com/kubernetes/kubernetes/blob/7935006af2925dc081605c17fcb5e656cedee286/pkg/registry/core/pod/strategy.go#L187
From what I can tell it allows the value to be negative and will then set it to 0 when executing the deletion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When DeletionGracePeriodSeconds
is non-zero it means the Pod can be observed because it will be deleted after that grace period. That means one can see the deleting Pod in the time range [deleted timestamp, deleted timestamp + grace period seconds)
.
From what I can tell it allows the value to be negative and will then set it to 0 when executing the deletion
I think it will be set to 1.
if period < 0 {
period = 1
}
// https://github.com/kubernetes/kubernetes/blob/7935006af2925dc081605c17fcb5e656cedee286/pkg/registry/core/pod/strategy.go#L163-L194 | ||
if gvk.Group == corev1.GroupName && gvk.Kind == "Pod" && gvk.Version == "v1" { | ||
pod := obj.(*corev1.Pod) | ||
// If the pod is not scheduled, it will be deleted immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think the following two assertions were copied from the upstream:
// if the pod is not scheduled, delete immediately
if len(pod.Spec.NodeName) == 0 {
period = 0
}
// if the pod is already terminated, delete immediately
if pod.Status.Phase == api.PodFailed || pod.Status.Phase == api.PodSucceeded {
period = 0
}
panic(fmt.Errorf("failed to get gvk for object %T: %w", obj, err)) | ||
} | ||
|
||
if !allowsGracefulDelete(gvk, obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if allowsGracefulDelete
is true
we will never delete the object? That seems wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't mean it will never delete the object but we can observe it at some certain time. The fake client can not simulate the graceful deletion now so I think it's reasonable to at least make it possible to observe such object for covering more cases within test cases.
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arkbriar 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 |
Rebased to resolve conflicts. |
Signed-off-by: arkbriar <[email protected]>
Signed-off-by: arkbriar <[email protected]>
Signed-off-by: arkbriar <[email protected]>
Signed-off-by: arkbriar <[email protected]>
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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. |
The change here allows user to build a fake client with graceful deleting Pods, which have their
.metadata.deletionTimestamp
set and have no finalizers. This is a special case allowed in the kube-apiserver.The PR reflects the behaviour precisely according the strategy code from Kubernetes.
It should fix the #2359.