Schedule SkipImmediately

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

design/schedule-skip-immediately-config_design.md

@@ -21,17 +21,17 @@ With this design, user can skip the first immediately due backup when schedule i
 - Changing the default behavior
 
 ## High-Level Design
-Add a new field with to the schedule spec and as a new cli flags for install, server, schedule commands; allowing user to skip immediately due backup when unpausing.
+Add a new field with to the schedule spec and as a new cli flags for install, server, schedule commands; allowing user to skip immediately due backup when unpausing or schedule creation.
 
 If CLI flag is specified during schedule unpause, velero will update the schedule spec accordingly and override prior spec for `skipImmediately``.
 
 ## Detailed Design
 ### CLI Changes
 `velero schedule unpause` will now take an optional bool flag `--skip-immediately` to allow user to override the behavior configured previously in the schedule spec.
 
-`velero schedule unpause schedule-1 --skip-immediately=false` will unpause the schedule but not skip the backup if due immediately from `schedule.status.lastBackup` timestamp. Backup will be run at the next cron schedule.
+`velero schedule unpause schedule-1 --skip-immediately=false` will unpause the schedule but not skip the backup if due immediately from `Schedule.Status.LastBackup` timestamp. Backup will be run at the next cron schedule.
 
-`velero schedule unpause schedule-1 --skip-immediately=true` will unpause the schedule and skip the backup if due immediately from `schedule.status.lastBackup` timestamp. Backup will also be run at the next cron schedule.
+`velero schedule unpause schedule-1 --skip-immediately=true` will unpause the schedule and skip the backup if due immediately from `Schedule.Status.LastBackup` timestamp. Backup will also be run at the next cron schedule.
 
 `velero schedule unpause schedule-1` will check `.spec.SkipImmediately` in the schedule to determine behavior. This field will default to false to maintain prior behavior.
 
@@ -62,16 +62,16 @@ type ScheduleSpec struct {
 	// +optional
 	Paused bool `json:"paused,omitempty"`
 
-+	// SkipImmediately specifies whether to skip backup if schedule is due immediately from `schedule.status.lastBackup` timestamp when schedule is unpaused or if schedule is new.
++	// SkipImmediately specifies whether to skip backup if schedule is due immediately from `Schedule.Status.LastBackup` timestamp when schedule is unpaused or if schedule is new.
 +	// If true, backup will be skipped immediately when schedule is unpaused if it is due based on .Status.LastBackupTimestamp or schedule is new, and will run at next schedule time.
 +	// If false, backup will not be skipped immediately when schedule is unpaused, but will run at next schedule time.
 +	// If empty, will follow server configuration (default: false). 
 +	// +optional
-+	SkipImmediately *bool `json:"skipImmediately,omitempty"`
++	SkipImmediately bool `json:"skipImmediately,omitempty"`
 }
 ```
 
-`LastUnpaused` will be added to `ScheduleStatus` struct to track the last time a schedule was unpaused to support configurable unpause behavior.
+`LastSkipped` will be added to `ScheduleStatus` struct to track the last time a schedule was skipped.
 ```diff
 // ScheduleStatus captures the current state of a Velero schedule
 type ScheduleStatus struct {
@@ -85,10 +85,10 @@ type ScheduleStatus struct {
 	// +nullable
 	LastBackup *metav1.Time `json:"lastBackup,omitempty"`
 
-+	// LastUnpaused is the last time a Schedule was unpaused
++	// LastSkipped is the last time a Schedule was skipped
 +	// +optional
 +	// +nullable
-+	LastUnpaused *metav1.Time `json:"lastUnpaused,omitempty"`
++	LastSkipped *metav1.Time `json:"lastSkipped,omitempty"`
 
 	// ValidationErrors is a slice of all validation errors (if
 	// applicable)
@@ -97,13 +97,33 @@ type ScheduleStatus struct {
 }
 ```
 
-`LastUnpaused` will be set to the current time when a schedule is unpaused. When a schedule is paused, `LastUnpaused` will be set to nil.
+When `schedule.spec.SkipImmediately` is `true`, `LastSkipped` will be set to the current time, and `schedule.spec.SkipImmediately` set to nil so it can be used again.
 
-When schedule is unpaused, and `schedule.status.lastBackup` is not nil, if `.spec.SkipImmediately` is true, a backup will not be created if the schedule is due immediately from lastBackup timestamp. Backup will be run at the next cron schedule.
+The `getNextRunTime()` function below is updated so `LastSkipped` which is after `LastBackup` will be used to determine next run time.
 
-When schedule is unpaused with `schedule.status.lastBackup` set to nil or schedule is newly created, normally a backup will be created immediately. If `.spec.SkipImmediately` is true, a backup will not be created. Backup will be run at the next cron schedule.
+```go
+func getNextRunTime(schedule *velerov1.Schedule, cronSchedule cron.Schedule, asOf time.Time) (bool, time.Time) {
+	var lastBackupTime time.Time
+	if schedule.Status.LastBackup != nil {
+		lastBackupTime = schedule.Status.LastBackup.Time
+	} else {
+		lastBackupTime = schedule.CreationTimestamp.Time
+	}
+	if schedule.Status.LastSkipped != nil && schedule.Status.LastSkipped.After(lastBackupTime) {
+		lastBackupTime = schedule.Status.LastSkipped.Time
+	}
 
-When schedule is created, if `.spec.SkipImmediately` is true, a backup will not be created. Backup will run at the next cron schedule.
+	nextRunTime := cronSchedule.Next(lastBackupTime)
+
+	return asOf.After(nextRunTime), nextRunTime
+}
+```
+
+When schedule is unpaused, and `Schedule.Status.LastBackup` is not nil, if `Schedule.Status.LastSkipped` is recent, a backup will not be created.
+
+When schedule is unpaused or created with `Schedule.Status.LastBackup` set to nil or schedule is newly created, normally a backup will be created immediately. If `Schedule.Status.LastSkipped` is recent, a backup will not be created.
+
+Backup will be run at the next cron schedule based on LastBackup or LastSkipped whichever is more recent.
 
 ## Alternatives Considered
 

pkg/apis/velero/v1/schedule_types.go

@@ -42,6 +42,14 @@ type ScheduleSpec struct {
 	// Paused specifies whether the schedule is paused or not
 	// +optional
 	Paused bool `json:"paused,omitempty"`
+
+	// SkipImmediately specifies whether to skip backup if schedule is due immediately from `schedule.status.lastBackup` timestamp when schedule is unpaused or if schedule is new.
+	// If true, backup will be skipped immediately when schedule is unpaused if it is due based on .Status.LastBackupTimestamp or schedule is new, and will run at next schedule time.
+	// If false, backup will not be skipped immediately when schedule is unpaused, but will run at next schedule time.
+	// If empty, will follow server configuration (default: false). 
+	// +kubebuilder:default=false
+	// +optional
+	SkipImmediately bool `json:"skipImmediately,omitempty"`
 }
 
 // SchedulePhase is a string representation of the lifecycle phase
@@ -75,6 +83,11 @@ type ScheduleStatus struct {
 	// +nullable
 	LastBackup *metav1.Time `json:"lastBackup,omitempty"`
 
+	// LastSkipped is the last time a Schedule was skipped
+	// +optional
+	// +nullable
+	LastSkipped *metav1.Time `json:"lastSkipped,omitempty"`
+
 	// ValidationErrors is a slice of all validation errors (if
 	// applicable)
 	// +optional

pkg/builder/schedule_builder.go

@@ -89,3 +89,9 @@ func (b *ScheduleBuilder) Template(spec velerov1api.BackupSpec) *ScheduleBuilder
 	b.object.Spec.Template = spec
 	return b
 }
+
+// SkipImmediately sets the Schedule's SkipImmediately.
+func (b *ScheduleBuilder) SkipImmediately(skip bool) *ScheduleBuilder {
+	b.object.Spec.SkipImmediately = skip
+	return b
+}

pkg/controller/schedule_controller.go

