Skip to content
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

Add support for mounting a persistent volume to the hail-search chart. #39

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

bpblanken
Copy link
Contributor

@bpblanken bpblanken commented Mar 27, 2024

For context, we're removing our copy of hail tables from gcs to instead mount a persistent disk.

- mountPath: {{ $.Values.environment.DATASETS_DIR }}
name: datasets
- name: mkdir-ssd-{{ $path | replace "/" "-" | replace "_" "-" | lower}}
{{- $localPath := $path | replace "SV" "SV_WGS" | replace "GCNV" "SV_WES" -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initialization action here is copying the annotations table from the mounted volume to the in-memory volume, once per reference genome/datasetType.

Copy link
Collaborator

@sjahl sjahl left a comment

Choose a reason for hiding this comment

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

Just a few comments in line, feel free to take or leave them. Otherwise looks good to me

- ReadOnlyMany
resources:
requests:
storage: 2000G
Copy link
Collaborator

Choose a reason for hiding this comment

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

unless we don't expect this to ever change (or if it simply needs to match the capacity attribute on the persistentvolume), i'd make this a variable as well. I don't have a strong preference here, but this unit could also be 2T (or 2Ti if you need the 1024-ish variant of a terabyte).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 this is actually a bug. I'm using {{ .Values.storageCapacity }} elsewhere in this file and just missed this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the values don't need to be the same, but they might as well be for simplicity.

labels:
{{- include "hail-search.labels" . | nindent 4 }}
spec:
storageClassName: ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not actually sure what happens with an empty class name specified here (i think GKE has 2 or 3 classes available by default. This could also not matter/be moot if you're just referencing persistent disks that already exist in GCE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was recommended/documented somewhere in the gke docs and here... I think it's moot because the persistent disks already exist and we're not asking for dynamic provisioning

the storage classes do exist already by virtue of gke it appears:

bblanken@wmbbe-67c seqr-pipeline-airflow % kubectl get storageclass | grep pd
E0329 17:03:55.507374   11981 memcache.go:255] couldn't get resource list for external.metrics.k8s.io/v1beta1: Got empty response for: external.metrics.k8s.io/v1beta1
airflow-redis-persistence-sc   pd.csi.storage.gke.io          Delete          Immediate              true                   9d
premium-rwo                    pd.csi.storage.gke.io          Delete          WaitForFirstConsumer   true                   9d
standard                       kubernetes.io/gce-pd           Delete          Immediate              true                   9d
standard-rwo (default)         pd.csi.storage.gke.io          Delete          WaitForFirstConsumer   true                   9d

potentially worth re-evaluating if we're gonna support this for open-source though.

@bpblanken bpblanken merged commit d7fdbd6 into main Apr 2, 2024
1 check passed
@bpblanken bpblanken deleted the benb/mount_pv branch April 2, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants