feat: CheCluster API v2 #1324

tolusha · 2022-02-17T12:39:21Z

Signed-off-by: Anatolii Bazko [email protected]

What does this PR do?

add CheCluster API v2

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#20758

How to test this PR?

N/A

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.

openshift-ci · 2022-02-17T12:39:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2022-02-17T12:42:10Z

Codecov Report

Merging #1324 (f379e5c) into main (16c92ec) will increase coverage by 1.03%.
The diff coverage is 61.74%.

❗ Current head f379e5c differs from pull request most recent head 72fe63a. Consider uploading reports for the commit 72fe63a to get more accurate results

@@            Coverage Diff             @@
##             main    #1324      +/-   ##
==========================================
+ Coverage   62.00%   63.04%   +1.03%     
==========================================
  Files          76       70       -6     
  Lines        6301     5812     -489     
==========================================
- Hits         3907     3664     -243     
+ Misses       2008     1785     -223     
+ Partials      386      363      -23

Impacted Files	Coverage Δ
controllers/che/checluster_controller.go	`0.00% <0.00%> (ø)`
controllers/che/checluster_validator.go	`0.00% <0.00%> (ø)`
controllers/devworkspace/solver/solver.go	`36.84% <0.00%> (ø)`
controllers/devworkspace/state.go	`3.22% <ø> (+1.18%)`	⬆️
pkg/common/operator-defaults/defaults.go	`15.97% <ø> (ø)`
pkg/common/utils/process.go	`0.00% <ø> (ø)`
pkg/common/utils/utils.go	`28.31% <ø> (ø)`
pkg/deploy/checluster.go	`56.52% <ø> (+8.37%)`	⬆️
pkg/deploy/checluster_util.go	`66.66% <ø> (ø)`
pkg/deploy/clusterrole.go	`100.00% <ø> (ø)`
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c92ec...72fe63a. Read the comment docs.

l0rd · 2022-03-25T13:41:24Z

I did a review of https://github.com/eclipse-che/che-operator/blob/20758/api/v2/checluster_types.go. Here are a few comments:

the Operator automatically creates and maintains several ConfigMaps

Can we list them?

At line 58 you should replace displayName="Devfile registry" with displayName="Dashboard"

At line 79 you should not you should remove ", the plugin and devfile registries"

What happens if server.clusterRoles is omitted? What are the default ClusterRoles?

In server.workspaceServiceAccountClusterRole description it's mentioned "The default roles are used when omitted or left blank." What are the default roles?

In server.WorkspaceNamespaceName description: "for a case when a user does not override it.". I don't think a user can override it: users cannot select the workspace namespace anymore. I would rather say: "If users namespaces are not pre-created this field defines the Kubernetes namespace created when a user starts his first workspace....". And also what happens when omitted or left blank?

Maybe server.serverTrustStoreConfigMapName should be renamed serverTrustStoreConfigMapName.javaTrustStoreConfigMapName?

It would be better if instead of the boolean server.gitSelfSignedCert we had server.gitServerTLSCertificateConfigMapName

For storage.pvcStrategy: We should mention that currently only common is supported and add a link to the issue: Persistent Volume strategies: add “per workspace” che#21185.

Isn't CheCluster.storage only related to workspaces (and database.pvc is server side)? In that case we should probably make it explicit in the comments, call it workspaceStorage and probably move it under CheCluster.server since everything related to workspaces is there.

In server.workspacesDefaultPlugins workspaces is plural whereas other fields use the singular (workspacePodNodeSelector etc...). For consistency we should rather use singular everywhere.

Do we really need database.postgresVersion? What's the default by the way?

server.workspacesDefaultPlugins should be renamed server.workspaceDefaultPluginsPerEditor

How an admin can specify the default editor?

How an admin can specify the default container image (if for instance there is no devfile.yaml in a github repository)

All server components Deployment can be overridden using DeploymentCustomSettings except the Gateway (server side) and the 3 sidecar (workspace side) where only image can be overriden. This is inconsistent.

