Full snapshot lease update retry on failure #711

anveshreddy18 · 2024-01-30T16:35:45Z

What this PR does / why we need it:

Currently a Full Snapshot Lease update is triggered when a Full Snapshot ( either scheduled or out-of-schedule ) is taken. And as the Full Snapshot is taken every 24 hrs ( configurable ), if there ever is a failure in updating the Full Snapshot lease, it has to wait for the next full snapshot for it to get updated, which is a long time to wait in this case. This creates a problem which is well documented in this issue by @unmarshall, thanks for that!.

This PR attempts to update the full snapshot lease by periodically trying to update it with an interval defined by FullSnapshotLeaseUpdateInterval in the snapshotter.healthConfig. The retry stops once the lease is upto date, so as to not make unnecessary calls to API server. Basically ensuring that full snapshot lease is upto date for most of the time.

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

NOTE: With the etcd-druid PR#764 getting merged, it allows to configure the fullSnapshotLeaseUpdateInterval from Etcd yaml.

Special notes for your reviewer:
To test this with kind setup, remove the get option under lease from the role used by etcd-test serviceaccount and trigger a full snapshot, the snapshotter won't be able to fetch the lease hence failure, check the backup-restore logs, now insert the get back and see the lease getting updated in the next call to lease update, and the periodic retry stops. Tip: decrease the FullSnapshotLeaseUpdateInterval time to 1 minute to make this process faster.

Release note:

Introduces periodic updates to the Full Snapshot Lease, addressing delays in lease updates during failures

Introduced a new flag `full-snapshot-lease-update-interval` that can be used to set the periodic interval for full snapshot lease update. If the flag is not set, default interval of 3 minutes is considered.

pkg/snapshot/snapshotter/snapshotter.go

pkg/snapshot/snapshotter/snapshot_lease_update.go

.ci/unit_test

pkg/snapshot/snapshotter/snapshot_lease_update.go

pkg/snapshot/snapshotter/snapshotter.go

pkg/snapshot/snapshotter/snapshotter_test.go

ishan16696

Overall looks good, just few nits.

pkg/snapshot/snapshotter/snapshotter_test.go

ishan16696

LGTM!!

@ishan16696

* Full snapshot lease update retry on failure * nit changes * Address review comments by @ishan16696 * Added unit tests for RenewFullSnapshotLeasePeriodically() func * check unit tests on prow * Address review comments * minor change in logs * Add a snapshotter method to set lease update interval * nit change * Address review comments * Resolve unit tests failure * Improve interval time for unit tests * nit change * Address review comments by @ishan16696 * Address review comments by @ishan16696 * make tests pass * nit change * Address review comments by @ishan16696

anveshreddy18 requested a review from a team as a code owner January 30, 2024 16:35

gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jan 30, 2024

anveshreddy18 added kind/bug Bug and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jan 30, 2024

gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Jan 31, 2024

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 Jan 31, 2024

ishan16696 reviewed Jan 31, 2024

View reviewed changes

pkg/snapshot/snapshotter/snapshotter.go Outdated Show resolved Hide resolved

anveshreddy18 self-assigned this Jan 31, 2024

shreyas-s-rao self-assigned this Feb 1, 2024

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 Feb 1, 2024

gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Feb 6, 2024

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 Feb 6, 2024

anveshreddy18 force-pushed the bug/full-snapshot-lease-update branch from ba80618 to f696a32 Compare February 6, 2024 06:25

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 Feb 6, 2024

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 Feb 6, 2024

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 Feb 6, 2024

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 Feb 6, 2024

anveshreddy18 mentioned this pull request Feb 8, 2024

Add a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR gardener/etcd-druid#763

Closed

ishan16696 requested changes Feb 8, 2024

View reviewed changes

Address review comments by @ishan16696

734f2ad

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 Mar 13, 2024

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 Mar 13, 2024

make tests pass

1277df9

gardener-robot-ci-3 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 Mar 13, 2024

nit change

6e3a8eb

gardener-robot-ci-3 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 Mar 13, 2024

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 Mar 14, 2024

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 Mar 14, 2024

anveshreddy18 force-pushed the bug/full-snapshot-lease-update branch from e133c02 to 4a797a1 Compare March 14, 2024 12:52

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 Mar 14, 2024

ishan16696 requested changes Mar 15, 2024

View reviewed changes

pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved

pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved

Address review comments by @ishan16696

883a63c

anveshreddy18 force-pushed the bug/full-snapshot-lease-update branch from 4a797a1 to 883a63c Compare March 15, 2024 03:43

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 Mar 15, 2024

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 Mar 15, 2024

ishan16696 approved these changes Mar 15, 2024

View reviewed changes

ishan16696 requested a review from renormalize March 15, 2024 10:50

renormalize approved these changes Mar 15, 2024

View reviewed changes

ishan16696 merged commit ef07e80 into gardener:master Mar 15, 2024
9 checks passed

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Mar 15, 2024

anveshreddy18 added this to the v0.29.0 milestone Mar 18, 2024

anveshreddy18 mentioned this pull request Mar 20, 2024

Fix a full snapshot lease renewal decision #720

Merged

This was referenced Jul 1, 2024

Remove the full-snapshot-lease-update-interval flag from etcdbr API #744

Merged

[rel-v0.28] Full snapshot lease update retry on failure #747

Merged

anveshreddy18 mentioned this pull request Jul 22, 2024

[Enhancement] Set Full Snap renewTime to the time full snapshot was taken instead of full snap lease update time. #752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full snapshot lease update retry on failure #711

Full snapshot lease update retry on failure #711

anveshreddy18 commented Jan 30, 2024 •

edited

Loading

ishan16696 left a comment

ishan16696 left a comment

Full snapshot lease update retry on failure #711

Full snapshot lease update retry on failure #711

Conversation

anveshreddy18 commented Jan 30, 2024 • edited Loading

ishan16696 left a comment

Choose a reason for hiding this comment

ishan16696 left a comment

Choose a reason for hiding this comment

anveshreddy18 commented Jan 30, 2024 •

edited

Loading