From 772b4e1f2afc25e36d5ce890317766b45f83903a Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Thu, 12 Jan 2023 15:29:32 +0530 Subject: [PATCH 01/10] Enhances the decision to take full snapshot during startup to avoid missing of any full-snapshot. --- pkg/miscellaneous/miscellaneous.go | 13 ++++++++++++ pkg/server/backuprestoreserver.go | 6 +----- pkg/snapshot/snapshotter/snapshotter.go | 27 ++++++++++++++++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 039f2fb49..37feffd40 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -550,3 +550,16 @@ func IsPeerURLTLSEnabled() (bool, error) { return peerURL.Scheme == https, nil } + +// GetPrevDayScheduledSnapTime returns the previous day schedule snapshot time. +func GetPrevDayScheduledSnapTime(nextSnapSchedule time.Time) time.Time { + return time.Date( + nextSnapSchedule.Year(), + nextSnapSchedule.Month(), + nextSnapSchedule.Day()-1, + nextSnapSchedule.Hour(), + nextSnapSchedule.Minute(), + nextSnapSchedule.Second(), + nextSnapSchedule.Nanosecond(), + nextSnapSchedule.Location()) +} diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 630323eb1..5a45d866c 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -353,16 +353,12 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // the delta snapshot memory limit), after which a full snapshot // is taken and the regular snapshot schedule comes into effect. - // TODO: write code to find out if prev full snapshot is older than it is - // supposed to be, according to the given cron schedule, instead of the - // hard-coded "24 hours" full snapshot interval - // Temporary fix for missing alternate full snapshots for Gardener shoots // with hibernation schedule set: change value from 24 ot 23.5 to // accommodate for slight pod spin-up delays on shoot wake-up const recentFullSnapshotPeriodInHours = 23.5 initialDeltaSnapshotTaken = false - if ssr.PrevFullSnapshot != nil && !ssr.PrevFullSnapshot.IsFinal && time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() <= recentFullSnapshotPeriodInHours { + if ssr.PrevFullSnapshot != nil && !ssr.PrevFullSnapshot.IsFinal && !ssr.IsScheduledFullSnapshotMissed() { ssrStopped, err := ssr.CollectEventsSincePrevSnapshot(ssrStopCh) if ssrStopped { b.logger.Info("Snapshotter stopped.") diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 5343a21b0..2bb394a9c 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -42,6 +42,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + recentFullSnapshotPeriodInHours = 23.5 +) + var ( emptyStruct struct{} snapstoreHash = make(map[string]interface{}) @@ -104,7 +108,7 @@ func NewSnapshotter(logger *logrus.Entry, config *brtypes.SnapshotterConfig, sto sdl, err := cron.ParseStandard(config.FullSnapshotSchedule) if err != nil { // Ideally this should be validated before. - return nil, fmt.Errorf("invalid schedule provied %s : %v", config.FullSnapshotSchedule, err) + return nil, fmt.Errorf("invalid full snapshot schedule provided %s : %v", config.FullSnapshotSchedule, err) } var prevSnapshot *brtypes.Snapshot @@ -473,12 +477,12 @@ func (ssr *Snapshotter) TakeDeltaSnapshot() (*brtypes.Snapshot, error) { defer rc.Close() if err := ssr.store.Save(*snap, rc); err != nil { - timeTaken := time.Now().Sub(startTime).Seconds() + timeTaken := time.Since(startTime).Seconds() metrics.SnapshotDurationSeconds.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta, metrics.LabelSucceeded: metrics.ValueSucceededFalse}).Observe(timeTaken) ssr.logger.Errorf("Error saving delta snapshots. %v", err) return nil, err } - timeTaken := time.Now().Sub(startTime).Seconds() + timeTaken := time.Since(startTime).Seconds() metrics.SnapshotDurationSeconds.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta, metrics.LabelSucceeded: metrics.ValueSucceededTrue}).Observe(timeTaken) logrus.Infof("Total time to save delta snapshot: %f seconds.", timeTaken) ssr.prevSnapshot = snap @@ -740,3 +744,20 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { snapstoreHash[ssr.snapstoreConfig.Provider] = newSnapstoreSecretHash return true } + +// IsScheduledFullSnapshotMissed checked whether the last scheduled full-snapshot was missed or not. +func (ssr *Snapshotter) IsScheduledFullSnapshotMissed() bool { + if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours { + return true + } + + now := time.Now() + nextSnapSchedule := ssr.schedule.Next(now) + timeLeftToTakeNextSnap := nextSnapSchedule.Sub(now) + + if miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) == ssr.PrevFullSnapshot.CreatedOn { + return false + } + + return timeLeftToTakeNextSnap.Hours()+time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours +} From 9cd9fbc7a563e2264ea5a3bcc822859df024170a Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Fri, 13 Jan 2023 18:10:14 +0530 Subject: [PATCH 02/10] Added unit tests. --- pkg/snapshot/snapshotter/snapshotter_test.go | 88 ++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 5545d9f8b..73c1fee3b 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -334,6 +334,94 @@ var _ = Describe("Snapshotter", func() { }) }) }) + }) + + Describe("Scenarios to take full-snapshot during startup", func() { + var ( + ssr *Snapshotter + currentMin int + currentHour int + ) + BeforeEach(func() { + currentHour = time.Now().Hour() + currentMin = time.Now().Minute() + snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} + store, err = snapstore.GetSnapstore(snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + }) + Context("Previous full snapshot was taken more than 24hrs before, so FullSnapshot missed", func() { + It("should return true", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 2 days before + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-2, currentHour, currentMin, 0, 0, time.Local), + } + isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + Expect(isFullSnapMissed).Should(BeTrue()) + }) + }) + + Context("Previous full snapshot was taken exactly at scheduled snapshot time, no FullSnapshot missed", func() { + It("should return false", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 1 days before at exactly at scheduled time + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), + } + isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + Expect(isFullSnapMissed).Should(BeFalse()) + }) + }) + + Context("Previous snapshot was taken within 24hrs and not gonna missed schedule fullsnapshot", func() { + It("should return false", func() { + scheduleHour := (currentHour + 4) % 24 + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 4hrs 10 mins before startup + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-4)%24, (currentMin-10)%60, 0, 0, time.Local), + } + isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + Expect(isFullSnapMissed).Should(BeFalse()) + }) + }) + + Context("Previous snapshot was taken within 24hrs and gonna miss 24hrs of schedule fullsnapshot window", func() { + It("should return true", func() { + scheduleHour := (currentHour + 8) % 24 + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 18hrs(<24hrs) before startup + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-18)%24, (currentMin)%60, 0, 0, time.Local), + } + isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + Expect(isFullSnapMissed).Should(BeTrue()) + }) + }) Context("##GarbageCollector", func() { var ( From 879dd7393b2df3de7e0e634acf564773fdc0a590 Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Fri, 13 Jan 2023 22:48:58 +0530 Subject: [PATCH 03/10] Update comments. --- pkg/miscellaneous/miscellaneous.go | 3 ++- pkg/snapshot/snapshotter/snapshotter.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 37feffd40..f9cf0c2a8 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -561,5 +561,6 @@ func GetPrevDayScheduledSnapTime(nextSnapSchedule time.Time) time.Time { nextSnapSchedule.Minute(), nextSnapSchedule.Second(), nextSnapSchedule.Nanosecond(), - nextSnapSchedule.Location()) + nextSnapSchedule.Location(), + ) } diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 2bb394a9c..871a608d6 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -745,7 +745,7 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { return true } -// IsScheduledFullSnapshotMissed checked whether the last scheduled full-snapshot was missed or not. +// IsScheduledFullSnapshotMissed checked whether the last scheduled full-snapshot is missed or scheduled full-snapshot can be missed. func (ssr *Snapshotter) IsScheduledFullSnapshotMissed() bool { if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours { return true @@ -759,5 +759,6 @@ func (ssr *Snapshotter) IsScheduledFullSnapshotMissed() bool { return false } + // to check whether schedule full-snapshot can be missed. return timeLeftToTakeNextSnap.Hours()+time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours } From 5ee80410d5f7734425890f7019fc6cb94f3390f6 Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Wed, 18 Jan 2023 12:19:13 +0530 Subject: [PATCH 04/10] Address review comments. --- pkg/server/backuprestoreserver.go | 9 ++-- pkg/snapshot/snapshotter/snapshotter.go | 43 ++++++++++++++++---- pkg/snapshot/snapshotter/snapshotter_test.go | 28 +++++++------ 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 5a45d866c..918e76a3e 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -353,12 +353,9 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // the delta snapshot memory limit), after which a full snapshot // is taken and the regular snapshot schedule comes into effect. - // Temporary fix for missing alternate full snapshots for Gardener shoots - // with hibernation schedule set: change value from 24 ot 23.5 to - // accommodate for slight pod spin-up delays on shoot wake-up - const recentFullSnapshotPeriodInHours = 23.5 + fullSnapshotMaxTimeWindowInHours := ssr.GetFullSnapshotMaxTimeWindow(b.config.SnapshotterConfig.FullSnapshotSchedule) initialDeltaSnapshotTaken = false - if ssr.PrevFullSnapshot != nil && !ssr.PrevFullSnapshot.IsFinal && !ssr.IsScheduledFullSnapshotMissed() { + if ssr.PrevFullSnapshot != nil && !ssr.PrevFullSnapshot.IsFinal && !ssr.WasScheduledFullSnapshotMissed(fullSnapshotMaxTimeWindowInHours) && !ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotMaxTimeWindowInHours) { ssrStopped, err := ssr.CollectEventsSincePrevSnapshot(ssrStopCh) if ssrStopped { b.logger.Info("Snapshotter stopped.") @@ -419,7 +416,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // Start snapshotter b.logger.Infof("Starting snapshotter...") - startWithFullSnapshot := ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || !(time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() <= recentFullSnapshotPeriodInHours) + startWithFullSnapshot := ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.WasScheduledFullSnapshotMissed(fullSnapshotMaxTimeWindowInHours) || ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotMaxTimeWindowInHours) if err := ssr.Run(ssrStopCh, startWithFullSnapshot); err != nil { if etcdErr, ok := err.(*errors.EtcdError); ok { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: etcdErr.Error()}).Inc() diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 871a608d6..129f85d4d 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "path" + "strings" "sync" "time" @@ -43,7 +44,12 @@ import ( ) const ( - recentFullSnapshotPeriodInHours = 23.5 + Min = 0 // Minutes field + Hour = 1 // Hours field + DayOfMonth = 2 // Day of month field + Month = 3 // Month field + DayOfWeek = 4 // Day of week field + defaultFullSnapMaxTimeWindowInHours = 23.5 ) var ( @@ -745,20 +751,41 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { return true } -// IsScheduledFullSnapshotMissed checked whether the last scheduled full-snapshot is missed or scheduled full-snapshot can be missed. -func (ssr *Snapshotter) IsScheduledFullSnapshotMissed() bool { - if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours { +// WasScheduledFullSnapshotMissed checks whether the last full-snapshot was missed or not. +func (ssr *Snapshotter) WasScheduledFullSnapshotMissed(timeWindow float64) bool { + if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow { return true } + now := time.Now() + nextSnapSchedule := ssr.schedule.Next(now) + + return miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) != ssr.PrevFullSnapshot.CreatedOn +} + +// IsNextFullSnapCrossTimeWindow checks whether the next schedule full-snapshot can be taken within the time window or not. +func (ssr *Snapshotter) IsNextFullSnapCrossTimeWindow(timeWindow float64) bool { now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) timeLeftToTakeNextSnap := nextSnapSchedule.Sub(now) - if miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) == ssr.PrevFullSnapshot.CreatedOn { - return false + return timeLeftToTakeNextSnap.Hours()+time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow +} + +// GetFullSnapshotMaxTimeWindow returns the maximum time period in hours for which backup-restore must have atleast 1 full-snapshot. +func (ssr *Snapshotter) GetFullSnapshotMaxTimeWindow(fullSnapScheduleSpec string) float64 { + // Split on whitespace. + schedule := strings.Fields(fullSnapScheduleSpec) + ssr.logger.Infof("schedule: %v", schedule) + if len(schedule) < 5 { + return defaultFullSnapMaxTimeWindowInHours } - // to check whether schedule full-snapshot can be missed. - return timeLeftToTakeNextSnap.Hours()+time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > recentFullSnapshotPeriodInHours + if schedule[DayOfMonth] == "*" && schedule[DayOfWeek] == "*" { + ssr.logger.Infof("defaultFullSnapMaxTimeWindowInHours: %v", defaultFullSnapMaxTimeWindowInHours) + return defaultFullSnapMaxTimeWindowInHours + } else if schedule[DayOfWeek] != "*" { + return defaultFullSnapMaxTimeWindowInHours * 7 + } + return defaultFullSnapMaxTimeWindowInHours } diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 73c1fee3b..b0565a3a5 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -338,18 +338,20 @@ var _ = Describe("Snapshotter", func() { Describe("Scenarios to take full-snapshot during startup", func() { var ( - ssr *Snapshotter - currentMin int - currentHour int + ssr *Snapshotter + currentMin int + currentHour int + fullSnapshotTimeWindow float64 ) BeforeEach(func() { + fullSnapshotTimeWindow = 24 currentHour = time.Now().Hour() currentMin = time.Now().Minute() snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} store, err = snapstore.GetSnapstore(snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) }) - Context("Previous full snapshot was taken more than 24hrs before, so FullSnapshot missed", func() { + Context("Previous full snapshot was taken more than 24hrs before", func() { It("should return true", func() { snapshotterConfig := &brtypes.SnapshotterConfig{ FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), @@ -362,12 +364,12 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-2, currentHour, currentMin, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + isFullSnapMissed := ssr.WasScheduledFullSnapshotMissed(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeTrue()) }) }) - Context("Previous full snapshot was taken exactly at scheduled snapshot time, no FullSnapshot missed", func() { + Context("Previous full snapshot was taken exactly at scheduled snapshot time, no FullSnapshot was missed", func() { It("should return false", func() { snapshotterConfig := &brtypes.SnapshotterConfig{ FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), @@ -380,12 +382,12 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + isFullSnapMissed := ssr.WasScheduledFullSnapshotMissed(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeFalse()) }) }) - Context("Previous snapshot was taken within 24hrs and not gonna missed schedule fullsnapshot", func() { + Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot will be taken within 24hs of time window", func() { It("should return false", func() { scheduleHour := (currentHour + 4) % 24 snapshotterConfig := &brtypes.SnapshotterConfig{ @@ -395,16 +397,16 @@ var _ = Describe("Snapshotter", func() { ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) - // Previous full snapshot was taken 4hrs 10 mins before startup + // Previous full snapshot was taken 4hrs 10 mins before startup of backup-restore ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-4)%24, (currentMin-10)%60, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + isFullSnapMissed := ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeFalse()) }) }) - Context("Previous snapshot was taken within 24hrs and gonna miss 24hrs of schedule fullsnapshot window", func() { + Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot can cross 24hs of time window", func() { It("should return true", func() { scheduleHour := (currentHour + 8) % 24 snapshotterConfig := &brtypes.SnapshotterConfig{ @@ -414,11 +416,11 @@ var _ = Describe("Snapshotter", func() { ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) - // Previous full snapshot was taken 18hrs(<24hrs) before startup + // Previous full snapshot was taken 18hrs(<24hrs) before startup of backup-restore ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-18)%24, (currentMin)%60, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsScheduledFullSnapshotMissed() + isFullSnapMissed := ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeTrue()) }) }) From 70da5d520837bf438153ee682bcd8f294eb2cd61 Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Wed, 25 Jan 2023 18:04:29 +0530 Subject: [PATCH 05/10] Address review comments. --- pkg/server/backuprestoreserver.go | 56 +++--- pkg/snapshot/snapshotter/snapshotter.go | 49 +++-- pkg/snapshot/snapshotter/snapshotter_test.go | 181 ++++++++++--------- 3 files changed, 149 insertions(+), 137 deletions(-) diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 918e76a3e..8c80aa0f4 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -307,8 +307,8 @@ func (b *BackupRestoreServer) runServer(ctx context.Context, restoreOpts *brtype // for the case when backup-restore becomes leading sidecar. func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Context, handler *HTTPHandler, ssr *snapshotter.Snapshotter, ss brtypes.SnapStore, ssrStopCh chan struct{}, ackCh chan struct{}) { var ( - err error - initialDeltaSnapshotTaken bool + err error + initialFullSnapshotTaken bool ) for { @@ -354,8 +354,31 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // is taken and the regular snapshot schedule comes into effect. fullSnapshotMaxTimeWindowInHours := ssr.GetFullSnapshotMaxTimeWindow(b.config.SnapshotterConfig.FullSnapshotSchedule) - initialDeltaSnapshotTaken = false - if ssr.PrevFullSnapshot != nil && !ssr.PrevFullSnapshot.IsFinal && !ssr.WasScheduledFullSnapshotMissed(fullSnapshotMaxTimeWindowInHours) && !ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotMaxTimeWindowInHours) { + initialFullSnapshotTaken = false + if ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) { + // need to take a full snapshot here + var snapshot *brtypes.Snapshot + metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta}).Set(0) + metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindFull}).Set(1) + if snapshot, err = ssr.TakeFullSnapshotAndResetTimer(false); err != nil { + metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: err.Error()}).Inc() + b.logger.Errorf("Failed to take substitute first full snapshot: %v", err) + continue + } + initialFullSnapshotTaken = true + if b.config.HealthConfig.SnapshotLeaseRenewalEnabled { + leaseUpdatectx, cancel := context.WithTimeout(ctx, brtypes.LeaseUpdateTimeoutDuration) + defer cancel() + if err = heartbeat.FullSnapshotCaseLeaseUpdate(leaseUpdatectx, b.logger, snapshot, ssr.K8sClientset, b.config.HealthConfig.FullSnapshotLeaseName, b.config.HealthConfig.DeltaSnapshotLeaseName); err != nil { + b.logger.Warnf("Snapshot lease update failed : %v", err) + } + } + if b.backoffConfig.Start { + b.backoffConfig.ResetExponentialBackoff() + } + } + + if !initialFullSnapshotTaken { ssrStopped, err := ssr.CollectEventsSincePrevSnapshot(ssrStopCh) if ssrStopped { b.logger.Info("Snapshotter stopped.") @@ -368,7 +391,6 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex b.logger.Warnf("Failed to take first delta snapshot: snapshotter failed with error: %v", err) continue } - initialDeltaSnapshotTaken = true if b.config.HealthConfig.SnapshotLeaseRenewalEnabled { leaseUpdatectx, cancel := context.WithTimeout(ctx, brtypes.LeaseUpdateTimeoutDuration) defer cancel() @@ -384,28 +406,6 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex } } - if !initialDeltaSnapshotTaken { - // need to take a full snapshot here - var snapshot *brtypes.Snapshot - metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta}).Set(0) - metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindFull}).Set(1) - if snapshot, err = ssr.TakeFullSnapshotAndResetTimer(false); err != nil { - metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: err.Error()}).Inc() - b.logger.Errorf("Failed to take substitute first full snapshot: %v", err) - continue - } - if b.config.HealthConfig.SnapshotLeaseRenewalEnabled { - leaseUpdatectx, cancel := context.WithTimeout(ctx, brtypes.LeaseUpdateTimeoutDuration) - defer cancel() - if err = heartbeat.FullSnapshotCaseLeaseUpdate(leaseUpdatectx, b.logger, snapshot, ssr.K8sClientset, b.config.HealthConfig.FullSnapshotLeaseName, b.config.HealthConfig.DeltaSnapshotLeaseName); err != nil { - b.logger.Warnf("Snapshot lease update failed : %v", err) - } - } - if b.backoffConfig.Start { - b.backoffConfig.ResetExponentialBackoff() - } - } - // Set snapshotter state to Active ssr.SetSnapshotterActive() @@ -416,7 +416,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // Start snapshotter b.logger.Infof("Starting snapshotter...") - startWithFullSnapshot := ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.WasScheduledFullSnapshotMissed(fullSnapshotMaxTimeWindowInHours) || ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotMaxTimeWindowInHours) + startWithFullSnapshot := ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) if err := ssr.Run(ssrStopCh, startWithFullSnapshot); err != nil { if etcdErr, ok := err.(*errors.EtcdError); ok { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: etcdErr.Error()}).Inc() diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 129f85d4d..160ed63a5 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -44,12 +44,12 @@ import ( ) const ( - Min = 0 // Minutes field - Hour = 1 // Hours field - DayOfMonth = 2 // Day of month field - Month = 3 // Month field - DayOfWeek = 4 // Day of week field - defaultFullSnapMaxTimeWindowInHours = 23.5 + min = iota // Minutes field + hour // Hours field + dayOfMonth // Day of month field + month // Month field + dayOfWeek // Day of week field + defaultFullSnapMaxTimeWindow = 23.5 // default full snapshot time window in hours ) var ( @@ -751,20 +751,32 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { return true } -// WasScheduledFullSnapshotMissed checks whether the last full-snapshot was missed or not. -func (ssr *Snapshotter) WasScheduledFullSnapshotMissed(timeWindow float64) bool { +// IsTakingFullSnapRequiresAtStartup checks whether to take a full-snapshot or not during startup of backup-restore. +func (ssr *Snapshotter) IsTakingFullSnapRequiresAtStartup(timeWindow float64) bool { if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow { return true } + if !ssr.WasScheduledFullSnapshotMissed(timeWindow) { + return false + } + return ssr.IsNextFullSnapCanCrossTimeWindow(timeWindow) +} + +// WasScheduledFullSnapshotMissed determines whether the preceding full-snapshot was missed or not. +func (ssr *Snapshotter) WasScheduledFullSnapshotMissed(timeWindow float64) bool { now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) - return miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) != ssr.PrevFullSnapshot.CreatedOn + if miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) == ssr.PrevFullSnapshot.CreatedOn { + ssr.logger.Info("previous full snapshot was taken at scheduled time, skipping the full snapshot at startup") + return false + } + return true } -// IsNextFullSnapCrossTimeWindow checks whether the next schedule full-snapshot can be taken within the time window or not. -func (ssr *Snapshotter) IsNextFullSnapCrossTimeWindow(timeWindow float64) bool { +// IsNextFullSnapCanCrossTimeWindow detemines whether the next scheduled full-snapshot will cross the time window or not. +func (ssr *Snapshotter) IsNextFullSnapCanCrossTimeWindow(timeWindow float64) bool { now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) timeLeftToTakeNextSnap := nextSnapSchedule.Sub(now) @@ -776,16 +788,15 @@ func (ssr *Snapshotter) IsNextFullSnapCrossTimeWindow(timeWindow float64) bool { func (ssr *Snapshotter) GetFullSnapshotMaxTimeWindow(fullSnapScheduleSpec string) float64 { // Split on whitespace. schedule := strings.Fields(fullSnapScheduleSpec) - ssr.logger.Infof("schedule: %v", schedule) if len(schedule) < 5 { - return defaultFullSnapMaxTimeWindowInHours + return defaultFullSnapMaxTimeWindow } - if schedule[DayOfMonth] == "*" && schedule[DayOfWeek] == "*" { - ssr.logger.Infof("defaultFullSnapMaxTimeWindowInHours: %v", defaultFullSnapMaxTimeWindowInHours) - return defaultFullSnapMaxTimeWindowInHours - } else if schedule[DayOfWeek] != "*" { - return defaultFullSnapMaxTimeWindowInHours * 7 + if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" { + return defaultFullSnapMaxTimeWindow + } else if schedule[dayOfWeek] != "*" { + return defaultFullSnapMaxTimeWindow * 7 } - return defaultFullSnapMaxTimeWindowInHours + + return defaultFullSnapMaxTimeWindow } diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index b0565a3a5..19f59c592 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -334,96 +334,6 @@ var _ = Describe("Snapshotter", func() { }) }) }) - }) - - Describe("Scenarios to take full-snapshot during startup", func() { - var ( - ssr *Snapshotter - currentMin int - currentHour int - fullSnapshotTimeWindow float64 - ) - BeforeEach(func() { - fullSnapshotTimeWindow = 24 - currentHour = time.Now().Hour() - currentMin = time.Now().Minute() - snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} - store, err = snapstore.GetSnapstore(snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - }) - Context("Previous full snapshot was taken more than 24hrs before", func() { - It("should return true", func() { - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), - } - - ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - // Previous full snapshot was taken 2 days before - ssr.PrevFullSnapshot = &brtypes.Snapshot{ - CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-2, currentHour, currentMin, 0, 0, time.Local), - } - isFullSnapMissed := ssr.WasScheduledFullSnapshotMissed(fullSnapshotTimeWindow) - Expect(isFullSnapMissed).Should(BeTrue()) - }) - }) - - Context("Previous full snapshot was taken exactly at scheduled snapshot time, no FullSnapshot was missed", func() { - It("should return false", func() { - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), - } - - ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - // Previous full snapshot was taken 1 days before at exactly at scheduled time - ssr.PrevFullSnapshot = &brtypes.Snapshot{ - CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), - } - isFullSnapMissed := ssr.WasScheduledFullSnapshotMissed(fullSnapshotTimeWindow) - Expect(isFullSnapMissed).Should(BeFalse()) - }) - }) - - Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot will be taken within 24hs of time window", func() { - It("should return false", func() { - scheduleHour := (currentHour + 4) % 24 - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), - } - - ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - // Previous full snapshot was taken 4hrs 10 mins before startup of backup-restore - ssr.PrevFullSnapshot = &brtypes.Snapshot{ - CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-4)%24, (currentMin-10)%60, 0, 0, time.Local), - } - isFullSnapMissed := ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotTimeWindow) - Expect(isFullSnapMissed).Should(BeFalse()) - }) - }) - - Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot can cross 24hs of time window", func() { - It("should return true", func() { - scheduleHour := (currentHour + 8) % 24 - snapshotterConfig := &brtypes.SnapshotterConfig{ - FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), - } - - ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) - Expect(err).ShouldNot(HaveOccurred()) - - // Previous full snapshot was taken 18hrs(<24hrs) before startup of backup-restore - ssr.PrevFullSnapshot = &brtypes.Snapshot{ - CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-18)%24, (currentMin)%60, 0, 0, time.Local), - } - isFullSnapMissed := ssr.IsNextFullSnapCrossTimeWindow(fullSnapshotTimeWindow) - Expect(isFullSnapMissed).Should(BeTrue()) - }) - }) Context("##GarbageCollector", func() { var ( @@ -662,6 +572,97 @@ var _ = Describe("Snapshotter", func() { }) }) }) + + Describe("Scenarios to take full-snapshot during startup", func() { + var ( + ssr *Snapshotter + currentMin int + currentHour int + fullSnapshotTimeWindow float64 + ) + BeforeEach(func() { + fullSnapshotTimeWindow = 24 + currentHour = time.Now().Hour() + currentMin = time.Now().Minute() + snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} + store, err = snapstore.GetSnapstore(snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + }) + Context("Previous full snapshot was taken more than 24hrs before", func() { + It("should return true", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 2 days before + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-2, currentHour, currentMin, 0, 0, time.Local), + } + isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapMissed).Should(BeTrue()) + }) + }) + + Context("Previous full snapshot was taken exactly at scheduled snapshot time, no FullSnapshot was missed", func() { + It("should return false", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 1 days before at exactly at scheduled time + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), + } + + isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapMissed).Should(BeFalse()) + }) + }) + + Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot will be taken within 24hs of time window", func() { + It("should return false", func() { + scheduleHour := (currentHour + 4) % 24 + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 4hrs 10 mins before startup of backup-restore + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-4)%24, (currentMin-10)%60, 0, 0, time.Local), + } + isFullSnapCanBeMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapCanBeMissed).Should(BeFalse()) + }) + }) + + Context("Previous snapshot was taken within 24hrs and next schedule full-snapshot likely to cross 24hs of time window", func() { + It("should return true", func() { + scheduleHour := (currentHour + 8) % 24 + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, scheduleHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // Previous full snapshot was taken 18hrs(<24hrs) before startup of backup-restore + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-18)%24, (currentMin)%60, 0, 0, time.Local), + } + isFullSnapCanBeMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapCanBeMissed).Should(BeTrue()) + }) + }) + }) }) // prepareExpectedSnapshotsList prepares the expected snapshotlist based on directory structure From 0789f9623243bbb48f968c432a68338f808bec82 Mon Sep 17 00:00:00 2001 From: ishan tyagi Date: Thu, 2 Feb 2023 19:14:19 +0530 Subject: [PATCH 06/10] Change one condition Added some more unit tests. --- pkg/server/backuprestoreserver.go | 4 +- pkg/snapshot/snapshotter/snapshotter.go | 2 +- pkg/snapshot/snapshotter/snapshotter_test.go | 79 +++++++++++++++++++- 3 files changed, 81 insertions(+), 4 deletions(-) diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 8c80aa0f4..f2045a7ee 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -355,7 +355,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex fullSnapshotMaxTimeWindowInHours := ssr.GetFullSnapshotMaxTimeWindow(b.config.SnapshotterConfig.FullSnapshotSchedule) initialFullSnapshotTaken = false - if ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) { + if ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) { // need to take a full snapshot here var snapshot *brtypes.Snapshot metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta}).Set(0) @@ -416,7 +416,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // Start snapshotter b.logger.Infof("Starting snapshotter...") - startWithFullSnapshot := ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) + startWithFullSnapshot := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) if err := ssr.Run(ssrStopCh, startWithFullSnapshot); err != nil { if etcdErr, ok := err.(*errors.EtcdError); ok { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: etcdErr.Error()}).Inc() diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 160ed63a5..87b548a16 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -753,7 +753,7 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { // IsTakingFullSnapRequiresAtStartup checks whether to take a full-snapshot or not during startup of backup-restore. func (ssr *Snapshotter) IsTakingFullSnapRequiresAtStartup(timeWindow float64) bool { - if time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow { + if ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow { return true } diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 19f59c592..6da46cfd2 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -573,7 +573,7 @@ var _ = Describe("Snapshotter", func() { }) }) - Describe("Scenarios to take full-snapshot during startup", func() { + Describe("Scenarios to take full-snapshot during startup of backup-restore", func() { var ( ssr *Snapshotter currentMin int @@ -588,6 +588,38 @@ var _ = Describe("Snapshotter", func() { store, err = snapstore.GetSnapstore(snapstoreConfig) Expect(err).ShouldNot(HaveOccurred()) }) + Context("No previous snapshot was taken", func() { + It("should return true", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // No previous snapshot was taken + ssr.PrevFullSnapshot = nil + isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapMissed).Should(BeTrue()) + }) + }) + Context("If previous snapshot was final full snapshot", func() { + It("should return true", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", (currentMin+1)%60, (currentHour+2)%24), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + // If previous snapshot was final full snapshot + ssr.PrevFullSnapshot = &brtypes.Snapshot{ + IsFinal: true, + } + isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + Expect(isFullSnapMissed).Should(BeTrue()) + }) + }) Context("Previous full snapshot was taken more than 24hrs before", func() { It("should return true", func() { snapshotterConfig := &brtypes.SnapshotterConfig{ @@ -663,6 +695,51 @@ var _ = Describe("Snapshotter", func() { }) }) }) + + Describe("Scenarios to get maximum time window for full snapshot", func() { + var ( + ssr *Snapshotter + currentMin int + currentHour int + fullSnapshotTimeWindow float64 + ) + BeforeEach(func() { + fullSnapshotTimeWindow = 23.5 + currentHour = time.Now().Hour() + currentMin = time.Now().Minute() + snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} + store, err = snapstore.GetSnapstore(snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + }) + Context("Full snapshot schedule for once a day", func() { + It("should return 24hours of timeWindow", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * *", currentMin, currentHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + timeWindow := ssr.GetFullSnapshotMaxTimeWindow(snapshotterConfig.FullSnapshotSchedule) + Expect(timeWindow).Should(Equal(fullSnapshotTimeWindow)) + }) + }) + + Context("Full snapshot schedule for once in a week", func() { + It("should return 24*7 hours of timeWindow", func() { + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d %d * * %d", currentMin, currentHour, time.Thursday), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + timeWindow := ssr.GetFullSnapshotMaxTimeWindow(snapshotterConfig.FullSnapshotSchedule) + Expect(timeWindow).Should(Equal(fullSnapshotTimeWindow * 7)) + }) + }) + }) + }) // prepareExpectedSnapshotsList prepares the expected snapshotlist based on directory structure From 2eabb535914bb7074742ee25dc4cd001a43c33b7 Mon Sep 17 00:00:00 2001 From: Ishan Tyagi Date: Thu, 9 Feb 2023 18:10:40 +0530 Subject: [PATCH 07/10] Address review comments. --- pkg/miscellaneous/miscellaneous.go | 10 +++++----- pkg/server/backuprestoreserver.go | 4 ++-- pkg/snapshot/snapshotter/snapshotter.go | 17 +++++++++-------- pkg/snapshot/snapshotter/snapshotter_test.go | 12 ++++++------ 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index f9cf0c2a8..a3cb5c1b8 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -357,7 +357,6 @@ func IsMultiNode(logger *logrus.Entry) bool { } config := map[string]interface{}{} - err = yaml.Unmarshal([]byte(configYML), &config) if err := yaml.Unmarshal([]byte(configYML), &config); err != nil { return false } @@ -551,13 +550,14 @@ func IsPeerURLTLSEnabled() (bool, error) { return peerURL.Scheme == https, nil } -// GetPrevDayScheduledSnapTime returns the previous day schedule snapshot time. -func GetPrevDayScheduledSnapTime(nextSnapSchedule time.Time) time.Time { +// GetPrevScheduledSnapTime returns the previous schedule snapshot time. +// TODO: Previous full snapshot time should be calculated on basis of previous cron schedule of full snapshot. +func GetPrevScheduledSnapTime(nextSnapSchedule time.Time, timeWindow float64) time.Time { return time.Date( nextSnapSchedule.Year(), nextSnapSchedule.Month(), - nextSnapSchedule.Day()-1, - nextSnapSchedule.Hour(), + nextSnapSchedule.Day(), + nextSnapSchedule.Hour()-int(timeWindow), nextSnapSchedule.Minute(), nextSnapSchedule.Second(), nextSnapSchedule.Nanosecond(), diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index f2045a7ee..f23bc0db4 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -355,7 +355,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex fullSnapshotMaxTimeWindowInHours := ssr.GetFullSnapshotMaxTimeWindow(b.config.SnapshotterConfig.FullSnapshotSchedule) initialFullSnapshotTaken = false - if ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) { + if ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotMaxTimeWindowInHours) { // need to take a full snapshot here var snapshot *brtypes.Snapshot metrics.SnapshotRequired.With(prometheus.Labels{metrics.LabelKind: brtypes.SnapshotKindDelta}).Set(0) @@ -416,7 +416,7 @@ func (b *BackupRestoreServer) runEtcdProbeLoopWithSnapshotter(ctx context.Contex // Start snapshotter b.logger.Infof("Starting snapshotter...") - startWithFullSnapshot := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotMaxTimeWindowInHours) + startWithFullSnapshot := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotMaxTimeWindowInHours) if err := ssr.Run(ssrStopCh, startWithFullSnapshot); err != nil { if etcdErr, ok := err.(*errors.EtcdError); ok { metrics.SnapshotterOperationFailure.With(prometheus.Labels{metrics.LabelError: etcdErr.Error()}).Inc() diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 87b548a16..43ddb31c0 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -49,7 +49,7 @@ const ( dayOfMonth // Day of month field month // Month field dayOfWeek // Day of week field - defaultFullSnapMaxTimeWindow = 23.5 // default full snapshot time window in hours + defaultFullSnapMaxTimeWindow = 24 // default full snapshot time window in hours ) var ( @@ -751,8 +751,8 @@ func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool { return true } -// IsTakingFullSnapRequiresAtStartup checks whether to take a full-snapshot or not during startup of backup-restore. -func (ssr *Snapshotter) IsTakingFullSnapRequiresAtStartup(timeWindow float64) bool { +// IsFullSnapshotRequiredAtStartup checks whether to take a full snapshot or not during the startup of backup-restore. +func (ssr *Snapshotter) IsFullSnapshotRequiredAtStartup(timeWindow float64) bool { if ssr.PrevFullSnapshot == nil || ssr.PrevFullSnapshot.IsFinal || time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow { return true } @@ -760,7 +760,7 @@ func (ssr *Snapshotter) IsTakingFullSnapRequiresAtStartup(timeWindow float64) bo if !ssr.WasScheduledFullSnapshotMissed(timeWindow) { return false } - return ssr.IsNextFullSnapCanCrossTimeWindow(timeWindow) + return ssr.IsNextFullSnapshotBeyondTimeWindow(timeWindow) } // WasScheduledFullSnapshotMissed determines whether the preceding full-snapshot was missed or not. @@ -768,15 +768,16 @@ func (ssr *Snapshotter) WasScheduledFullSnapshotMissed(timeWindow float64) bool now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) - if miscellaneous.GetPrevDayScheduledSnapTime(nextSnapSchedule) == ssr.PrevFullSnapshot.CreatedOn { + ssr.logger.Infof(" previous schedule: %v", miscellaneous.GetPrevScheduledSnapTime(nextSnapSchedule, timeWindow)) + if miscellaneous.GetPrevScheduledSnapTime(nextSnapSchedule, timeWindow) == ssr.PrevFullSnapshot.CreatedOn { ssr.logger.Info("previous full snapshot was taken at scheduled time, skipping the full snapshot at startup") return false } return true } -// IsNextFullSnapCanCrossTimeWindow detemines whether the next scheduled full-snapshot will cross the time window or not. -func (ssr *Snapshotter) IsNextFullSnapCanCrossTimeWindow(timeWindow float64) bool { +// IsNextFullSnapshotBeyondTimeWindow determines whether the next scheduled full snapshot will exceed the given time window or not. +func (ssr *Snapshotter) IsNextFullSnapshotBeyondTimeWindow(timeWindow float64) bool { now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) timeLeftToTakeNextSnap := nextSnapSchedule.Sub(now) @@ -784,7 +785,7 @@ func (ssr *Snapshotter) IsNextFullSnapCanCrossTimeWindow(timeWindow float64) boo return timeLeftToTakeNextSnap.Hours()+time.Since(ssr.PrevFullSnapshot.CreatedOn).Hours() > timeWindow } -// GetFullSnapshotMaxTimeWindow returns the maximum time period in hours for which backup-restore must have atleast 1 full-snapshot. +// GetFullSnapshotMaxTimeWindow returns the maximum time period in hours for which backup-restore must take atleast one full snapshot. func (ssr *Snapshotter) GetFullSnapshotMaxTimeWindow(fullSnapScheduleSpec string) float64 { // Split on whitespace. schedule := strings.Fields(fullSnapScheduleSpec) diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 6da46cfd2..4d9f28487 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -599,7 +599,7 @@ var _ = Describe("Snapshotter", func() { // No previous snapshot was taken ssr.PrevFullSnapshot = nil - isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeTrue()) }) }) @@ -616,7 +616,7 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ IsFinal: true, } - isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeTrue()) }) }) @@ -633,7 +633,7 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-2, currentHour, currentMin, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeTrue()) }) }) @@ -652,7 +652,7 @@ var _ = Describe("Snapshotter", func() { CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day()-1, (currentHour+2)%24, (currentMin+1)%60, 0, 0, time.Local), } - isFullSnapMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapMissed).Should(BeFalse()) }) }) @@ -671,7 +671,7 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-4)%24, (currentMin-10)%60, 0, 0, time.Local), } - isFullSnapCanBeMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapCanBeMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapCanBeMissed).Should(BeFalse()) }) }) @@ -690,7 +690,7 @@ var _ = Describe("Snapshotter", func() { ssr.PrevFullSnapshot = &brtypes.Snapshot{ CreatedOn: time.Date(time.Now().Year(), time.Now().Month(), time.Now().Day(), (currentHour-18)%24, (currentMin)%60, 0, 0, time.Local), } - isFullSnapCanBeMissed := ssr.IsTakingFullSnapRequiresAtStartup(fullSnapshotTimeWindow) + isFullSnapCanBeMissed := ssr.IsFullSnapshotRequiredAtStartup(fullSnapshotTimeWindow) Expect(isFullSnapCanBeMissed).Should(BeTrue()) }) }) From aab027c11c7bab82b46598444b891bfdce680566 Mon Sep 17 00:00:00 2001 From: Ishan Tyagi Date: Thu, 9 Feb 2023 19:36:06 +0530 Subject: [PATCH 08/10] Address review comments. Fixed unit tests. --- pkg/snapshot/snapshotter/snapshotter.go | 1 - pkg/snapshot/snapshotter/snapshotter_test.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 43ddb31c0..2137e4352 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -768,7 +768,6 @@ func (ssr *Snapshotter) WasScheduledFullSnapshotMissed(timeWindow float64) bool now := time.Now() nextSnapSchedule := ssr.schedule.Next(now) - ssr.logger.Infof(" previous schedule: %v", miscellaneous.GetPrevScheduledSnapTime(nextSnapSchedule, timeWindow)) if miscellaneous.GetPrevScheduledSnapTime(nextSnapSchedule, timeWindow) == ssr.PrevFullSnapshot.CreatedOn { ssr.logger.Info("previous full snapshot was taken at scheduled time, skipping the full snapshot at startup") return false diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index 4d9f28487..ca68e48ce 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -704,7 +704,7 @@ var _ = Describe("Snapshotter", func() { fullSnapshotTimeWindow float64 ) BeforeEach(func() { - fullSnapshotTimeWindow = 23.5 + fullSnapshotTimeWindow = 24 currentHour = time.Now().Hour() currentMin = time.Now().Minute() snapstoreConfig = &brtypes.SnapstoreConfig{Container: path.Join(outputDir, "default.bkp")} From 44788947486799ff50ed90180553c3eabe678f2e Mon Sep 17 00:00:00 2001 From: Ishan Tyagi Date: Fri, 10 Feb 2023 17:44:52 +0530 Subject: [PATCH 09/10] Address review comments. --- pkg/snapshot/snapshotter/snapshotter.go | 7 ++++++- pkg/snapshot/snapshotter/snapshotter_test.go | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 2137e4352..8a2846749 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "path" + "strconv" "strings" "sync" "time" @@ -792,10 +793,14 @@ func (ssr *Snapshotter) GetFullSnapshotMaxTimeWindow(fullSnapScheduleSpec string return defaultFullSnapMaxTimeWindow } - if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" { + if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" && !strings.Contains(schedule[hour], "/") { return defaultFullSnapMaxTimeWindow } else if schedule[dayOfWeek] != "*" { return defaultFullSnapMaxTimeWindow * 7 + } else if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" && strings.Contains(schedule[hour], "/") { + if timeWindow, err := strconv.ParseFloat(schedule[hour][strings.Index(schedule[hour], "/")+1:], 64); err == nil { + return timeWindow + } } return defaultFullSnapMaxTimeWindow diff --git a/pkg/snapshot/snapshotter/snapshotter_test.go b/pkg/snapshot/snapshotter/snapshotter_test.go index ca68e48ce..2c3aa771d 100644 --- a/pkg/snapshot/snapshotter/snapshotter_test.go +++ b/pkg/snapshot/snapshotter/snapshotter_test.go @@ -738,6 +738,22 @@ var _ = Describe("Snapshotter", func() { Expect(timeWindow).Should(Equal(fullSnapshotTimeWindow * 7)) }) }) + + Context("Full snapshot schedule for every 4 hours", func() { + It("should return 4 hours of timeWindow", func() { + // every 4 hour + scheduleHour := 4 + snapshotterConfig := &brtypes.SnapshotterConfig{ + FullSnapshotSchedule: fmt.Sprintf("%d */%d * * *", 0, scheduleHour), + } + + ssr, err = NewSnapshotter(logger, snapshotterConfig, store, etcdConnectionConfig, compressionConfig, healthConfig, snapstoreConfig) + Expect(err).ShouldNot(HaveOccurred()) + + timeWindow := ssr.GetFullSnapshotMaxTimeWindow(snapshotterConfig.FullSnapshotSchedule) + Expect(timeWindow).Should(Equal(float64(scheduleHour))) + }) + }) }) }) From fd18b36ff16dbcbad64cfb55488f19dad60d7884 Mon Sep 17 00:00:00 2001 From: Ishan Tyagi Date: Fri, 10 Feb 2023 18:12:56 +0530 Subject: [PATCH 10/10] Address review comments 2. --- pkg/snapshot/snapshotter/snapshotter.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/snapshot/snapshotter/snapshotter.go b/pkg/snapshot/snapshotter/snapshotter.go index 8a2846749..ccfb9dece 100644 --- a/pkg/snapshot/snapshotter/snapshotter.go +++ b/pkg/snapshot/snapshotter/snapshotter.go @@ -793,11 +793,11 @@ func (ssr *Snapshotter) GetFullSnapshotMaxTimeWindow(fullSnapScheduleSpec string return defaultFullSnapMaxTimeWindow } - if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" && !strings.Contains(schedule[hour], "/") { - return defaultFullSnapMaxTimeWindow - } else if schedule[dayOfWeek] != "*" { + if schedule[dayOfWeek] != "*" { return defaultFullSnapMaxTimeWindow * 7 - } else if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" && strings.Contains(schedule[hour], "/") { + } + + if schedule[dayOfMonth] == "*" && schedule[dayOfWeek] == "*" && strings.Contains(schedule[hour], "/") { if timeWindow, err := strconv.ParseFloat(schedule[hour][strings.Index(schedule[hour], "/")+1:], 64); err == nil { return timeWindow }