fixup! fix(provisionerd): only heartbeat when logs aren't being flushed

coadler · coadler · commit 5938fdb43abc · 2023-04-12T18:53:20.000-05:00
diff --git a/provisionerd/proto/provisionerd.pb.go b/provisionerd/proto/provisionerd.pb.go
diff --git a/provisionerd/proto/provisionerd.proto b/provisionerd/proto/provisionerd.proto
@@ -109,9 +109,6 @@ message UpdateJobRequest {
     repeated provisioner.TemplateVariable template_variables = 4;
 	repeated provisioner.VariableValue user_variable_values = 5;
     bytes readme = 6;
-	// Heartbeat is true when the request is a heartbeat indicating job
-	// liveness.
-	bool heartbeat = 7;
 }
 
 message UpdateJobResponse {
diff --git a/provisionerd/runner/runner.go b/provisionerd/runner/runner.go
@@ -207,7 +207,7 @@ func (r *Runner) Run() {
 	defer r.stop()
 
 	go r.doCleanFinish(ctx)
-	go r.heartbeat(ctx)
+	go r.heartbeatRoutine(ctx)
 	for r.failedJob == nil && r.completedJob == nil {
 		r.cond.Wait()
 	}
@@ -311,6 +311,29 @@ func (r *Runner) ForceStop() {
 	r.cond.Signal()
 }
 
+func (r *Runner) sendHeartbeat(ctx context.Context) (*proto.UpdateJobResponse, error) {
+	ctx, span := r.startTrace(ctx, "updateHeartbeat")
+	defer span.End()
+
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	if !r.okToSend {
+		return nil, errUpdateSkipped
+	}
+
+	// Skip sending a heartbeat if we've sent an update recently.
+	if lastUpdate := r.lastUpdate.Load(); lastUpdate != nil {
+		if time.Since(*lastUpdate) < r.updateInterval {
+			span.SetAttributes(attribute.Bool("heartbeat_skipped", true))
+			return &proto.UpdateJobResponse{}, nil
+		}
+	}
+
+	return r.update(ctx, &proto.UpdateJobRequest{
+		JobId: r.job.JobId,
+	})
+}
+
 func (r *Runner) update(ctx context.Context, u *proto.UpdateJobRequest) (*proto.UpdateJobResponse, error) {
 	ctx, span := r.startTrace(ctx, tracing.FuncName())
 	defer span.End()
@@ -324,7 +347,6 @@ func (r *Runner) update(ctx context.Context, u *proto.UpdateJobRequest) (*proto.
 		attribute.Int64("template_variables_len", int64(len(u.TemplateVariables))),
 		attribute.Int64("user_variable_values_len", int64(len(u.UserVariableValues))),
 		attribute.Int64("readme_len", int64(len(u.Readme))),
-		attribute.Bool("heartbeat", u.Heartbeat),
 	)
 
 	r.mutex.Lock()
@@ -333,14 +355,6 @@ func (r *Runner) update(ctx context.Context, u *proto.UpdateJobRequest) (*proto.
 		return nil, errUpdateSkipped
 	}
 
-	// Skip sending a heartbeat if we've sent an update recently.
-	if lastUpdate := r.lastUpdate.Load(); u.Heartbeat && lastUpdate != nil {
-		if time.Since(*lastUpdate) < r.updateInterval {
-			span.SetAttributes(attribute.Bool("heartbeat_skipped", true))
-			return &proto.UpdateJobResponse{}, nil
-		}
-	}
-
 	return r.sender.UpdateJob(ctx, u)
 }
 
@@ -505,9 +519,10 @@ func (r *Runner) do(ctx context.Context) (*proto.CompletedJob, *proto.FailedJob)
 	}
 }
 
-// heartbeat periodically sends updates on the job, which keeps coder server from assuming the job
-// is stalled, and allows the runner to learn if the job has been canceled by the user.
-func (r *Runner) heartbeat(ctx context.Context) {
+// heartbeatRoutine periodically sends updates on the job, which keeps coder server
+// from assuming the job is stalled, and allows the runner to learn if the job
+// has been canceled by the user.
+func (r *Runner) heartbeatRoutine(ctx context.Context) {
 	ctx, span := r.startTrace(ctx, tracing.FuncName())
 	defer span.End()
 
@@ -521,10 +536,7 @@ func (r *Runner) heartbeat(ctx context.Context) {
 		case <-ticker.C:
 		}
 
-		resp, err := r.update(ctx, &proto.UpdateJobRequest{
-			JobId:     r.job.JobId,
-			Heartbeat: true,
-		})
+		resp, err := r.sendHeartbeat(ctx)
 		if err != nil {
 			err = r.Fail(ctx, r.failedJobf("send periodic update: %s", err))
 			if err != nil {
@@ -1109,6 +1121,7 @@ func (r *Runner) failedJobf(format string, args ...interface{}) *proto.FailedJob
 func (r *Runner) startTrace(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
 	return r.tracer.Start(ctx, name, append(opts, trace.WithAttributes(
 		semconv.ServiceNameKey.String("coderd.provisionerd"),
+		attribute.String("job_id", r.job.JobId),
 	))...)
 }
 

Original file line number	Diff line number	Diff line change
`@@ -109,9 +109,6 @@ message UpdateJobRequest {`
`109`	`109`	`repeated provisioner.TemplateVariable template_variables = 4;`
`110`	`110`	`repeated provisioner.VariableValue user_variable_values = 5;`
`111`	`111`	`bytes readme = 6;`
`112`		`- // Heartbeat is true when the request is a heartbeat indicating job`
`113`		`- // liveness.`
`114`		`- bool heartbeat = 7;`
`115`	`112`	`}`
`116`	`113`
`117`	`114`	`message UpdateJobResponse {`