-
Notifications
You must be signed in to change notification settings - Fork 29
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
Executable Monitor Change #62
Conversation
…r the echo server
…echo server for test
…precation. Slight change to line checking in the executable monitor
…nly print the rest of the log if the run doesn't find an exit condition originally
ReadOutputFile = open("output.log", "r") | ||
|
||
# Launch the executable | ||
exe = subprocess.Popen([exe_abs_path], stdout=WriteOutputFile, stderr=WriteOutputFile, universal_newlines=True) |
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.
This is clobbering the environment passed in, which makes it tough to debug which dynamic libraries are in use.
# While a timeout hasn't happened, the executable is running, and an exit condition has not been met | ||
while ( not exit_condition_met ): | ||
# Uncomment this sleep for a short duration between loops to not steal all system resources | ||
# time.sleep(.05) |
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.
Does this need to be uncommented? Is the call to readline() blocking?
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.
I left it in intentionally for if running it locally if somebody saw it more or less starving the other tasks on the system, such as the subprocess being run.
@@ -41,6 +41,10 @@ | |||
type=int, | |||
required=False, | |||
help='Exit status that indicates that the executable completed successfully. Required if --success-line is not used.') | |||
parser.add_argument('--retry-attempts', |
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.
Not a fan of having retry attempts built in here. We should require that checks running in CI run reliably every time.
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.
It's actually entirely there for if doing an external network test where a failure due to not being able to connect is something that could happen. Where thought that having the ability to retry for something like that might be valuable in the future.
logging.info("EXECUTABLE RUN SUMMARY:\n") | ||
timeout_time_seconds = cur_time_seconds + args.timeout_seconds | ||
|
||
logging.info("START OF DEVICE OUTPUT\n") |
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.
Spurious newline.
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.
Fixed
exit_condition_met = True | ||
|
||
if not exe_exitted: | ||
logging.info(f"EXECUTABLE DID NOT EXIT, MANUALLY KILLING NOW\n") |
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.
Spurious newline.
# Capture remaining output and check for the successful line | ||
for exe_stdout_line in ReadOutputFile.readlines(): | ||
logging.info(exe_stdout_line) | ||
if args.success_line is not None and args.success_line in exe_stdout_line: |
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.
Should success_line be a regex rather than a particular line?
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.
Not a bad idea, but think for now want to stick with this instead of the regex. Can add that in down the line if we think it would be useful.
A few questions on this. |
Utilize File I/O to ensure that calling readline() does not hang forever if no string is written out of the subproccess.