Skip to content
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

Remove che server properties that are outdated #21411

Open
l0rd opened this issue May 18, 2022 · 25 comments
Open

Remove che server properties that are outdated #21411

l0rd opened this issue May 18, 2022 · 25 comments
Assignees
Labels
area/che-server kind/technical-debt Technical debt issue lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. severity/P2 Has a minor but important impact to the usage or development of the system.

Comments

@l0rd
Copy link
Contributor

l0rd commented May 18, 2022

Is your enhancement related to a problem? Please describe

In today documentation we mention dozens of configuration options (sourced directly from che-server properties file) that are not used anymore and are confusing for admins that may set them and won't understand why they don't have any effect.

Describe the solution you'd like

Review che.properties file and identify:

  • properties that don't have any effect on a Che instance anymore and should be removed or hided in documentation
  • properties that enabled some features that we don't have anymore and that we should provide in dashboard or DevWorkspace Operator

Describe alternatives you've considered

No response

Additional context

No response

@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label May 18, 2022
@l0rd l0rd mentioned this issue May 18, 2022
49 tasks
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label May 18, 2022
@ibuziuk ibuziuk added kind/technical-debt Technical debt issue severity/P1 Has a major impact to usage or development of the system. and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. kind/enhancement A feature request - must adhere to the feature request template. labels May 18, 2022
@ibuziuk ibuziuk mentioned this issue Jun 7, 2022
64 tasks
@ibuziuk ibuziuk mentioned this issue Jun 27, 2022
68 tasks
@ibuziuk ibuziuk mentioned this issue Jul 15, 2022
51 tasks
@ibuziuk
Copy link
Member

ibuziuk commented Jul 18, 2022

one of the subtask for this issue could be removing the PVC configuration on the che-server end [1] che.infra.kubernetes.pvc.strategy
Basically, with DWO in place che-server should not handle the PVC creation / configuration at all.

eclipse-che/che-operator#1442 (comment) cc: @AObuchow

[1] https://github.com/eclipse-che/che-server/blob/main/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties#L375-L392

@ibuziuk ibuziuk mentioned this issue Aug 5, 2022
67 tasks
@l0rd
Copy link
Contributor Author

l0rd commented Sep 16, 2022

Downstream issue

@AObuchow
Copy link

AObuchow commented Sep 23, 2022

So far, the following are the properties I've identified as outdated/obsolete, as well as some very brief reasoning. It's worth noting that for DWO to have feature parity with che-server, the below-mentioned fields which come from DWO's constants file should probably be replaced with fields in the DWOC.


# Your projects are synchronized from the {prod-short} server into the machine running each
# workspace. This is the directory in the machine where your projects are placed.
che.workspace.projects.storage=/projects

Replaced by constants.DefaultProjectsSourcesRoot in DWO


# Defines the directory inside the machine where all the workspace logs are placed.
# Provide this value into the machine, for example, as an environment variable.
# This is to ensure that agent developers can use this directory to back up agent logs.
che.workspace.logs.root_dir=/workspace_logs

From what I understand, workspace logs are retrieved by doing a oc logs -f <workspace pod name> -n $NAMESPACE <container name>


# Configures environment variable HTTP_PROXY to a specified value in containers powering workspaces.
che.workspace.http_proxy=

# Configures environment variable HTTPS_PROXY to a specified value in containers powering workspaces.
che.workspace.https_proxy=

# Configures environment variable NO_PROXY to a specified value in containers powering workspaces.
che.workspace.no_proxy=

replaced by the ProxyConfig in the DWOC


# Defines wait time that limits the Kubernetes workspace start time.
che.infra.kubernetes.workspace_start_timeout_min=8

Replaced by DWOC ProgressTimeout field


# Defines image pull strategy for sidecars. Possible values are: `Always`,
# `Never`, `IfNotPresent`. For any other value, `Always` is assumed for images
# with the `:latest` tag, or `IfNotPresent` for all other cases.
che.workspace.sidecar.image_pull_policy=Always

Replaced by DWOC ImagePullPolicy field.


# Period of inactive workspaces suspend job execution.
che.workspace.activity_check_scheduler_period_s=60

Replaced by DWOC IdleTimeout field, though I am not sure.


