che #9062: Adding property for setting workspace pod termination grace period for Kubernetes / OpenShift infrastructures #9118
Conversation
ibuziuk
requested review from
garagatyi,
riuvshin and
skabashnyuk
as code owners
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
ci-test |
benoitf
added
status/code-review
| @@ -91,5 +95,8 @@ public void provision(KubernetesEnvironment k8sEnv, RuntimeIdentity identity) | |||
| uniqueNamesProvisioner.provision(k8sEnv, identity); | |||
| ramLimitProvisioner.provision(k8sEnv, identity); | |||
| securityContextProvisioner.provision(k8sEnv, identity); | |||
|
|
|||
| // 4 stage - set Kubernetes pod termination grace period | |||
| podTerminationGracePeriodProvisioner.provision(k8sEnv, identity); | |||
There was a problem hiding this comment.
I'd say that it should be in 3 stage like restartPolicyRewriter and securityContextProvisioner.
There was a problem hiding this comment.
thanks for the pointer will fix
| public void provision(KubernetesEnvironment k8sEnv, RuntimeIdentity identity) | ||
| throws InfrastructureException { | ||
| for (Pod pod : k8sEnv.getPods().values()) { | ||
| if (!isTerminationGracePeriodSet(pod)) { |
There was a problem hiding this comment.
Maybe it would be better to override user-defined grace period. And do not override only if the grace period is configured to 0.
If not then it makes sense to document it and maybe rename configuration property like default_termination_grace_period_sec.
I vote for overriding user-defined grace period.
@garagatyi @ibuziuk WDYT?
There was a problem hiding this comment.
Not sure we should override the value specified by a user. What for? I opt for overriding only values that are not specified. About renaming I think current name is OK, and name with prefix default would be OK too.
In case we need to set it to 0, in any case, we will be able to add it as a separate flag later.
There was a problem hiding this comment.
What for?
To be able to guarantee that workspace will be stopped(or start will be interrupted) in predictable time.
There was a problem hiding this comment.
And what if user's app needs more time, e.g. 1 minute?
There was a problem hiding this comment.
The situation is similar to the following: a user's container needs more time to start than workspace start timeout is configured.
Since removing of pods is the last phase of workspace stop, and we remove all pods asynchronously, I'm OK not to override a value defined by a user. But let's document it in che.properties.
There was a problem hiding this comment.
+1 to document it. Let's start without rewriting user's timeouts and then we will figure out whether we need to change it
| @@ -416,6 +416,13 @@ che.infra.kubernetes.ingress.annotations_json=NULL | |||
| che.infra.kubernetes.pod.security_context.run_as_user=NULL | |||
| che.infra.kubernetes.pod.security_context.fs_group=NULL | |||
|
|
|||
| # Defines grace termination period for pods that will be created by Kubernetes / OpenShift infrastructures | |||
| # | |||
| # Grace termination period of Kubernetes / OpenShift pods defaults to 30 seconds, which results in | |||
There was a problem hiding this comment.
Doc says defaults to 30 seconds but default value is
che.infra.kubernetes.pod.termination_grace_period_sec=0
Please check and sync these values
There was a problem hiding this comment.
well, default to 0 is done deliberately and I think we should stick to it (which would significantly decrease time required for workspace stop on k8s / openshift). Probably I should just remove info about default k8s config, which you find misleading
|
@eivantsov Should we add some changes in docs? WDYT? |
| import org.eclipse.che.api.workspace.server.spi.InfrastructureException; | ||
| import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment; | ||
|
|
||
| public class PodTerminationGracePeriodProvisioner implements ConfigurationProvisioner { |
| public void provision(KubernetesEnvironment k8sEnv, RuntimeIdentity identity) | ||
| throws InfrastructureException { | ||
| for (Pod pod : k8sEnv.getPods().values()) { | ||
| if (!isTerminationGracePeriodSet(pod)) { |
There was a problem hiding this comment.
Not sure we should override the value specified by a user. What for? I opt for overriding only values that are not specified. About renaming I think current name is OK, and name with prefix default would be OK too.
In case we need to set it to 0, in any case, we will be able to add it as a separate flag later.
|
Yes, a few lines here https://github.com/eclipse/che-docs/blob/master/src/main/pages/setup-openshift/openshift-config.md won't hurt |
|
@eivantsov ok, thanks for the pointer - will add some docs |
|
@eivantsov @skabashnyuk @sleshchenko PR has been updated. PR for docs eclipse-che/che-docs#376 |
…ce period for Kubernetes / OpenShift infrastructures Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
benoitf
removed
the
status/code-review
What does this PR do?
che #9062: Adding property for setting workspace pod termination grace period for Kubernetes / OpenShift infrastructures
What issues does this PR fix or reference?
#9062
Release Notes
Adding property for setting workspace pod termination grace period for Kubernetes / OpenShift infrastructures
Docs PR
eclipse-che/che-docs#376