-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use templates only to deploy Che to OpenShift #9190
Conversation
|
||
All of the below commands create a PVC for Che server and Che deployment uses this PVC. | ||
|
||
It is possible to avoid using PVC for Che server - just skip creating pvc `oc apply -f pvc/che-server-pvc.yaml` and `oc set volume dc/che --add -m /data --name=che-data-volume --claim-name=che-data-volume` commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, PVC is used by Single-User Che to store H2 Database file. Please, add a note that without PVC Single-User Che will lose all data after Pod restarting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point will do. For single user PVC is created by default - this is what I suggest in an example command.
But once again - good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
value: "${PROTOCOL}://che-${NAMESPACE}.${ROUTING_SUFFIX}/agent-binaries/linux_amd64/bootstrapper/bootstrapper" | ||
- name: CHE_INFRA_KUBERNETES_MACHINE__START__TIMEOUT__MIN | ||
value: '5' | ||
- name: CHE_INFRA_KUBERNETES_MASTER__URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this parameter have value? It makes sense to configure it for multi-cluster mode (When Che Server is started on one OS cluster while Workspace are created on another one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
- name: CHE_INFRA_KUBERNETES_PVC_STRATEGY | ||
value: "${CHE_INFRA_KUBERNETES_PVC_STRATEGY}" | ||
- name: CHE_INFRA_KUBERNETES_PVC_PRECREATE__SUBPATHS | ||
value: 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a variable here. It makes sense to configure it. On newest OpenShift instances it may be disabled, and workspace start may speed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
value: '5' | ||
- name: CHE_INFRA_KUBERNETES_MASTER__URL | ||
- name: CHE_INFRA_KUBERNETES_OAUTH__TOKEN | ||
value: "${TOKEN}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why OPENSHIFT_USERNAME
, OPENSHIFT_PASSWORD
but TOKEN
? Maybe it would be better to name them in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
- name: CHE_INFRA_KUBERNETES_MASTER__URL | ||
- name: CHE_INFRA_KUBERNETES_OAUTH__TOKEN | ||
value: "${TOKEN}" | ||
- name: CHE_INFRA_KUBERNETES_PASSWORD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to place CHE_INFRA_KUBERNETES_PASSWORD
and CHE_INFRA_KUBERNETES_USERNAME
together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
value: "${CHE_KEYCLOAK_AUTH__SERVER__URL}" | ||
- name: CHE_JDBC_URL | ||
value: "${CHE_JDBC_URL}" | ||
- name: CHE_OAUTH_GITHUB_CLIENTID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can it be configured if there is no specified value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test: false | ||
triggers: | ||
- type: ConfigChange | ||
status: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that it makes sense to specify status here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
- apiVersion: v1 | ||
kind: DeploymentConfig | ||
metadata: | ||
creationTimestamp: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it removed? I still see few creationTimestamp
fields in this template. Do I have an issue with browser cache?
dockerfiles/keycloak/Dockerfile
Outdated
@@ -11,3 +11,4 @@ ADD . /scripts/ | |||
USER root | |||
RUN chgrp -R 0 /scripts && \ | |||
chmod -R g+rwX /scripts | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that mandatory change? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
deploy/openshift/templates/README.md
Outdated
|
||
## Templates and Parameters | ||
|
||
Templates are provided with a set of predefined params which you can override with with `-p key=value`. If you miss envs and parameters, you can add them to your template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could add which command will list parameters for a given template ?
$ oc process --parameters -f <filename>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note in readme
deploy/openshift/templates/README.md
Outdated
|
||
## Creating Workspace Objects | ||
|
||
By default, che SA is used to create workspace objects in the same namespace with Che server. This is configurable - you may provide login and password or token, and set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI sometimes Che is written che
sometimes Che
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any word expected after and set
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
deploy/openshift/templates/README.md
Outdated
3. Once done, go to `https://keycloak.${NAMESPACE}.${ROUTING_SUFFIX}`, log in to admin console. | ||
Default credentials are `admin:admin`. | ||
Go to Clients, `che-public` client and edit **Valid Redirect URIs** and **Web Origins** URLs so that they use **https** protocol. | ||
You do not need to do that if you initially deploy Che with https support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like https here have spaces around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
deploy/openshift/templates/README.md
Outdated
oc apply -f pvc/che-server-pvc.yaml; \ | ||
oc process -f che-server-template.yaml -p ROUTING_SUFFIX=$(minishift ip).nip.io \ | ||
-p PROTOCOL=https \ | ||
-p WS_PROTOCOL=wss \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could WS_PROTOCOL/TLS be automatically computed based on the fact that it's https ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish a template can handle some ifs. I failed to find a good universal solution. Any help is appreciated.
deploy/openshift/templates/README.md
Outdated
``` | ||
oc new-project che | ||
|
||
oc process -f che-server-template.yaml -p ROUTING_SUFFIX=$(minishift ip).nip.io | oc apply -f -; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead oc process -f <template> | oc apply -f -
you could use a single command oc new-app -f <template> ....
.
Besides the fact that it's a single command oc new-app
has an option to add applications env vars even if these were not defined in the template: -e ENV_VAR_KEY=ENV_VAR_VALUE
(c.f. oc new-app reference). That may be useful for our use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Examples updated
deploy/openshift/templates/README.md
Outdated
|
||
Templates are provided with a set of predefined params which you can override with with `-p key=value`. If you miss envs and parameters, you can add them to your template. | ||
|
||
IMPORTANT! **ROUTING_SUFFIX** value in the below commands `$(minishift ip).nip.io` works for MiniShift only. You MUST provide **own routing suffix** when deploying to OCP cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have a command to get this value if there is a command for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I knew such a command :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we create a dummy service then we got the default routing suffix and then we can display/reuse it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. Dummy service for what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eivantsov If I create a dummy application on openshift like nginx app and I create a service nginx and expose port 80, openshift will provide me an URL. From this URL I can guess the "routing suffix" so user has a way to find it (through a command that can try to create app, create service and route and then display the url and delete project)
It's just to have a way to provide to users "hey we can give you some hints around this routing suffix"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lame workaround may be updating Che dc after routes for Che and Keycloak are created. This will add complexity to commands, but this way, no input is required from a user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my approach, it was a way to see what the route could be.
in your approach, we create using defaults, grab values and update again.
in both case it would help "newcomers" so I'm fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a sub-section on How to get routing suffix.
deploy/openshift/templates/README.md
Outdated
``` | ||
oc new-project che | ||
|
||
oc process -f che-server-template.yaml -p ROUTING_SUFFIX=$(minishift ip).nip.io | oc apply -f -; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly oc process
supports golang templates with -t
option and looks like they support if
clauses https://golang.org/pkg/text/template/
With this approach, we would have something similar to helm I suppose.
deploy/openshift/templates/README.md
Outdated
|
||
oc process -f che-server-template.yaml -p ROUTING_SUFFIX=$(minishift ip).nip.io | oc apply -f -; \ | ||
oc apply -f pvc/che-server-pvc.yaml; \ | ||
oc set volume dc/che --add -m /data --name=che-data-volume --claim-name=che-data-volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using set before applying template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have dc/che at that point. I need to create Che deploymentConfig and then set volume for it.
The spec says oc set volumes RESOURCE/NAME --add|--remove|--list [options]
so I need an object to modify, while process just produces output.
deploy/openshift/templates/README.md
Outdated
|
||
By default, che SA is used to create workspace objects in the same namespace with Che server. This is configurable - you may provide login and password or token, and set | ||
|
||
## Upgrade from http to https |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this section somewhere in the end - more advanced stuff
|
||
## Deploy multi user Che with bundled Keycloak and Postgres (https) | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like step with creation of the project is missing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it
timeoutSeconds: 60 | ||
name: che | ||
ports: | ||
- containerPort: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ports are declared twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIxed
persistentVolumeClaim: | ||
claimName: keycloak-log | ||
test: false | ||
status: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
resources: | ||
requests: | ||
storage: 1Gi | ||
status: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
resources: | ||
requests: | ||
storage: 1Gi | ||
status: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
persistentVolumeClaim: | ||
claimName: postgres-data | ||
test: false | ||
status: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
targetPort: 5432 | ||
selector: | ||
io.kompose.service: postgres | ||
status: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
resources: | ||
requests: | ||
storage: 1Gi | ||
status: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed?
Do we want to retire the minishift add-on eventually as well? @l0rd @eivantsov |
@gorkem good question. That's two sets of yamls to maintain. Though from user's perspective, applying an addon is somewhat easier. I tried to craft a simple CLI that deploys to MiniShift and OCP https://github.com/eivantsov/che-cli, lame code but works well, and all a user needs to do is:
to deploy to MiniShift. Works with OSO Pro too. I know Florent has some rust cli too. Maybe we should look in this direction and start with an OpenShift CLI that will make it easy to deploy to any OpenShift cluster? |
@gorkem that's a good point. A copy of these |
@l0rd can't rh use templates for upsteam repo? |
How? I mean we could add instructions for rh-che developers to clone upstream repo and use these |
@l0rd yes, agree.. Even now, for multi user setup a user will have to git clone Che to get all yamls. Having them versioned and packaged as tar.gz is a very good idea. |
@l0rd could we just add a pom.xml on these upstream templates so that they could be published on m2 repository ? |
@benoitf do you mean maven central or Che nexus? |
@eivantsov if #6444 is done it should be maven central as well :-) |
@benoitf 👍 Yeah, we don't really need an additional repo to publish the artifact |
@eivantsov this PR looks like Penelope's shroud :-) What is missing to merge it? I am asking because I have opened a similar PR and I am waiting impatiently for this one to be merged. |
@l0rd thanks for your patience and a wonderful metaphor. I have been waiting for all interested parties to have their say on templates themselves, cross platform cli that deploys to all infras and related stuff. Besides, I have introduced some changes to Now that that QA tests show no regression , with a light heart, I merge this PR. Thanks for all suggestions and helpful remarks - they did help make templates and deployment docs better. P.S. Old scripts and deployment yamls will stay here for a while and will be deleted in a separate PR. Related docs PR: eclipse-che/che-docs#391 |
The purpose of this PR is to:
What is done
Why not a single template?
There's no universal template that will fit all use cases. It will be easier to use upstream che-server template for those who depend on it.
Is it tested?
Yes. MiniShift and OSO. Both single and multi user. http and https
Will existing scripts go away?
Eventually, yes. But not immediately. As soon as those who depend on current deployment scripts are ready to migrate, scripts will be deleted.
Does Che deployment have all envs as parameters?
No, only those that make sense at a first glance. Further PRs are welcome.
Will these changes break CI tests?
Even though there are changes in eclipse/che-keycloak Dockerfile, a different entrypoint script is used in Keycloak deployment. So, both deployment script and templates works well, even though they use the same images.
What about Keycloak configuration pod?
Master and Che realm, as well as admin user for Keycloak and Che are created in keycloak image entrypoint.
Will docs be provided?
There's README.md to start with, but eventually docs will be updated to reference template based deployment method.
How to test?
Follow instructions in README. Use eivantsov/keycloak as Keycloak image for Keycloak deployment:
oc process -f multi/keycloak-template.yaml -p ROUTING_SUFFIX=$(minishift ip).nip.io -p IMAGE_KEYCLOAK=eivantsov/keycloak | oc apply -f -