[OpenShift] Adding property to set workspaces memory request on OpenShift

[l0rd]

Adding a new property che.openshift.workspace.memory.request. If this property is set it will be used in the spec of the workspace containers (c.f. OpenShift documentation on memory requests).

This fixes "Insufficient memory" problem we currently have on Minishift.

Changelog

Adding property to set workspaces memory request on OpenShift

[codenvy-ci]

Can one of the admins verify this patch?

[skabashnyuk]

wdyt about adding measurement to the name of property like che.workspace.default_memory_mb=1024?

[l0rd]

@skabashnyuk we want to stay as close as possible to openshift spec and there the unit of measure is part of the value (e.g. che.workspace.default_memory=1024Mi). But that may eventually be a good idea because it would be consistent with other che properties. @amisevsk what do you think?

[amisevsk]

@skabashnyuk @l0rd The String used for this property can be anything that OpenShift will accept, as it is passed pretty much directly to OpenShift. I worry that including units could cause more confusion, since e.g. setting the property to 1024Mi can result in OpenShift returning 1Gi when you get the limit from OpenShift. If this value needs to be used for anything other than passing it to OpenShift, it would probably need pretty careful handling no matter what we specify as a unit.

Perhaps a better solution is to add documentation to the che.properties file specifying the constraints on these Strings?

[amisevsk]

LGTM. I've added a few questions but think it's good to merge as-is.

[amisevsk]

WDYT about leaving the cpu.limit in place? It does make things longer and less readable, but in terms of a general solution it would be useful to be able to specify this value, even if it doesn't apply to our specific case. It would be handy e.g. if running on minishift with a simple quota in place.

[l0rd]

@amisevsk Yes I think it makes sense to have cpu limit and request. Even if we don't need that right now we may need it later. But I would rather add it in second PR.

[amisevsk]

Since there is some confusion about the format of these properties, maybe we can link to the OpenShift documentation in the description. This does have the problem of having to keep that link up to date though.

[l0rd]

I've updated the che.properties pointing to the documentation

[garagatyi]

@amisevsk We usually don't hesitate to explicitly set measurement unit in name of a property. Example https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties#L91-L91

I agree that allowing to set it is quite error prone.
BTW if you choose to use single measurement unit instead of clever, please, add it into property name - it is not official rule for the project that makes properties consistent.