# Optionally configures node selector for workspace pod. The format is comma-separated
# `key=value` pairs, for example: `disktype=ssd,cpu=xlarge,foo=bar`
che.workspace.pod.node_selector=NULL

# Optionally configures tolerations for workspace pod. The format is a string representing a JSON Array of taint tolerations,
# or `NULL` to disable it. The objects contained in the array have to follow the
# link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#toleration-v1-core[toleration v1 core specifications].
# For example: `[{"effect":"NoExecute","key":"aNodeTaint","operator":"Equal","value":"aValue"}]`
che.workspace.pod.tolerations_json=NULL

Replaced by attributes added in Namespace pod tolerations by amisevsk · Pull Request #696 · devfile/devworkspace-operator · GitHub


# By default, when users access a workspace with its URL, the workspace
# automatically starts (if currently stopped). Set this to `false` to disable this behavior.
che.workspace.auto_start=true

From what I understand, workspace phases (running and stopped) and their URLS are handled by the devworkspace controller and devworkspace routing controller


# Workspace threads pool configuration. This pool is used for workspace-related
# operations that require asynchronous execution, for example, starting and stopping.
# Possible values are: `fixed` and `cached`.
che.workspace.pool.type=fixed

Starting and stopping workspaces is handled by DWO.


# This property is ignored when pool type is different from `fixed`.
# It configures the exact size of the pool. When set, the `multiplier` property is ignored.
# If this property is not set (`0`, `<0`, `NULL`), then the pool size equals the number of cores.
# See also `che.workspace.pool.cores_multiplier`.
che.workspace.pool.exact_size=30

# This property is ignored when `che.workspace.pool.exact_size`
# is set and pool type is not set to `fixed`. When set, the pool size is `N_CORES * multiplier`.
che.workspace.pool.cores_multiplier=2

Related to che.workspace.pool.type


# This property specifies how many threads to use for workspace server liveness probes.
che.workspace.probe_pool_size=10

# Number of sequential successful pings to a server after which it is treated as available.
# For the {prod-short} Operator, the property is common for all servers, for example: `workspace agent`, `terminal`, and `exec`.
che.workspace.server.ping_success_threshold=1

# Interval (in milliseconds) between successive pings to a workspace server.
che.workspace.server.ping_interval_milliseconds=3000

