-
Notifications
You must be signed in to change notification settings - Fork 81
aws backup|restore support and backup cronjob #118
Conversation
srfaytkn
commented
Oct 10, 2020
- aws backup support
- cronjob
|
hello @moxious , can you set the backup chart to be downloaded directly from the helm hub? |
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.
thanks for doing this PR. Really substantive work and I'd like to work with you to get this in. Submitting a review here -- in terms of the way you've approached the task I'm all good with it. I like the CLOUD_PROVIDER and secret name approach to fan out to multiple different clouds, and I also like the job scheduling.
labels: | ||
app.kubernetes.io/managed-by: {{ .Release.Service | quote }} | ||
app.kubernetes.io/instance: {{ .Release.Name | quote }} | ||
helm.sh/chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" | ||
app.kubernetes.io/name: "graph-backup" | ||
app.kubernetes.io/component: backup | ||
spec: | ||
backoffLimit: 3 | ||
template: | ||
schedule: {{.Values.jobSchedule | quote }} |
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.
I really like the jobSchedule approach, but please put it in an if block, and allow the user to not specify it for one-off backups. I.e. automating this on a regular basis is a good idea, but between intervals, users may wish to take a one-off hot backup before performing a maintenance operation like a rolling upgrade.
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.
For this case, a hot backup can be created using kubectl create job --from=cronjob/<name of cronjob> <name of job>
. I can still if this situation is requested
checkPropertyOwners: "false" | ||
jobSchedule: "0 */12 * * *" |
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.
I don't feel strongly about this but for a lot of production customers, once every 24 hours would be a better default
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 value is default. User can be change while creating chart.
I don't personally know how to do this, but I haven't yet invested a lot of deep time to figure out how. We have an issue in the repo with all of the things that have been tried here, with background if you're interested. #16 I was thinking on that issue that I needed someone who was more familiar with helm repositories to help out with a few facts. I think fundamentally doing that (publishing the charts correctly in an automated way) is 100% the right thing to do and I'll do it, supposing I know how. |
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.
Last set of minor changes; if these can be addressed it's mergeable. I tested it locally with GCP and will test again with AWS later today.
Process I plan for this PR is to merge it onto master later, but then I have to make & push the regular backup/restore containers before I can issue a release. Otherwise, just on master this won't work without users adjusting the image & imageTag for the backup & restore containers. Easy enough to fix, but not something we can do on the PR.
Once this is merged & the containers are pushed into the right places and I have a green build on master, I'll probably do a release.
#restore deployment-scenarios