Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design #6517: Schedule Skip Immediately Config #6829

Closed

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Sep 15, 2023

Design to add new skipImmediately to schedule spec and related CLI flags for server, install, etc. to allow immediately due run to be skipped.

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

For #6517

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.

Implementation: #7169

@github-actions github-actions bot added Dependencies Pull requests that update a dependency file has-changelog has-e2e-tests has-unit-tests labels Sep 15, 2023
@kaovilai kaovilai closed this Sep 15, 2023
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch from 19c8836 to c85638d Compare September 15, 2023 15:38
@github-actions github-actions bot removed has-e2e-tests Dependencies Pull requests that update a dependency file has-changelog has-unit-tests labels Sep 15, 2023
@kaovilai kaovilai reopened this Sep 15, 2023
@github-actions github-actions bot added the Area/Design Design Documents label Sep 15, 2023
@kaovilai
Copy link
Member Author

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Sep 15, 2023
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch from b930bb7 to 6b87058 Compare September 15, 2023 16:08
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5cbfd9f) 61.75% compared to head (bf3efc3) 61.94%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6829      +/-   ##
==========================================
+ Coverage   61.75%   61.94%   +0.18%     
==========================================
  Files         259      259              
  Lines       27894    27903       +9     
==========================================
+ Hits        17227    17285      +58     
+ Misses       9459     9410      -49     
  Partials     1208     1208              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaovilai kaovilai force-pushed the design-schedule-unpaused branch 2 times, most recently from 1e52b7f to 22abe45 Compare September 15, 2023 16:36
@kaovilai kaovilai marked this pull request as ready for review September 15, 2023 16:37
@kaovilai kaovilai changed the title design schedule unpaused Design #6517: Schedule Unpause Trigger Config Sep 15, 2023
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch 3 times, most recently from 8c4aa56 to 92bed60 Compare September 15, 2023 16:59
@draghuram
Copy link
Contributor

Proposal looks good to me though I would have preferred trigger to be false by default. By far, that is the most intuitive and least surprising.

@kaovilai
Copy link
Member Author

I don't mind moving it to false by default if community agrees.

Just didn't wanna break anyone relying on this.. but to be fair this will go in a new minor version so probably enough to make note of the changes.

@kaovilai
Copy link
Member Author

kaovilai commented Sep 15, 2023

After looking at the code.. I think saying schedule unpause always cause backup to trigger is inaccurate.

To be more precise, the following code is used, based on last backup time to generate if it needs to run after unpausing.

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
	}

	nextRunTime := cronSchedule.Next(lastBackupTime)

	return asOf.After(nextRunTime), nextRunTime
}

So perhaps it would take note when unpause happens (added to ScheduleStatus struct), and then if --unpause-triggers=false and unpause timestamp says less than a second ago, it will generate next run time based on unpauseTimestamp rather than

	nextRunTime := cronSchedule.Next(lastBackupTime)

@kaovilai kaovilai changed the title Design #6517: Schedule Unpause Trigger Config Design #6517: Schedule Skip Immediately Config Nov 20, 2023
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch 4 times, most recently from 2c4aee0 to d58302c Compare November 21, 2023 04:23
ywk253100
ywk253100 previously approved these changes Nov 24, 2023
sseago
sseago previously approved these changes Nov 27, 2023
@kaovilai kaovilai dismissed stale reviews from sseago and ywk253100 via d50e577 November 28, 2023 15:19
@kaovilai kaovilai requested review from ywk253100 and sseago November 28, 2023 16:51
sseago
sseago previously approved these changes Nov 28, 2023
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch 2 times, most recently from 7bed30a to b9be014 Compare November 30, 2023 18:33
@kaovilai
Copy link
Member Author

Switched out added ScheduleStatus field from LastUnpaused to LastSkipped as that is more practical and more useful.

Signed-off-by: Tiger Kaovilai <[email protected]>

switch from "unpause triggers" to "skip immediately" for clarity

Signed-off-by: Tiger Kaovilai <[email protected]>

Apply suggestions from code review

Signed-off-by: Tiger Kaovilai <[email protected]>

Uncomment velero server option

Signed-off-by: Tiger Kaovilai <[email protected]>

Backup will also be triggered at the next cron schedule.

Signed-off-by: Tiger Kaovilai <[email protected]>

Clarify: unpauseTriggers trigger based from lastBackup timestamp,  CRD default blocks server flags

Signed-off-by: Tiger Kaovilai <[email protected]>

`velero schedule unpause schedule-1` will check `.spec.UnpauseTriggers`

Signed-off-by: Tiger Kaovilai <[email protected]>

Add `LastUnpaused` to ScheduleStatus

Signed-off-by: Tiger Kaovilai <[email protected]>

Add `velero install`

Signed-off-by: Tiger Kaovilai <[email protected]>
@kaovilai kaovilai force-pushed the design-schedule-unpaused branch from b33d446 to bf3efc3 Compare December 1, 2023 04:43
@reasonerjt reasonerjt added this to the v1.13 milestone Dec 4, 2023
@kaovilai
Copy link
Member Author

kaovilai commented Dec 8, 2023

technically this already went in via #7169

@kaovilai kaovilai closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Design Design Documents kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes target/FC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants