Merge pull request #7894 from hashicorp/b-cronexpr-dst-fix

Fix Daylight saving transition handling
hashicorp · May 12, 2020 · cf47153 · cf47153
2 parents 5bc75fd + 1e7ebf5
commit cf47153
Show file tree

Hide file tree

Showing 14 changed files with 465 additions and 268 deletions.
diff --git a/api/go.mod b/api/go.mod
@@ -5,8 +5,8 @@ go 1.12
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/docker/go-units v0.3.3
-	github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75
 	github.com/gorilla/websocket v1.4.1
+	github.com/hashicorp/cronexpr v1.1.0
 	github.com/hashicorp/go-cleanhttp v0.5.1
 	github.com/hashicorp/go-rootcerts v1.0.2
 	github.com/kr/pretty v0.1.0

diff --git a/api/go.sum b/api/go.sum
@@ -4,10 +4,12 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/docker/go-units v0.3.3 h1:Xk8S3Xj5sLGlG5g67hJmYMmUgXv5N4PhkjJHHqrwnTk=
 github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
-github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75 h1:f0n1xnMSmBLzVfsMMvriDyA75NB/oBgILX2GcHXIQzY=
-github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75/go.mod h1:g2644b03hfBX9Ov0ZBDgXXens4rxSxmqFBbhvKv2yVA=
 github.com/gorilla/websocket v1.4.1 h1:q7AeDBpnBk8AogcD4DSag/Ukw/KV+YhzLj2bP5HvKCM=
 github.com/gorilla/websocket v1.4.1/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
+github.com/hashicorp/cronexpr v0.0.0-20200507212857-921335d977b6 h1:hMI/9mZ+/xcLlMG1VjW/KwScPOJRDyY30b4aUzxfz0g=
+github.com/hashicorp/cronexpr v0.0.0-20200507212857-921335d977b6/go.mod h1:P4wA0KBl9C5q2hABiMO7cp6jcIg96CDh1Efb3g1PWA4=
+github.com/hashicorp/cronexpr v1.1.0 h1:dnNsWtH0V2ReN7JccYe8m//Bj14+PjJDntR1dz0Cixk=
+github.com/hashicorp/cronexpr v1.1.0/go.mod h1:P4wA0KBl9C5q2hABiMO7cp6jcIg96CDh1Efb3g1PWA4=
 github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM=
 github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
 github.com/hashicorp/go-rootcerts v1.0.2 h1:jzhAVGtqPKbwpyCPELlgNWhE1znq+qwJtW5Oi2viEzc=

diff --git a/api/jobs.go b/api/jobs.go
@@ -7,7 +7,7 @@ import (
 	"strconv"
 	"time"
 
-	"github.com/gorhill/cronexpr"
+	"github.com/hashicorp/cronexpr"
 )
 
 const (
@@ -648,9 +648,11 @@ func (p *PeriodicConfig) Canonicalize() {
 // passed time.
 func (p *PeriodicConfig) Next(fromTime time.Time) (time.Time, error) {
 	if *p.SpecType == PeriodicSpecCron {
-		if e, err := cronexpr.Parse(*p.Spec); err == nil {
-			return cronParseNext(e, fromTime, *p.Spec)
+		e, err := cronexpr.Parse(*p.Spec)
+		if err != nil {
+			return time.Time{}, fmt.Errorf("failed parsing cron expression %q: %v", *p.Spec, err)
 		}
+		return cronParseNext(e, fromTime, *p.Spec)
 	}
 
 	return time.Time{}, nil
@@ -670,6 +672,7 @@ func cronParseNext(e *cronexpr.Expression, fromTime time.Time, spec string) (t t
 
 	return e.Next(fromTime), nil
 }
+
 func (p *PeriodicConfig) GetLocation() (*time.Location, error) {
 	if p.TimeZone == nil || *p.TimeZone == "" {
 		return time.UTC, nil

diff --git a/nomad/periodic_test.go b/nomad/periodic_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/testutil"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type MockJobEvalDispatcher struct {
@@ -173,29 +174,22 @@ func TestPeriodicDispatch_Add_UpdateJob(t *testing.T) {
 	t.Parallel()
 	p, _ := testPeriodicDispatcher(t)
 	job := mock.PeriodicJob()
-	if err := p.Add(job); err != nil {
-		t.Fatalf("Add failed %v", err)
-	}
+	err := p.Add(job)
+	require.NoError(t, err)
 
 	tracked := p.Tracked()
-	if len(tracked) != 1 {
-		t.Fatalf("Add didn't track the job: %v", tracked)
-	}
+	require.Lenf(t, tracked, 1, "did not track the job")
 
 	// Update the job and add it again.
 	job.Periodic.Spec = "foo"
-	if err := p.Add(job); err != nil {
-		t.Fatalf("Add failed %v", err)
-	}
+	err = p.Add(job)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "failed parsing cron expression")
 
 	tracked = p.Tracked()
-	if len(tracked) != 1 {
-		t.Fatalf("Add didn't update: %v", tracked)
-	}
+	require.Lenf(t, tracked, 1, "did not update")
 
-	if !reflect.DeepEqual(job, tracked[0]) {
-		t.Fatalf("Add didn't properly update: got %v; want %v", tracked[0], job)
-	}
+	require.Equalf(t, job, tracked[0], "add did not properly update")
 }
 
 func TestPeriodicDispatch_Add_Remove_Namespaced(t *testing.T) {

diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
@@ -23,7 +23,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/gorhill/cronexpr"
+	"github.com/hashicorp/cronexpr"
 	"github.com/hashicorp/go-msgpack/codec"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/go-version"
@@ -4424,9 +4424,11 @@ func CronParseNext(e *cronexpr.Expression, fromTime time.Time, spec string) (t t
 func (p *PeriodicConfig) Next(fromTime time.Time) (time.Time, error) {
 	switch p.SpecType {
 	case PeriodicSpecCron:
-		if e, err := cronexpr.Parse(p.Spec); err == nil {
-			return CronParseNext(e, fromTime, p.Spec)
+		e, err := cronexpr.Parse(p.Spec)
+		if err != nil {
+			return time.Time{}, fmt.Errorf("failed parsing cron expression: %q: %v", p.Spec, err)
 		}
+		return CronParseNext(e, fromTime, p.Spec)
 	case PeriodicSpecTest:
 		split := strings.Split(p.Spec, ",")
 		if len(split) == 1 && split[0] == "" {