Added support for multiple user-specified applications to be run in integration tests - #168
Conversation
…to kbiery/multi_ctrl_proc_support
…le_exclusions' into kbiery/multi_ctrl_proc_support
…grationtest_commandline.py (we now have full run control console output stored in log files).
… to help make their meaning and use more clear; also cleaned up the code and comments a little bit.
…cts like filenames, preferring names of applications instead, since they are more durable.
…sed to all apps that support them, not just the unified shell.
…culating parameterization combinations; removed the unneeded call to check_system_resources in create_config_files
…rationtest_drunc.py
…used in integtests.
Updated links and added code snippets for DAQ session ingredients.
…strings to InformationAboutSpecialVariables.md.
| cmd_set_1 = DAQCommandSet("drunc", dunerc_commands_1, CommandWaitParameters(style=CommandWaitStyle.ECHO)) | ||
| cmd_set_2 = DAQCommandSet("pmshell", pmshell_command, CommandWaitParameters(style=CommandWaitStyle.TIME)) | ||
| cmd_set_3 = DAQCommandSet("drunc", dunerc_commands_2, CommandWaitParameters(style=CommandWaitStyle.ECHO)) |
There was a problem hiding this comment.
Can you say a few words in the docs about what DAQCommandSet does and what CommandWaitParaameters refer to and their purpose? the command set looks quite self explanatory from the example but I'm still not sure what the CommandWaitParameters are for, especially the difference between ECHO and TIME
There was a problem hiding this comment.
I've added some explanation about CommandWaitParameters in the InformationAboutSpecialVariables.md documentation. Please let me know if this helps.
…mpt that appears at the front of an output line.
|
I ran the tests with today's nightly, and the tests behaved as expected. The PM passed, but will be followed up with your PR The only comments I have are on the definition on some of the variables - I struggled to follow some of them. I will give an abridged discussion below. For For Otherwise thank you for this, it looks like it will be very useful. |
…ability of the use of the exit command (sent to the apps).
…roc_mgmt.py, plus a couple nice-to-have changes.
Clarified the explanation of the ECHO wait style and its usage in command sets. Added details about the motivation for the CommandWaitParameters class and its supported wait styles.
Corrected spelling of 'strictly' and improved clarity in the explanation of the 'CommandWaitParameters' class.
added python code type for code snippets
Expanded explanation of CommandWaitParameters and wait styles in integration tests.
|
@PawelPlesniak , I've added some text about CommandWaitParameters in this section of the new document. Please let me know if that helps. |
|
Here are updated instructions for testing these changes: |
|
Does this preempt #167? Can similar functionality be performed with the CommandSets? (e.g. wait 20 seconds for the run to start, find a daq_application by its label, and send a |
|
My sense is that this functionality does not overlap with #167... This functionality is focused on sending application-supported commands into running instances of the applications. At the moment, it doesn't support the running of Linux-system-level commands like |
|
@PawelPlesniak , I've renamed |
emmuhamm
left a comment
emmuhamm
left a comment
There was a problem hiding this comment.
Thanks for this Kurt! I've left a couple of comments mainly targetted towards readability, but feature wise it all works as expected :)
I'll go through this again and the other PR I haven't gone through tomorrow as well
| trimmed_line = decoded_line.strip() | ||
| if len(trimmed_line) == 0: | ||
| continue | ||
| if "ocumented" in trimmed_line: |
There was a problem hiding this comment.
is this meant to be ocumented?
| should_be_printed = verbosity_level >= IntegtestVerbosityLevels.full_output | ||
|
|
||
| # check for errors and warnings for all verbosity levels | ||
| if should_be_printed == False: |
There was a problem hiding this comment.
| if should_be_printed == False: | |
| if not should_be_printed: |
Pythonic :) (very minor feel free to ignore)
There was a problem hiding this comment.
In other news, there are several
if should_be_printed == False:
...
if should_be_printed == False:
...
if should_be_printed == False:
...Should these be condensed into a single if statement instead?
There was a problem hiding this comment.
Actually going through the code again, the logic is a tad bit tangled for my taste.
There is
- is checking if should_be_printed is above a level
- Several checks to flip the boolean
- Finally running the print if the boolean is true.
I think it can be separated more cleanly. One way to do this is to have several variables so that
check_1_pass = is printed above a certain level
check_2_pass = stuff in string
...
should_be_printed = any(check_1_pass, check_2_pass)
^^^ the above is pseudocode, theres a pythonic way of doing this.
But at least this way we can see that the logic is separated out and is easily extendable.
Of course theres other ways to refactorise this but this is one i can think of off the top of my head
| local_excl_string_map.setdefault("SSH_SHELL_process_manager", []).extend( | ||
| ["LogLevel=error", r'key:\s+"DUNEDAQ_ERS_', | ||
| r"drunc.utils.ConnectivityServiceClient\s+404 Client Error: NOT FOUND for url:"] | ||
| ) | ||
| local_excl_string_map.setdefault("drunc", []).extend( | ||
| local_excl_string_map.setdefault("drunc-unified-shell", []).extend( | ||
| ["LogLevel=error", r'key:\s+"DUNEDAQ_ERS_', r"DUNEDAQ_ERS_.*erstrace", "export DUNEDAQ_ERS_", | ||
| r"NewConnectionError.* Failed to establish a new connection: \[Errno 111\] Connection refused", | ||
| r"drunc.utils.ConnectivityServiceClient\s+404 Client Error: NOT FOUND for url:"] | ||
| ) |
There was a problem hiding this comment.
some_dict = {
"SSH_SHELL_process_manager" : ["LogLevel=error", r'key:\s+"DUNEDAQ_ERS_',
r"drunc.utils.ConnectivityServiceClient\s+404 Client Error: NOT FOUND for url:"],
# (and so onfor drunc-unified-shell and drunc-process-manager-shell)
}
...
for app, extensions in some_dict.items():
local_excl_string_map.setdefault(app, []).extend(extensions)Mainly because:
- its more legible
- in the context of making drunc more service-based, I imagine there will be much more apps that will need to be added here when the time comes. Im already aware of the session manager shell slowly coming into existence, and maybe the resource manager, and also the dune rc shell (?), so making this as simple to extend is beneficial
|
Hi @emmuhamm ,
For the different conditions that can cause a line to be printed, the current logic tries to be efficient and not run subsequent tests once one of the conditions determines that the line should be printed. Using multiple different booleans and or-ing them together at the end would not have the same efficiency. Are there other suggested patterns that preserve the efficiency? For the suggested change in |
|
Hi Kurt, Thanks for this!
Python should_be_printed = (
verbosity_level >= IntegtestVerbosityLevels.full_output
or _is_error_or_warning(decoded_line)
or (verbosity_level >= IntegtestVerbosityLevels.drunc_boot_terminate and _is_boot_terminate_msg(decoded_line))
or (verbosity_level >= IntegtestVerbosityLevels.drunc_transitions and _is_transition_msg(decoded_line))
)If the thing in the In any case this is a legibility comment rather than a fundamental logic comment, so I'm happy either way.
No I dont think so, it might be better if we still have the separate exclusion entries for now until from drunc we're able to say with confidence everything behaves the same. To write the idea I had in more detail in a more pythonic way: # always-applied exclusions
default_exclusion_map = {
"drunc-unified-shell": ["Substate.*In error.*Endpoint"],
}
# only needed when DRUNC debug messages are enabled
debug_exclusion_map = {
"SSH_SHELL_process_manager": [
"LogLevel=error", r'key:\s+"DUNEDAQ_ERS_',
r"drunc.utils.ConnectivityServiceClient\s+404 Client Error: NOT FOUND for url:",
],
"drunc-unified-shell": [
"LogLevel=error", r'key:\s+"DUNEDAQ_ERS_', r"DUNEDAQ_ERS_.*erstrace", "export DUNEDAQ_ERS_",
r"NewConnectionError.* Failed to establish a new connection: \[Errno 111\] Connection refused",
r"drunc.utils.ConnectivityServiceClient\s+404 Client Error: NOT FOUND for url:",
],
"drunc-process-manager": [
"LogLevel=error", r'key:\s+"DUNEDAQ_ERS_', r"DUNEDAQ_ERS_.*erstrace", "export DUNEDAQ_ERS_",
],
}
local_excl_string_map = copy.deepcopy(excluded_substring_map)
if verbosity_helper.compare_level(IntegtestVerbosityLevels.drunc_debug):
for app, extensions in debug_exclusion_map.items():
local_excl_string_map.setdefault(app, []).extend(extensions)
for app, extensions in default_exclusion_map.items():
local_excl_string_map.setdefault(app, []).extend(extensions) |
|
@emmuhamm , That said, I wonder if you would be OK with me/us leaving the code as it is today, and allowing me to take some time later to think about (and probably confer with others) about the readability of various options. Would that be OK? |
emmuhamm
left a comment
emmuhamm
left a comment
There was a problem hiding this comment.
Oh yeah of course, happy for this to go in as is!
5 participants
Description
In response to a request from Emir for the ability to start a
drunc-process-managerinstance in addition to thedrunc-unified-shellinstance in an integration test, functionality has been added to theintegrationtestinfrastructure to support user-specified applications.This new functionality is controlled by a new special variable that developers can include in their integtests. This new special variable is described in a new document in this repository. This document will eventually describe all of the special variables that can/should be used in integtests, and it can be viewed here.
These changes have been made in a largely backward-compatible way, so most existing regression/integration tests will not need to be changed. However, the status information that is passed back to the integtest/pytest "tests" now has information about all of the control applications that were run in the DAQ session, so integtests that inspect those results will now need to loop over the list of results instead of simply looking at a single status object.
The console output that users will see when they run an integtest with these changes is largely unchanged, but at a verbosity level of 4 or greater, users will now see information about the processes that are started and the commands that are sent to them.
These changes are correlated with the ones in DUNE-DAQ/drunc#997. The changes in the
druncrepo include a new integtest that demonstrates multiple user-specified applications that are run in the DAQ session for the test.One of the reasons for creating this PR in "draft" mode is that there are still some changes needed in the
drunc/process_manager_test.pyin order to get it to work.Here are suggested commands for testing these changes:
Type of change
Testing checklist
dbt-build --unittest)pytest -s minimal_system_quick_test.py)dunedaq_integtest_bundle.sh)Further checks