internal/relui: add stop button to relui
Adds a stop button which cancels the workflow's context. Also, style the
buttons on the workflow page a little nicer.
Removes a guard around workflow cancellation that doesn't appear to be
strictly necessary. We still want to mark the task failed in this (and
probably most) scenarios.
Updates test docker target to include workflow package.
Updates golang/go#53317
Change-Id: Ib5df013d23345e9ebe0dc56c10ad27843efe3448
Reviewed-on: https://go-review.googlesource.com/c/build/+/411062
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Alex Rakoczy <alex@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Alex Rakoczy <alex@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
diff --git a/cmd/relui/Dockerfile.test b/cmd/relui/Dockerfile.test
index 50365b1..e5d0235 100644
--- a/cmd/relui/Dockerfile.test
+++ b/cmd/relui/Dockerfile.test
@@ -15,4 +15,4 @@
RUN go build golang.org/x/build/cmd/relui
-ENTRYPOINT go test -v ./cmd/relui/... ./internal/relui/...
+ENTRYPOINT go test -v ./cmd/relui/... ./internal/relui/... ./internal/workflow/...
diff --git a/internal/relui/static/styles.css b/internal/relui/static/styles.css
index 051f9e5..a84c8bf 100644
--- a/internal/relui/static/styles.css
+++ b/internal/relui/static/styles.css
@@ -100,6 +100,9 @@
.WorkflowList-titleTime {
font-size: 1rem;
}
+.WorkflowList-titleStop {
+ float: right;
+}
.WorkflowList-paramData:first-child {
text-transform: capitalize;
}
@@ -236,6 +239,7 @@
font-weight: bold;
}
.Button {
+ border: none;
background: #375eab;
border-radius: 0.1875rem;
box-shadow: 0 0.1875rem 0.0625rem -0.125rem rgba(0, 0, 0, 0.2),
@@ -247,6 +251,10 @@
padding: 0.5rem 1rem;
text-decoration: none;
}
+.Button--small {
+ padding: 0.25rem 0.5rem;
+ font-size: 0.75rem;
+}
.Button:hover,
.Button:focus {
background: #3b65b3;
@@ -260,3 +268,13 @@
0 0.5rem 0.625rem 0.0625rem rgba(0, 0, 0, 0.14),
0 0.1875rem 0.875rem 0.125rem rgba(0, 0, 0, 0.12);
}
+.Button--red {
+ background: #d14836;
+}
+.Button--red:hover,
+.Button--red:focus {
+ background: #c53727;
+}
+.Button--red:active {
+ background: #d14836;
+}
diff --git a/internal/relui/templates/home.html b/internal/relui/templates/home.html
index 9df7c33..d5534f1 100644
--- a/internal/relui/templates/home.html
+++ b/internal/relui/templates/home.html
@@ -17,6 +17,14 @@
<span class="WorkflowList-titleTime">
{{$workflow.CreatedAt.UTC.Format "2006/01/02 15:04 MST"}}
</span>
+ {{if not (or $workflow.Finished $workflow.Error)}}
+ <div class="WorkflowList-titleStop">
+ <form action="{{baseLink (printf "/workflows/%s/stop" $workflow.ID) }}" method="post">
+ <input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
+ <input name="workflow.stop" class="Button Button--red" type="submit" value="STOP" onclick="return this.form.reportValidity() && confirm('This will stop the workflow and all in-flight tasks.\n\nAre you sure you want to proceed?')" />
+ </form>
+ </div>
+ {{end}}
</h3>
<table class="WorkflowList-params">
<tbody>
@@ -102,7 +110,7 @@
<div class="TaskList-retryTask">
<form action="{{baseLink (printf "/workflows/%s/tasks/%s/retry" $workflow.ID $task.Name) }}" method="post">
<input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
- <input name="task.reset" type="submit" value="Retry" onclick="return this.form.reportValidity() && confirm('This will retry the task and clear workflow errors.\n\nReady to proceed?')" />
+ <input class="Button Button--small" name="task.reset" type="submit" value="Retry" onclick="return this.form.reportValidity() && confirm('This will retry the task and clear workflow errors.\n\nReady to proceed?')" />
</form>
</div>
{{end}}
@@ -110,7 +118,7 @@
<div class="TaskList-approveTask">
<form action="{{baseLink (printf "/workflows/%s/tasks/%s/approve" $workflow.ID $task.Name) }}" method="post">
<input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
- <input name="task.approve" type="submit" value="Approve" onclick="return this.form.reportValidity() && confirm('This will mark the task approved and resume the workflow.\n\nReady to proceed?')" />
+ <input class="Button Button--small" name="task.approve" type="submit" value="Approve" onclick="return this.form.reportValidity() && confirm('This will mark the task approved and resume the workflow.\n\nReady to proceed?')" />
</form>
</div>
{{end}}
diff --git a/internal/relui/web.go b/internal/relui/web.go
index bdb290f..6b6a6b6 100644
--- a/internal/relui/web.go
+++ b/internal/relui/web.go
@@ -82,6 +82,7 @@
layout := template.Must(template.New("layout.html").Funcs(helpers).ParseFS(templates, "templates/layout.html"))
s.homeTmpl = template.Must(template.Must(layout.Clone()).Funcs(helpers).ParseFS(templates, "templates/home.html"))
s.newWorkflowTmpl = template.Must(template.Must(layout.Clone()).Funcs(helpers).ParseFS(templates, "templates/new_workflow.html"))
+ s.m.POST("/workflows/:id/stop", s.stopWorkflowHandler)
s.m.POST("/workflows/:id/tasks/:name/retry", s.retryTaskHandler)
s.m.POST("/workflows/:id/tasks/:name/approve", s.approveTaskHandler)
s.m.Handler(http.MethodGet, "/workflows/new", http.HandlerFunc(s.newWorkflowHandler))
@@ -258,7 +259,7 @@
id, err := uuid.Parse(params.ByName("id"))
if err != nil {
log.Printf("retryTaskHandler(_, _, %v) uuid.Parse(%v): %v", params, params.ByName("id"), err)
- http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+ http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
if err := s.retryTask(r.Context(), id, params.ByName("name")); err != nil {
@@ -306,7 +307,7 @@
id, err := uuid.Parse(params.ByName("id"))
if err != nil {
log.Printf("approveTaskHandler(_, _, %v) uuid.Parse(%v): %v", params, params.ByName("id"), err)
- http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+ http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
q := db.New(s.db)
@@ -323,3 +324,17 @@
s.w.l.Logger(id, t.Name).Printf("USER-APPROVED")
http.Redirect(w, r, s.BaseLink("/"), http.StatusSeeOther)
}
+
+func (s *Server) stopWorkflowHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) {
+ id, err := uuid.Parse(params.ByName("id"))
+ if err != nil {
+ log.Printf("stopWorkflowHandler(_, _, %v) uuid.Parse(%v): %v", params, params.ByName("id"), err)
+ http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
+ return
+ }
+ if !s.w.cancelWorkflow(id) {
+ http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+ return
+ }
+ http.Redirect(w, r, s.BaseLink("/"), http.StatusSeeOther)
+}
diff --git a/internal/relui/web_test.go b/internal/relui/web_test.go
index f225f68..7ce6d9a 100644
--- a/internal/relui/web_test.go
+++ b/internal/relui/web_test.go
@@ -27,6 +27,7 @@
"github.com/jackc/pgx/v4"
"github.com/jackc/pgx/v4/pgxpool"
"golang.org/x/build/internal/relui/db"
+ "golang.org/x/build/internal/workflow"
)
// testStatic is our static web server content.
@@ -449,7 +450,7 @@
{
desc: "invalid workflow id",
params: map[string]string{"id": "invalid", "name": "greeting"},
- wantCode: http.StatusNotFound,
+ wantCode: http.StatusBadRequest,
wantWorkflows: []db.Workflow{unchangedWorkflow},
},
{
@@ -593,7 +594,7 @@
{
desc: "invalid workflow id",
params: map[string]string{"id": "invalid", "name": "greeting"},
- wantCode: http.StatusNotFound,
+ wantCode: http.StatusBadRequest,
},
{
desc: "wrong workflow id",
@@ -680,3 +681,75 @@
})
}
}
+
+func TestServerStopWorkflow(t *testing.T) {
+ wfID := uuid.New()
+ cases := []struct {
+ desc string
+ params map[string]string
+ wantCode int
+ wantHeaders map[string]string
+ wantLogs []db.TaskLog
+ wantCancel bool
+ }{
+ {
+ desc: "no params",
+ wantCode: http.StatusNotFound,
+ },
+ {
+ desc: "invalid workflow id",
+ params: map[string]string{"id": "invalid"},
+ wantCode: http.StatusBadRequest,
+ },
+ {
+ desc: "wrong workflow id",
+ params: map[string]string{"id": uuid.New().String()},
+ wantCode: http.StatusNotFound,
+ },
+ {
+ desc: "successful stop",
+ params: map[string]string{"id": wfID.String()},
+ wantCode: http.StatusSeeOther,
+ wantHeaders: map[string]string{
+ "Location": "/",
+ },
+ wantCancel: true,
+ },
+ }
+ for _, c := range cases {
+ t.Run(c.desc, func(t *testing.T) {
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ req := httptest.NewRequest(http.MethodPost, path.Join("/workflows/", c.params["id"], "stop"), nil)
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ rec := httptest.NewRecorder()
+ worker := NewWorker(NewDefinitionHolder(), nil, nil)
+
+ wf := &workflow.Workflow{ID: wfID}
+ if err := worker.markRunning(wf, cancel); err != nil {
+ t.Fatalf("worker.markRunning(%v, %v) = %v, wanted no error", wf, cancel, err)
+ }
+
+ s := NewServer(nil, worker, nil, SiteHeader{})
+ s.m.ServeHTTP(rec, req)
+ resp := rec.Result()
+
+ if resp.StatusCode != c.wantCode {
+ t.Errorf("rep.StatusCode = %d, wanted %d", resp.StatusCode, c.wantCode)
+ }
+ for k, v := range c.wantHeaders {
+ if resp.Header.Get(k) != v {
+ t.Errorf("resp.Header.Get(%q) = %q, wanted %q", k, resp.Header.Get(k), v)
+ }
+ }
+ if c.wantCancel {
+ <-ctx.Done()
+ return
+ }
+ if ctx.Err() != nil {
+ t.Errorf("tx.Err() = %v, wanted no error", ctx.Err())
+ }
+ })
+ }
+}
diff --git a/internal/relui/worker.go b/internal/relui/worker.go
index d14a3fb..e8f77a3 100644
--- a/internal/relui/worker.go
+++ b/internal/relui/worker.go
@@ -27,6 +27,8 @@
WorkflowFinished(ctx context.Context, workflowID uuid.UUID, outputs map[string]interface{}, err error) error
}
+type stopFunc func()
+
// Worker runs workflows, and persists their state.
type Worker struct {
dh *DefinitionHolder
@@ -41,7 +43,7 @@
// running is a set of currently running Workflow ids. Run uses
// this set to prevent starting a simultaneous execution of a
// currently running Workflow.
- running map[string]struct{}
+ running map[string]stopFunc
}
// NewWorker returns a Worker ready to accept and run workflows.
@@ -52,7 +54,7 @@
l: l,
done: make(chan struct{}),
pending: make(chan *workflow.Workflow, 1),
- running: make(map[string]struct{}),
+ running: make(map[string]stopFunc),
}
}
@@ -72,13 +74,15 @@
return ctx.Err()
case wf := <-w.pending:
eg.Go(func() error {
- if err := w.markRunning(wf); err != nil {
+ runCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
+ if err := w.markRunning(wf, cancel); err != nil {
log.Println(err)
return nil
}
defer w.markStopped(wf)
- outputs, err := wf.Run(ctx, w.l)
+ outputs, err := wf.Run(runCtx, w.l)
if wfErr := w.l.WorkflowFinished(ctx, wf.ID, outputs, err); wfErr != nil {
return fmt.Errorf("w.l.WorkflowFinished(_, %q, %v, %q) = %w", wf.ID, outputs, err, wfErr)
}
@@ -88,13 +92,13 @@
}
}
-func (w *Worker) markRunning(wf *workflow.Workflow) error {
+func (w *Worker) markRunning(wf *workflow.Workflow, stop func()) error {
w.mu.Lock()
defer w.mu.Unlock()
if _, ok := w.running[wf.ID.String()]; ok {
return fmt.Errorf("workflow %q already running", wf.ID)
}
- w.running[wf.ID.String()] = struct{}{}
+ w.running[wf.ID.String()] = stop
return nil
}
@@ -104,6 +108,17 @@
delete(w.running, wf.ID.String())
}
+func (w *Worker) cancelWorkflow(id uuid.UUID) bool {
+ w.mu.Lock()
+ defer w.mu.Unlock()
+ stop, ok := w.running[id.String()]
+ if !ok {
+ return ok
+ }
+ stop()
+ return ok
+}
+
func (w *Worker) run(wf *workflow.Workflow) error {
select {
case <-w.done:
diff --git a/internal/workflow/workflow.go b/internal/workflow/workflow.go
index 3ef4da7..2b699b0 100644
--- a/internal/workflow/workflow.go
+++ b/internal/workflow/workflow.go
@@ -604,11 +604,6 @@
state := <-stateChan
w.tasks[state.def] = &state
- if state.err != nil && ctx.Err() != nil {
- // Don't report failures that occur after cancellation has begun.
- // They may be due to the cancellation itself.
- continue
- }
listener.TaskStateChanged(w.ID, state.def.name, state.toExported())
}
}
diff --git a/internal/workflow/workflow_test.go b/internal/workflow/workflow_test.go
index 7630476..f9b6f78 100644
--- a/internal/workflow/workflow_test.go
+++ b/internal/workflow/workflow_test.go
@@ -319,12 +319,13 @@
}
storage.assertState(t, w, map[string]*workflow.TaskState{
"run once": {Name: "run once", Finished: true, Result: "ran"},
- "block": {Name: "block"}, // We cancelled the workflow before it could save its state.
+ "block": {Name: "block", Finished: true, Error: "context canceled"}, // We cancelled the workflow before it could save its state.
})
block = false
wfState := &workflow.WorkflowState{ID: w.ID, Params: nil}
taskStates := storage.states[w.ID]
+ taskStates["block"] = &workflow.TaskState{Name: "block"}
w2, err := workflow.Resume(wd, wfState, taskStates)
if err != nil {
t.Fatal(err)
@@ -376,6 +377,7 @@
}
func (l *mapListener) assertState(t *testing.T, w *workflow.Workflow, want map[string]*workflow.TaskState) {
+ t.Helper()
if diff := cmp.Diff(l.states[w.ID], want, cmpopts.IgnoreFields(workflow.TaskState{}, "SerializedResult")); diff != "" {
t.Errorf("task state didn't match expections: %v", diff)
}
@@ -399,7 +401,7 @@
}
outputs, err := w.Run(ctx, listener)
if err != nil {
- t.Fatal(err)
+ t.Fatalf("w.Run() = _, %v, wanted no error", err)
}
return outputs
}
