Skip to content

drivers: set -1 exit code in case executor gets killed#25453

Merged
pkazmierczak merged 7 commits intomainfrom
b-exit-code-on-executor-failure
Mar 20, 2025
Merged

drivers: set -1 exit code in case executor gets killed#25453
pkazmierczak merged 7 commits intomainfrom
b-exit-code-on-executor-failure

Conversation

@pkazmierczak
Copy link
Copy Markdown
Contributor

@pkazmierczak pkazmierczak commented Mar 19, 2025

Nomad driver handles incorrectly set exit code 0 in case of executor failure. This corrects that behavior.

Fixes #17782
Internal ref: https://hashicorp.atlassian.net/browse/NET-12155

@pkazmierczak pkazmierczak requested review from jrasell and tgross March 20, 2025 10:47
@pkazmierczak pkazmierczak marked this pull request as ready for review March 20, 2025 10:47
@pkazmierczak pkazmierczak requested review from a team as code owners March 20, 2025 10:47
@pkazmierczak pkazmierczak added backport/ent/1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent labels Mar 20, 2025
Copy link
Copy Markdown
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we're seeing a build failure on test-windows in the rawexec driver package. It's not obvious to me why and the test run doesn't give more information than that.

Comment thread .changelog/25453.txt Outdated
Co-authored-by: Tim Gross <tgross@hashicorp.com>
@pkazmierczak
Copy link
Copy Markdown
Contributor Author

pkazmierczak commented Mar 20, 2025

It's not obvious to me why and the test run doesn't give more information than that.

Took me a while to figure it out, too. But it's an easy one:

undefined: syscall.Kill [windows,amd64]

Copy link
Copy Markdown
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +620 to +623
// Kill the executor and wait until it's gone
pid := driverState.ReattachConfig.Pid
must.NoError(t, err)
must.NoError(t, syscall.Kill(pid, syscall.SIGKILL))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use os.FindProcess and then (*Process) Kill to do this in a cross-platform way, but it's probably easier to just keep this unix-specific.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are other reasons we skip this test on non-unix systems, but yes. Will keep that in mind next time.

@pkazmierczak pkazmierczak merged commit cb8f4ea into main Mar 20, 2025
30 checks passed
@pkazmierczak pkazmierczak deleted the b-exit-code-on-executor-failure branch March 20, 2025 14:06
@github-actions
Copy link
Copy Markdown

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exit code reported by Nomad task events is not always truthful.

2 participants