-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: cron flow for canaries #1109
Conversation
pkg/jobs/canary/canary_jobs.go
Outdated
entry = findCronEntry(canary) | ||
if entry != nil && time.Until(entry.Next) < 1*time.Hour { | ||
// run all regular canaries on startup | ||
// Run canary on startup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run all regularly scheduled canaries on startup (<1h) and not daily/weekly schedules
675fb21
to
590de90
Compare
if entry != nil && time.Until(entry.Next) < 1*time.Hour { | ||
// run all regular canaries on startup | ||
//Run all regularly scheduled canaries on startup (<1h) and not daily/weekly schedules | ||
if entry != nil && time.Until(entry.Next) < 1*time.Hour && !exists { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this condition ever satisfied? If the cron entry is not nil, then the update time will also exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have updated the code to modify entry
pkg/jobs/canary/canary_jobs.go
Outdated
if entry != nil && time.Until(entry.Next) < 1*time.Hour { | ||
// run all regular canaries on startup | ||
//Run all regularly scheduled canaries on startup (<1h) and not daily/weekly schedules | ||
if entry != nil && time.Until(entry.Next) < 1*time.Hour && !exists { | ||
job = entry.Job.(CanaryJob) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, from a ux perspective if I add a check that runs every 15 minutes I don't want to wait for 15m to see if it worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moshloop I meant just this one line
job = entry.Job.(CanaryJob)
pkg/jobs/canary/canary_jobs.go
Outdated
if err != nil { | ||
return fmt.Errorf("failed to schedule canary %s/%s: %v", canary.Namespace, canary.Name, err) | ||
} | ||
*entry = CanaryScheduler.Entry(entryID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This panics in the first run when the entry
var is nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a23a001
to
ee1410e
Compare
Fixes: #1105