Skip to content

Commit

Permalink
template: lock task handle before trying script check (#23917)
Browse files Browse the repository at this point in the history
In #23663 we fixed the template hook so that `change_mode="script"` didn't lose
track of the task handle during restores. But this revealed a second bug which
is that access to the handle is not locked while in use, which can allow it to
be removed concurrently.

Fixes: #23875
  • Loading branch information
tgross authored Sep 12, 2024
1 parent 4ade277 commit 07aca67
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 0 deletions.
2 changes: 2 additions & 0 deletions client/allocrunner/taskrunner/template/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,8 @@ func (tm *TaskTemplateManager) handleScriptError(script *structs.ChangeScript, m
func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *sync.WaitGroup) {
defer wg.Done()

tm.handleLock.Lock()
defer tm.handleLock.Unlock()
if tm.handle == nil {
failureMsg := fmt.Sprintf(
"Template failed to run script %v with arguments %v because task driver handle is not available",
Expand Down
24 changes: 24 additions & 0 deletions client/allocrunner/taskrunner/template/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,7 @@ func TestTaskTemplateManager_ScriptExecution(t *testing.T) {
key2 := "bar"
content1_1 := "cat"
content1_2 := "dog"
content1_3 := "goldfish"
t1 := &structs.Template{
EmbeddedTmpl: `
FOO={{key "bam"}}
Expand Down Expand Up @@ -1219,6 +1220,29 @@ OUTER:
t.Fatal(t, "should have received an event")
}
}

// remove the reference to the task handle and update the template contents
// again
harness.manager.SetDriverHandle(nil)
harness.consul.SetKV(t, key1, []byte(content1_3))
timeout = time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second)

OUTER2:
for {
select {
case <-harness.mockHooks.RestartCh:
t.Fatal(t, "restart not expected")
case ev := <-harness.mockHooks.EmitEventCh:
if strings.Contains(
ev.DisplayMessage, "task driver handle is not available") {
break OUTER2
}
case <-harness.mockHooks.SignalCh:
t.Fatal(t, "signal not expected")
case <-timeout:
t.Fatal(t, "should have received an event that task driver handle is unavailable")
}
}
}

// TestTaskTemplateManager_ScriptExecutionFailTask tests whether we fail the
Expand Down

0 comments on commit 07aca67

Please sign in to comment.