Port nailgun client to rust #10865

gshuflin · 2020-09-25T21:23:08Z

This commit ports the nailgun client used in remote_pants_runner.py from Python to Rust, using the nails crate. This is in preparation for using the Rust nailgun library's heartbeat functionality to replace the current UNIX signal-based mechanism for the client to inform pantsd that a given pants run has been stopped, although this commit does not yet remove that signal-based code.

Removing the Python version of the client allows a lot of Python code around the client side of the nailgun protocol to be removed, and also results in the removal of the logic for retrying connecting to the server on failure. Since a lot of this code is old, and predates the current design of pantsd, I think it makes sense to remove this code now, and add back any retry logic we see as still necessary in future commits.

coveralls · 2020-09-25T22:17:26Z

Coverage remained the same at 0.0% when pulling 2f98874 on gshuflin:heartbeat into 6eea7c4 on pantsbuild:master.

jsirois · 2020-10-02T17:06:36Z

src/python/pants/base/exception_sink.py

        self_process = psutil.Process()
        children = self_process.children()
-        logger.debug(f"Sending SIGINT to child processes: {children}")
+        logger.warning(f"Sending SIGINT to child processes: {children}")


It would be good to replace SIGINT with the _received_signal or signal.strsignal(_received_signal) or signal.getsignal(_received_signal).

signal.strsignal apparently only exists after python 3.8

jsirois · 2020-10-02T17:07:07Z

src/python/pants/base/exception_sink.py

        self_process = psutil.Process()
        children = self_process.children()
-        logger.debug(f"Sending SIGINT to child processes: {children}")
+        logger.warning(f"Sending SIGINT to child processes: {children}")
        for child_process in children:
            child_process.send_signal(signal.SIGINT)


s/signal.SIGINT/_received_signal/

jsirois · 2020-10-02T17:09:11Z

src/python/pants/bin/remote_pants_runner.py

-    def __init__(self, pailgun_client: NailgunClient, pid: int, timeout: float = 1):
-        self._pailgun_client = pailgun_client
-        self._timeout = timeout
+    def __init__(self, pid: int):
        self.pid = pid
        super().__init__(pantsd_instance=False)

    def _forward_signal_with_timeout(self, signum, signame):


It would be good to rename this helper to _forward_signal since it no longer deals with timeouts.

jsirois · 2020-10-02T17:09:50Z

src/python/pants/bin/remote_pants_runner.py

-    def _backoff(attempt):
-        """Minimal backoff strategy for daemon restarts."""
-        time.sleep(attempt + (attempt - 1))
-
    def run(self) -> ExitCode:
        """Runs pants remotely with retry and recovery for nascent executions."""


Maybe lop off the second half of the doc sentence.

jsirois · 2020-10-02T17:11:47Z

src/python/pants/bin/remote_pants_runner.py

+        pantsd_signal_handler = PailgunClientSignalHandler(pid=pid)
+
+        with ExceptionSink.trapped_signals(pantsd_signal_handler), STTYSettings.preserved():
+            return cast(int, rust_nailgun_client.execute(signal_fn, command, args, modified_env))


It would be good to stick to the alias and cast to ExitCode.

stuhood

Thanks a lot @gshuflin !

stuhood · 2020-10-02T17:24:32Z

src/rust/engine/nailgun/src/lib.rs

@@ -27,6 +27,8 @@
 // Arc<Mutex> can be more clear than needing to grok Orderings:
 #![allow(clippy::mutex_atomic)]
 #![type_length_limit = "2058438"]
+#![allow(dead_code)]
+#![allow(unused_variables)]



This file is already pretty dense: it would be good to split it into client.rs and server.rs probably, loaded by a mostly empty lib.rs.

stuhood · 2020-10-02T17:25:26Z

src/rust/engine/nailgun/src/lib.rs

+#![allow(dead_code)]
+#![allow(unused_variables)]


These should be removed in a few different files where they were added.

stuhood · 2020-10-02T17:28:15Z

src/python/pants/base/exception_sink.py

        for child_process in children:
            child_process.send_signal(signal.SIGINT)

+    def handle_sigint(self, signum: int, _frame):
+        logger.warning("Calling handle_sigint in pantsd")


