Create LocalProcessLauncher and ExceptionHandlingLauncher #164
Conversation
Signed-off-by: Chase Engelbrecht <[email protected]>
Signed-off-by: Chase Engelbrecht <[email protected]>
Signed-off-by: Chase Engelbrecht <[email protected]>
Signed-off-by: Chase Engelbrecht <[email protected]>
| def __init__(self, pci, shell_executor, metrics_store, clock=time.Clock): | ||
| super().__init__(shell_executor) | ||
| self.logger = logging.getLogger(__name__) | ||
| self.pci = pci |
There was a problem hiding this comment.
Is there a reason why you chose pci over provision_config_instance? I think the latter is more descriptive
There was a problem hiding this comment.
I've gone back and forth on this in my mind a couple times. I settled on pci for brevity since many of the accesses to this object will be something like self.pci.variables["abcd"]["efgh"].
But ultimately clarity > brevity so I will refactor. I have used pci in a few of the other PRs as well so I will create a followup PR after this one to replace all of the references in 1 go
| if not env.get("OPENSEARCH_JAVA_OPTS"): | ||
| env["OPENSEARCH_JAVA_OPTS"] = "-XX:+ExitOnOutOfMemoryError" | ||
|
|
||
| # we just blindly trust telemetry here... |
There was a problem hiding this comment.
Is this a note or will this be a TODO to refactor this portion so that we don't blindly trust telemetry in the future?
There was a problem hiding this comment.
This is an inherited comment. I don't think it's a TODO so much as calling out that the telemetry devices can set environment variables that may break the launching of OpenSearch. Essentially loosely passing the burden of validating the env vars that a telemetry device specifies to the telemetry device itself.
A better solution would probably be an allowlist, but I don't have enough familiarity with the scope of telemetry environment variables to propose one.
|
|
||
| def _get_pid_from_file(self, pid_file_name): | ||
| with open(pid_file_name, "rb") as f: | ||
| buf = f.read() |
There was a problem hiding this comment.
Nit: what does buf represent?
There was a problem hiding this comment.
It's the byte buffer read from the file
Signed-off-by: Chase Engelbrecht <[email protected]>
…og statements to be more specific Signed-off-by: Chase Engelbrecht <[email protected]>
| try: | ||
| self.launcher.stop(host, nodes) | ||
| except Exception as e: | ||
| raise LaunchError("Launching node(s) on host \"{}\" failed".format(host), e) |
There was a problem hiding this comment.
Might want to change the log statement here to suggest its a failure during stop process
Description
Migrates the ProcessLauncher to a LocalProcessLauncher. Adds an ExceptionHandlingLauncher to decorate the Launcher implementations with exception handling.
I initially tried to generalize the LocalProcessLauncher to use shell commands instead of helpers such as
ioorosmodules. This would have allowed the implementation to be used by non-local providers. However, the telemetry devices andjava_resolverare tightly coupled to evaluating the local machine as well and would have required significant refactoring.Rather than going this route, I decided that a separate implementation for ExternalProcessLauncher could be created to handle the generalized flow of this class without coupling the logic to the local machine. This seems to the be quickest and cleanest path forwards. If there is shared logic between these implementations, it will be broken out to avoid duplication.
The LocalProcessLauncher is mostly unchanged from the original ProcessLauncher, with the exception of using the executor instead of the previous
subprocess.Popencall. Additionally, I refactored a few of the methods to make them more modular.The LocalProcessLauncher was tested by migrating the unit tests from the original ProcessLauncher. Additionally, I wired it into the builder system and ran the
test_tar_distributionsandtest_sourcesintegration tests.Issues Resolved
#134
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.