Support pause/unpause schedules #5279

ywk253100 · 2022-09-05T01:54:23Z

Support pause/unpause schedule

Fixes #2363

Signed-off-by: Wenkai Yin(尹文开) yinw@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

codecov-commenter · 2022-09-05T02:21:37Z

Codecov Report

Merging #5279 (4b9dbfa) into main (100d6b4) will decrease coverage by 0.04%.
The diff coverage is 46.42%.

@@            Coverage Diff             @@
##             main    #5279      +/-   ##
==========================================
- Coverage   40.84%   40.80%   -0.05%     
==========================================
  Files         234      236       +2     
  Lines       20260    20349      +89     
==========================================
+ Hits         8276     8303      +27     
- Misses      11383    11444      +61     
- Partials      601      602       +1

Impacted Files	Coverage Δ
pkg/cmd/cli/delete_options.go	`12.50% <0.00%> (ø)`
pkg/cmd/util/output/schedule_describer.go	`0.00% <0.00%> (ø)`
pkg/cmd/util/output/schedule_printer.go	`0.00% <0.00%> (ø)`
...g/controller/backup_storage_location_controller.go	`58.91% <0.00%> (ø)`
pkg/controller/backup_sync_controller.go	`43.90% <0.00%> (+1.33%)`	⬆️
pkg/controller/gc_controller.go	`56.07% <0.00%> (+0.51%)`	⬆️
pkg/controller/schedule_controller.go	`67.69% <0.00%> (-4.07%)`	⬇️
pkg/cmd/cli/select_option.go	`61.90% <61.90%> (ø)`
pkg/util/kube/predicate.go	`65.85% <70.37%> (+8.71%)`	⬆️
pkg/util/kube/periodical_enqueue_source.go	`74.46% <100.00%> (ø)`
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

blackpiglet · 2022-09-05T08:50:07Z

@ywk253100
I just created another PR #5283. It is related to this one, and should have confict.
Please take look at it when available.

blackpiglet · 2022-09-05T08:53:34Z

pkg/controller/schedule_controller.go

@@ -89,6 +89,11 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, errors.Wrapf(err, "error getting schedule %s", req.String())
 	}

+	if schedule.Spec.Paused {


There is already status.Phase SchedulePhaseEnabled providing similar functionality.
I know the purpose of the phase is used to indicate schedule is validated, but it's still a little confusing, and I think schedule controller's status checking logic needs modification. Maybe some refactoring is also needed.

blackpiglet · 2022-09-05T08:57:19Z

pkg/controller/schedule_controller.go

+		log.Info("the schedule is paused, skip")
+		return ctrl.Result{}, nil
+	}
+
 	if schedule.Status.Phase != "" &&
 		schedule.Status.Phase != velerov1.SchedulePhaseNew &&
 		schedule.Status.Phase != velerov1.SchedulePhaseEnabled {


By this condition checking code, if a schedule is marked as FailedValidation, it will not be handled anymore.
To me, this seems not right. Invalid schedule should have a chance for modification.

blackpiglet · 2022-09-05T09:02:53Z

pkg/controller/schedule_controller.go

@@ -89,6 +89,11 @@ func (c *scheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, errors.Wrapf(err, "error getting schedule %s", req.String())
 	}

+	if schedule.Spec.Paused {
+		log.Info("the schedule is paused, skip")


To me, I think this checking can be put into PeriodicalEnqueueSource fitlerFuncs.
If the schedule is in pending state, no updating should be reasonable.

I also put the generated backup state checking into fitler functions, maybe it should be put into reconcile function, because not handling schedule that has backup in progress could be a surprise for user.

blackpiglet · 2022-09-07T06:08:11Z

pkg/controller/backup_sync_controller.go

@@ -284,35 +282,18 @@ func (b *backupSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		mgr.GetClient(),
 		&velerov1api.BackupStorageLocationList{},
 		backupSyncReconcilePeriod,
-		kube.PeriodicalEnqueueSourceOption{
-			OrderFunc: backupSyncSourceOrderFunc,


OrderFunc shouldn't be removed.

blackpiglet · 2022-09-08T07:24:39Z

pkg/controller/schedule_controller_test.go

@@ -62,6 +62,10 @@ func TestReconcileOfSchedule(t *testing.T) {
 			name:     "schedule with phase FailedValidation triggers no backup",
 			schedule: newScheduleBuilder(velerov1api.SchedulePhaseFailedValidation).Result(),
 		},
+		{
+			name:     "paused schedule triggers no backup",
+			schedule: newScheduleBuilder(velerov1api.SchedulePhase("")).Result(),


The paused is not set in the test case.

The pause cannot be tested easily in UT after moving the check logic into pridicate, I have remove this line and we'll cover the test in the E2E

Support pause/unpause schedule Fixes vmware-tanzu#2363 Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>

github-actions bot assigned ywk253100 Sep 5, 2022

github-actions bot requested review from dsu-igeek and qiuming-best September 5, 2022 01:54

github-actions bot added has-changelog has-unit-tests labels Sep 5, 2022

ywk253100 force-pushed the 220829_pause branch from 4700db8 to 8cb7bbc Compare September 5, 2022 02:11

ywk253100 requested a review from blackpiglet September 5, 2022 02:12

ywk253100 mentioned this pull request Sep 5, 2022

Add E2E test case for pause/unpause schedules #5280

Closed

blackpiglet reviewed Sep 5, 2022

View reviewed changes

ywk253100 force-pushed the 220829_pause branch 2 times, most recently from 4633466 to c5631c1 Compare September 7, 2022 02:35

blackpiglet reviewed Sep 7, 2022

View reviewed changes

ywk253100 force-pushed the 220829_pause branch from c5631c1 to c9254d2 Compare September 8, 2022 03:52

blackpiglet reviewed Sep 8, 2022

View reviewed changes

ywk253100 force-pushed the 220829_pause branch from c9254d2 to ef6652a Compare September 8, 2022 23:29

Support pause/unpause schedules

4b9dbfa

Support pause/unpause schedule Fixes vmware-tanzu#2363 Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>

ywk253100 force-pushed the 220829_pause branch from ef6652a to 4b9dbfa Compare September 15, 2022 02:42

blackpiglet approved these changes Sep 15, 2022

View reviewed changes

qiuming-best approved these changes Sep 20, 2022

View reviewed changes

qiuming-best merged commit 07da9b9 into vmware-tanzu:main Sep 20, 2022

kaovilai mentioned this pull request Apr 25, 2024

Add Schedule.spec.pause to docs #7740

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pause/unpause schedules #5279

Support pause/unpause schedules #5279

ywk253100 commented Sep 5, 2022

codecov-commenter commented Sep 5, 2022 •

edited

Loading

blackpiglet commented Sep 5, 2022

blackpiglet Sep 5, 2022

blackpiglet Sep 5, 2022

blackpiglet Sep 5, 2022 •

edited

Loading

blackpiglet Sep 7, 2022

blackpiglet Sep 8, 2022

ywk253100 Sep 8, 2022

Support pause/unpause schedules #5279

Support pause/unpause schedules #5279

Conversation

ywk253100 commented Sep 5, 2022

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov-commenter commented Sep 5, 2022 • edited Loading

Codecov Report

blackpiglet commented Sep 5, 2022

blackpiglet Sep 5, 2022

Choose a reason for hiding this comment

blackpiglet Sep 5, 2022

Choose a reason for hiding this comment

blackpiglet Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

blackpiglet Sep 7, 2022

Choose a reason for hiding this comment

blackpiglet Sep 8, 2022

Choose a reason for hiding this comment

ywk253100 Sep 8, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 5, 2022 •

edited

Loading

blackpiglet Sep 5, 2022 •

edited

Loading