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 backup-version file in backup tarball #1117

Merged
merged 1 commit into from Jan 22, 2019

Conversation

wwitzel3
Copy link
Contributor

@wwitzel3 wwitzel3 commented Dec 5, 2018

Closes #134

pkg/backup/backup.go Outdated Show resolved Hide resolved
@wwitzel3 wwitzel3 changed the title Add Ark version to backup-version file in backup tarball Add backup-version file in backup tarball Dec 5, 2018
pkg/backup/backup.go Outdated Show resolved Hide resolved
@ncdc ncdc changed the title Add backup-version file in backup tarball [do not merge yet] Add backup-version file in backup tarball Dec 7, 2018
@wwitzel3 wwitzel3 force-pushed the issue-134 branch 2 times, most recently from 16e6f68 to 6be86b2 Compare December 18, 2018 22:42
@wwitzel3
Copy link
Contributor Author

@ncdc PTAL

@skriss
Copy link
Member

skriss commented Jan 3, 2019

Now that I'm thinking about this PR - we will now store some metadata as objects directly in object storage (e.g. ark-backup.json), and some (e.g. backup-version) as files within the tarball. Do we have a principled stance on what should go where? Would it make sense to duplicate any of it?

@nrb
Copy link
Contributor

nrb commented Jan 3, 2019

@skriss That's a good question on the duplication. My current thought is that the backup version should be in the tarball, so that it could be processed if moved around. Information that's useful for periodic checks should be outside the tarball, though, since we don't want to download and extract them all on a regular basis.

@wwitzel3
Copy link
Contributor Author

wwitzel3 commented Jan 3, 2019

I remember during the original discussion we wanted to intentionally keep backup-version in the tarball for the reason mentioned by @nrb

@wwitzel3
Copy link
Contributor Author

wwitzel3 commented Jan 4, 2019

So where did we land on this? There was also some conversation in #ark-dr that I wanted to echo here.

I am going to update this so that we will have a metadata folder which will hold the backup-version file in the backup tarball.

@wwitzel3 wwitzel3 changed the title [do not merge yet] Add backup-version file in backup tarball WIP: Add backup-version file in backup tarball Jan 4, 2019
@ncdc
Copy link
Contributor

ncdc commented Jan 4, 2019

Nolan is correct - we want the backup-version somewhere in the tarball so that we can identify its format version even if we don't have the json to go with it. Maybe we should also include the json in it as well...

👍 to metadata/backup-version or even just metadata/version

@skriss
Copy link
Member

skriss commented Jan 4, 2019

SGTM for now, though we probably need more thinking/work about how a tarball without an ark-backup.json file would be portable, if that's a goal. Can address later.

@ncdc
Copy link
Contributor

ncdc commented Jan 4, 2019

I guess it probably can't be 100% portable, since we have other information in the bucket (snapshot details, logs)

@skriss
Copy link
Member

skriss commented Jan 7, 2019

this lgtm pending moving the file into a metadata/ dir

@wwitzel3 wwitzel3 changed the title WIP: Add backup-version file in backup tarball Add backup-version file in backup tarball Jan 7, 2019
@wwitzel3
Copy link
Contributor Author

wwitzel3 commented Jan 7, 2019

Ok, I think this is ready to go now. PTAL @skriss @ncdc

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

lgtm

@wwitzel3
Copy link
Contributor Author

wwitzel3 commented Jan 8, 2019

This is ready to merge if everyone is happy with it.

pkg/backup/backup.go Outdated Show resolved Hide resolved
pkg/backup/backup.go Outdated Show resolved Hide resolved
Signed-off-by: Wayne Witzel III <wwitzel3@vmware.com>
Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

I like this trend of moving logic and data to outside of controllers. This lgtm. 👍

@skriss
Copy link
Member

skriss commented Jan 22, 2019

@ncdc I think we were just waiting for your approval here since you had a comment - merge if happy :)

@ncdc
Copy link
Contributor

ncdc commented Jan 22, 2019

Didn't realize y'all were waiting on me!

@ncdc ncdc merged commit 5847dca into vmware-tanzu:master Jan 22, 2019
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.

None yet

5 participants