Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Initial commit of Redash chart. #5071

Closed
wants to merge 17 commits into from
Closed

Initial commit of Redash chart. #5071

wants to merge 17 commits into from

Conversation

grugnog
Copy link
Contributor

@grugnog grugnog commented Apr 16, 2018

What this PR does / why we need it:

This adds a Redash chart.

Redash is an open source collaborative data analytics and visualization tool.

Special notes for your reviewer:

A version of this chart has been running without issue in an internal context for several weeks.

Note that Redash is developed by a startup of the same name. I am not affiliated with that company, but I will open an issue on their Github tracker to make them aware of this PR and see if they have input or interest in co-maintaining.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 16, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @grugnog. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 16, 2018
@unguiculus
Copy link
Member

/assign

@unguiculus
Copy link
Member

Please compare with what you get with helm create using Helm 2.8+ and update accordingly.

@grugnog
Copy link
Contributor Author

grugnog commented Apr 20, 2018

@unguiculus I think this should be ready for further review.

@grugnog
Copy link
Contributor Author

grugnog commented Apr 27, 2018

@unguiculus did you get a chance to look at the revisions?

@unguiculus unguiculus changed the title Initial commmit of Redash chart. Initial commit of Redash chart. May 2, 2018
*/}}
{{- define "redash.fullname" -}}
{{- if .Values.fullnameOverride -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If fullnameOverride is set, you get the same for redash.fullname, redash.adhocWorker.fullname, etc. I guess this is not intended and will cause problems/name clashes.

@@ -0,0 +1,57 @@
apiVersion: extensions/v1beta1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps/v1beta2

@@ -0,0 +1,57 @@
apiVersion: extensions/v1beta1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps/v1beta2

@@ -0,0 +1,120 @@
apiVersion: extensions/v1beta1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps/v1beta2

@@ -37,8 +37,8 @@ We truncate at 63 chars because some Kubernetes name fields are limited to this
If release name contains chart name it will be used as a full name.
*/}}
{{- define "redash.adhocWorker.fullname" -}}
{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- if .Values.adhocWorker.fullnameOverride -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need separate fullnameOverride for the components and configurable component names? How about doing something like this:

{{- define "redash.adhocWorker.fullname" -}}
{{- template "redash.fullname" . -}}-adhoc-worker
{{- end -}}

Copy link
Contributor Author

@grugnog grugnog May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unguiculus I don't need this personally - I was trying mainly to keep things consistent with the Helm 2.8+ create template and also copying the pattern used by some of the other multi-deployment charts like Prometheus.

If you think it's cleaner to drop fullnameOverride I am happy to remove it. If so let me know if you think fullnameOverride should be removed from all deployments or kept on the primary/web deployment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest you follow the example above. You'll have one fullnameOverride affecting all derived fullnames.

@jurgenweber
Copy link
Contributor

can we get this finished and merged.. It would be fabulous.

@grugnog
Copy link
Contributor Author

grugnog commented Jul 9, 2018

@unguiculus I think this should be ready for further review.

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest you move the chart to stable before we merge.

imageTag: "9.5.6-alpine"
postgresUser: redash
postgresPassword: redash
postgresDatabase: redash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't set a default value for the password

## ref: https://github.com/kubernetes/charts/blob/master/stable/redis/README.md
##
redis:
redisPassword: redash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't set a default value for the password

## This PostgreSQL instance is used for all Redash state storage
## ref: https://github.com/kubernetes/charts/blob/master/stable/postgresql/README.md
##
postgresql:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to define an external postgresql database instead of running its own? for setups that already have a DB running (i.e. using AWS managed service) this would remove the need for persistence too.

@analyticalmonk
Copy link

@grugnog @unguiculus Hi, are there plans to merge this anytime soon? It would be great to have a redash helm chart available in the stable repo.

@bobbui
Copy link
Contributor

bobbui commented Aug 6, 2018

this is really helpful, I've been using this for some time now. it's pretty stable

@sfrique
Copy link
Contributor

sfrique commented Aug 6, 2018

Hey, i think redash plans to release the v5 sometime in the near future.
As this charts wasn't merge or anything like this, wouldn't make sense to start the work and get v5 out instead of v4 and then upgrade to v5? Just a thought.

I hope to be using the v5 on kubernetes when it's released. I have an working v4 not on kubernetes, was about to migrate to kubernetes, but then i saw something about the update so I am going to wait for it.

I am willing to do some work on the v5 for kubernetes if needed!

@arikfr
Copy link

arikfr commented Aug 6, 2018

As this charts wasn't merge or anything like this, wouldn't make sense to start the work and get v5 out instead of v4 and then upgrade to v5? Just a thought.

I don't think there should be anything in this Chart that should change (aside from the Docker image reference) between v4 and v5. Hence no point in waiting for v5 to finalize or merge this.

@sfrique
Copy link
Contributor

sfrique commented Aug 6, 2018

@arikfr great!!

I didn't take a look on redash v5 upgrade neither this chart yet, so i wouldn't know, i just added my 2c!

@grugnog
Copy link
Contributor Author

grugnog commented Aug 25, 2018

Sorry for the delay in getting back to this. I have moved to stable and also updated the image tag.

Avoiding default passwords is a bit complex as Redash expects the password as part of connection URL strings (REDASH_DATABASE_URL and REDIS_URL). The autogenerated passwords are in a secret and not accessible during templating, so we would need to inject a script via Configmap to assemble each URL environment variable as part of the container startup from the parts (hostname, password etc). Alternatively we could see if Redash would be able to update the Docker image to accept the URL components instead of just a single URL string?

@mattfarina mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@grugnog
Copy link
Contributor Author

grugnog commented Sep 5, 2018

I am working on a change to split out DB env variables and merge them in a shell script loader (that could hopefully eventually become part of the upstream entrypoint). This should allow Redash to be configured to use the autogenerated. It's still a WIP, but at https://github.com/grugnog/charts/commits/redash-env if you want to take a look at the direction.

@sfrique
Copy link
Contributor

sfrique commented Oct 4, 2018

@grugnog are you still working on this?

By the way, version 5 is released, you may use it!

Let me know if not, so I will see if I have time take over or not!

@driscollAtBitGo
Copy link

@grugnog Thanks for putting this together; super helpful! Any estimate on when you think it'll land? I'd love to give it a whirl. Also any particular reason 'scheduledworker-deployment..yaml' has a double period in the file name? I've not seen that before.

@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Jul 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: grugnog
To complete the pull request process, please assign unguiculus
You can assign the PR to them by writing /assign @unguiculus in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@grugnog
Copy link
Contributor Author

grugnog commented Jul 20, 2019

DCO is signed (sorry for delay - just got back from vacation!).

Signed-off-by: Owen Barton <owen.barton@civicactions.com>
@brunowego
Copy link

@unguiculus ping 🥇

@monotek
Copy link
Collaborator

monotek commented Jul 24, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 24, 2019
@maorfr
Copy link
Member

maorfr commented Aug 13, 2019

please take a look at helm/helm#6095 (comment)

we strongly encourage people to self host chart repos:
https://github.com/helm/hub/blob/master/Repositories.md

you can even do that using GitHub pages:
https://github.com/helm/hub/blob/master/docs/github.md#github-pages-as-a-repository

@norrs
Copy link

norrs commented Aug 20, 2019

@grugnog : Hey, I would like to request some changes which we did to our vendored chart from this PR inside our organization.

Please consider adding the following under envin the following files: server-deployment.yaml , adhocworker-deployment.yaml and scheduledworker-deployment.yaml

{{- if .Values.envSecrets.secretKey.existingSecret }}
- name: REDASH_SECRET_KEY
  valueFrom:
    secretKeyRef:
      name: {{ .Values.envSecrets.secretKey.existingSecret | quote }}
      key: secretKey
{{- end }}

In values.yaml you can provide the following default values:

envSecrets:
  secretKey: {}
    #existingSecret:

This way users can use existing secret inside the cluster the secret key that is used in newer versions for redash. See https://version.redash.io/ . This is required for us running with redash7.
(and production usages want to avoid putting secrets together with deployments, as they either use things like vault etc combined with sealed secret controller)

Here is our internal upgrade transcript we logged when we did our upgrade from redash5 to redash7 below. Maybe it could be useful as a contrib readme or something.

How to upgrade Redash (database migration)

Example transcript below, in the following example redash5 is old deployment,
and redash7 is new deployment.

Scale all worker deployments down

...but leave postgresql/redis running:

kubectl scale deploy/redash5 --replicas=0
kubectl scale deploy/redash5-adhocworker --replicas=0
kubectl scale deploy/redash5-scheduledworker --replicas=0

Create database dump

pg_dump -Fc -U redash redash > /tmp/2019-08-09-upgrade-redash-compressed.backup

Scale old deployment up again

kubectl scale deploy/redash5 --replicas=1
kubectl scale deploy/redash5-adhocworker --replicas=2
kubectl scale deploy/redash5-scheduledworker --replicas=2

But instruct #general in slack that new edits in queries will not be included
in the upgrade!

Upgrade chart

Since its not merged to upstream yet ( #5071
) , we do the following for vendoring:

git clone https://github.com/grugnog/charts.git
git checkout redash
(cd stable/redash && helm dependency update)
mkdir /tmp/new-redash
helm package stable/redash --destination /tmp/new-redash
tar -C /tmp/new-redash -zxvf /tmp/new-redash/redash-1.0.0.tgz 
mv /tmp/new-redash/redash ~/zedge/deployments/charts/

Ensure we use chart values which refer to correct secrets,
so ensure you inspect the new chart to ensure values are correct.

As of upgrading redash7, redash chart expects specific naming on
redash-postgresql secret to use the { Release.Name }.

After chart upgrade in a new Helm release

We sadly get a new postgresql instance.

Scale down new deployment while we restore database contents.

kubectl scale deploy/redash7 --replicas=0
kubectl scale deploy/redash7-adhocworker --replicas=0
kubectl scale deploy/redash7-scheduledworker --replicas=0

Restore database

Copy dump back in

kubectl cp ~/tmp/2019-08-09-upgrade-redash-compressed.backup redash7-postgresql-0:/tmp/2019-08-09-upgrade-redash-compressed.backup

Enter postgresql pod

kubectl exec -ti redash7-postgresql-0 /bin/bash -i

Restore contents:

pg_restore -h localhost -p 5432 -U redash -d redash --clean /tmp/2019-08-09-upgrade-redash-compressed.backup

(You can ignore the comment about it not being allowed to set a comment on a SQL procedure)


### Scale up again

kubectl scale deploy/redash7 --replicas=1
kubectl scale deploy/redash7-adhocworker --replicas=2
kubectl scale deploy/redash7-scheduledworker --replicas=2


### Run migrations manually

Enter redash7 deployment POD:

```bash
. /config/dynamicenv.sh && /app/bin/docker-entrypoint manage db upgrade

The /config/dynamicenv.sh sets the required variables for using manage db
commands. (ie the *_URL environment variables)

Success?!

Visit https://redash.example.net (depends if you use release name in helm set to redash.

Other useful tricks

Remember to have a shell which have inherited the /config/dynamicenv.sh !

Check the current version and migration history:

/app/bin/docker-entrypoint manage db current
/app/bin/docker-entrypoint manage db history

See the SQL commands that will be run:

/app/bin/docker-entrypoint manage db upgrade --sql

Migrate

/app/bin/docker-entrypoint manage db upgrade

OAuth configuration

Configuration towards Google OAuth is done in GCP Google OAuth project, you
require special access to this project. When rolling a new version, you
usually want to ensure this one is updated to accept the new javascript
origins, ie https://redash.example.net and a new callback URL before
upgrading to ensure redash versions can run along side each other.

(This section about oauth configuration you could drop for this contrib upgrade readme if you want to include it)

@stale
Copy link

stale bot commented Sep 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2019
@zakkg3
Copy link
Collaborator

zakkg3 commented Sep 19, 2019

nono dont stale this, merge it!

@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2019
@yzorg
Copy link

yzorg commented Sep 26, 2019

nono dont stale this, merge it!

@zakkg3 This is a long thread, so let me summarize the bad news...

It looks like this PR will not be merged because:

Have you considered hosting this chart and adding it to helm hub?
https://github.com/helm/hub/blob/master/Repositories.md

from #5071 (comment)

and a more concise take, from another PR: linkerd/linkerd2#3292 (comment)

So I'll open an issue on getredash/redash asking if they'd like to own the helm chart. In the past they've had kube docs and scripts, so I think it is likely they will want to host the helm cart under the getredash org.

If they are not interested we could submit @grugnog's repo to chart hub. I assume we'll need to cleanup that repo.

@yzorg
Copy link

yzorg commented Oct 1, 2019

Anyone who likes or is following this thread, please vote on the Redash feature discussion:
Helm chart repo
https://discuss.redash.io/t/helm-chart-repo/4594

@stale
Copy link

stale bot commented Oct 31, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 31, 2019
@arikfr
Copy link

arikfr commented Oct 31, 2019

@yzorg @grugnog as per @yzorg's request, I've opened a repo under the getredash org on GitHub to host the Helm Chart. I've made both of you as maintainers of this repo (you will need to accept the invite here).

Thanks!

@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 31, 2019
@grugnog
Copy link
Contributor Author

grugnog commented Nov 4, 2019

@arikfr @yzorg - thanks so much for making this connection and establishing a new repo part of the getredash org.
I think this is the perfect place for the chart to live and I am excited to keep moving this forward there.

I have pushed a rebased version of this PR to that repository and opened a handful of initial tickets. Let's continue work there - PRs are very welcome :D

@grugnog grugnog closed this Nov 4, 2019
@okgolove
Copy link
Collaborator

okgolove commented Nov 4, 2019

The end of the big story...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). ok-to-test size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet