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

Skip the full snapshot if initial delta snapshot is taken and #222

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

swapnilgm
Copy link
Contributor

last full snapshot is within 24hr

Signed-off-by: Swapnil Mhamane swapnil.mhamane@sap.com

What this PR does / why we need it:

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

Special notes for your reviewer:

Release note:

Skip the first full snapshot on start,  if initial delta snapshot is taken and last full snapshot is recent enough than 24hr.

@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 21, 2020
@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 21, 2020
@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 21, 2020
@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 21, 2020
@swapnilgm swapnilgm changed the title [WIP] Skip the full snapshot if initial delta snapshot is taken and Skip the full snapshot if initial delta snapshot is taken and Apr 22, 2020
@shreyas-s-rao
Copy link
Collaborator

Can we reduce the full snapshot "recent" duration to something like 15 or 30 minutes? So that sidecar can will take full snapshot only if last full snapshot was more than 30 minutes ago. This is because when etcd starts restarting, there's a higher chance of a data corruption and restoration, and this restoration will take longer if the last full snapshot was taken too long ago. A full snapshot taken 30 minutes ago will have far lesser number of delta snapshots, so the restoration will be quick and there's a smaller downtime there. WDYT?

@swapnilgm
Copy link
Contributor Author

swapnilgm commented Apr 22, 2020

I think restoration time is separate problem. Let's not try to solve it here. Currently we are fine with whatever RPO its provides. The restoration time in generic issue and should be solved with compaction idea that we have in backlog.

last full snapshot is within 24hr

Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@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 23, 2020
@swapnilgm
Copy link
Contributor Author

@shreyas-s-rao @georgekuruvillak If you don't have any further concerns, can we merge this?

Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

/lgtm
Maybe you can also add the bootstrap script change with this PR itself.

@gardener-robot-ci-2 gardener-robot-ci-2 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 24, 2020
@swapnilgm
Copy link
Contributor Author

I have updated the bootstrap script. Please go through it once and merge this.

@gardener-robot-ci-1 gardener-robot-ci-1 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 24, 2020
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

Thanks for making the change. Please address the below comments.

doc/usage/getting_started.md Show resolved Hide resolved
@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 27, 2020
@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 27, 2020
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>

Update chart/etcd-backup-restore/templates/etcd-bootstrap-configmap.yaml

Co-Authored-By: Shreyas Rao <42259948+shreyas-s-rao@users.noreply.github.com>
@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 27, 2020
Copy link
Contributor

@georgekuruvillak georgekuruvillak left a comment

Choose a reason for hiding this comment

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

PTAL

pkg/server/backuprestoreserver.go Show resolved Hide resolved
@swapnilgm
Copy link
Contributor Author

if your queries has been resolved. Will you merge this PR.

Copy link
Contributor

@georgekuruvillak georgekuruvillak left a comment

Choose a reason for hiding this comment

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

lgtm

@shreyas-s-rao
Copy link
Collaborator

/lgtm

@ghost ghost added the reviewed/lgtm Has approval for merging label Apr 28, 2020
@shreyas-s-rao shreyas-s-rao merged commit 105deb6 into gardener:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Skip first full snapshot if last full snapshot is recent enough
6 participants