We should mention that when gateway.ingress is specified, gateway.route should not. And that gateway.route is for OpenShift only. It would be even better if both could be merged together so that we only have gateway.ingress

The sentence "The generated host name will follow this pattern: <route-name>-<route-namespace>.<domain>" is confusing because an admin cannot specify <route-name> and <route-namespace>. I would replace that with "The generated hostname will follow this pattern: che-<che-namespace>.<domain>, where <che-namespace> is the namespace where the CheCluster CRD is created".

What about renaming route.host and ingress.host as route.hostname and ingress.hostname respectively?

We can safely replace every instance of host name with hostname

Setting the hostname is a common task. With the new model it's hidden under the auth section: auth>gateway>ingress>hostname. I am wondering if we should not rather organize things like this:

ingress
  > labels 
  > annotations 
  > domain 
  > hostname 
  > tlsSecret
  > auth
    > identityProviderURL
    > oAuthClientName
    > oAuthSecret
    > gateway
      > ...

In CheClusterSpecMetrics there is the following description: "/ Enables metrics the Che server endpoint. Default to true.". Doesn't it control DevWorkspace controller metrics too? If it's only for che-server it should be moved under server if it's for DevWorkspace it should be mentioned. And it shoudl be also mentioned that this is distinct from workspace telemetry that can be enabled through a plugin editor only.

The server.proxy is described as "// Proxy server settings" but then in the Proxy struct: This drives the appropriate changes in the JAVA_OPTS and https(s)_proxy variables in the Che server and workspaces containers. So are the proxy settings propagated to the che-server, to the workspaces or both? What about other server side components as the dashboard, the registries etc...?

server.customCheProperties should be server.customCheServerProperties (server.extraProperties || server.additionalProperties)

server.debug is a string but should be a boolean

workspaceServiceAccountClusterRole is described as "Custom cluster role bound to the user for the Che workspaces": is it bound to the user or is bound to the SA?

Finally I think it makes sense to have all the workspace related fields grouped under workspaces so I have imagined the following structure:

> workspaces
    > defaultPlugins
    > defaultNamespace
        > nameTemplate
        > userClusterRoles (or serviceAccountClusterRoles?)
    > storage
    > nodeSelector
    > tolerations
    > trustedCerts
        > trustStoreConfigMapName
        > gitTrustedCertsConfigMapName
> serverComponents
    > dashboard
    > cheServer
        > clusterRoles
        > deployment
        > loglevel
        > debug
    > database
    > pluginRegistry
    > devfileRegistry
    > imagePuller
    > devWorkspace
    > metrics?
> containerRegistry
    > hostname
    > organization

tolusha · 2022-04-06T13:47:10Z

/test v8-che-behind-proxy

tolusha · 2022-04-06T13:47:18Z

/test v9-devworkspace-happy-path

tolusha · 2022-04-06T13:47:29Z

/test v9-upgrade-stable-to-next

tolusha · 2022-04-06T17:18:30Z

/test v8-che-behind-proxy

tolusha · 2022-04-06T18:43:28Z

/test v9-devworkspace-happy-path

tolusha · 2022-04-06T18:43:34Z

/test v9-upgrade-stable-to-next

tolusha · 2022-04-07T15:39:17Z

/test v9-upgrade-stable-to-next

Signed-off-by: Anatolii Bazko <[email protected]>

ibuziuk

@l0rd devEnvironments / networking LGTM
no more suggestions from my end

amisevsk

Approving to not block merge, a few comments below but generally looks good and comments can be addressed in a separate PR if need be.

api/checluster_conversion_to_test.go

api/v2/checluster_types.go

amisevsk · 2022-05-31T15:31:43Z

api/v2/checluster_types.go

+	Deployment Deployment `json:"deployment,omitempty"`
+	// Maximum number of the running workspaces per user.
+	// +optional
+	RunningLimit string `json:"runningLimit,omitempty"`


This might make more sense as a dashboard option (rather than DevWorkspace). Currently, the dashboard is what imposes the limit on running workspaces, and DWO does not have functionality to do this.

api/v2/checluster_types.go

amisevsk · 2022-05-31T15:33:22Z

