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

Error handling for deletion of delta snapshots #793

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Shreyas-s14
Copy link

What this PR does / why we need it:
This PR changes the error handling for deletion of delta snapshots by replacing the current implementation with concatenation of errors using the errors.Join() method upto a threshold of 5. The process of continues with the deletion of the rest of the "deletable" delta snapshots even when an error is encountered, instead of halting the process. When the number of errors becomes greater than the threshold, it errors out for the current cycle.

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

@Shreyas-s14 Shreyas-s14 requested a review from a team as a code owner October 16, 2024 09:38
@CLAassistant
Copy link

CLAassistant commented Oct 16, 2024

CLA assistant check
All committers have signed the CLA.

@gardener-robot gardener-robot added the needs/review Needs review label Oct 16, 2024
@gardener-robot
Copy link

@Shreyas-s14 Thank you for your contribution.

@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Oct 16, 2024
@gardener-robot-ci-1
Copy link
Contributor

Thank you @Shreyas-s14 for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

Copy link
Contributor

@anveshreddy18 anveshreddy18 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 PR @Shreyas-s14. Have few suggestions. PTAL

pkg/snapshot/snapshotter/garbagecollector.go Show resolved Hide resolved
pkg/snapshot/snapshotter/garbagecollector.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
pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Oct 18, 2024
@anveshreddy18 anveshreddy18 self-assigned this Oct 18, 2024
@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Oct 21, 2024
@anveshreddy18 anveshreddy18 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 Oct 22, 2024
@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 Oct 22, 2024
@@ -528,6 +528,51 @@ var _ = Describe("Snapshotter", func() {
Expect(len(list)).Should(Equal(3))
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also add a positive test case when there is no error in deleting snapshots.

Expect(err).Should(HaveOccurred())
})
})
Context("When the number of errors while deleting are greater than the threshold", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Context("When the number of errors while deleting are greater than the threshold", func() {
Context("When the number of errors while deleting are greater than or equal to threshold", func() {

@@ -85,14 +86,15 @@ func (ssr *Snapshotter) RunGarbageCollector(stopCh <-chan struct{}) {
eod = now.Truncate(24 * time.Hour).Add(23 * time.Hour).Add(59 * time.Minute).Add(59 * time.Second)
trackingWeek = 0
)
// snapStream indicates the list of snapshot, where first snapshot is base/full snapshot followed by list of incremental snapshots based on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move this comment to above as snapStream is used there but defined here.

@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Oct 23, 2024
@anveshreddy18 anveshreddy18 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 Oct 23, 2024
@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 Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/changes Needs (more) changes 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 size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Error Handling in Delta Snapshot Deletion Process
6 participants