Workspace liveness (started, stopped or failed detection is provided by DWO.


# RAM limit for each sidecar that has no RAM settings in the {prod-short} plug-in configuration.
# Value less than or equal to `0` is interpreted as disabling the limit.
che.workspace.sidecar.default_memory_limit_mb=128

Replaced by DWO's constants.SidecarDefaultMemoryLimit.


# RAM request for each sidecar that has no RAM settings in the {prod-short} plug-in configuration.
che.workspace.sidecar.default_memory_request_mb=64

Replaced by DWO's constants.SidecarDefaultMemoryRequest.


# CPU limit default for each sidecar that has no CPU settings in the {prod-short} plug-in configuration.
# Specify it either in floating point cores or in integer millicores, for example: `0.125` or `125m`.
# Value less than or equal to `0` is interpreted as disabling the limit.
che.workspace.sidecar.default_cpu_limit_cores=-1

Replaced by DWO's constants.SidecarDefaultCpuLimit.


# CPU request default for each sidecar that has no CPU settings in the {prod-short} plug-in configuration.
# Specify it either in floating point cores or in integer millicores, for example: `0.125` or `125m`.
che.workspace.sidecar.default_cpu_request_cores=-1

Replaced by DWO's constants.SidecarDefaultCpuRequest.


If any of these properties are not safe for removal, please let me know - thank you!
CC: @amisevsk @tolusha @ibuziuk @l0rd

@tolusha
Copy link
Contributor

tolusha commented Sep 23, 2022

  1. I've rechecked the properties above and they are safe to delete.

@tolusha
Copy link
Contributor

tolusha commented Sep 23, 2022

  1. There are bunch of properties in multiuser.properties file:
  • Che system section. I believe it can be removed, but we have mentions in che-docs
    • che.system.admin_name
    • che.system.super_privileged_mode
  • Workspace limits section:
    • che.limits.workspace.env.ram IDK
    • che.limits.workspace.idle.timeout can be configured from CheCluster CR spec.devEnvironments.secondsOfInactivityBeforeIdling
    • che.limits.workspace.run.timeout can be configured from CheCluster CR spec.devEnvironments.secondsOfRunBeforeIdling
  • Users workspace limits section:
    • che.limits.user.workspaces.ram IDK
    • che.limits.user.workspaces.run.count Can be configured from CheCluster CR spec.components.devWorkspace.runningLimit
    • che.limits.user.workspaces.count IDK
  • Properties from Organizations workspace limits can be deleted, since there is no Organization notion anymore.
  • Properties from Keycloak configuration and Multi-user-specific OpenShift infrastructure configuration can be deleted since there is no Keycloak anymore.

@tolusha
Copy link
Contributor

tolusha commented Sep 23, 2022

  1. With regard to che.properties

Besides that:
Both properties are used to configure WorkspaceActivityChecker and can be deleted:

che.workspace.activity_cleanup_scheduler_period_s
che.workspace.activity_cleanup_scheduler_initial_delay_s

Both properties are used to configure TemporaryWorkspaceRemover and be deleted:

che.workspace.cleanup_temporary_period_min
che.workspace.cleanup_temporary_initial_delay_min

The following properties are not used:

che.workspace.maven_options
che.workspace.java_options

It seems they are not used, but I am not 100% sure:

che.workspace.default_cpu_request_cores
che.workspace.default_cpu_limit_cores
che.workspace.default_memory_request_mb
che.workspace.default_memory_limit_mb

I believe they can be replaced with https://devfile.io/docs/2.1.0/limiting-resources-usage:

che.workspace.sidecar.default_cpu_request_cores
che.workspace.sidecar.default_cpu_limit_cores
che.workspace.sidecar.default_memory_request_mb
che.workspace.sidecar.default_memory_limit_mb

The following properties are not used:
che.workspace.plugin_broker.*
che.server.secure_exposer.jwtproxy.*

@AObuchow
Copy link

Thank you so much @tolusha for confirming those properties are safe to remove & for pointing to more properties that can be removed :)

@tolusha
Copy link
Contributor

tolusha commented Sep 26, 2022

BTW, There are some bunch of internal properties which make no sense to expose to end user as well.

che.api.*
che.websocket.*
schedule.core_pool_size
db.schema.*

I think we can add some kind of annotation to theirs descriptions like # @Internal or # @Deprecated (if we are talking about outdated properties) and take it into account while parsing content https://github.com/eclipse-che/che-docs/blob/b06ea3335df56663473eafc8c053c523365329f9/tools/environment_docs_gen.sh#L54

@ibuziuk
Copy link
Member

ibuziuk commented Oct 4, 2022

@AObuchow I would probably split the removal of the properties to a couple of PRs by domain similar to what has been done for storage related properties - eclipse-che/che-server#330

che.workspace.* indeed is the next good candidate ;-)

@ibuziuk
Copy link
Member

ibuziuk commented Nov 3, 2022

the removal of all the outmoded properties could take some time, and I would propose for the time being not publish them at all as part of Eclipse Che / Dev Spaces docs - #21804 cc: @AObuchow @themr0c

@nickboldt
Copy link
Contributor

is che.infra.kubernetes.ingress_start_timeout_min also obsolete? Asking for https://chat.google.com/room/AAAAm3qdpQU/zY6f27THlNM

@AObuchow
Copy link

is che.infra.kubernetes.ingress_start_timeout_min also obsolete? Asking for chat.google.com/room/AAAAm3qdpQU/zY6f27THlNM

I may be mistaken, but I think it is obsolete, as we use the minikube ingress addon with devworkspace operator. I'm not sure what the procedure is for other Kubernetes clusters, though. @amisevsk may be able to confirm.

@amisevsk
Copy link
Contributor

I'm not entirely sure if that property is still used -- it's documented as

# Defines the timeout (in minutes) that limits the period for which {orch-ingress} becomes ready.
che.infra.kubernetes.ingress_start_timeout_min=5

Reading the GChat discussion, I believe the goal is to increase the start timeout for workspaces -- this used to be done via CHE_INFRA_KUBERNETES_WORKSPACE_START_TIMEOUT_MIN, which is no longer used. Start timeout is currently managed by DWO and can be configured via the DWOC; I've created issue #21820 to track enabling it to be configured via CheCluster.

@nickboldt
Copy link
Contributor

See related PR: eclipse-che/che-server#442

@mkuznyetsov
Copy link
Contributor

See related PR: eclipse-che/che-server#442

in making this PR, I have come to the conclusion that the following properties seem to be unused:
che.factory.default_editor
che.workspace.devfile.default_editor

as default editor is being set in Che CR already , and usecases known to me of creating factories indeed seems to ignore these properties. So perhaps we can safely remove them and rewrite the code that still uses them?

@ibuziuk ibuziuk assigned vinokurig and unassigned AObuchow Apr 19, 2023
@max-cx
Copy link

max-cx commented Apr 25, 2023

@max-cx
Copy link

max-cx commented Apr 25, 2023

@l0rd, cc @vinokurig

Hi Mario,

Looks like https://access.redhat.com/documentation/en-us/red_hat_openshift_dev_spaces/3.1/html/administration_guide/configuring-che#understanding-devspaces-server-advanced-configuration (that led you to open this issue) was removed since you opened this issue, as you can see in https://access.redhat.com/documentation/en-us/red_hat_openshift_dev_spaces/3.5/html-single/administration_guide/index#advanced-configuration-options-for-the-devspaces-server-component-understanding-devspaces-server-advanced-configuration. So I have three questions:

Question 1: Is this issue a duplicate of #21804?

Question 2: Do you want us (@vinokurig or a technical writer) to simply add a link (rather than pull and republish that info) to https://github.com/eclipse-che/che-server/blob/main/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties, and same for its downstream counterpart file (if any), somewhere in the docs?

Question 3: Do you still want @vinokurig to go through https://github.com/eclipse-che/che-server/blob/main/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties and update it (even despite the fact that we no longer have that info in the docs)?

@max-cx
Copy link

max-cx commented Apr 26, 2023

@max-cx
Copy link

max-cx commented Apr 26, 2023

Additionally, it is not clear to me whether the tables in https://www.eclipse.org/che/docs/stable/administration-guide/checluster-custom-resource-fields-reference/ are up to date. This might require opening a separate issue but still worth raising here.

@l0rd
Copy link
Contributor Author

l0rd commented Apr 27, 2023

@max-cx this issue is about updating the server properties so that we can resume publishing them in the doc. From a priority point of view though, I don't think this is critical. I haven't heard any complain about some che-server property not being documented. Setting severity/P2. Please feel free to revert it to P1 if you have any customer / user request for that.

@l0rd l0rd added severity/P2 Has a minor but important impact to the usage or development of the system. and removed severity/P1 Has a major impact to usage or development of the system. labels Apr 27, 2023
@max-cx
Copy link

max-cx commented Apr 28, 2023

@vinokurig and @l0rd, can @vinokurig just update the server properties file(s) (for both upstream and downstream) so that all the info in the file(s) is correct? After that, you let us know that the file is updated, and then a writer can add a link to that file in the docs. WDYT?

IMO, the properties file is easier to read on GitHub. In our docs, esp. the downstream docs, the formatting that is added makes it voluminous and difficult to scroll through.

@l0rd
Copy link
Contributor Author

l0rd commented Apr 28, 2023

@max-cx cleaning up che.properties is not easy (this issue has been open for 1 year) and the benefits are not clear. So until we get a customer/user request for that I would put it on hold.

@che-bot
Copy link
Contributor

che-bot commented Oct 25, 2023

Issues go stale after 180 days of inactivity. lifecycle/stale issues rot after an additional 7 days of inactivity and eventually close.

Mark the issue as fresh with /remove-lifecycle stale in a new comment.

If this issue is safe to close now please do so.

Moderators: Add lifecycle/frozen label to avoid stale mode.

@che-bot che-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2023
@amisevsk
Copy link
Contributor

/remove-lifecycle stale

@amisevsk amisevsk added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Oct 25, 2023
@che-bot che-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/che-server kind/technical-debt Technical debt issue lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. severity/P2 Has a minor but important impact to the usage or development of the system.
Projects
None yet
Development

No branches or pull requests