Helm chart deploy registries #13890

sparkoo · 2019-07-18T06:54:48Z

What does this PR do?

Helm chart to deploy devfile and plugin registries with Che by default.

registries charts are taken from their origin repos (https://github.com/eclipse/che-devfile-registry/tree/master/deploy/kubernetes/che-devfile-registry and https://github.com/eclipse/che-plugin-registry/tree/master/kubernetes/che-plugin-registry) and bit modified
there are new helm values which make sense to be publically set:
- cheDevfileRegistry.deploy and chePluginRegistry.deploy default to true
- cheDevfileRegistry.url and chePluginRegistry.url default empty. When empty, they are composed by helm to work with local deployment.
memory limits for registries were set based on @metlos experiments
overrides url values set in che.properties. Only situation when it's used is when deploys are set to false and urls are not set.
works with chectl

What issues does this PR fix or reference?

#13633 which is subtask of #13557

Release Notes

n/a

Docs PR

n/a

che-bot · 2019-07-18T06:54:49Z

Can one of the admins verify this patch?

che-bot · 2019-07-18T06:54:49Z

Can one of the admins verify this patch?

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2019-07-18T07:16:33Z

I've added new bool variables for each registry deploy cheDevfileRegistry.deploy and chePluginRegistry.deploy. This might be replaced by just url variables with logic "When url is set, don't deploy. When url is empty, deploy registry.". However, this would break scenario when user wants to set the url and deploy registry at the same time. I'm not sure that this scenario is valid though.
Thoughts?

skabashnyuk · 2019-07-18T07:24:04Z

deploy/kubernetes/helm/che/templates/configmap.yaml

+{{- if .Values.cheDevfileRegistry.url }}
+  CHE_WORKSPACE_DEVFILE__REGISTRY__URL: {{ .Values.cheDevfileRegistry.url | quote }}
+{{- else if .Values.cheDevfileRegistry.deploy }}
+  CHE_WORKSPACE_DEVFILE__REGISTRY__URL: http://{{ printf .Values.global.cheDevfileRegistryUrlFormat .Release.Namespace .Values.global.ingressDomain }}


how it will work if che would be configured to work over https?

hmm, good question. It probably will not work. I guess we would need to have registries also on https then. I'll look at that.

fixed af3eaeb
I'd like to avoid duplication of print, but I don't know how to do it to keep readability.

skabashnyuk · 2019-07-18T07:24:15Z

deploy/kubernetes/helm/che/templates/configmap.yaml

+{{- if .Values.chePluginRegistry.url }}
+  CHE_WORKSPACE_PLUGIN__REGISTRY__URL: {{ .Values.chePluginRegistry.url | quote }}
+{{- else if .Values.chePluginRegistry.deploy }}
+  CHE_WORKSPACE_PLUGIN__REGISTRY__URL: http://{{ printf .Values.global.chePluginRegistryUrlFormat .Release.Namespace .Values.global.ingressDomain }}/v3


same question about https?

skabashnyuk · 2019-07-18T07:27:14Z

Does this pr compatible with che-incubator/chectl#189 these changes?

musienko-maxim · 2019-07-18T07:30:44Z

Can one of the admins verify this PR?

sparkoo · 2019-07-18T07:43:22Z

@skabashnyuk no, because I've changed the che.workspace.devfileRegistryUrl variable to cheDevfileRegistry.url. It's passed here in chectl https://github.com/che-incubator/chectl/blob/master/src/installers/helm.ts#L184, so it's an easy fix. However, as I'm thinking about it, I would rather revert my change here, as it is Che configuration and not Registry configuration.

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2019-07-18T08:05:05Z

@skabashnyuk ok, I've reverted the variable rename and it is still not compatible as chectl have it's defaults to openshift.io registries and is always sending the parameter, overriding anything what was set before. I guess patch chectl to NOT have default registries urls is way to go here. Agree?

skabashnyuk · 2019-07-18T08:06:47Z

I guess patch chectl to NOT have default registries urls is way to go here. Agree?

I believe that makes sense.
CC @l0rd

davidfestal · 2019-07-18T08:44:31Z

I've added new bool variables for each registry deploy cheDevfileRegistry.deploy and chePluginRegistry.deploy

@l0rd When discussing about the implementation of this issue on the operator side, we had agreed on using the externalDevfileRegistry field in the Custom resource, with a default value to false, instead of having a deployDevfileRegistry with default to true. This seemed more consistent with how it is managed for Keycloak (at least in the operator) and how defaults are managed in Go K8s CRDs.

Maybe this should be the same in the helm charts and chectl options ?
cheDevfileRegistry.external for exemple here ?

deploy/kubernetes/helm/che/custom-charts/che-devfile-registry/templates/ingress.yaml

davidfestal · 2019-07-18T10:16:21Z

deploy/kubernetes/helm/che/values.yaml

 #    devfileRegistryUrl: "https://che-devfile-registry.openshift.io/"
 #    pluginRegistryUrl: "https://che-plugin-registry.openshift.io/v3"

+cheDevfileRegistry:
+  deploy: true


Couldn't we change this to external: false, to be consistent with what was decided for the operator (and what is already in place for the Keycloak case )?
cc @l0rd

If this is the way how we're doing that in operator, I agree. Do you know how to do NOT in requirements.yaml condition: ? https://github.com/sparkoo/che/blob/helm-chart-deploy-registries/deploy/kubernetes/helm/che/requirements.yaml#L14 I can't find anything how to achieve that and all my experiments are failing so far :/

If this is the way how we're doing that in operator, I agree

it is

Do you know how to do NOT in requirements.yaml condition ?

TBH I don't know. Not a Helm expert, but what if you use an else as mentioned here: https://github.com/helm/helm/blob/master/docs/chart_template_guide/control_structures.md#ifelse

Sorry, I didn't read the question correctly: if/else has nothing to do with your question it seems. So no, I don't know. Maybe using a calculated value that could be defined as the opposite of external ?

as far as I know, requirements.yaml is just clean yaml, no interpolation. And I believe all values there must be taken from values.yaml, which is the again just clean yaml. For keycloak we're doing check for true here https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/requirements.yaml#L18 global.cheDedicatedKeycloak. So we might go with deploy=true way, just for technology conventions sake here.

Yeah, it seems that you will have to do the same as for Keycloak.

I assume that CheCtl should do the same as the operator though, to maintain some sort of consistency.
@l0rd do you confirm ?

You mean that there will be some inconsistency amongst helm and operator in the semantic of the parameter right? Yes it doesn't seam like a big deal and we can let chectl manage it

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2019-07-19T07:28:52Z

chectl PR to don't send registries defaults che-incubator/chectl#228
@skabashnyuk @davidfestal @l0rd

sparkoo · 2019-07-19T12:56:13Z

All comments were fixed. Can you please re-review the changes and approve/comment the PR? Same for chectl PR here che-incubator/chectl#228
@skabashnyuk @l0rd @davidfestal @sleshchenko (anyone else?)

l0rd · 2019-07-20T07:00:22Z

deploy/kubernetes/helm/che/custom-charts/che-plugin-registry/templates/deployment.yaml

+      app: che-plugin-registry
+  strategy:
+    type: RollingUpdate
+    rollingParams:


I get unknown field "rollingParams" in io.k8s.api.extensions.v1beta1.DeploymentStrategy

It doesn't look like rollingParams is in a DeploymentStrategy (c.f. k8s api reference). That's curious. Have you tested on OpenShift?

Anyway chaging to

strategy: type: RollingUpdate rollingUpdate: maxSurge: 25% maxUnavailable: 25%

worked

no, I've tested on minikube. Looks like values were ignored, but I didn't get any error. Fixed

I got configs from registries repos (https://github.com/eclipse/che-devfile-registry/blob/master/deploy/kubernetes/che-devfile-registry/templates/deployment.yaml#L24), I'll fix them also.

eclipse-che/che-plugin-registry#187
eclipse-che/che-devfile-registry#44

l0rd · 2019-07-20T07:10:08Z

deploy/kubernetes/helm/che/custom-charts/che-plugin-registry/values.yaml

+#
+
+chePluginRegistry:
+  image: quay.io/openshiftio/che-plugin-registry


We are moving to the new quay.io/eclipse organisation. Please use quay.io/eclipse/che-plugin-registry:nightly here.

l0rd · 2019-07-20T07:10:30Z

deploy/kubernetes/helm/che/custom-charts/che-devfile-registry/values.yaml

+#
+
+cheDevfileRegistry:
+  image: quay.io/openshiftio/che-devfile-registry


We are moving to the new quay.io/eclipse organisation. Please use quay.io/eclipse/che-devfile-registry:nightly here.

Signed-off-by: Michal Vala <mvala@redhat.com>

benoitf · 2019-07-22T08:17:36Z

deploy/kubernetes/helm/che/custom-charts/che-devfile-registry/templates/deployment.yaml

+kind: Deployment
+metadata:
+  labels:
+    app: che-devfile-registry


app:che component: devfile-registry

like other components ? https://github.com/eclipse/che/blob/8ddc04ba03d4f7745782cf2e6fdb3d1405711c21/deploy/kubernetes/helm/che/custom-charts/che-keycloak/templates/deployment.yaml#L14-L15
?

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2019-07-22T09:20:00Z

I see no more unfixed comments. Could you please re-review/approve/comment? @l0rd @davidfestal @benoitf

Signed-off-by: Michal Vala <mvala@redhat.com>

benoitf · 2019-07-22T13:05:17Z

deploy/kubernetes/helm/che/custom-charts/che-plugin-registry/templates/deployment.yaml

+        - containerPort: 8080
+        livenessProbe:
+          httpGet:
+            path: /plugins/


AFAIK /plugins is deprecated so should we wait for another path like v3/plugins ?

PR for plugin-registry eclipse-che/che-plugin-registry#189

Signed-off-by: Michal Vala <mvala@redhat.com>

sleshchenko · 2019-07-22T13:36:57Z

ci-build

davidfestal · 2019-07-22T13:51:04Z

@sparkoo what was the reason to do changes in commit bb0af81 ?

This introduces inconsistency with the already-merged implementation of the registry deployment on the Che operator side (https://github.com/eclipse/che-operator/pull/51/files#diff-d9445e614f0d728fd140771b0fd29c50R381)

sparkoo · 2019-07-22T13:55:39Z

@davidfestal hmm, I've followed other components pattern here #13890 (comment)

sparkoo · 2019-07-22T14:34:32Z

@davidfestal @benoitf che-devfile-registry or devfile-registry ? I like the latter more. However, operator uses the former.

davidfestal · 2019-07-22T16:04:50Z

@davidfestal @benoitf che-devfile-registry or devfile-registry ? I like the latter more. However, operator uses the former.

@sparkoo @benoitf if you are OK about app=che && component=devfile-registry being the way to go, I'll create a PR on the operator repo.

However in CheCtl code, in the server stop command, I see this code:

      {
        title: 'Wait until Keycloak pod is deleted',
        enabled: (ctx: any) => !ctx.isAlreadyStopped && !ctx.isNotReadyYet && ctx.foundKeycloakDeployment,
        task: async (_ctx: any, task: any) => {
          await kh.waitUntilPodIsDeleted('app=keycloak', flags.chenamespace)
          task.title = `${task.title}...done.`
        }
      },

That doesn't seem very consistent to me. @l0rd any idea ?

davidfestal · 2019-07-22T16:06:22Z

@davidfestal @benoitf che-devfile-registry or devfile-registry ? I like the latter more. However, operator uses the former.

@sparkoo @benoitf if you are OK about app=che && component=devfile-registry being the way to go, I'll create a PR on the operator repo.

However in CheCtl code, in the server stop command, I see this code:

      {
        title: 'Wait until Keycloak pod is deleted',
        enabled: (ctx: any) => !ctx.isAlreadyStopped && !ctx.isNotReadyYet && ctx.foundKeycloakDeployment,
        task: async (_ctx: any, task: any) => {
          await kh.waitUntilPodIsDeleted('app=keycloak', flags.chenamespace)
          task.title = `${task.title}...done.`
        }
      },

That doesn't seem very consistent to me. @l0rd any idea ?

benoitf · 2019-07-22T16:13:30Z

@davidfestal probably a missing code refactoring when selector methods were introduced
https://github.com/che-incubator/chectl/blob/3f36ffa2378df2444f9588855bdcdecdaa42c251/src/commands/server/start.ts#L326-L335

benoitf · 2019-07-22T16:14:12Z

@davidfestal

@sparkoo @benoitf if you are OK about app=che && component=devfile-registry being the way to go, I'll create a PR on the operator repo.

I like it as well

BTW I was late commenting as github was kind of not working :-)

benoitf · 2019-07-22T16:14:53Z

@davidfestal server:stop looks like a bug

sparkoo · 2019-07-23T06:33:16Z

stop.ts is hot candidate for refactoring. Lot duplicate code in this scale/wait pattern https://github.com/che-incubator/chectl/blob/master/src/commands/server/stop.ts#L154. This selector https://github.com/che-incubator/chectl/blob/master/src/commands/server/stop.ts#L221 also won't work with this deployment https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-postgres/templates/deployment.yaml#L16.
But I'd like to close this one. If we agree on app=che,component=devfile-registry, and update operator to same convention, can we merge this PR? Chectl counterpart is here che-incubator/chectl#228 and is working with this PR.

skabashnyuk · 2019-07-23T07:10:56Z

@benoitf wdyt can we merge this pr? Or should we do some changes in chectl first?

davidfestal · 2019-07-23T07:28:15Z

Seems to me that we should merge this and further work on chectl afterwards. There are already pending PRs on chectl that we are waiting for.

benoitf · 2019-07-23T07:30:10Z

@skabashnyuk can merge here

sparkoo requested review from l0rd, nickboldt, rhopp and sleshchenko as code owners July 18, 2019 06:54

skabashnyuk requested a review from monaka July 18, 2019 06:57

helm charts to deploy registries with che

d3dacb6

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo mentioned this pull request Jul 18, 2019

Che registries (plugin+devfile) should be part of Che deployment #13557

Closed

3 tasks

skabashnyuk reviewed Jul 18, 2019

View reviewed changes

helm chart registries url variables back to che.workspace.

6553689

Signed-off-by: Michal Vala <mvala@redhat.com>

davidfestal requested changes Jul 18, 2019

View reviewed changes

sparkoo added 2 commits July 18, 2019 15:09

helm chart templates for registries uses global che values

20fa8ed

Signed-off-by: Michal Vala <mvala@redhat.com>

helm chart use https for registries when tls enabled

af3eaeb

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo mentioned this pull request Jul 19, 2019

remove devfile and plugin registries defaults che-incubator/chectl#228

Merged

1 task

sparkoo mentioned this pull request Jul 19, 2019

helm charts - ingress.class could be extracted to variable (refactoring) #13927

Closed

l0rd reviewed Jul 20, 2019

View reviewed changes

helm charts - update registries images to eclipse ones

6b73f9d

Signed-off-by: Michal Vala <mvala@redhat.com>

fix helm charts deployment config for registries

8ddc04b

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo mentioned this pull request Jul 22, 2019

fix helm chart deployment strategy config eclipse-che/che-devfile-registry#44

Merged

benoitf reviewed Jul 22, 2019

View reviewed changes

sparkoo added 2 commits July 22, 2019 10:28

update helm charts deployment api to apps/v1

59cb12d

Signed-off-by: Michal Vala <mvala@redhat.com>

unify registries helm charts deployment metadata with other components

c180333

Signed-off-by: Michal Vala <mvala@redhat.com>

davidfestal approved these changes Jul 22, 2019

View reviewed changes

skabashnyuk approved these changes Jul 22, 2019

View reviewed changes

l0rd approved these changes Jul 22, 2019

View reviewed changes

fix ingress and service metadata in registries helm charts

bb0af81

Signed-off-by: Michal Vala <mvala@redhat.com>

benoitf reviewed Jul 22, 2019

View reviewed changes

fix helm charts plugin registry probes

56c9f83

Signed-off-by: Michal Vala <mvala@redhat.com>

benoitf approved these changes Jul 23, 2019

View reviewed changes

skabashnyuk merged commit ec50704 into eclipse-che:master Jul 23, 2019

sparkoo deleted the helm-chart-deploy-registries branch July 23, 2019 08:12

davidfestal mentioned this pull request Jul 23, 2019

Fix registry labels and probes eclipse-che/che-operator#54

Merged

sparkoo mentioned this pull request Jul 24, 2019

Failed to deploy by Helm #13704

Closed

Helm chart deploy registries #13890

Helm chart deploy registries #13890

Conversation

sparkoo commented Jul 18, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

che-bot commented Jul 18, 2019

che-bot commented Jul 18, 2019

sparkoo commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented Jul 18, 2019 • edited Loading

musienko-maxim commented Jul 18, 2019

sparkoo commented Jul 18, 2019

sparkoo commented Jul 18, 2019

skabashnyuk commented Jul 18, 2019

davidfestal commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparkoo commented Jul 19, 2019 • edited Loading

sparkoo commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparkoo commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Jul 22, 2019

davidfestal commented Jul 22, 2019

sparkoo commented Jul 22, 2019

sparkoo commented Jul 22, 2019

davidfestal commented Jul 22, 2019

davidfestal commented Jul 22, 2019

benoitf commented Jul 22, 2019

benoitf commented Jul 22, 2019

benoitf commented Jul 22, 2019

sparkoo commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

davidfestal commented Jul 23, 2019

benoitf commented Jul 23, 2019

sparkoo commented Jul 18, 2019 •

edited

Loading

skabashnyuk commented Jul 18, 2019 •

edited

Loading

sparkoo commented Jul 19, 2019 •

edited

Loading