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

[Feature] Expose api to trigger out-of-schedule delta snapshot #192

Merged

Conversation

swapnilgm
Copy link
Contributor

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:

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

Special notes for your reviewer:

Release note:

Expose http API to trigger out-of-schedule delta snapshot.

@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) 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 Aug 26, 2019
@swapnilgm swapnilgm added area/backup Backup related exp/beginner Issue that requires only basic skills kind/api-change API change with impact on API users needs/lgtm Needs approval for merging needs/review Needs review platform/all priority/normal labels Sep 11, 2019
@swapnilgm swapnilgm added this to the 0.8.0 milestone Sep 16, 2019
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.

@swapnilgm Thanks for the well-written PR! There is one case that's not been handled: when delta snapshots are disabled, triggering out-of-schedule delta snapshot is not able to take a delta snapshot because watchCh is not set, so no events are collected. Please fix this. Other than this, LGTM.

@swapnilgm swapnilgm force-pushed the feature/trigger-delta-snapshot branch from 1beabc1 to 24feaff Compare September 27, 2019 05:03
@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 Sep 27, 2019
@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 Sep 27, 2019
@swapnilgm
Copy link
Contributor Author

@shreyas-s-rao PTAL.

Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

LGTM

when delta snapshots are disabled, triggering out-of-schedule delta snapshot is not able to take a delta snapshot because watchCh is not set, so no events are collected.

Can we add a test case for this please?

@swapnilgm swapnilgm force-pushed the feature/trigger-delta-snapshot branch from 24feaff to 0cdcb0c Compare September 27, 2019 06:04
@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 Sep 27, 2019
@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 Sep 27, 2019
@swapnilgm
Copy link
Contributor Author

Can we add a test case for this please?

Done

Copy link
Collaborator

@amshuman-kr amshuman-kr 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 the test-case! LGTM

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.

@swapnilgm Thanks for making the requested changes! Overall LGTM. Just a few small corrections here and there. Please address them. Thanks.

Makefile Outdated Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter.go Outdated Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved
@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 Sep 27, 2019
@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 Sep 27, 2019
Co-Authored-By: Shreyas Rao <[email protected]>
Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the feature/trigger-delta-snapshot branch from efeb22e to 045aa57 Compare September 27, 2019 06:33
@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 Sep 27, 2019
@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 Sep 27, 2019
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 👍🏼

@shreyas-s-rao shreyas-s-rao merged commit 18cfa3b into gardener:master Sep 27, 2019
@swapnilgm swapnilgm deleted the feature/trigger-delta-snapshot branch September 27, 2019 06:56
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backup Backup related exp/beginner Issue that requires only basic skills kind/api-change API change with impact on API users needs/lgtm Needs approval for merging needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review platform/all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose an API to trigger delta snapshot
7 participants