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

Add incremental backup-restore mechanism #29

Merged
merged 8 commits into from
Jun 13, 2018
Merged

Add incremental backup-restore mechanism #29

merged 8 commits into from
Jun 13, 2018

Conversation

swapnilgm
Copy link
Contributor

@swapnilgm swapnilgm commented Jun 5, 2018 •
edited
Loading

Cloeses #2

Swapnil Mhamane added 2 commits June 4, 2018 17:25
Add incremental snpashot support
6629152 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
Add restoration from increamentatal snapshot
819788e
@swapnilgm swapnilgm added needs/review Needs review needs/tests Needs (more) tests size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Jun 5, 2018
@swapnilgm swapnilgm added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jun 5, 2018
@swapnilgm swapnilgm changed the title Add incremental backup mechanism Add incremental backup-restore mechanism Jun 5, 2018
@swapnilgm swapnilgm added kind/enhancement Enhancement, improvement, extension platform/all status/new Issue is new and unprocessed labels Jun 5, 2018
Update garbage collection logic
55cd889 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
Optimize logic for json unmarshalling of delta snapshots
36145e2 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@swapnilgm swapnilgm removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jun 7, 2018
Update examples
6712697 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@swapnilgm swapnilgm 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 Jun 7, 2018
@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 Jun 7, 2018
Add some more unit tests
05f276d 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@swapnilgm swapnilgm 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 Jun 8, 2018
@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 Jun 8, 2018
@swapnilgm swapnilgm 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 Jun 8, 2018
@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 Jun 8, 2018
@swapnilgm swapnilgm force-pushed the feature/incremental-backup branch 2 times, most recently from b6b1531 to 66e83eb Compare June 8, 2018 15:51
@swapnilgm swapnilgm 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 Jun 8, 2018
@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 Jun 8, 2018
Fix: Set the http stastus ok on first snapshot
048f7d2 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@swapnilgm
Copy link
Contributor Author

@georgekuruvillak Please review and test it thoroughly.

Optimize error handling on watch
8f9f92d 
Signed-off-by: Swapnil Mhamane <swapnil.mhamane@sap.com>
@georgekuruvillak georgekuruvillak added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 13, 2018
georgekuruvillak
georgekuruvillak reviewed Jun 13, 2018
View reviewed changes
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.

Nice work! I have some queries though. Can you check it out.

cmd/restore.go Outdated
@@ -113,3 +112,21 @@ func initialClusterFromName(name string) string {
}
return fmt.Sprintf("%s=http://localhost:2380", n)
}

// getLatestFullSnapshotAndDeltaSnapList resturns the latest snapshot
Copy link
Contributor

Choose a reason for hiding this comment

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

'returns'

pkg/initializer/initializer.go Outdated
@@ -135,3 +133,21 @@ func removeContents(dir string) error {
}
return nil
}

// getLatestFullSnapshotAndDeltaSnapList resturns the latest snapshot
func getLatestFullSnapshotAndDeltaSnapList(store snapstore.SnapStore) (*snapstore.Snapshot, snapstore.SnapList, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid the function duplication? The interface meets 'GetLatest' for snapstore, ryt?

pkg/snapshot/restorer/restorer.go
ctx = context.TODO()
)

for _, e := range events {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we commit on subsequent loop? Can we not fetch the ops in the same loop and commit at the end of the loop? Can we not expect events to be sequential? If that was not the case, we would lose some events because of irregular ordering, wont we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do expect events to be sequential here. On etcd we can have transaction with multile put and delete operation. In that case, we get separate events with same key-value -modification-revision. So, if trace the logic here properly, you can see the we are collecting ops and committing it under same transaction before kv.ModRevision change. And last commit out of for-loop is just for commit the events collected in last loop iterations.

pkg/snapshot/restorer/restorer.go
return applyEventsToEtcd(client, events[newRevisionIndex:])
}

// applyDeltaSnapshot applies thw events from delta snapshot to etcd
Copy link
Contributor

Choose a reason for hiding this comment

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

'the' :)

pkg/snapshot/restorer/restorer.go
return nil
}

// applyFirstDeltaSnapshot applies thw events from first delta snapshot to etcd
Copy link
Contributor

Choose a reason for hiding this comment

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

'the' :)

pkg/snapshot/restorer/restorer.go
return e, nil
}

// applyDeltaSnapshot applies thw events from time sorted list of delta snapshot to etcd sequentially
Copy link
Contributor

Choose a reason for hiding this comment

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

'the' :)

@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 Jun 13, 2018
@swapnilgm swapnilgm merged commit 1a46b5c into gardener:master Jun 13, 2018
@swapnilgm swapnilgm deleted the feature/incremental-backup branch June 13, 2018 15:46
@swapnilgm
Copy link
Contributor Author

Thank you George for reviewing it. I have addressed the changes in next PR on the way. As discussed, i I shall merge this branch now.

@swapnilgm swapnilgm mentioned this pull request Jun 15, 2018
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgekuruvillak georgekuruvillak georgekuruvillak left review comments

Assignees
No one assigned
Labels
kind/enhancement Enhancement, improvement, extension needs/review Needs review needs/tests Needs (more) tests platform/all size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/new Issue is new and unprocessed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swapnilgm @georgekuruvillak @gardener-robot-ci-1