CHE-220: Improving 'removeContainer' API. Refactoring #5766
Conversation
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
| .withName(deploymentName); | ||
|
|
||
| if (deployment != null) { | ||
| deployment.scale(0, true); |
There was a problem hiding this comment.
Does waiting for the scale down take significantly longer than the previous way, or does the scale down take grace termination period into account? I ask because we had the issue on OpenShift where stopping and restarting a pod quickly could case PV issues.
There was a problem hiding this comment.
there should not be PV issues since this change was applied in openshift-connector rebased, but grace termination period is taken into account
There was a problem hiding this comment.
@amisevsk btw in master I do not see that we use "terminationGracePeriodSeconds", So while testing it on minishift scale down does not happen fast and require some time - this might be the main reason of quota limit issue that was reported today
There was a problem hiding this comment.
I think this PR is not applied to master #5007
But this is a separate conversation, I will provide another PR for that
There was a problem hiding this comment.
Sounds good - thanks @ibuziuk !
| List<ReplicaSet> replicaSets = KubernetesResourceUtil.getReplicaSetByLabel(OpenShiftConnector.OPENSHIFT_DEPLOYMENT_LABEL, deploymentName, namespace); | ||
|
|
||
| try (OpenShiftClient openShiftClient = new DefaultOpenShiftClient()) { | ||
| if (replicaSets != null && replicaSets.size() > 0) { |
There was a problem hiding this comment.
I worry about the order of removal here, since replica sets are controlled by deployments. It shouldn't be a problem since the deployment is scaled to zero first, but I think the deployment should be deleted before the replica set just in case. IIRC there are cases where a deployment will ensure a replica set exists, causing it to maybe be recreated before the deployment is deleted.
There was a problem hiding this comment.
ok, lemme change the order
| @@ -165,6 +162,7 @@ | |||
| public static final String OPENSHIFT_DEPLOYMENT_LABEL = "deployment"; | |||
| public static final String CHE_MOUNTED_WORKSPACE_FOLDER = "/workspace-logs"; | |||
| public static final String WORKSPACE_LOGS_FOLDER_SUFFIX = "-logs"; | |||
| public static final int OPENSHIFT_WAIT_POD_TIMEOUT = 240; | |||
There was a problem hiding this comment.
It may be worth it to leave this private and just have a similar field in OpenShiftDeploymentCleaner. There's no reason the timeout on scaling down deployments needs to be the same as the one for waiting for containers to start (in fact, we may want the timeout for scaling down to be less than 4 minutes, that seems like a long time). Ideally, the scale down happens in under 5 seconds.
There was a problem hiding this comment.
@amisevsk good point, but I'm not sure about 5 seconds - in che starter[1] stop timeout is 150 ms
There was a problem hiding this comment.
well, 150 ms is under 5 seconds ;)
Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
|
@amisevsk PR has been updated |
|
@ibuziuk can you please test on OSIO if this PR solves redhat-developer/rh-che#217? |
|
PR for grace period has been sent - #5773 |
Signed-off-by: Ilya Buziuk ibuziuk@redhat.com
What does this PR do?
Enhances OpenShift resource deletion for workspaces (scaling down deployment before deleting resources)
What issues does this PR fix or reference?
https://issues.jboss.org/browse/CHE-220
Changelog
Enhances OpenShift resource deletion for workspaces (scaling down deployment before deleting resources)
Release Notes
N/A
Docs PR
N/A