B 2117 windows group process #4153

ninoles · 2018-04-14T18:10:11Z

This PR is a best attempt to resolve #2117. Windows processes never cleanly map to Posix semantic so there is a couple of issues here:

We currently supposed the process is a console process. This will allow for the more common case of subprocess handling which are subprocesses launched from a shell script.
Other signals like Ctrl-C, Ctrl-Close, Ctrl-Terminate aren't handled. It would required additional config support and testing for correctly mapping them. As a workaround, it is possible to handle such case by creating a launcher that handle the Ctrl-break and kill its children with the appropriate event.
I do my best to limit the changes to what's necessary to make it run and build. The Syslog handling function are however duplicate between the executor_unix.go and executor_windows.go files. I'm not sure why those functions were in executor_unix.go initially (and why executor_unix.go has so many build targets), but may be they can go back in executor.go.

- Set CREATE_NEW_PROCESS_GROUP for Windows subprocess. - Ensure we only kill actual process that need to.

schmichael

This looks great! Is there a job file you've been using to test this? I'd love to give it a spin myself before merging.

Thanks for the PR!

schmichael · 2018-04-16T17:42:05Z

CHANGELOG.md

@@ -3,6 +3,9 @@
 IMPROVEMENTS:
 * client: Create new process group on process startup. [[GH-3572](https://github.com/hashicorp/nomad/issues/3572)]

+BUG FIXES:
+ * driver/exec: Create process group for Windows process and send Ctrl-Break signal on Shutdown [[GH-2117](https://github.com/hashicorp/nomad/issues/2117)]


We use the pull request number instead of the issue number for these links. The issues in the URL is just a typo we haven't fixed in our make changelogfmt script because Github is nice enough to redirect it anyway. :)

schmichael · 2018-04-16T17:59:13Z

client/driver/executor/executor_windows.go

+
+// configure new process group for child process
+func (e *UniversalExecutor) setNewProcessGroup() error {
+	// We need to check that as build flags includes windows for this file


Remove this comment

schmichael · 2018-04-16T18:05:24Z

client/driver/executor/executor_windows.go

+	"github.com/hashicorp/nomad/client/driver/logging"
+)
+
+func (e *UniversalExecutor) LaunchSyslogServer() (*SyslogServerState, error) {


Like you pointed out in the issue description I don't think there was any reason for this to be in the executor_unix.go file instead of executor.go proper. Would you mind moving it there to avoid duplication?

dadgar · 2018-04-17T00:10:03Z

client/driver/executor/executor_unix.go

+			return err
+		}
+		return nil
+	} else {


Remove else statement and just fall through to proc.Kill()

dadgar · 2018-04-17T00:10:39Z

client/driver/executor/executor_unix.go

+	}
+}
+
+func (e *UniversalExecutor) shutdownProcess(proc *os.Process) error {


Comment that this sends the shutdown signal and doesn't necessarily kill the process

dadgar · 2018-04-17T00:11:03Z

client/driver/executor/executor_windows.go

@@ -0,0 +1,111 @@
+// +build windows


Unnecessary since the file has _windows.go

schmichael · 2018-04-17T16:20:45Z

Since #3572 broke the Windows build and fixing it would conflict with this PR, I'm going to merge this as-is and fixup the comments in a followup PR.

Thanks for the contribution @ninoles!

* `driver/raw_exec` instead of `exec` because `exec` driver isn't supported on Windows * Link to PR not issue

github-actions · 2023-03-06T02:18:09Z

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.

ninoles added 2 commits April 14, 2018 13:58

- Clean up for windows compilation.

7023b63

- Set CREATE_NEW_PROCESS_GROUP for Windows subprocess. - Ensure we only kill actual process that need to.

Add changelog.

3260fc8

schmichael self-assigned this Apr 16, 2018

schmichael reviewed Apr 16, 2018

View reviewed changes

dadgar requested changes Apr 17, 2018

View reviewed changes

schmichael merged commit 081ef6b into hashicorp:master Apr 17, 2018

ninoles mentioned this pull request Apr 17, 2018

B 2117 windows group process #4168

Merged

schmichael added a commit that referenced this pull request Apr 17, 2018

docs: fixup changelog for #4153

7f7f86d

* `driver/raw_exec` instead of `exec` because `exec` driver isn't supported on Windows * Link to PR not issue

schmichael mentioned this pull request Apr 17, 2018

Minor Windows executor cleanups #4169

Closed

schmichael added a commit that referenced this pull request Apr 18, 2018

docs: fixup changelog for #4153

0014159

* `driver/raw_exec` instead of `exec` because `exec` driver isn't supported on Windows * Link to PR not issue

schmichael mentioned this pull request Jun 8, 2018

executor: fix Windows blocking on pipe close #4400

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

B 2117 windows group process #4153

B 2117 windows group process #4153

ninoles commented Apr 14, 2018 •

edited

Loading

schmichael left a comment

schmichael Apr 16, 2018

schmichael Apr 16, 2018

schmichael Apr 16, 2018

dadgar Apr 17, 2018

dadgar Apr 17, 2018

dadgar Apr 17, 2018

schmichael commented Apr 17, 2018

github-actions bot commented Mar 6, 2023

B 2117 windows group process #4153

B 2117 windows group process #4153

Conversation

ninoles commented Apr 14, 2018 • edited Loading

schmichael left a comment

Choose a reason for hiding this comment

schmichael Apr 16, 2018

Choose a reason for hiding this comment

schmichael Apr 16, 2018

Choose a reason for hiding this comment

schmichael Apr 16, 2018

Choose a reason for hiding this comment

dadgar Apr 17, 2018

Choose a reason for hiding this comment

dadgar Apr 17, 2018

Choose a reason for hiding this comment

dadgar Apr 17, 2018

Choose a reason for hiding this comment

schmichael commented Apr 17, 2018

github-actions bot commented Mar 6, 2023

ninoles commented Apr 14, 2018 •

edited

Loading