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.

Sign up for GitHub

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

Jump to bottom

implement @start job #52

Closed
wants to merge 1 commit into from
Closed

implement @start job #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cron/cron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ func startFunc(wg *sync.WaitGroup, exitCtx context.Context, logger *logrus.Entry
var cronIteration uint64
nextRun := time.Now()

if expression == crontab.StartExpr {
fn(nextRun, logger)
return
}
Comment on lines +146 to +149
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather handle this purely within the expression interface. Right now, this feels a little hacky, and as a result it means start jobs will not get to finish if you CTRL+C (jobWg.Wait() won't wait for anything and wg.Done() will be called immediately). We also, get incomplete logging.

To do this cleanly, I think we need 2 changes to the interface for expression.Next():

  • Give it a counter of how many times we have gone through this loop already (we ahve this variable in cronIteration already).
  • Return a pointer to a time, then we can check for null and break out of this loop if null.


// NOTE: if overlapping is disabled (default), this does not run multiple
// instances of the job concurrently
for {
Expand Down
9 changes: 8 additions & 1 deletion crontab/crontab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,18 @@ func ParseCrontab(reader io.Reader) (*Crontab, error) {
continue
}

jobLine, err := parseJobLine(line)
// replace start with so the line will parse without errors
jobLine, err := parseJobLine(strings.Replace(line, StartExpressionValue, "0 0 0 1 1 * *", 1))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to call parseJobLine if we're not really trying to parse a cron expression here? If the job line starts with @start, how about we just split that out and return a crontab line directly, as opposed to roundtrip through parseJobLine with dummy data?

if err != nil {
return nil, err
}

// set custom expression for startup job
if strings.HasPrefix(line, StartExpressionValue) {
jobLine.Expression = StartExpr
jobLine.Schedule = StartExpressionValue
}

jobs = append(jobs, &Job{CrontabLine: *jobLine, Position: position})
position++
}
Expand Down
13 changes: 13 additions & 0 deletions crontab/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package crontab

import "time"

const StartExpressionValue = "@start"

var StartExpr = &StartExpression{}

type StartExpression struct{}

func (se StartExpression) Next(fromTime time.Time) time.Time {
return time.Date(9999, 12, 31, 23, 59, 59, 0, time.Local)
}
39 changes: 31 additions & 8 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ func main() {
}()
}

firstRun := true

for true {
promMetrics.Reset()

Expand All @@ -118,20 +120,41 @@ func main() {
if *test {
logrus.Info("crontab is valid")
os.Exit(0)
break
}

var wg sync.WaitGroup
exitCtx, notifyExit := context.WithCancel(context.Background())

for _, job := range tab.Jobs {
cronLogger := logrus.WithFields(logrus.Fields{
"job.schedule": job.Schedule,
"job.command": job.Command,
"job.position": job.Position,
})
if firstRun {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep the code a little simpler and re-run the start jobs if you reload the crontab.

There are 2 reasons for this:

  • First, it keeps the code simpler. We don't need 2 blocks to run start jobs and regular jobs separately, etc.
  • It enables the user to do more. With the approach you have here, you cannot run @start jobs when reloading the crontab. With the approach I'm proposing, you can choose to not run them (remove them from the crontab before reloading), or to run them (don't remove them).

@start might be a bit un-ideal as a name then. In the issue folks were mentioning using @reboot. I think that'd work well here.

Copy link

@hongkongkiwi hongkongkiwi Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think restarting the cron jobs when container restarts is very reasonable. Could we get this into the main code? I really would like to use it and keep using the mainline code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is not a good idea to run all @start jobs again on a reload. if i decleare a job with @start i want the job to run only once at container start.
if we want to be compatible with other cron scheduler we should use @reboot.
if the job should be executed every time the crontab is reloaded, @reboot is a bad name option. mabye @reload is a better option to make it clear, but I'm not happy with it either.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the other review comments, it seems this needs a decision, on two points:

  • Name. I agree that compatibility with @reboot sounds like a good idea, rather than a custom @start.
  • Whether to run on reload or not. For compatibility with other cron systems, I agree it should not run on reload. This code could probably be simplified. There's no "on load" functionality in Vixie's crontab, so I don't understand why the flexibility would be worth having (without a specific usecase.) That said, I store the crontab in container images, so I never use reload.

logrus.Info("running start jobs")
for _, job := range tab.Jobs {
if job.Schedule == crontab.StartExpressionValue {
cronLogger := logrus.WithFields(logrus.Fields{
"job.schedule": job.Schedule,
"job.command": job.Command,
"job.position": job.Position,
})

cron.StartJob(&wg, tab.Context, job, exitCtx, cronLogger, *overlapping, &promMetrics)
}
}

firstRun = false
wg.Wait()

cron.StartJob(&wg, tab.Context, job, exitCtx, cronLogger, *overlapping, &promMetrics)
logrus.Info("start jobs finished")
}

for _, job := range tab.Jobs {
if job.Schedule != crontab.StartExpressionValue {
cronLogger := logrus.WithFields(logrus.Fields{
"job.schedule": job.Schedule,
"job.command": job.Command,
"job.position": job.Position,
})

cron.StartJob(&wg, tab.Context, job, exitCtx, cronLogger, *overlapping, &promMetrics)
}
}

termChan := make(chan os.Signal, 1)
Expand Down