From 7727d535a4440ba8837c0cc8fade70cdd2749457 Mon Sep 17 00:00:00 2001 From: Ashish Amarnath Date: Thu, 3 Dec 2020 07:52:36 -0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20BSLs=20with=20validation=20disab?= =?UTF-8?q?led=20should=20be=20validated=20at=20least=20once=20(#3084)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 BSLs with validation disabled should be validated at least once Signed-off-by: Ashish Amarnath * review comments Signed-off-by: Ashish Amarnath --- changelogs/unreleased/3084-ashish-amarnath | 1 + internal/storage/storagelocation.go | 24 ++++++++++------- internal/storage/storagelocation_test.go | 26 ++++++++++++------- ...backup_storage_location_controller_test.go | 4 +-- 4 files changed, 34 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/3084-ashish-amarnath diff --git a/changelogs/unreleased/3084-ashish-amarnath b/changelogs/unreleased/3084-ashish-amarnath new file mode 100644 index 000000000..986a230d5 --- /dev/null +++ b/changelogs/unreleased/3084-ashish-amarnath @@ -0,0 +1 @@ +🐛 BSLs with validation disabled should be validated at least once diff --git a/internal/storage/storagelocation.go b/internal/storage/storagelocation.go index 9ac3cd99f..c5d6dac70 100644 --- a/internal/storage/storagelocation.go +++ b/internal/storage/storagelocation.go @@ -51,24 +51,28 @@ func IsReadyToValidate(bslValidationFrequency *metav1.Duration, lastValidationTi validationFrequency = bslValidationFrequency.Duration } - if validationFrequency == 0 { - log.Debug("Validation period for this backup location is set to 0, skipping validation") - return false - } - if validationFrequency < 0 { log.Debugf("Validation period must be non-negative, changing from %d to %d", validationFrequency, defaultLocationInfo.StoreValidationFrequency) validationFrequency = defaultLocationInfo.StoreValidationFrequency } lastValidation := lastValidationTime - if lastValidation != nil { // always ready to validate the first time around, so only even do this check if this has happened before - nextValidation := lastValidation.Add(validationFrequency) // next validation time: last validation time + validation frequency - if time.Now().UTC().Before(nextValidation) { // ready only when NOW is equal to or after the next validation time - return false - } + if lastValidation == nil { + // Regardless of validation frequency, we want to validate all BSLs at least once. + return true } + if validationFrequency == 0 { + // Validation was disabled so return false. + log.Debug("Validation period for this backup location is set to 0, skipping validation") + return false + } + + // We want to validate BSL only if the set validation frequency/ interval has elapsed. + nextValidation := lastValidation.Add(validationFrequency) // next validation time: last validation time + validation frequency + if time.Now().UTC().Before(nextValidation) { // ready only when NOW is equal to or after the next validation time + return false + } return true } diff --git a/internal/storage/storagelocation_test.go b/internal/storage/storagelocation_test.go index 282518760..c71769b0e 100644 --- a/internal/storage/storagelocation_test.go +++ b/internal/storage/storagelocation_test.go @@ -36,17 +36,23 @@ func TestIsReadyToValidate(t *testing.T) { bslValidationFrequency *metav1.Duration lastValidationTime *metav1.Time defaultLocationInfo DefaultBackupLocationInfo - - // serverDefaultValidationFrequency time.Duration - // backupLocation *velerov1api.BackupStorageLocation - ready bool + ready bool }{ { - name: "don't validate, since frequency is set to zero", + name: "validate when true when validation frequency is zero and lastValidationTime is nil", bslValidationFrequency: &metav1.Duration{Duration: 0}, defaultLocationInfo: DefaultBackupLocationInfo{ StoreValidationFrequency: 0, }, + ready: true, + }, + { + name: "don't validate when false when validation is disabled and lastValidationTime is not nil", + bslValidationFrequency: &metav1.Duration{Duration: 0}, + lastValidationTime: &metav1.Time{Time: time.Now()}, + defaultLocationInfo: DefaultBackupLocationInfo{ + StoreValidationFrequency: 0, + }, ready: false, }, { @@ -63,7 +69,8 @@ func TestIsReadyToValidate(t *testing.T) { defaultLocationInfo: DefaultBackupLocationInfo{ StoreValidationFrequency: 1, }, - ready: false, + lastValidationTime: &metav1.Time{Time: time.Now()}, + ready: false, }, { name: "validate as per default setting when location setting is not set", @@ -77,7 +84,8 @@ func TestIsReadyToValidate(t *testing.T) { defaultLocationInfo: DefaultBackupLocationInfo{ StoreValidationFrequency: 0, }, - ready: false, + lastValidationTime: &metav1.Time{Time: time.Now()}, + ready: false, }, { name: "don't validate when now is before the NEXT validation time (validation frequency + last validation time)", @@ -112,8 +120,8 @@ func TestIsReadyToValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) log := velerotest.NewLogger() - - g.Expect(IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo, log)).To(BeIdenticalTo(tt.ready)) + actual := IsReadyToValidate(tt.bslValidationFrequency, tt.lastValidationTime, tt.defaultLocationInfo, log) + g.Expect(actual).To(BeIdenticalTo(tt.ready)) }) } } diff --git a/pkg/controller/backup_storage_location_controller_test.go b/pkg/controller/backup_storage_location_controller_test.go index 7da53fc8f..6328bafc5 100644 --- a/pkg/controller/backup_storage_location_controller_test.go +++ b/pkg/controller/backup_storage_location_controller_test.go @@ -120,13 +120,13 @@ var _ = Describe("Backup Storage Location Reconciler", func() { wantErr bool }{ { - backupLocation: builder.ForBackupStorageLocation("ns-1", "location-1").ValidationFrequency(0).Result(), + backupLocation: builder.ForBackupStorageLocation("ns-1", "location-1").ValidationFrequency(0).LastValidationTime(time.Now()).Result(), isValidError: nil, expectedPhase: "", wantErr: false, }, { - backupLocation: builder.ForBackupStorageLocation("ns-1", "location-2").ValidationFrequency(0).Result(), + backupLocation: builder.ForBackupStorageLocation("ns-1", "location-2").ValidationFrequency(0).LastValidationTime(time.Now()).Result(), isValidError: nil, expectedPhase: "", wantErr: false,