api/v2/checluster_types.go

+	// The editor id to specify default plug-ins for.
+	Editor string `json:"editor,omitempty"`


Is this only an ID or can it also be a URI?

amisevsk · 2022-05-31T15:42:06Z

api/v2/checluster_types.go

+// +operator-sdk:csv:customresourcedefinitions:order=0
+// +operator-sdk:csv:customresourcedefinitions:resources={{Ingress,v1},{Route,v1},{ConfigMap,v1},{Service,v1},{Secret,v1},{Deployment,apps/v1},{Role,v1},{RoleBinding,v1},{ClusterRole,v1},{ClusterRoleBinding,v1}}
+// +kubebuilder:storageversion
+type CheCluster struct {


I might have missed it in reading this file, but consider adding +kubebuilder:printcolumn:name annotations here to automatically print the dashboard URL, etc. when listing objects on the cluster. For example, something like

// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.cheClusterRunning",description="Status for the CheCluster" // +kubebuilder:printcolumn:name="Dashboard URL",type="string",JSONPath=".status.cheURL",description="The URL for the Che Dashboard"

If this is handled by the +operator-sdk annotations, then ignore this comment.

An example of how this is used in the DevWorkspace CR: [link]

I think it is covered by +operator-sdk annotations

openshift-ci · 2022-06-01T06:18:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, ibuziuk, l0rd, mmorhun, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Anatolii Bazko <[email protected]>

openshift-ci · 2022-06-02T11:20:03Z

New changes are detected. LGTM label has been removed.

tolusha · 2022-06-03T07:46:08Z

/retest

Signed-off-by: Anatolii Bazko <[email protected]>

deerskindoll

Hi, I tried to spot every potential edit in the document but I might end up suggesting another batch of edits.

api/v2/checluster_types.go

Signed-off-by: Anatolii Bazko <[email protected]>

tolusha · 2022-06-06T13:13:41Z

@deerskindoll
Thank you for review!

openshift-ci · 2022-06-06T14:17:34Z

@tolusha: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v9-operator-test	`7463489`	link	true	`/test v9-operator-test`
ci/prow/v8-operator-test	`7463489`	link	true	`/test v8-operator-test`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

* feat: CheCluster API v2 Signed-off-by: Anatolii Bazko <[email protected]>

tolusha added the needs-ok-to-test label Feb 17, 2022

openshift-ci bot added the do-not-merge/work-in-progress label Feb 17, 2022

tolusha mentioned this pull request Feb 17, 2022

Che Deploy Sprint #214 eclipse-che/che#21094

Closed

18 tasks

tolusha mentioned this pull request Feb 21, 2022

Che Deploy Sprint #215 eclipse-che/che#21190

Closed

4 tasks

tolusha changed the title ~~feat: add conversion webhook~~ feat: Simplify Eclipse Che configuration Feb 22, 2022

openshift-ci bot added the needs-rebase label Feb 23, 2022

tolusha force-pushed the 20758 branch 2 times, most recently from 5552c48 to 69d1b79 Compare March 17, 2022 09:09

openshift-ci bot removed the needs-rebase label Mar 17, 2022

tolusha force-pushed the 20758 branch 2 times, most recently from 2e3741b to 92e79f2 Compare March 17, 2022 13:07

openshift-ci bot added the needs-rebase label Mar 21, 2022

tolusha force-pushed the 20758 branch from 92e79f2 to 6a60a0e Compare March 22, 2022 14:38

openshift-ci bot removed the needs-rebase label Mar 22, 2022

tolusha force-pushed the 20758 branch from d89ad8a to 631a265 Compare April 5, 2022 10:32

tolusha marked this pull request as ready for review April 6, 2022 11:33

tolusha requested review from nickboldt, AndrienkoAleksandr and flacatus as code owners April 6, 2022 11:33

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 6, 2022

Ingress -> Networking, Workspaces -> DevEnvironments

fde6d79

Signed-off-by: Anatolii Bazko <[email protected]>

ibuziuk approved these changes May 31, 2022

View reviewed changes

openshift-ci bot assigned ibuziuk May 31, 2022

openshift-ci bot added the lgtm label May 31, 2022

amisevsk approved these changes May 31, 2022

View reviewed changes

openshift-ci bot assigned amisevsk May 31, 2022

l0rd approved these changes Jun 1, 2022

View reviewed changes

openshift-ci bot assigned l0rd Jun 1, 2022

Fix remarks

3b3489f

Signed-off-by: Anatolii Bazko <[email protected]>

openshift-ci bot removed the lgtm label Jun 2, 2022

Fixups

f379e5c

Signed-off-by: Anatolii Bazko <[email protected]>

ibuziuk mentioned this pull request Jun 3, 2022

feat: support workspace defaults #1395

Merged

11 tasks

tolusha added 6 commits June 3, 2022 16:16

Fixups

72fe63a

Signed-off-by: Anatolii Bazko <[email protected]>

Fixups

fefa43c

Signed-off-by: Anatolii Bazko <[email protected]>

Fixups

1d86f51

Signed-off-by: Anatolii Bazko <[email protected]>

Fixups

5d98233

Signed-off-by: Anatolii Bazko <[email protected]>

Fixups

58c9e05

Signed-off-by: Anatolii Bazko <[email protected]>

Fixups

86b2cb9

Signed-off-by: Anatolii Bazko <[email protected]>

deerskindoll reviewed Jun 6, 2022

View reviewed changes

Fix remarks

7463489

Signed-off-by: Anatolii Bazko <[email protected]>

tolusha merged commit 0bc1049 into main Jun 9, 2022

tolusha deleted the 20758 branch June 9, 2022 08:35

tolusha mentioned this pull request Jun 9, 2022

feat: oauth-proxy configuration enhancements for k8s config #1398

Closed

11 tasks

che-bot added this to the 7.48 milestone Jun 9, 2022

karatkep pushed a commit to karatkep/che-operator that referenced this pull request Jun 10, 2022

feat: CheCluster API v2 (eclipse-che#1324)

3e55ad2

* feat: CheCluster API v2 Signed-off-by: Anatolii Bazko <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CheCluster API v2 #1324

feat: CheCluster API v2 #1324

tolusha commented Feb 17, 2022 •

edited

Loading

openshift-ci bot commented Feb 17, 2022

codecov bot commented Feb 17, 2022 •

edited

Loading

l0rd commented Mar 25, 2022 •

edited by tolusha

Loading

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 7, 2022

ibuziuk left a comment

amisevsk left a comment

amisevsk May 31, 2022

amisevsk May 31, 2022

amisevsk May 31, 2022

amisevsk May 31, 2022

tolusha Jun 6, 2022

openshift-ci bot commented Jun 1, 2022

openshift-ci bot commented Jun 2, 2022

tolusha commented Jun 3, 2022

deerskindoll left a comment

tolusha commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022 •

edited

Loading

		// The editor id to specify default plug-ins for.
		Editor string `json:"editor,omitempty"`

feat: CheCluster API v2 #1324

feat: CheCluster API v2 #1324

Conversation

tolusha commented Feb 17, 2022 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

openshift-ci bot commented Feb 17, 2022

codecov bot commented Feb 17, 2022 • edited Loading

Codecov Report

l0rd commented Mar 25, 2022 • edited by tolusha Loading

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 6, 2022

tolusha commented Apr 7, 2022

ibuziuk left a comment

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk May 31, 2022

Choose a reason for hiding this comment

amisevsk May 31, 2022

Choose a reason for hiding this comment

amisevsk May 31, 2022

Choose a reason for hiding this comment

amisevsk May 31, 2022

Choose a reason for hiding this comment

tolusha Jun 6, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Jun 1, 2022

openshift-ci bot commented Jun 2, 2022

tolusha commented Jun 3, 2022

deerskindoll left a comment

Choose a reason for hiding this comment

tolusha commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022 • edited Loading

tolusha commented Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading

l0rd commented Mar 25, 2022 •

edited by tolusha

Loading

openshift-ci bot commented Jun 6, 2022 •

edited

Loading