Fix task leak during client restore when allocrunner prerun hook fails #17104
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ```release-note:bug | ||
| client: clean up resources upon failure to restore task during client restart | ||
| ``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| // Copyright (c) HashiCorp, Inc. | ||
| // SPDX-License-Identifier: MPL-2.0 | ||
|
|
||
| // FailHook is designed to fail for testing purposes, | ||
| // so should never be included in a release. | ||
| //go:build !release | ||
|
|
||
| package allocrunner | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/hashicorp/go-hclog" | ||
| "github.com/hashicorp/hcl/v2/hclsimple" | ||
|
|
||
| "github.com/hashicorp/nomad/client/allocrunner/interfaces" | ||
| ) | ||
|
|
||
| var ErrFailHookError = errors.New("failed successfully") | ||
|
|
||
| func NewFailHook(l hclog.Logger, name string) *FailHook { | ||
| return &FailHook{ | ||
| name: name, | ||
| logger: l.Named(name), | ||
| } | ||
| } | ||
|
|
||
| type FailHook struct { | ||
| name string | ||
| logger hclog.Logger | ||
| Fail struct { | ||
| Prerun bool `hcl:"prerun,optional"` | ||
| PreKill bool `hcl:"prekill,optional"` | ||
| Postrun bool `hcl:"postrun,optional"` | ||
| Destroy bool `hcl:"destroy,optional"` | ||
| Update bool `hcl:"update,optional"` | ||
| PreTaskRestart bool `hcl:"pretaskrestart,optional"` | ||
| Shutdown bool `hcl:"shutdown,optional"` | ||
| } | ||
| } | ||
|
|
||
| func (h *FailHook) Name() string { | ||
| return h.name | ||
| } | ||
|
|
||
| func (h *FailHook) LoadConfig(path string) *FailHook { | ||
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| h.logger.Error("couldn't load config", "error", err) | ||
| return h | ||
| } | ||
| if err := hclsimple.DecodeFile(path, nil, &h.Fail); err != nil { | ||
| h.logger.Error("error parsing config", "path", path, "error", err) | ||
| } | ||
| return h | ||
| } | ||
|
|
||
| var _ interfaces.RunnerPrerunHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) Prerun() error { | ||
| if h.Fail.Prerun { | ||
| return fmt.Errorf("prerun %w", ErrFailHookError) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var _ interfaces.RunnerPreKillHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) PreKill() { | ||
| if h.Fail.PreKill { | ||
| h.logger.Error("prekill", "error", ErrFailHookError) | ||
| } | ||
| } | ||
|
|
||
| var _ interfaces.RunnerPostrunHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) Postrun() error { | ||
| if h.Fail.Postrun { | ||
| return fmt.Errorf("postrun %w", ErrFailHookError) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var _ interfaces.RunnerDestroyHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) Destroy() error { | ||
| if h.Fail.Destroy { | ||
| return fmt.Errorf("destroy %w", ErrFailHookError) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var _ interfaces.RunnerUpdateHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) Update(request *interfaces.RunnerUpdateRequest) error { | ||
| if h.Fail.Update { | ||
| return fmt.Errorf("update %w", ErrFailHookError) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var _ interfaces.RunnerTaskRestartHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) PreTaskRestart() error { | ||
| if h.Fail.PreTaskRestart { | ||
| return fmt.Errorf("destroy %w", ErrFailHookError) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var _ interfaces.ShutdownHook = &FailHook{} | ||
|
|
||
| func (h *FailHook) Shutdown() { | ||
| if h.Fail.Shutdown { | ||
| h.logger.Error("shutdown", "error", ErrFailHookError) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,12 @@ import ( | |
| "time" | ||
|
|
||
| "github.com/golang/snappy" | ||
| "github.com/kr/pretty" | ||
| "github.com/shoenig/test" | ||
| "github.com/shoenig/test/must" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/hashicorp/nomad/ci" | ||
| "github.com/hashicorp/nomad/client/allocdir" | ||
| "github.com/hashicorp/nomad/client/allocrunner/interfaces" | ||
|
|
@@ -41,10 +47,6 @@ import ( | |
| "github.com/hashicorp/nomad/plugins/device" | ||
| "github.com/hashicorp/nomad/plugins/drivers" | ||
| "github.com/hashicorp/nomad/testutil" | ||
| ) | ||
|
|
||
| type MockTaskStateUpdater struct { | ||
|
|
@@ -662,6 +664,61 @@ func TestTaskRunner_Restore_System(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| // TestTaskRunner_MarkFailedKill asserts that MarkFailedKill marks the task as failed | ||
| // and cancels the killCtx so a subsequent Run() will do any necessary task cleanup. | ||
| func TestTaskRunner_MarkFailedKill(t *testing.T) { | ||
| ci.Parallel(t) | ||
|
|
||
| // set up some taskrunner | ||
| alloc := mock.MinAlloc() | ||
| task := alloc.Job.TaskGroups[0].Tasks[0] | ||
| conf, cleanup := testTaskRunnerConfig(t, alloc, task.Name) | ||
| t.Cleanup(cleanup) | ||
| tr, err := NewTaskRunner(conf) | ||
| must.NoError(t, err) | ||
|
|
||
| // side quest: set this lifecycle coordination channel, | ||
| // so early in tr MAIN, it doesn't randomly follow that route. | ||
| // test config creates this already closed, but not so in real life. | ||
| startCh := make(chan struct{}) | ||
| t.Cleanup(func() { close(startCh) }) | ||
| tr.startConditionMetCh = startCh | ||
|
|
||
| // function under test: should mark the task as failed and cancel kill context | ||
| reason := "because i said so" | ||
| tr.MarkFailedKill(reason) | ||
|
|
||
| // explicitly check kill context. | ||
| select { | ||
| case <-tr.killCtx.Done(): | ||
| default: | ||
| t.Fatal("kill context should be done") | ||
| } | ||
|
|
||
| // Run() should now follow the kill path. | ||
| go tr.Run() | ||
|
|
||
| select { // it should finish up very quickly | ||
| case <-tr.WaitCh(): | ||
| case <-time.After(time.Second): | ||
| t.Error("task not killed (or not as fast as expected)") | ||
| } | ||
|
|
||
| // check state for expected values and events | ||
| state := tr.TaskState() | ||
|
|
||
| // this gets set directly by MarkFailedKill() | ||
| test.True(t, state.Failed, test.Sprint("task should have failed")) | ||
| // this is set in Run() | ||
| test.Eq(t, structs.TaskStateDead, state.State, test.Sprint("task should be dead")) | ||
| // reason "because i said so" should be a task event message | ||
| foundMessages := make(map[string]bool) | ||
| for _, event := range state.Events { | ||
| foundMessages[event.DisplayMessage] = true | ||
| } | ||
| test.True(t, foundMessages[reason], test.Sprintf("expected '%s' in events: %#v", reason, foundMessages)) | ||
|
Comment on lines
+715
to
+719
There was a problem hiding this comment. "Map contains value that meets this condition" seems like it'd be nice new frequently-used assertion for @shoenig's test library. But out of scope for this PR. |
||
| } | ||
|
|
||
| // TestTaskRunner_TaskEnv_Interpolated asserts driver configurations are | ||
| // interpolated. | ||
| func TestTaskRunner_TaskEnv_Interpolated(t *testing.T) { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