@@ -98,11 +98,15 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		}
 		return ctrl.Result{}, errors.Wrapf(err, "error getting schedule %s", req.String())
 	}
-
 	c.metrics.InitSchedule(schedule.Name)
-
+	
 	original := schedule.DeepCopy()
 
+	if schedule.Spec.SkipImmediately {
+		schedule.Spec.SkipImmediately = false
+		schedule.Status.LastSkipped = &metav1.Time{Time: c.clock.Now()}
+	}
+
 	// validation - even if the item is Enabled, we can't trust it
 	// so re-validate
 	currentPhase := schedule.Status.Phase
@@ -116,7 +120,9 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 
 	// update status if it's changed
-	if currentPhase != schedule.Status.Phase {
+	if currentPhase != schedule.Status.Phase ||
+		original.Status.LastSkipped != schedule.Status.LastSkipped ||
+		original.Status.LastBackup != schedule.Status.LastBackup {
 		if err := c.Patch(ctx, schedule, client.MergeFrom(original)); err != nil {
 			return ctrl.Result{}, errors.Wrapf(err, "error updating phase of schedule %s to %s", req.String(), schedule.Status.Phase)
 		}
@@ -131,13 +137,15 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	// If there are backup created by this schedule still in New or InProgress state,
 	// skip current backup creation to avoid running overlap backups.
 	// As the schedule must be validated before checking whether it's due, we cannot put the checking log in Predicate
-	if c.ifDue(schedule, cronSchedule) && !c.checkIfBackupInNewOrProgress(schedule) {
+	due, nextRunTime := c.ifDue(schedule, cronSchedule)
+	durationTillNextRun := nextRunTime.Sub(c.clock.Now())
+	if due && !c.checkIfBackupInNewOrProgress(schedule) {
 		if err := c.submitBackup(ctx, schedule); err != nil {
-			return ctrl.Result{}, errors.Wrapf(err, "error submit backup for schedule %s", req.String())
+			return ctrl.Result{RequeueAfter: durationTillNextRun}, errors.Wrapf(err, "error submit backup for schedule %s", req.String())
 		}
 	}
 
-	return ctrl.Result{}, nil
+	return ctrl.Result{RequeueAfter: durationTillNextRun}, nil
 }
 
 func parseCronSchedule(itm *velerov1.Schedule, logger logrus.FieldLogger) (cron.Schedule, []string) {
@@ -207,16 +215,16 @@ func (c *scheduleReconciler) checkIfBackupInNewOrProgress(schedule *velerov1.Sch
 }
 
 // ifDue check whether schedule is due to create a new backup.
-func (c *scheduleReconciler) ifDue(schedule *velerov1.Schedule, cronSchedule cron.Schedule) bool {
+func (c *scheduleReconciler) ifDue(schedule *velerov1.Schedule, cronSchedule cron.Schedule) (bool, time.Time) {
 	isDue, nextRunTime := getNextRunTime(schedule, cronSchedule, c.clock.Now())
 	log := c.logger.WithField("schedule", kube.NamespaceAndName(schedule))
 
 	if !isDue {
 		log.WithField("nextRunTime", nextRunTime).Debug("Schedule is not due, skipping")
-		return false
+		return false, nextRunTime
 	}
 
-	return true
+	return true, nextRunTime
 }
 
 // submitBackup create a backup from schedule.
@@ -248,6 +256,9 @@ func getNextRunTime(schedule *velerov1.Schedule, cronSchedule cron.Schedule, asO
 	} else {
 		lastBackupTime = schedule.CreationTimestamp.Time
 	}
+	if schedule.Status.LastSkipped != nil && schedule.Status.LastSkipped.After(lastBackupTime) {
+		lastBackupTime = schedule.Status.LastSkipped.Time
+	}
 
 	nextRunTime := cronSchedule.Next(lastBackupTime)
 

pkg/controller/schedule_controller_test.go

@@ -36,6 +36,7 @@ import (
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 )
 
+// Test reconcile function of schedule controller. Pause is not covered as event filter will not allow it through
 func TestReconcileOfSchedule(t *testing.T) {
 	require.Nil(t, velerov1.AddToScheme(scheme.Scheme))
 
@@ -52,6 +53,7 @@ func TestReconcileOfSchedule(t *testing.T) {
 		expectedValidationErrors []string
 		expectedBackupCreate     *velerov1.Backup
 		expectedLastBackup       string
+		expectedLastSkipped       string
 		backup                   *velerov1.Backup
 	}{
 		{
@@ -88,6 +90,13 @@ func TestReconcileOfSchedule(t *testing.T) {
 			expectedBackupCreate: builder.ForBackup("ns", "name-20170101120000").ObjectMeta(builder.WithLabels(velerov1.ScheduleNameLabel, "name")).Result(),
 			expectedLastBackup:   "2017-01-01 12:00:00",
 		},
+		{
+			name:                 "schedule with phase New and SkipImmediately gets validated and does not trigger a backup",
+			schedule:             newScheduleBuilder(velerov1.SchedulePhaseNew).CronSchedule("@every 5m").SkipImmediately(true).Result(),
+			fakeClockTime:        "2017-01-01 12:00:00",
+			expectedPhase:        string(velerov1.SchedulePhaseEnabled),
+			expectedLastSkipped:        "2017-01-01 12:00:00",
+		},
 		{
 			name:                 "schedule with phase Enabled gets re-validated and triggers a backup if valid",
 			schedule:             newScheduleBuilder(velerov1.SchedulePhaseEnabled).CronSchedule("@every 5m").Result(),
@@ -103,6 +112,13 @@ func TestReconcileOfSchedule(t *testing.T) {
 			expectedBackupCreate: builder.ForBackup("ns", "name-20170101120000").ObjectMeta(builder.WithLabels(velerov1.ScheduleNameLabel, "name")).Result(),
 			expectedLastBackup:   "2017-01-01 12:00:00",
 		},
+		{
+			name:                 "schedule that's already run but has SkippedImmediately do not get LastBackup updated",
+			schedule:             newScheduleBuilder(velerov1.SchedulePhaseEnabled).CronSchedule("@every 5m").LastBackupTime("2000-01-01 00:00:00").SkipImmediately(true).Result(),
+			fakeClockTime:        "2017-01-01 12:00:00",
+			expectedLastBackup:   "2000-01-01 00:00:00",
+			expectedLastSkipped:  "2017-01-01 12:00:00",
+		},
 		{
 			name:          "schedule already has backup in New state.",
 			schedule:      newScheduleBuilder(velerov1.SchedulePhaseEnabled).CronSchedule("@every 5m").LastBackupTime("2000-01-01 00:00:00").Result(),
@@ -119,8 +135,8 @@ func TestReconcileOfSchedule(t *testing.T) {
 				testTime time.Time
 				err      error
 			)
-
-			reconciler := NewScheduleReconciler("namespace", logger, client, metrics.NewServerMetrics())
+			name := t.Name()
+			reconciler := NewScheduleReconciler("namespace" + name, logger, client, metrics.NewServerMetrics())
 
 			if test.fakeClockTime != "" {
 				testTime, err = time.Parse("2006-01-02 15:04:05", test.fakeClockTime)
@@ -151,8 +167,14 @@ func TestReconcileOfSchedule(t *testing.T) {
 			}
 			if len(test.expectedLastBackup) > 0 {
 				require.Nil(t, err)
+				require.NotNil(t, schedule.Status.LastBackup)
 				assert.Equal(t, parseTime(test.expectedLastBackup).Unix(), schedule.Status.LastBackup.Unix())
 			}
+			if len(test.expectedLastSkipped) > 0 {
+				require.Nil(t, err)
+				require.NotNil(t, schedule.Status.LastSkipped)
+				assert.Equal(t, parseTime(test.expectedLastSkipped).Unix(), schedule.Status.LastSkipped.Unix())
+			}
 
 			backups := &velerov1.BackupList{}
 			require.Nil(t, client.List(ctx, backups))