Any process that loads the ExceptionSink will end up with this handler installed, so this message might not always be accurate.

stuhood · 2020-10-02T17:29:07Z

src/python/pants/bin/remote_pants_runner.py

-            reason=KeyboardInterrupt("Sending user interrupt to pantsd"),
-        )
-        self._pailgun_client.maybe_send_signal(signum)
+        ExceptionSink._signal_sent = signum


Rather than reaching out to do this explicitly, should this handler be calling super() or something?

No, I think it's a mistake for PailgunClientSignalHandler and SignalHandler to have a subclass-superclass relationship actually. If we were going to keep this code long term I would probably want to rewrite SignalHandler as an "interface"-like superclass and have the remote and non-remote cases subclass it separately.

Ok. You'll probably want to expose some other API to set the signal then, rather than accessing the private field.

I would also recommend adding a comment docstring to ExceptionSink._signal_sent, as we appear to be depending on it here, and it appears to be the only class-level field that does not have a comment to document it.

stuhood · 2020-10-02T17:32:48Z

src/python/pants/option/global_options.py

+            deprecation_start_version="",
+            help='["DEPRECATED: the pailgun client has been rewritten to no longer use this"].'
+            "The length of time (in seconds) to wait for further output after sending a "
            "signal to the remote pantsd process before killing it.",


Suggested change

deprecation_start_version="",

help='["DEPRECATED: the pailgun client has been rewritten to no longer use this"].'

"The length of time (in seconds) to wait for further output after sending a "

"signal to the remote pantsd process before killing it.",

removal_version="2.1.0.dev0",

removal_hint="the pailgun client has been rewritten to no longer use this",

help="The length of time (in seconds) to wait for further output after sending a "

"signal to the remote pantsd process before killing it.",

stuhood · 2020-10-02T17:52:19Z

src/rust/engine/nailgun/src/lib.rs

+    Either::Right((exited, execution_result_future)) => {
+      Err("Exiting nailgun client future via explicit quit message".to_string())


So, this is handling the case where ExceptionSink._signal_sent ends up set?

This is an interesting situation, and rolls back to what we had discussed about replacing the weird python nailgun "timeout" thing with "send SIGINT to the server the first time someone ctrl+c's and SIGTERM the second time". I think that the exact mechanism that might be good here is:

when the client receives the first SIGINT, it should send SIGINT to the server, but not kill itself... this will allow it to continue to receive data as the server shuts down (and in a heartbeat future, for it to drain the socket after the server shuts down cleanly when heartbeats stop arriving)

when the client receives the second SIGINT, it should SIGTERM to the server, and then set this exit condition (and in a heartbeat future, only kill itself without waiting for the server to notice, and rely on the client going away causing the server to tear down).

I think this makes sense, but I'd rather implement it as part of the commit that implements heartbeats, in the interest of getting this (now rather lengthy) PR merged.

stuhood · 2020-10-02T17:53:35Z

src/rust/engine/src/externs/interface.rs

@@ -33,6 +33,8 @@
  clippy::transmute_ptr_to_ptr,
  clippy::zero_ptr
 )]
+#![allow(dead_code)]
+#![allow(unused_variables)]


Ditto, here and elsewhere.

stuhood · 2020-10-02T17:56:29Z

src/rust/engine/src/externs/interface.rs

