First version of VRE integrated chart#2
Conversation
| # JUPYTERHUB_SINGLEUSER_APP: "notebook.notebookapp.NotebookApp" | ||
| RUCIO_MODE: "replica" | ||
| RUCIO_WILDCARD_ENABLED: "1" | ||
| RUCIO_BASE_URL: "https://vre-rucio.cern.ch" |
|
|
||
| ingress: | ||
| enabled: true | ||
| ingressClassName: null |
There was a problem hiding this comment.
This is at least confusing to me, the annotations below assume traefik
There was a problem hiding this comment.
there is a traefik built in reana
| reana: | ||
| enabled: true | ||
|
|
||
| postgres: |
There was a problem hiding this comment.
Move to a top-level config, not within reana scope, and set DB connection info appropriately
| consumer_secret: "testsecret" | ||
|
|
||
| database: | ||
| user: |
There was a problem hiding this comment.
This is to be set from secret in flux, or locally as needed. By default I'd put nothing. Maybe it can be generated like postgres chart does.
|
|
||
| ingress_override: true | ||
|
|
||
| reana_hostname: "reana-vre.obsuks4.unige.ch" |
There was a problem hiding this comment.
template it so that there's a hostbase path or pick a name, and jhub and reana hostnames are derived from that?
| size: 100M | ||
| storageClass: | ||
|
|
||
| name: cern-vre-shared-volume-storage-class |
There was a problem hiding this comment.
This depends on the release name which is really bad, maybe there at least needs to be a templates/storage.yaml
There was a problem hiding this comment.
This is required by reana, we can not change it here in integrated chart. I will check if there is an nfs provisioner where name can be templated.
|
I added a job which will run bootstrap commands, so it should install without manual interventions. I did not test much yet, but please do give it a try, @paullaycock . |
| chartPath: vre | ||
| valuesFiles: | ||
| - vre/values.yaml | ||
| # copy vre/values-custom-copy.yaml to vre/values-custom.yaml, set the necessary values, and uncomment this line |
There was a problem hiding this comment.
| # copy vre/values-custom-copy.yaml to vre/values-custom.yaml, set the necessary values, and uncomment this line | |
| # copy vre/values-custom-example.yaml to vre/values-custom.yaml, set the necessary values, and uncomment this line |
| deploy: | ||
| helm: | ||
| releases: | ||
| - name: cern-vre |
There was a problem hiding this comment.
| - name: cern-vre | |
| - name: cern-vre # DO NOT CHANGE! |
| echo "[client]" >> /opt/rucio/etc/rucio.cfg; | ||
| echo "rucio_host = $RUCIO_BASE_URL" >> /opt/rucio/etc/rucio.cfg; | ||
| echo "auth_host = $RUCIO_AUTH_URL" >> /opt/rucio/etc/rucio.cfg; | ||
| echo "account = $JUPYTERHUB_USER" >> /opt/rucio/etc/rucio.cfg; |
There was a problem hiding this comment.
Need to add
echo "ca_cert = /certs/rucio_ca.pem" >> /opt/rucio/etc/rucio.cfg;
rucio whoami then works
There was a problem hiding this comment.
At least when using the ESCAPE IAM + Rucio instance
volodymyrss
force-pushed
the
first-version-vre-chart
branch
from
ac4f363 to
5c08af4
Compare
garciagenrique
left a comment
garciagenrique
left a comment
There was a problem hiding this comment.
Thanks for opening this PR. Let me add some general comments here.
I see that there are a lot of annotations that are cern-vre specific - which in the end are cern infrastructure specific. For example, the names of the volume and storage class naming, as well as the rucio env vars (among others).
Let me share here my (biased) vision of how I envision this repo (which doesn't need to be like this of course).
What would you think of creating an agnostic "base helm layer" (let's call it vre) which only deploys the "main services" - JupyterHub, Rucio and Reana? Then each vre flavor, that ideally inherits from the base layer, would add their own or specific services (nfs, grafana, EOS, CERN MONIT, etc.), change the chart values to point to their specific services (storage, network, db, idp, rucio and REANA server, etc), and name the charts as desired (cern-vre, etap, etc).
|
There is a question right now if it makes sense to use try using new pre-release reana 0.95.0, or port the CRM changes back onto 0.9.4. |
I think it is always a good idea to try the latest version @volodymyrss. Any reason to use 0.9.4 ? |
The latest version is not released, it's in alpha. Right now I am having an issue with it, related to parsing user token info from IAM . I think it was working before though, so it's strange the issue appeared both in dev and staging setup. Maybe IAM changed. Anyway, alpha is not stable and will require continuous updates to our development. When we last talked with @tiborsimko I think it was said it's too early to try 0.95.0, but I also did not see a huge number of changes in reana github org since, and the latest release is 0.95.0-alpha.2 is in Feb. Or @garciagenrique you probably know of what is the plan for REANA version adoption in the VRE? |
|
I remembered why we moved to alpha version of REANA. It's partly because we wanted to use included DB, but I saw this https://github.com/reanahub/reana/blob/0.9.4/helm/reana/templates/reana-db.yaml#L45 which seems to set debug values only in non-debug environment - so there seems to be maybe a bug? Of course we could use external db but it seems strange. Second reason is that we are actually developing reana chart, so it's weird to start from old version. But then, current alpha version stopped working, so this needs some investigation. edit: I think alpha version is not compatible with Indigo IAM, because recent versions of I am sure it is possible to adjust In principle until 0.95.0 is stable, we could use 0.9.4 with a patch for the DB credentials in debug mode. Overall, it seems that to finish our project, we need a bit deeper changes to involvement reana and invenio_oauthclient, and count on new reana to be released. An advice from @tiborsimko would be welcome, but I can also look myself deeper and propose further reana PRs. |
volodymyrss
force-pushed
the
first-version-vre-chart
branch
from
b6e35a9 to
32decf0
Compare
|
We discussed we need to talk about storage usage. To avoid duplication of data. We are also thinking about using RSE per VRE instance. |
volodymyrss
requested review from
Copilot
and removed request for
paullaycock
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the first integrated version of the VRE (Virtual Research Environment) Helm chart, consolidating JupyterHub, REANA, and observability components into a unified deployment solution.
Key changes include:
- Complete VRE Helm chart with JupyterHub, REANA, NFS provisioner, and optional monitoring stack (Grafana, Prometheus, Loki)
- Bootstrap job for automated REANA admin user creation and database initialization
- Development tooling including justfile, Skaffold configuration, and pre-commit hooks with Helm linting
- CI/CD workflows for automated chart releases to GitHub Pages
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| vre/values.yaml | Main configuration file defining all chart values for JupyterHub, REANA, NFS provisioner, and observability tools |
| vre/values.schema.json | JSON schema for validating chart values |
| vre/values-custom-example.yaml | Example configuration template for customizing deployment-specific settings |
| vre/templates/bootstrap.yaml | Kubernetes Job template for bootstrapping REANA with admin user and database setup |
| vre/templates/roles.yaml | RBAC resources (ServiceAccount, Role, RoleBinding) for bootstrap job permissions |
| vre/templates/reana-ingress.yaml | Custom ingress configuration for REANA service routing |
| vre/templates/_helpers.tpl | Helm template helper functions for generating resource names |
| vre/Chart.yaml | Chart metadata and dependency declarations for sub-charts |
| vre/README.md | Auto-generated chart documentation with values table |
| README.md | Repository documentation with installation, development, and troubleshooting guides |
| skaffold.yaml | Skaffold configuration for local development and deployment |
| justfile | Task automation recipes for version management, deployment, and linting |
| Makefile | Alternative make-based task definitions for cluster setup |
| dev/kind-config.yaml | Kind cluster configuration for local testing |
| .pre-commit-config.yaml | Pre-commit hooks for codespell, helm-docs, helmlint, and kubeconform validation |
| .github/workflows/release.yaml | GitHub Actions workflow for releasing charts to GitHub Pages |
| .github/workflows/pre-commit.yaml | GitHub Actions workflow for running pre-commit checks on PRs |
| .github/cr.yaml | Configuration for chart-releaser tool |
| .gitignore | Git ignore patterns for chart locks and custom values |
| vre/.gitignore | VRE-specific ignore patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reana_workflow_controller: | ||
| imagePullPolicy: IfNotPresent | ||
| image: docker.io/volodymyrsavchenko/reana-workflow-controller:latest |
There was a problem hiding this comment.
The reana_workflow_controller configuration is duplicated in this file. It appears on lines 282-287 and again on lines 341-343 with different image specifications. This duplication could lead to confusion about which configuration will be applied.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| user: | ||
| password: |
There was a problem hiding this comment.
[nitpick] Empty values for required database credentials. While these may be intended to be set via secrets, consider adding placeholder comments like # set through secret for clarity, similar to the pattern used for JupyterHub OAuth credentials (lines 37-38).
| user: | |
| password: | |
| user: # set through secret | |
| password: # set through secret |
| # - name: cvmfs-vre | ||
| # persistentVolumeClaim: | ||
| # claimName: cvmfs-vre-pvc | ||
| # - name: eospilot-eulake # mounts the EOS RSE needed for the Rucio JupiterLab extension |
There was a problem hiding this comment.
Typo in comment: "JupiterLab" should be "JupyterLab".
| # - name: eospilot-eulake # mounts the EOS RSE needed for the Rucio JupiterLab extension | |
| # - name: eospilot-eulake # mounts the EOS RSE needed for the Rucio JupyterLab extension |
| # mountPath: /cvmfs | ||
| # # CVMFS automount volumes must be mounted with HostToContainer mount propagation. | ||
| # mountPropagation: HostToContainer | ||
| # - name: eospilot-eulake # mounts the EOS RSE needed for the Rucio JupiterLab extension |
There was a problem hiding this comment.
Typo in comment: "JupiterLab" should be "JupyterLab".
|
Some remarks about the test deployment that I have done on https://reana-vre.obsuks5.unige.ch:
|
|
Finally the install command looks like: $ helm upgrade --install escape-vre vre/escape-vre -n test --create-namespace --devel \
--set nfs-server-provisioner.persistence.storageClass=vspherecsi-resize \
--set nfs-server-provisioner.persistence.size=20Gi --set reana.reana_hostname=reana-vre.obsuks5.unige.ch \
--set jupyterhub.hub.config.RucioAuthenticator.oauth_callback_url=https://jhub-vre.obsuks5.unige.ch/hub/oauth_callback\
--set reana.secrets.login.iam.consumer_key=XXXXX --set reana.secrets.login.iam.consumer_secret=YYYYYYYYYYYYYYRemarks:
$ kubectl exec -i -t deployment/escape-vre-server -- flask reana-admin token-grant \
--email user@example.com --admin-access-token $REANA_ACCESS_TOKENOne last thing: the redirect URIs to be used in INDIGO IAM have to be: |
|
@volodymyrss I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
Thanks a lot @degauden , I add this to the readme as a case deployment, specializing generic instructions. |
|
Hi @volodymyrss , what's the status of this PR? |
Since I removed draft status it has been! |
There was a problem hiding this comment.
Hi @volodymyrss!
These updates look great, thank you very much!
I think a rebase would help a lot, could we try to aggregate commits and also use the conventional commits layout?
On top of that I'm approving, I only have a few minor questions (the review has been long enough already):
- Default admin password is
adminpassword- users MUST change this; also I saw test IAM credentials in values (testkey/testsecret). Is there a way to make them dummy without being hardcoded? I'm also fine in leaving them there for the moment, if it's too much work. blockWithIptables: falsehas aTODO: revertcomment. Should we do that?- I can see a dependency from
volodymyrsavchenko/reana-workflow-controller; is there any update on an upstream version?
Ok, I will make couple of cosmetic changes and squash
There are different approaches to this, but ultimately dummy values have to come from somewhere, right? And in that way, they'd be hardcoded. They may be in values like now, or in the template default. I find it's move obvious when they are in values, so they are less likely to be forgotten. Or I can make them empty, and make install fail with a clear message if they are not set. Perhaps this is best.
That was our experiments when we hoped to update reana with ETAP-specific changes. I will drop it. |
volodymyrss
force-pushed
the
first-version-vre-chart
branch
from
167d601 to
05e56ca
Compare
…b with VRE extensions and REANA.
volodymyrss
force-pushed
the
first-version-vre-chart
branch
from
05e56ca to
b50f105
Compare
|
|
Thanks all! That should do in the first approximation. It's also live in https://etap.obsuks7.unige.ch/ |
Further features will be added following #3