-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Introduce separate channel for trigger workloads #48835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
edb0d73
da6c247
665b665
b151939
970f8a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -385,6 +385,16 @@ class WatchedSubprocess: | |
| _requests_fd: int | ||
| """File descriptor for request handling.""" | ||
|
|
||
| """" | ||
| It require special case to handle the workloads and api calls to api-server in Triggerer, due to mixing up messages | ||
| a separate channel is used to send the workloads from parent process to child process the child process. | ||
| connect_stdin will use this channel to read the workloads in read_workload method. | ||
| trigger_stdin: trigger parent process uses this to send the workloads to child process. | ||
| _trigger_requests_fd: trigger child process uses this to read the workloads. | ||
| """ | ||
| _trigger_requests_fd: int | None = None | ||
| trigger_stdin: BinaryIO | None = None | ||
|
|
||
| _num_open_sockets: int = 4 | ||
| _exit_code: int | None = attrs.field(default=None, init=False) | ||
|
|
||
|
|
@@ -413,11 +423,19 @@ def start( | |
| child_comms, read_msgs = mkpipe() | ||
| child_logs, read_logs = mkpipe() | ||
|
|
||
| if "TriggerRunnerSupervisor" in cls.__name__: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Open these sockets only when in triggerer, for others we dont need it. |
||
| trigger_child_stdin, trigger_feed_stdin = mkpipe(remote_read=True) | ||
| else: | ||
| trigger_child_stdin, trigger_feed_stdin = None, None | ||
|
|
||
| pid = os.fork() | ||
| if pid == 0: | ||
| # Close and delete of the parent end of the sockets. | ||
| cls._close_unused_sockets(feed_stdin, read_stdout, read_stderr, read_msgs, read_logs) | ||
|
|
||
| if trigger_feed_stdin: | ||
| cls._close_unused_sockets(trigger_feed_stdin) | ||
|
|
||
| # Python GC should delete these for us, but lets make double sure that we don't keep anything | ||
| # around in the forked processes, especially things that might involve open files or sockets! | ||
| del constructor_kwargs | ||
|
|
@@ -439,6 +457,15 @@ def start( | |
|
|
||
| requests_fd = child_comms.fileno() | ||
|
|
||
| if "TriggerRunnerSupervisor" in cls.__name__: | ||
| additional_trigger_fds = { | ||
| "trigger_stdin": trigger_feed_stdin, | ||
| "trigger_requests_fd": trigger_child_stdin.fileno(), # type: ignore[union-attr] | ||
| } | ||
| cls._close_unused_sockets(trigger_child_stdin) | ||
| else: | ||
| additional_trigger_fds = {} | ||
|
|
||
| # Close the remaining parent-end of the sockets we've passed to the child via fork. We still have the | ||
| # other end of the pair open | ||
| cls._close_unused_sockets(child_stdin, child_stdout, child_stderr, child_comms, child_logs) | ||
|
|
@@ -450,6 +477,7 @@ def start( | |
| process=psutil.Process(pid), | ||
| requests_fd=requests_fd, | ||
| process_log=logger, | ||
| **additional_trigger_fds, # type: ignore[arg-type] | ||
| **constructor_kwargs, | ||
| ) | ||
|
|
||
|
|
@@ -668,6 +696,8 @@ def _check_subprocess_exit(self, raise_on_timeout: bool = False) -> int | None: | |
| self._exit_code = self._process.wait(timeout=0) | ||
| log.debug("%s process exited", type(self).__name__, exit_code=self._exit_code) | ||
| self._close_unused_sockets(self.stdin) | ||
| if self.trigger_stdin: | ||
| self._close_unused_sockets(self.trigger_stdin) | ||
| except psutil.TimeoutExpired: | ||
| if raise_on_timeout: | ||
| raise | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -540,6 +540,13 @@ class CommsDecoder(Generic[ReceiveMsgType, SendMsgType]): | |
|
|
||
| input: TextIO | ||
|
|
||
| """" | ||
| It require special case to handle the workloads and api calls to api-server, due to mixing up messages | ||
| a separate channel is used to send the workloads from parent process to child process the child process. | ||
| connect_stdin will use this channel to read the workloads in read_workload method. | ||
| """ | ||
|
Comment on lines
+543
to
+547
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember correctly those should field docstring should go bellow the attr they describe, not above. |
||
| trigger_input: TextIO = attrs.field(init=False, default=None) | ||
|
|
||
| request_socket: FileIO = attrs.field(init=False, default=None) | ||
|
|
||
| # We could be "clever" here and set the default to this based type parameters and a custom | ||
|
|
||
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.
That type ignore seems of. It wasn't necessary before and it seems really similar now: