[feature] CRW-11940: Add CLI Activity Tracker configuration to CheCluster CRD - #2170
[feature] CRW-11940: Add CLI Activity Tracker configuration to CheCluster CRD#2170vrubezhny wants to merge 1 commit into
Conversation
|
@vrubezhny: This pull request references CRW-11940 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vrubezhny The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest-required |
db8b71d to
c1c3c1e
Compare
|
|
||
| // CLI Watcher configuration for preventing workspace idling during active CLI processes. | ||
| // +optional | ||
| CliWatcher *CliWatcherConfig `json:"cliWatcher,omitempty"` |
There was a problem hiding this comment.
Do we have some better naming?
activityMonitor ?
cc @ibuziuk
There was a problem hiding this comment.
Note that's it's actually not for the whole 'activityMonitor`-thing, but only a part of what we have for Activity Tracking related to only user CLI processes running in Terminal.
So, maybe still something like cliActivityMonitor?
There was a problem hiding this comment.
I'm OK with both names if they keep mentioning that's these properties are for CLI processes that need to be tracked/monitored.
And I'd changed the env. variable names that are to be generated in ConfigMaps accordingly - like CLI_ACTIVITY_MONITOR_* or CLI_ACTIVITY_TRACKER_*.
There was a problem hiding this comment.
In the latest version I've changed it to use cliActivityTracker and CLI_ACTIVITY_TRACKER_*.
We can change this later at any time before we merge it, if any objections.
|
/che-ai-assistant ok-pr-review Task completed. |
tolusha
left a comment
There was a problem hiding this comment.
Consider omitting cliWatcher from the top-level DevEnvironments default annotation
The +kubebuilder:default:= annotation on the DevEnvironments field now includes cliWatcher: {enabled: false}. This means every existing CheCluster CR will get cliWatcher: {enabled: false} injected by the API server's defaulting webhook on CRD upgrade, which shows up as a noisy diff in CR audits. The CliWatcher field is already a pointer with omitempty, and the controller already guards it with if checluster.Spec.DevEnvironments.CliWatcher != nil, so the feature stays off without the struct-level default. The +kubebuilder:default:=false on Enabled still applies when a user explicitly creates a cliWatcher: section.
Document actual defaults that che-machine-exec applies when numeric fields are absent
The CRD field comments say "Set to -1 to use the default value calculated by che-machine-exec" but do not state what those values actually are. Would it be possible to add a brief note (e.g., "defaults to 30s") so admins can reason about resource consumption without having to read che-machine-exec source?
The default values are removed from the spec, it's up to che-machine-exec to dynamically calculate them in function of the
Each field is described and possible (but not guaranteed defaults has given as examples), also the consequence of omitting a property or setting it to |
c1c3c1e to
e9cdae0
Compare
…ster CRD Add spec.devEnvironments.cliActivityTracker section to expose CLI Activity Tracker settings at the CheCluster CR level. The operator propagates these as CLI_ACTIVITY_TRACKER_* env vars via the che-user-settings ConfigMap to all workspace containers. Issue: https://issues.redhat.com/browse/CRW-11940 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com> Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
e9cdae0 to
56bb729
Compare
This PR adds
spec.devEnvironments.cliActivityTrackersection to the CheCluster CRD, allowing administrators to enable and configure CLI Activity Tracker settings cluster-wide. When enabled, the operator writesCLI_ACTIVITY_TRACKER_*environment variables to theche-user-settingsConfigMap, which is auto-mounted to all workspace containers via DevWorkspace Operator. che-machine-exec reads these env vars and uses them as admin-level configuration for monitoring active CLI processes and preventing workspace idling.What does this PR do?
Adds
spec.devEnvironments.cliActivityTrackersection to the CheCluster CRD, allowing administrators to enable and configure CLI Activity Tracker settings cluster-wide.New CRD fields under
spec.devEnvironments.cliActivityTracker:enabled*boolsecondsOfCheckPeriod*int32secondsOfActivityWindow*int32secondsOfGracePeriod*int32secondsOfMaxProcessAge*int32All timing fields are optional. When omitted, set to -1, or set to 0, che-machine-exec calculates defaults dynamically based on the workspace idle timeout. Minimum validation is set to -1 for all timing fields.
How it works:
spec.devEnvironments.cliActivityTrackerfrom the CheCluster CRenabledis explicitlytrue, it writesCLI_ACTIVITY_TRACKER_*env vars to theche-user-settingsConfigMapenabledisfalse,nil, or the entire section is omitted, no env vars are writtenEnv var mapping:
enabledCLI_ACTIVITY_TRACKER_ENABLEDsecondsOfCheckPeriodCLI_ACTIVITY_TRACKER_CHECK_PERIODsecondsOfActivityWindowCLI_ACTIVITY_TRACKER_ACTIVITY_WINDOWsecondsOfGracePeriodCLI_ACTIVITY_TRACKER_GRACE_PERIODsecondsOfMaxProcessAgeCLI_ACTIVITY_TRACKER_MAX_PROCESS_AGEScreenshot/screencast of this PR
N/A — no UI changes. This is a CRD and controller change.
What issues does this PR fix or reference?
How to test this PR?
Tests added:
TestCreatesDataInNamespaceextended with CLI Activity Tracker assertions including -1 and nil field handlingTestOptionalFeaturesOmitEnvVarsverifies no env vars are written when the feature is disabled (nil, false, or nil-enabled with timing fields set)Manual testing:
kubectl patch checluster/eclipse-che -n eclipse-che --type=merge \ -p '{"spec":{"devEnvironments":{"cliActivityTracker":{"enabled":true,"secondsOfCheckPeriod":30,"secondsOfActivityWindow":900}}}}'che-user-settingsConfigMap in the user namespace containsCLI_ACTIVITY_TRACKER_ENABLED=true,CLI_ACTIVITY_TRACKER_CHECK_PERIOD=30,CLI_ACTIVITY_TRACKER_ACTIVITY_WINDOW=900secondsOfGracePeriod) do not appear in the ConfigMapenabled: falseor remove thecliActivityTrackersection — verify noCLI_ACTIVITY_TRACKER_*env vars in the ConfigMapOpenShift
or
on Minikube
Common Test Scenarios
PR Checklist
As the author of this Pull Request I made sure that:
Reviewers
Reviewers, please comment how you tested the PR when approving it.