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.

Sign up for GitHub

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

Jump to bottom

Fix and optimize restoration of delta snapshots #609

Merged
merged 7 commits into from
Apr 5, 2023
Merged

Fix and optimize restoration of delta snapshots #609

merged 7 commits into from
Apr 5, 2023

Conversation

shreyas-s-rao
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao commented Apr 3, 2023 •
edited
Loading

What this PR does / why we need it:
Fix cleanup of temporary directory used for restoration and improve and optimize restoration in general. As part of this task, this PR does the following:

  • Instead of cleaning up the temp files after restoration succeeds, cleanup each temp delta snapshot file as and when the delta snapshot is successfully applied to the embedded etcd
  • Create temp directory before restoration, and remove the temp directory after restoration (to avoid any minute chances of leftover temp files), so temp files are removed even if restoration of delta snapshots fails
  • Make the restoration temp dir configurable via CLI flag --restoration-temp-snapshots-dir
  • Decompress downloaded delta snapshots just before applying them instead of persisting decompressed snapshot files to disk, thus saving disk space
  • Name the temp files after delta snapshot file names to make debugging easier
  • Improve logging for restoration of delta snapshots
  • Change behavior of compactor package to re-use --data-dir CLI flag for restoration of etcd
  • Cosmetic changes to compactor package

Which issue(s) this PR fixes:
Fixes #604

Special notes for your reviewer:
/invite @unmarshall @aaronfern @ishan16696

Release note:

Optimize disk usage during restoration of delta snapshots, and remove scope for errors in the process.

Introduce CLI flag `--restoration-temp-snapshots-dir` to configure directory used for temporarily persisting delta snapshots during restoration.

Fix behavior of `--data-dir` for `etcdbrctl compact` command to be consistent with the flag's usage in other `etcdbrctl` commands.

@shreyas-s-rao shreyas-s-rao added kind/bug Bug component/etcd-backup-restore ETCD Backup & Restore area/logging Logging related labels Apr 3, 2023
@shreyas-s-rao shreyas-s-rao requested a review from a team as a code owner April 3, 2023 17:55
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Apr 3, 2023
@shreyas-s-rao shreyas-s-rao changed the title Fix/restoration temp files Fix and optimize restoration of delta snapshots Apr 3, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 3, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 3, 2023
@aaronfern aaronfern added this to the v0.23.0 milestone Apr 4, 2023
unmarshall
unmarshall reviewed Apr 4, 2023
View reviewed changes
pkg/compactor/compactor.go Show resolved Hide resolved
pkg/compactor/compactor.go Show resolved Hide resolved
pkg/compactor/compactor.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/types/restorer.go Outdated Show resolved Hide resolved
@shreyas-s-rao
Copy link
Collaborator Author

@unmarshall thanks for your review. I've address your concerns and made the necessary changes. PTAL

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 4, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2023
@shreyas-s-rao
Copy link
Collaborator Author

/reviewed ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2023
@shreyas-s-rao
Copy link
Collaborator Author

/reviewed ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 4, 2023
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Apr 4, 2023
@shreyas-s-rao
Copy link
Collaborator Author

@aaronfern thanks for your review. I've addressed your concerns. PTAL

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 5, 2023
unmarshall
unmarshall reviewed Apr 5, 2023
View reviewed changes
pkg/compactor/compactor.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/compactor/compactor.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/types/restorer.go Outdated Show resolved Hide resolved
aaronfern
aaronfern reviewed Apr 5, 2023
View reviewed changes
pkg/snapshot/restorer/restorer.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer.go Outdated Show resolved Hide resolved
@shreyas-s-rao
Copy link
Collaborator Author

@unmarshall @aaronfern thanks for reviewing again. I've addressed your concerns now. PTAL

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@aaronfern aaronfern added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@shreyas-s-rao
Copy link
Collaborator Author

@unmarshall I have changed the CLI flag to restoration-temp-snapshots-dir and RestorationConfig field to TempSnapshotsDir as requested. PTAL

@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@shreyas-s-rao shreyas-s-rao mentioned this pull request Apr 5, 2023
Merged
unmarshall
unmarshall approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Apr 5, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 5, 2023
@shreyas-s-rao
Copy link
Collaborator Author

/merge squash

@gardener-robot gardener-robot added the merge/squash Should be merged via 'Squash and merge' label Apr 5, 2023
@aaronfern aaronfern merged commit 83e7f58 into gardener:master Apr 5, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 5, 2023
@shreyas-s-rao shreyas-s-rao deleted the fix/restoration-temp-files branch April 5, 2023 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfern aaronfern aaronfern approved these changes

@unmarshall unmarshall unmarshall approved these changes

@ishan16696 ishan16696 Awaiting requested review from ishan16696

Assignees
No one assigned
Labels
area/logging Logging related component/etcd-backup-restore ETCD Backup & Restore kind/bug Bug merge/squash Should be merged via 'Squash and merge' needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v0.23.0
Development

Successfully merging this pull request may close these issues.

[Enhancement] Improve performance of restoration, especially for larger backups
7 participants
@shreyas-s-rao @unmarshall @aaronfern @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot