From 5d8d0fe9c396f119194308091e6fd60f226f23d9 Mon Sep 17 00:00:00 2001 From: ajanikow Date: Mon, 30 Sep 2019 12:10:38 +0000 Subject: [PATCH 1/2] bug/update_arango_backup_policy_schedule --- pkg/backup/handlers/arango/policy/handler.go | 41 +++++++++------ .../arango/policy/handler_scheduler_test.go | 52 +++++++++++++++++++ .../arango/policy/handler_suite_test.go | 7 +++ 3 files changed, 83 insertions(+), 17 deletions(-) diff --git a/pkg/backup/handlers/arango/policy/handler.go b/pkg/backup/handlers/arango/policy/handler.go index 5904d18a6..0825b8fc3 100644 --- a/pkg/backup/handlers/arango/policy/handler.go +++ b/pkg/backup/handlers/arango/policy/handler.go @@ -101,17 +101,19 @@ func (h *handler) processBackupPolicy(policy *backupApi.ArangoBackupPolicy) (bac }, nil } - if policy.Status.Scheduled.IsZero() { - expr, err := cron.ParseStandard(policy.Spec.Schedule) - if err != nil { - h.eventRecorder.Warning(policy, policyError, "Policy Error: %s", err.Error()) + now := time.Now() - return backupApi.ArangoBackupPolicyStatus{ - Message: fmt.Sprintf("error while parsing expr: %s", err.Error()), - }, nil - } + expr, err := cron.ParseStandard(policy.Spec.Schedule) + if err != nil { + h.eventRecorder.Warning(policy, policyError, "Policy Error: %s", err.Error()) + + return backupApi.ArangoBackupPolicyStatus{ + Message: fmt.Sprintf("error while parsing expr: %s", err.Error()), + }, nil + } - next := expr.Next(time.Now()) + if policy.Status.Scheduled.IsZero() { + next := expr.Next(now) return backupApi.ArangoBackupPolicyStatus{ Scheduled: meta.Time{ @@ -121,7 +123,19 @@ func (h *handler) processBackupPolicy(policy *backupApi.ArangoBackupPolicy) (bac } // Check if schedule is required - if policy.Status.Scheduled.Unix() > time.Now().Unix() { + if policy.Status.Scheduled.Unix() > now.Unix() { + // check if we need to update schedule in case that string changed + // in other case schedule string will be updated after scheduling objects + next := expr.Next(now) + + if next != policy.Status.Scheduled.Time { + return backupApi.ArangoBackupPolicyStatus{ + Scheduled: meta.Time{ + Time: next, + }, + }, nil + } + return policy.Status, nil } @@ -161,13 +175,6 @@ func (h *handler) processBackupPolicy(policy *backupApi.ArangoBackupPolicy) (bac h.eventRecorder.Normal(policy, backupCreated, "Created ArangoBackup: %s/%s", b.Namespace, b.Name) } - expr, err := cron.ParseStandard(policy.Spec.Schedule) - if err != nil { - return backupApi.ArangoBackupPolicyStatus{ - Message: fmt.Sprintf("error while parsing expr: %s", err.Error()), - }, nil - } - next := expr.Next(time.Now()) h.eventRecorder.Normal(policy, rescheduled, "Rescheduled for: %s", next.String()) diff --git a/pkg/backup/handlers/arango/policy/handler_scheduler_test.go b/pkg/backup/handlers/arango/policy/handler_scheduler_test.go index 795582830..3974fc951 100644 --- a/pkg/backup/handlers/arango/policy/handler_scheduler_test.go +++ b/pkg/backup/handlers/arango/policy/handler_scheduler_test.go @@ -201,3 +201,55 @@ func Test_Scheduler_Valid_MultipleObject_Selector(t *testing.T) { require.NotNil(t, backups[1].Spec.PolicyName) require.Equal(t, policy.Name, *backups[1].Spec.PolicyName) } + +func Test_Reschedule(t *testing.T) { + // Arrange + handler := newFakeHandler() + + name := string(uuid.NewUUID()) + namespace := string(uuid.NewUUID()) + + selectors := map[string]string{ + "SELECTOR": string(uuid.NewUUID()), + } + + policy := newArangoBackupPolicy("* 13 * * *", namespace, name, selectors, backupApi.ArangoBackupTemplate{}) + + // Act + createArangoBackupPolicy(t, handler, policy) + + t.Run("First schedule", func(t *testing.T) { + require.NoError(t, handler.Handle(newItemFromBackupPolicy(operation.Update, policy))) + + // Assert + newPolicy := refreshArangoBackupPolicy(t, handler, policy) + require.Empty(t, newPolicy.Status.Message) + + require.Equal(t, 13, newPolicy.Status.Scheduled.Hour()) + }) + + t.Run("First schedule - second iteration", func(t *testing.T) { + require.NoError(t, handler.Handle(newItemFromBackupPolicy(operation.Update, policy))) + + // Assert + newPolicy := refreshArangoBackupPolicy(t, handler, policy) + require.Empty(t, newPolicy.Status.Message) + + require.Equal(t, 13, newPolicy.Status.Scheduled.Hour()) + }) + + t.Run("Change schedule", func(t *testing.T) { + policy = refreshArangoBackupPolicy(t, handler, policy) + policy.Spec.Schedule = "3 3 * * *" + updateArangoBackupPolicy(t, handler, policy) + + require.NoError(t, handler.Handle(newItemFromBackupPolicy(operation.Update, policy))) + + // Assert + newPolicy := refreshArangoBackupPolicy(t, handler, policy) + require.Empty(t, newPolicy.Status.Message) + + require.Equal(t, 3, newPolicy.Status.Scheduled.Hour()) + require.Equal(t, 3, newPolicy.Status.Scheduled.Minute()) + }) +} \ No newline at end of file diff --git a/pkg/backup/handlers/arango/policy/handler_suite_test.go b/pkg/backup/handlers/arango/policy/handler_suite_test.go index aa6fede41..d2bb20245 100644 --- a/pkg/backup/handlers/arango/policy/handler_suite_test.go +++ b/pkg/backup/handlers/arango/policy/handler_suite_test.go @@ -109,6 +109,13 @@ func createArangoBackupPolicy(t *testing.T, h *handler, policies ...*backupApi.A } } +func updateArangoBackupPolicy(t *testing.T, h *handler, policies ...*backupApi.ArangoBackupPolicy) { + for _, policy := range policies { + _, err := h.client.BackupV1alpha().ArangoBackupPolicies(policy.Namespace).Update(policy) + require.NoError(t, err) + } +} + func newArangoDeployment(namespace string, labels map[string]string) *database.ArangoDeployment { name := string(uuid.NewUUID()) return &database.ArangoDeployment{ From 754f8aba2ecca89e23695345641e2cfd85f3edbb Mon Sep 17 00:00:00 2001 From: ajanikow Date: Mon, 30 Sep 2019 12:59:14 +0000 Subject: [PATCH 2/2] Add Cron test --- .../backup/v1alpha/backup_policy_validate.go | 2 +- .../arango/policy/handler_scheduler_test.go | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pkg/apis/backup/v1alpha/backup_policy_validate.go b/pkg/apis/backup/v1alpha/backup_policy_validate.go index 327e64616..3df57b1dd 100644 --- a/pkg/apis/backup/v1alpha/backup_policy_validate.go +++ b/pkg/apis/backup/v1alpha/backup_policy_validate.go @@ -38,7 +38,7 @@ func (a *ArangoBackupPolicy) Validate() error { } func (a *ArangoBackupPolicySpec) Validate() error { - if expr, err := cron.Parse(a.Schedule); err != nil { + if expr, err := cron.ParseStandard(a.Schedule); err != nil { return fmt.Errorf("error while parsing expr: %s", err.Error()) } else if expr.Next(time.Now()).IsZero() { return fmt.Errorf("invalid schedule format") diff --git a/pkg/backup/handlers/arango/policy/handler_scheduler_test.go b/pkg/backup/handlers/arango/policy/handler_scheduler_test.go index 3974fc951..f5f6b3fc1 100644 --- a/pkg/backup/handlers/arango/policy/handler_scheduler_test.go +++ b/pkg/backup/handlers/arango/policy/handler_scheduler_test.go @@ -252,4 +252,39 @@ func Test_Reschedule(t *testing.T) { require.Equal(t, 3, newPolicy.Status.Scheduled.Hour()) require.Equal(t, 3, newPolicy.Status.Scheduled.Minute()) }) +} + +func Test_Validate(t *testing.T) { + acceptedSchedules := []string{ + "0 0 * * MON,TUE,WED,THU,FRI", + "* * * * *", + } + + for _, c := range acceptedSchedules { + t.Run(c, func(t *testing.T) { + // Arrange + handler := newFakeHandler() + + name := string(uuid.NewUUID()) + namespace := string(uuid.NewUUID()) + + selectors := map[string]string{ + "SELECTOR": string(uuid.NewUUID()), + } + + policy := newArangoBackupPolicy(c, namespace, name, selectors, backupApi.ArangoBackupTemplate{}) + + require.NoError(t, policy.Validate()) + + // Act + createArangoBackupPolicy(t, handler, policy) + + require.NoError(t, handler.Handle(newItemFromBackupPolicy(operation.Update, policy))) + + // Assert + newPolicy := refreshArangoBackupPolicy(t, handler, policy) + require.Empty(t, newPolicy.Status.Message) + require.NotEmpty(t, newPolicy.Status.Scheduled) + }) + } } \ No newline at end of file