+  data executor: PyExecutor;
+  data port: u16;
+
+  def execute(&self, py_signal_fn: PyObject, command: PyString, args: PyList, env: PyDict) -> CPyResult<PyInt> {


The cpython crate can do more complicated argument transforms (via FromPyObject: note the implementations), so I think that this signature can be at least:

Suggested change

def execute(&self, py_signal_fn: PyObject, command: PyString, args: PyList, env: PyDict) -> CPyResult<PyInt> {

def execute(&self, py_signal_fn: PyObject, command: String, args: Vec<String>, env: PyDict) -> CPyResult<PyInt> {

Ok: then regarding my comment here: #10865 (comment), it sounds like the massaging into rust types is already done, and you can probably ignore that comment safely.

stuhood · 2020-10-02T17:58:40Z

src/rust/engine/src/externs/interface.rs

+        let output = loop {
+          let event = receiver.recv_timeout(timeout);
+          if let Some(_termination) = maybe_break_execution_loop(&python_signal_fn) {
+            break Err("Quitting becuase of explicit interrupt".to_string());


s/becuase/because/

stuhood · 2020-10-02T18:03:32Z

src/rust/engine/src/externs/interface.rs

+        let _spawned_fut = executor.spawn(async move {
+          let exit_code = nailgun_fut.await;
+          let _ = sender.send(exit_code);
+        });


You should not need to spawn a task. If you got an error like "not running on the tokio runtime", you should "enter" the runtime (which sets some thread-local state that tokio uses to find the runtime):

let result = executor.enter(|| { // ... do some stuff on the runtime })

But also, using executor.block_on should enter the runtime... so I think that you might be able to replace most of this function body with:

let exit_code = executor.block_on(nailgun_fut);

cosmicexplorer

I had a few comments asking about how we know that some futures won't be leaked, and a few asking for a TODO to wrap up the tty/no tty complexity into an enum. Feel free to answer or not, and to add TODOs or not, at your own discretion! ^_^

I love this work, it looks great, and the above comments were mostly trying to make it easier for me or others to work with and extend it once we have our great rust client! None of them should be considered a blocking change.

cosmicexplorer · 2020-10-03T13:08:04Z

src/python/pants/base/exception_sink.py

@@ -59,14 +59,16 @@ def _toggle_ignoring_sigint(self, toggle: bool) -> None:
            with self._ignore_sigint_lock:
                self._ignoring_sigint = toggle

-    def handle_sigint(self, signum: int, _frame):
+    def _send_signal_to_children(self, received_signal: int) -> None:


Could we have a tiny docstring here? Something like:

def _send_signal_to_children(self, received_signal: int) -> None: """Send a signal to any children of this process in order. Pants may have spawned 0, 1, or multiple subprocesses via Python or Rust. Upon receiving a signal, this method is invoked to propagate the signal to all children, regardless of how they were spawned. """

Suggested change

def _send_signal_to_children(self, received_signal: int) -> None:

def _send_signal_to_children(self, signum: int, signame: str) -> None:

This would allow us to include the direct signal name in the debug message, instead of having to look it up.

cosmicexplorer · 2020-10-03T13:09:48Z

src/python/pants/option/global_options.py

@@ -635,6 +635,8 @@ def register_bootstrap_options(cls, register):
            advanced=True,
            type=float,
            default=5.0,
+            removal_version="2.1.0.dev0",
+            removal_hint="The pailgun client has been rewritten to no longer use this",


Suggested change

removal_hint="The pailgun client has been rewritten to no longer use this",

removal_hint="The pailgun client now uses a more robust API which avoids the need for any timeout logic.",

Looks like we could also remove the TODO(#7514)?

I think I'll have to reword this again since cf. stu's above comment we do need some kind of timeout logic after all.

cosmicexplorer · 2020-10-03T13:12:45Z

src/python/pants/base/exception_sink.py

        self_process = psutil.Process()
        children = self_process.children()
-        logger.debug(f"Sending SIGINT to child processes: {children}")
+        logger.debug(f"Sending signal number {received_signal} to child processes: {children}")


Suggested change

logger.debug(f"Sending signal number {received_signal} to child processes: {children}")

logger.debug(f"Sending signal {signame} ({signum}) to child processes: {children}")

cosmicexplorer · 2020-10-03T13:53:00Z

src/python/pants/base/exception_sink.py

        ExceptionSink._signal_sent = signum
+        self._send_signal_to_children(signum)


Suggested change

self._send_signal_to_children(signum)

self._send_signal_to_children(signum, 'SIGINT')

For example.

cosmicexplorer · 2020-10-03T13:55:00Z

src/python/pants/bin/remote_pants_runner.py

-            reason=KeyboardInterrupt("Sending user interrupt to pantsd"),
-        )
-        self._pailgun_client.maybe_send_signal(signum)
+        ExceptionSink._signal_sent = signum


I would also recommend adding a comment docstring to ExceptionSink._signal_sent, as we appear to be depending on it here, and it appears to be the only class-level field that does not have a comment to document it.

cosmicexplorer · 2020-10-03T14:28:20Z

src/rust/engine/nailgun/src/server.rs

+  }
+
+  ///
+  /// Corresponds to `ttynames_to_env` in `nailgun_protocol.py`. See this struct's rustdocs.


Does "corresponds" mean that it reads the env vars which are set in that method?

No idea, I'm just moving this code around. I'd prefer to keep any proposed changes to server.rs out of scope for this PR, since this is just moving code that used to exist in lib.rs into a separate file.

cosmicexplorer · 2020-10-03T14:33:50Z

src/rust/engine/src/externs/interface.rs

+          }
+        };
+        debug!("Sending message to nailgun client task to exit.");
+        match exit_sender.send(()) {


It might be useful to add a TODO which wraps up the exit sending here. From looking at the code, it's not immediately clear to me what exit_sender.send()) does compared to seeing sender.send(exit_code) above. Maybe just comments or renaming these variables might be helpful, but it's confusing to see the exit_sender not sending the exit_code, for example.

cosmicexplorer · 2020-10-03T14:34:12Z

src/rust/engine/src/externs/interface.rs

@@ -715,7 +810,7 @@ fn nailgun_server_await_shutdown(
  executor_ptr: PyExecutor,
  nailgun_server_ptr: PyNailgunServer,
 ) -> PyUnitResult {
-  with_executor(py, executor_ptr, |executor| {
+  with_executor(py, &executor_ptr, |executor| {


Yes! Thank you for looking into this!

cosmicexplorer · 2020-10-03T14:34:43Z

tests/python/pants_test/logging/native_engine_logging_integration_test.py

@@ -29,7 +29,7 @@ def test_pantsd_file_logging(self) -> None:
                ["--backend-packages=pants.backend.python", "list", "3rdparty::"]
            )
            ctx.checker.assert_started()
-            assert "[DEBUG] connecting to pantsd on port" in daemon_run.stderr_data
+            assert "[DEBUG] Connecting to pantsd on port" in daemon_run.stderr


Good change! Thanks!

cosmicexplorer · 2020-10-03T14:41:03Z

src/rust/engine/src/externs/interface.rs

+  data executor: PyExecutor;
+  data port: u16;
+
+  def execute(&self, py_signal_fn: PyObject, command: String, args: Vec<String>, env: PyDict) -> CPyResult<PyInt> {


Consider moving the body of this method into a private function just above, e.g.

fn _execute_nailgun_client(client: &PyNailgunClient, py_signal_fn: Value, command: String, args: Vec<String>, env: Vec<String, String>) -> i64 { ... }

The benefit of that is that it would decouple the "massage cpython objects into rust" part from the nailgun execution. Since this is within a macro definition, it seems like there is benefit to reducing the amount of logic defined inline. Totally subjective.

This is probably fine, see Stu's comment on argument transforms from cpython here: #10865 (comment).

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-build-wheels]

This reverts commit ee50791. [ci skip-build-wheels]

Commit ee50791, which ported the pantsd client nailgun functionality from python to rust, has a bug in it that makes inputting text in `./pants repl` unusably laggy. This commit should be reverted until a solution for that bug is found.

…sbuild#10929)" This reverts commit dbf744f. [ci skip-build-wheels]

### Problem #10865 previously landed to port Pants' nailgun client to Rust. It was reverted in #10929 due to an issue with TTY access, where (in particular), the `repl` goal was mostly unresponsive. ### Solution The unresponsive `repl` was due to a bug in the `nails` library, where `stdin` was being consumed eagerly regardless of whether the server signaled that it would like to receive `stdin`. Pants sends an environment variable to the server that indicates which TTY the client is connected to, and the server will directly connect to that TTY if it can. When the server directly connects to the client's TTY, it does not accept `stdin`, but since `stdin` was read eagerly by the `nails` client (and ending up stuck in a buffer, since the server would not request it), the result was two different processes reading `stdin` from the TTY: the client, and the server. Bump to `nails` `0.7.0`, which [makes `stdin` initialization lazy](stuhood/nails@6b8c19a).

gshuflin force-pushed the heartbeat branch from 1e0ba3c to ef46758 Compare September 27, 2020 05:57

gshuflin changed the title ~~WIP initial rust-py types~~ WIP Port nailgun client to rust Sep 27, 2020

gshuflin force-pushed the heartbeat branch 6 times, most recently from 398d719 to 3a77186 Compare October 2, 2020 01:20

gshuflin requested review from stuhood, jsirois and Eric-Arellano October 2, 2020 01:25

gshuflin changed the title ~~WIP Port nailgun client to rust~~ Port nailgun client to rust Oct 2, 2020

gshuflin marked this pull request as ready for review October 2, 2020 01:26

jsirois reviewed Oct 2, 2020

View reviewed changes

stuhood reviewed Oct 2, 2020

View reviewed changes

cosmicexplorer approved these changes Oct 3, 2020

View reviewed changes

gshuflin force-pushed the heartbeat branch 4 times, most recently from 5a639ca to d777079 Compare October 7, 2020 06:49

gshuflin added 9 commits October 7, 2020 11:48

Add nails crate

a4f916f

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Port nailgun client to rust

39a772d

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Remove python nailgun client code

0c8d446

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Fix test

a45cb8f

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

PR feedback

b952952

[ci skip-build-wheels]

break nailgun crate into multiple files

5f5080c

[ci skip-build-wheels]

Use current working directory

a9967b2

[ci skip-build-wheels]

better docstrings

b7f4ef2

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Better variable name

b727836

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

gshuflin added 4 commits October 7, 2020 11:48

Restore retrying logic

5bb9bdd

[ci skip-build-wheels]

Add comment for _signal_sent

e86a946

[ci skip-build-wheels]

Fix lockfile

346f0bf

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Fix types

2f98874

[ci skip-build-wheels]

gshuflin force-pushed the heartbeat branch from 82942bc to 2f98874 Compare October 7, 2020 18:48

gshuflin merged commit ee50791 into pantsbuild:master Oct 7, 2020

gshuflin deleted the heartbeat branch October 7, 2020 19:27

gshuflin added a commit to gshuflin/pants that referenced this pull request Oct 8, 2020

Revert "Port nailgun client to rust (pantsbuild#10865)"

1427b45

This reverts commit ee50791. [ci skip-build-wheels]

Eric-Arellano mentioned this pull request Oct 11, 2020

Prepare 2.0.0rc0 #10943

Merged

stuhood mentioned this pull request Nov 11, 2020

Re-enable concurrent runs for pantsd in v2 #7654

Open

stuhood added a commit to stuhood/pants that referenced this pull request Nov 11, 2020

Revert "Revert "Port nailgun client to rust (pantsbuild#10865)" (pant…

910f518

…sbuild#10929)" This reverts commit dbf744f. [ci skip-build-wheels]

stuhood mentioned this pull request Nov 12, 2020

Re-land the port of Pants' nailgun client to Rust #11147

Merged

stuhood mentioned this pull request Nov 25, 2020

Rust client for pantsd #10061

Closed

		Either::Right((exited, execution_result_future)) => {
		Err("Exiting nailgun client future via explicit quit message".to_string())

	def execute(&self, py_signal_fn: PyObject, command: PyString, args: PyList, env: PyDict) -> CPyResult<PyInt> {
	def execute(&self, py_signal_fn: PyObject, command: String, args: Vec<String>, env: PyDict) -> CPyResult<PyInt> {

	def _send_signal_to_children(self, received_signal: int) -> None:
	def _send_signal_to_children(self, signum: int, signame: str) -> None:

	removal_hint="The pailgun client has been rewritten to no longer use this",
	removal_hint="The pailgun client now uses a more robust API which avoids the need for any timeout logic.",

	logger.debug(f"Sending signal number {received_signal} to child processes: {children}")
	logger.debug(f"Sending signal {signame} ({signum}) to child processes: {children}")

		ExceptionSink._signal_sent = signum
		self._send_signal_to_children(signum)

	self._send_signal_to_children(signum)
	self._send_signal_to_children(signum, 'SIGINT')

		#![allow(dead_code)]
		#![allow(unused_variables)]

Port nailgun client to rust #10865

Port nailgun client to rust #10865

Conversation

gshuflin commented Sep 25, 2020 • edited Loading

coveralls commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Sep 25, 2020 •

edited

Loading

coveralls commented Sep 25, 2020 •

edited

Loading