UX touchups

nomad/fsm.go

@@ -425,7 +425,7 @@ func (n *nomadFSM) applyUpsertJob(buf []byte, index uint64) interface{} {
 	// tracking it.
 	if err := n.periodicDispatcher.Add(req.Job); err != nil {
 		n.logger.Printf("[ERR] nomad.fsm: periodicDispatcher.Add failed: %v", err)
-		return err
+		return fmt.Errorf("failed adding job to periodic dispatcher: %v", err)
 	}
 
 	// Create a watch set

nomad/job_endpoint.go

@@ -1219,7 +1219,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse)
 	if args.Job.IsPeriodic() && args.Job.Periodic.Enabled {
 		reply.NextPeriodicLaunch, err = args.Job.Periodic.Next(time.Now().In(args.Job.Periodic.GetLocation()))
 		if err != nil {
-			return fmt.Errorf("Failed to parse cron time %v", err)
+			return fmt.Errorf("Failed to parse cron expression: %v", err)
 		}
 	}
 

nomad/leader.go

@@ -402,7 +402,8 @@ func (s *Server) restorePeriodicDispatcher() error {
 		// nextLaunch is the next launch that should occur.
 		nextLaunch, err := job.Periodic.Next(launch.Launch.In(job.Periodic.GetLocation()))
 		if err != nil {
-			return fmt.Errorf("failed to parse periodic time: %v", err)
+			s.logger.Printf("[ERR] nomad.periodic: failed to determine next periodic launch for job %s: %v", job.NamespacedID(), err)
+			continue
 		}
 
 		// We skip force launching the job if  there should be no next launch

nomad/periodic.go

@@ -223,7 +223,7 @@ func (p *PeriodicDispatch) Add(job *structs.Job) error {
 	p.tracked[tuple] = job
 	next, err := job.Periodic.Next(time.Now().In(job.Periodic.GetLocation()))
 	if err != nil {
-		return fmt.Errorf("[ERR] nomad.periodic unable to parse cron expression: %v", err)
+		return fmt.Errorf("failed adding job %s: %v", job.NamespacedID(), err)
 	}
 	if tracked {
 		if err := p.heap.Update(job, next); err != nil {
@@ -349,10 +349,9 @@ func (p *PeriodicDispatch) dispatch(job *structs.Job, launchTime time.Time) {
 
 	nextLaunch, err := job.Periodic.Next(launchTime)
 	if err != nil {
-		p.logger.Printf("[ERR] nomad.periodic: failed to parse periodic job %v", err)
-	}
-	if err := p.heap.Update(job, nextLaunch); err != nil {
-		p.logger.Printf("[ERR] nomad.periodic: failed to update next launch of periodic job %q (%s): %v", job.ID, job.Namespace, err)
+		p.logger.Printf("[ERR] nomad.periodic: failed to parse next periodic launch for job %s: %v", job.NamespacedID(), err)
+	} else if err := p.heap.Update(job, nextLaunch); err != nil {
+		p.logger.Printf("[ERR] nomad.periodic: failed to update next launch of periodic job %s: %v", job.NamespacedID(), err)
 	}
 
 	// If the job prohibits overlapping and there are running children, we skip

nomad/structs/structs.go

@@ -1997,6 +1997,14 @@ type Job struct {
 	JobModifyIndex uint64
 }
 
+// NamespacedID returns the namespaced id useful for logging
+func (j *Job) NamespacedID() *NamespacedID {
+	return &NamespacedID{
+		ID:        j.ID,
+		Namespace: j.Namespace,
+	}
+}
+
 // Canonicalize is used to canonicalize fields in the Job. This should be called
 // when registering a Job. A set of warnings are returned if the job was changed
 // in anyway that the user should be made aware of.
@@ -2649,11 +2657,13 @@ func (p *PeriodicConfig) Canonicalize() {
 	p.location = l
 }
 
-func cronParseNext(e *cronexpr.Expression, fromTime time.Time) (t time.Time, err error) {
+// cronParseNext is a helper that parses the next time for the given expression
+// but captures any panic that may occur in the underlying library.
+func cronParseNext(e *cronexpr.Expression, fromTime time.Time, spec string) (t time.Time, err error) {
 	defer func() {
 		if recover() != nil {
 			t = time.Time{}
-			err = fmt.Errorf("Unable to parse cron expression from time")
+			err = fmt.Errorf("failed parsing cron expression: %q", spec)
 		}
 	}()
 
@@ -2668,7 +2678,7 @@ 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)
+			return cronParseNext(e, fromTime, p.Spec)
 		}
 	case PeriodicSpecTest:
 		split := strings.Split(p.Spec, ",")

nomad/structs/structs_test.go

@@ -2015,7 +2015,10 @@ func TestPeriodicConfig_NextCron(t *testing.T) {
 	for i, spec := range specs {
 		p := &PeriodicConfig{Enabled: true, SpecType: PeriodicSpecCron, Spec: spec}
 		p.Canonicalize()
-		n := p.Next(from)
+		n, err := p.Next(from)
+		if err != nil {
+			t.Fatalf("Next returned error: %v", err)
+		}
 		if expected[i] != n {
 			t.Fatalf("Next(%v) returned %v; want %v", from, n, expected[i])
 		}
@@ -2050,8 +2053,14 @@ func TestPeriodicConfig_DST(t *testing.T) {
 	e1 := time.Date(2017, time.March, 11, 10, 0, 0, 0, time.UTC)
 	e2 := time.Date(2017, time.March, 12, 9, 0, 0, 0, time.UTC)
 
-	n1 := p.Next(t1).UTC()
-	n2 := p.Next(t2).UTC()
+	n1, err1 := p.Next(t1)
+	n2, err2 := p.Next(t2)
+	if err1 != nil || err2 != nil {
+		t.Fatalf("bad: %v %v", err1, err2)
+	}
+
+	n1 = n1.UTC()
+	n2 = n2.UTC()
 
 	if !reflect.DeepEqual(e1, n1) {
 		t.Fatalf("Got %v; want %v", n1, e1)