Fix fluent dsl #1827
Fix fluent dsl #1827
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
rohanKanojia
added
the
changelog missing
| @@ -922,13 +922,13 @@ public OperationInfo forOperationType(String type) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Deletable<Boolean> withGracePeriod(long gracePeriodSeconds) | |||
| public FilterWatchListDeletable<T, L, Boolean, Watch, Watcher<T>> withGracePeriod(long gracePeriodSeconds) | |||
There was a problem hiding this comment.
Since, it works without breaking anything I am fine with it.
I don't see however, how it solves the two methods being mutually exclusive.
There was a problem hiding this comment.
withGracePeriod now return a Deletable and on it you can only call delete, you need to do an unsafe cast to be able to chain further methods (i.e. such as withPropagationPolicy)
There was a problem hiding this comment.
added a "demonstration" test for it
|
added changelog and tests for the changes. |
| @@ -39,7 +39,8 @@ | |||
| protected boolean cascading; | |||
| protected boolean reloadingFromServer; | |||
|
|
|||
| protected long gracePeriodSeconds; | |||
| // Default to k8s 30s value: https://kubernetes.io/docs/concepts/workloads/pods/pod/#termination-of-pods | |||
| protected long gracePeriodSeconds = 30L; | |||
There was a problem hiding this comment.
I run into this issue today when I found out that when deleting pods the termination grace period in my pods is always overwritten to 0 and the pods are basically force deleted.
However, I'm wondering if this is the right fix. It still overwrites whatever the user configured in his pod which is IMHO wrong. I think the default should be -1 which will mean that the gracePeriod is not set in the request and the default from the resource will be used by Kubernetes => that will be either 30 seconds if the user didn't specified anything or whatever the user specified in his pods.
There was a problem hiding this comment.
haven't had time to test the grace period to -1, but, if it works, I see your concern and I agree on the approach.
There was a problem hiding this comment.
this PR got already merged, do you mind to follow up?
There was a problem hiding this comment.
@andreaTP : oops, my bad. I didn't go through this conversation. I would appreciate if you could test it with gracePeriod set to -1 and create a follow up PR
There was a problem hiding this comment.
I opened #1833 to change the default grace period value.
rohanKanojia
removed
the
changelog missing
|
[merge] |
The return type of
withGracePeriodand ofwithPropagationPolicyprevent additional composition (and make the two settings mutually exclusive ATM).The
gracePerioddefault to0Lusing the empty constructor ofOperationContextwhich is possibly harmful (e.g. it will trigger a force kill).