-
Notifications
You must be signed in to change notification settings - Fork 123
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
[K8C-75] initial medusa integration #29
Conversation
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 bunch of nits and one confusing variable name. The comments regarding k8ssandra docker org are not blockers.
charts/backup/Chart.yaml
Outdated
# This is the chart version. This version number should be incremented each time you make changes | ||
# to the chart and its templates, including the app version. | ||
# Versions are expected to follow Semantic Versioning (https://semver.org/) | ||
version: 0.1.0 |
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.
version: 0.1.0 | |
version: 0.4.0 |
Version number must match other charts
charts/backup/templates/backup.yaml
Outdated
{{ include "backup.labels" . | indent 4 }} | ||
spec: | ||
name: {{ .Values.name }} | ||
cassandraDatacenter: {{ .Values.cassandraDatacenter.name }} |
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.
cassandraDatacenter: {{ .Values.cassandraDatacenter.name }} | |
cassandraDatacenter: {{ .Values.cassandraDatacenter.name }} | |
Newline at end of file
charts/backup/values.yaml
Outdated
name: backup | ||
|
||
cassandraDatacenter: | ||
name: dc1 |
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.
name: dc1 | |
name: dc1 | |
Newline at end of file
secret: | ||
secretName: {{ .Values.backupRestore.medusa.bucketSecret }} | ||
{{- end }} |
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.
{{- end }} | |
{{- end }} | |
Newline at end of file
name: {{ .Release.Name }}-medusa-operator-k8ssandra | ||
labels: | ||
{{ include "k8ssandra-cluster.labels" . | indent 4 }} | ||
{{- end }} |
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.
{{- end }} | |
{{- end }} | |
Newline at end of file
|
||
# Must be set and specify the name of the secret that stores the key file for Google | ||
# Cloud or AWS | ||
bucketSecret: "" |
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.
This was confusing at first glance. I know GCS uses a key file, what does this look like for AWS? I was looking for the ACCESS KEY and SECRET KEY fields. Seeing the word SECRET here made me think we were missing an ACCESS field.
The comment makes it clearer, but maybe we should have a different parameter name like cloudSecretCredentialsFile
.
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.
This is an area than needs some TLC. Medusa looks for an AWS conf file that looks like this:
[default]
aws_access_key_id = my_key_id
aws_secret_access_key = my_secret_key
And the secret I have been using looks like this:
apiVersion: v1
kind: Secret
metadata:
name: medusa-bucket-key
type: Opaque
stringData:
medusa_s3_credentials: |-
[default]
aws_access_key_id = my_key_id
aws_secret_access_key = my_secret_key
If there is a way for the user to securely pass his key and secret key, then we could create the secret. For now, I am deferring to the user to create the secret. I do not really like that. It is too many moving parts.
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.
Totally cool with that, maybe the comment should say this is a k8s secret? I would consider putting most of your comment in a doc block (and making sure @johnsmartco is aware of it for the docs reference section).
# We need to use a patched version of cass-operator for now that has changes needed in | ||
# for Reaper and Medusa integration. Images will be built from | ||
# https://github.com/jsanda/cass-operator/tree/k8ssandra. | ||
image: jsanda/cass-operator:91205f4d8f1e |
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.
Same comment re: k8ssandra
docker org. Although we should get 1.5 cut and use the upstream image.
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.
Some of the changes in this patched version are in my Reaper PR which has not gotten any attention in a while. And then there is a not so pleasant bug with volumes and podTemplateSpec. Lastly, we need changes for Medusa support. None of that stuff will be in 1.5 :(
charts/restore/Chart.yaml
Outdated
# This is the chart version. This version number should be incremented each time you make changes | ||
# to the chart and its templates, including the app version. | ||
# Versions are expected to follow Semantic Versioning (https://semver.org/) | ||
version: 0.1.0 |
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.
version: 0.1.0 | |
version: 0.4.0 |
Update to match version numbers in other charts. We may want to make a script to handle updating this...
name: {{ .Values.cassandraDatacenter.name }} | ||
# The CRD currently requires clusterName but the operator uses reuses the cluster name | ||
# from the backup CassandraDatacenter | ||
clusterName: {{ .Values.cassandraDatacenter.name }} |
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.
clusterName: {{ .Values.cassandraDatacenter.name }} | |
clusterName: {{ .Values.cassandraDatacenter.name }} | |
New line at end of file
charts/k8ssandra-cluster/templates/medusa-operator/leader_election_role.yaml
Outdated
Show resolved
Hide resolved
charts/k8ssandra-cluster/templates/medusa-operator/leader_election_role_binding.yaml
Outdated
Show resolved
Hide resolved
|
||
# Must be set and specify the name of the secret that stores the key file for Google | ||
# Cloud or AWS | ||
bucketSecret: "" |
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.
Totally cool with that, maybe the comment should say this is a k8s secret? I would consider putting most of your comment in a doc block (and making sure @johnsmartco is aware of it for the docs reference section).
Summary of changes:
cassdc.yaml
template to deploy medusa restore initContainercassdc.yaml
template to deploy medusa backup containerStill on the TODO list: