From: Vratko Polak Date: Wed, 10 Jul 2019 11:59:50 +0000 (+0200) Subject: Bash functions style cleanup X-Git-Url: https://gerrit.fd.io/r/gitweb?p=csit.git;a=commitdiff_plain;h=36d56bdb7f9f394047e2df3f29bf47db877b649c Bash functions style cleanup + Update rst documentation for bash style + Command substitution: + Clarify when to use backticks. + Recommend avoiding nested command substitution. + Do not recommend putting command substitution results into quotes. + Function definition content: + Move "set -exuo pipefail" after comment only blocks. + Other set flags allowed for functions with good reasons. + Apply the new recommendations. - Blank lines unified in code but no written recommendation in rst. + Add missing references to functions called, variables read or set. + Add TODOs to where lists would be long. + Minor improvements to function descriptions. + Make "if" expressions more python-like. + Add missing "|| die" (or "|| true") where spotted. + Downgrade DEFAULT_NIC to a local variable. + Add TODO to list reasons for blacklisted tags. Change-Id: I05dce030a8c2cb1b3a242d8b977e8fe150d8ee20 Signed-off-by: Vratko Polak --- diff --git a/docs/bash_code_style.rst b/docs/bash_code_style.rst index e5dbc9c06e..e955f72ab4 100644 --- a/docs/bash_code_style.rst +++ b/docs/bash_code_style.rst @@ -82,22 +82,24 @@ Safety + TODO: Consider giving examples both for good and bad usage. - + Command substitution on right hand side of assignment should be safe + + Command substitution on right hand side of assignment are safe without quotes. - + But still, quotes are RECOMMENDED, unless line length is a concern. - + Note that command substitution limits the scope for quotes, so it is NOT REQUIRED to escape the quotes in deeper levels. - + TODO: Do we prefer backtics, or "dollar round-bracket"? + + Both backtics and "dollar round-bracket" provide command substitution. + The folowing rules are RECOMMENDED: + + + For simple constructs, use "dollar round-bracket". - + Backticks are more readable, especially when there are - round brackets in the surrounding text. + + If there are round brackets in the surrounding text, use backticks, + as some editor highlighting logic can get confused. - + Backticks do not allow nested command substitution. + + Avoid nested command substitution. - + But we might want to avoid nested command substitution anyway? + + Put intermediate results into local variables, + use "|| die" on each step of command substitution. + Code SHOULD NOT be structured in a way where word splitting is intended. @@ -541,21 +543,25 @@ Library documentation + Then SHALL be the function definitions, and inside: - + "set -exuo pipefail" again. - - + Following that SHALL be the function documentation explaining API contract. + + The body SHALL sart with the function documentation explaining API contract. Similar to Robot [Documentation] or Python function-level docstring. + See below. - + Following that SHALL be varius TODOs, FIXMEs and code itself. + + Following that SHALL be various top-level TODOs and FIXMEs. + + + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs. + + + "set -exuo pipefail" SHALL be the first executable line + in the function body, except functions which legitimely need + different flags. Those SHALL also start with appropriate "set" command(s). + + + Lines containing code itself SHALL follow. + "Code itself" SHALL include comment lines explaining any non-obvious logic. - + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs. - - + There SHALL be two empty lines before next function definition. + + There SHALL be two empty lines between function definitions. More details on function documentation: diff --git a/resources/libraries/bash/function/ansible.sh b/resources/libraries/bash/function/ansible.sh index 663861e8d8..5bf122e4b0 100644 --- a/resources/libraries/bash/function/ansible.sh +++ b/resources/libraries/bash/function/ansible.sh @@ -17,6 +17,7 @@ set -exuo pipefail function ansible_hosts () { + # Run ansible playbook on hosts in working topology file. Ansible scope is # determined by tags passed as parameters to this function. # @@ -51,15 +52,15 @@ function ansible_hosts () { function installed () { - set -exuo pipefail - # Check if the given utility is installed. Fail if not installed. # # Arguments: # - ${1} - Utility to check. - # Returns: + # Returns (implicitly): # - 0 - If command is installed. # - 1 - If command is not installed. + set -exuo pipefail + command -v "${1}" -} \ No newline at end of file +} diff --git a/resources/libraries/bash/function/artifacts.sh b/resources/libraries/bash/function/artifacts.sh index fc9c886a3b..fe755af9d9 100644 --- a/resources/libraries/bash/function/artifacts.sh +++ b/resources/libraries/bash/function/artifacts.sh @@ -17,12 +17,17 @@ set -exuo pipefail function download_artifacts () { - # Get and/or install VPP artifacts from packagecloud.io. + + # Download or install VPP artifacts from packagecloud.io. # # Variables read: # - CSIT_DIR - Path to existing root of local CSIT git repository. # Variables set: # - REPO_URL - FD.io Packagecloud repository. + # Functions conditionally called (see their documentation for side effects): + # - download_ubuntu_artifacts + # - download_centos_artifacts + # - download_opensuse_artifacts set -exuo pipefail @@ -51,12 +56,14 @@ function download_artifacts () { } function download_ubuntu_artifacts () { - # Get and/or install Ubuntu VPP artifacts from packagecloud.io. + + # Download or install Ubuntu VPP artifacts from packagecloud.io. # # Variables read: # - REPO_URL - FD.io Packagecloud repository. # - VPP_VERSION - VPP version. - # - INSTALL - If install packages or download only. Default: download + # - INSTALL - Whether install packages (if set to "true") or download only. + # Default: "false". set -exuo pipefail @@ -112,7 +119,7 @@ function download_ubuntu_artifacts () { done set -x - if [ "${INSTALL:-false}" = true ]; then + if [[ "${INSTALL:-false}" == "true" ]]; then sudo apt-get -y install "${artifacts[@]}" || { die "Install VPP artifacts failed." } @@ -124,12 +131,14 @@ function download_ubuntu_artifacts () { } function download_centos_artifacts () { - # Get and/or install CentOS VPP artifacts from packagecloud.io. + + # Download or install CentOS VPP artifacts from packagecloud.io. # # Variables read: # - REPO_URL - FD.io Packagecloud repository. # - VPP_VERSION - VPP version. - # - INSTALL - If install packages or download only. Default: download + # - INSTALL - Whether install packages (if set to "true") or download only. + # Default: "false". set -exuo pipefail @@ -145,7 +154,7 @@ function download_centos_artifacts () { artifacts+=(${packages[@]/%/-${VPP_VERSION-}}) fi - if [ "${INSTALL:-false}" = true ]; then + if [[ "${INSTALL:-false}" == "true" ]]; then sudo yum -y install "${artifacts[@]}" || { die "Install VPP artifact failed." } @@ -157,12 +166,14 @@ function download_centos_artifacts () { } function download_opensuse_artifacts () { - # Get and/or install OpenSuSE VPP artifacts from packagecloud.io. + + # Download or install OpenSuSE VPP artifacts from packagecloud.io. # # Variables read: # - REPO_URL - FD.io Packagecloud repository. # - VPP_VERSION - VPP version. - # - INSTALL - If install packages or download only. Default: download + # - INSTALL - Whether install packages (if set to "true") or download only. + # Default: "false". set -exuo pipefail @@ -178,7 +189,7 @@ function download_opensuse_artifacts () { artifacts+=(${packages[@]/%/-${VPP_VERSION-}}) fi - if [ "${INSTALL:-false}" = true ]; then + if [[ "${INSTALL:-false}" == "true" ]]; then sudo yum -y install "${artifacts[@]}" || { die "Install VPP artifact failed." } diff --git a/resources/libraries/bash/function/artifacts_hc.sh b/resources/libraries/bash/function/artifacts_hc.sh index 86c7f49819..7a866f6596 100644 --- a/resources/libraries/bash/function/artifacts_hc.sh +++ b/resources/libraries/bash/function/artifacts_hc.sh @@ -16,12 +16,16 @@ set -exuo pipefail function download_artifacts_hc () { - # Get and/or install HC artifacts from packagecloud.io. + + # Download or install HC artifacts from packagecloud.io. # # Variables read: # - CSIT_DIR - Path to existing root of local CSIT git repository. # Variables set: # - REPO_URL - FD.io Packagecloud repository. + # Functions conditionally called (see their documentation for side effects): + # - download_ubuntu_artifacts_hc + # - download_centos_artifacts_hc set -exuo pipefail @@ -48,12 +52,14 @@ function download_artifacts_hc () { } function download_ubuntu_artifacts_hc () { - # Get and/or install Ubuntu HC artifacts from packagecloud.io. + + # Download or install Ubuntu HC artifacts from packagecloud.io. # # Variables read: # - REPO_URL - FD.io Packagecloud repository. # - HC_VERSION - HC version. - # - INSTALL - If install packages or download only. Default: download + # - INSTALL - Whether install packages (if set to "true") or download only. + # Default: "false". set -exuo pipefail @@ -69,7 +75,7 @@ function download_ubuntu_artifacts_hc () { artifacts+=(${hc[@]/%/=${HC_VERSION-}}) fi - if [ "${INSTALL:-false}" = true ]; then + if [[ "${INSTALL:-false}" == "true" ]]; then sudo apt-get -y install "${artifacts[@]}" || { die "Install HC artifacts failed." } @@ -81,12 +87,14 @@ function download_ubuntu_artifacts_hc () { } function download_centos_artifacts_hc () { - # Get and/or install CentOS HC artifacts from packagecloud.io. + + # Download or install CentOS HC artifacts from packagecloud.io. # # Variables read: # - REPO_URL - FD.io Packagecloud repository. # - HC_VERSION - HC version. - # - INSTALL - If install packages or download only. Default: download + # - INSTALL - Whether install packages (if set to "true") or download only. + # Default: "false". set -exuo pipefail @@ -102,7 +110,7 @@ function download_centos_artifacts_hc () { artifacts+=(${hc[@]/%/-${HC_VERSION-}}) fi - if [ "${INSTALL:-false}" = true ]; then + if [[ "${INSTALL:-false}" == "true" ]]; then sudo yum -y install "${artifacts[@]}" || { die "Install HC artifact failed." } @@ -112,4 +120,3 @@ function download_centos_artifacts_hc () { } fi } - diff --git a/resources/libraries/bash/function/branch.sh b/resources/libraries/bash/function/branch.sh index e340a0c658..558a2b617e 100644 --- a/resources/libraries/bash/function/branch.sh +++ b/resources/libraries/bash/function/branch.sh @@ -21,8 +21,6 @@ set -exuo pipefail function checkout_csit_for_vpp () { - set -exuo pipefail - # This should be useful mainly for vpp-csit jobs (and timed csit-vpp jobs), # which want to use csit oper branches (especially for vpp stable branches). # This allows the Jenkins job to checkout CSIT master branch, @@ -51,6 +49,8 @@ function checkout_csit_for_vpp () { # Functions called: # - die - Print to stderr and exit, defined in "common" library. + set -exuo pipefail + case "${1}" in "stable/"*) branch_id="origin/${1/stable\//oper-rls}" @@ -74,10 +74,10 @@ function checkout_csit_for_vpp () { csit_branch="${csit_branch#origin/}" || die override_ref="${CSIT_REF-}" if [[ -n "${override_ref}" ]]; then - git fetch --depth=1 https://gerrit.fd.io/r/csit "${override_ref}" - git checkout FETCH_HEAD + git fetch --depth=1 https://gerrit.fd.io/r/csit "${override_ref}" || die + git checkout FETCH_HEAD || die else - git checkout "${csit_branch}" + git checkout "${csit_branch}" || die fi - popd + popd || die } diff --git a/resources/libraries/bash/function/common.sh b/resources/libraries/bash/function/common.sh index 96c7940f79..078ed70197 100644 --- a/resources/libraries/bash/function/common.sh +++ b/resources/libraries/bash/function/common.sh @@ -23,7 +23,6 @@ set -exuo pipefail function activate_docker_topology () { - set -exuo pipefail # Create virtual vpp-device topology. Output of the function is topology # file describing created environment saved to a file. @@ -34,9 +33,12 @@ function activate_docker_topology () { # - NODENESS - Node multiplicity of desired testbed. # - FLAVOR - Node flavor string, usually describing the processor. # - IMAGE_VER_FILE - Name of file that contains the image version. + # - CSIT_DIR - Directory where ${IMAGE_VER_FILE} is located. # Variables set: # - WORKING_TOPOLOGY - Path to topology file. + set -exuo pipefail + source "${BASH_FUNCTION_DIR}/device.sh" || { die "Source failed!" } @@ -48,7 +50,7 @@ function activate_docker_topology () { # We execute reservation over csit-shim-dcr (ssh) which runs sourced # script's functions. Env variables are read from ssh output # back to localhost for further processing. - hostname=$(grep search /etc/resolv.conf | cut -d' ' -f3) + hostname=$(grep search /etc/resolv.conf | cut -d' ' -f3) || die ssh="ssh root@${hostname} -p 6022" run="activate_wrapper ${NODENESS} ${FLAVOR} ${device_image}" # backtics to avoid https://midnight-commander.org/ticket/2142 @@ -91,8 +93,6 @@ function activate_docker_topology () { function activate_virtualenv () { - set -exuo pipefail - # Update virtualenv pip package, delete and create virtualenv directory, # activate the virtualenv, install requirements, set PYTHONPATH. @@ -107,6 +107,8 @@ function activate_virtualenv () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + root_path="${1-$CSIT_DIR}" env_dir="${root_path}/env" req_path=${2-$CSIT_DIR/requirements.txt} @@ -130,8 +132,6 @@ function activate_virtualenv () { function archive_tests () { - set -exuo pipefail - # Create .tar.xz of generated/tests for archiving. # To be run after generate_tests, kept separate to offer more flexibility. @@ -140,6 +140,8 @@ function archive_tests () { # File rewriten: # - ${ARCHIVE_DIR}/tests.tar.xz - Archive of generated tests. + set -exuo pipefail + tar c "${GENERATED_DIR}/tests" | xz -9e > "${ARCHIVE_DIR}/tests.tar.xz" || { die "Error creating archive of generated tests." } @@ -148,8 +150,6 @@ function archive_tests () { function check_download_dir () { - set -exuo pipefail - # Fail if there are no files visible in ${DOWNLOAD_DIR}. # # Variables read: @@ -159,6 +159,8 @@ function check_download_dir () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + if [[ ! "$(ls -A "${DOWNLOAD_DIR}")" ]]; then die "No artifacts downloaded!" fi @@ -167,12 +169,12 @@ function check_download_dir () { function cleanup_topo () { - set -exuo pipefail - # Variables read: # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed. # - PYTHON_SCRIPTS_DIR - Path to directory holding the reservation script. + set -exuo pipefail + python "${PYTHON_SCRIPTS_DIR}/topo_cleanup.py" -t "${WORKING_TOPOLOGY}" # Not using "|| die" as some callers might want to ignore errors, # e.g. in teardowns, such as unreserve. @@ -181,8 +183,6 @@ function cleanup_topo () { function common_dirs () { - set -exuo pipefail - # Set global variables, create some directories (without touching content). # Variables set: @@ -200,6 +200,8 @@ function common_dirs () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + BASH_FUNCTION_DIR="$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")" || { die "Some error during localizing this source directory." } @@ -239,8 +241,6 @@ function common_dirs () { function compose_pybot_arguments () { - set -exuo pipefail - # Variables read: # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed. # - DUT - CSIT test/ subdirectory, set while processing tags. @@ -251,6 +251,8 @@ function compose_pybot_arguments () { # - PYBOT_ARGS - String holding part of all arguments for pybot. # - EXPANDED_TAGS - Array of strings pybot arguments compiled from tags. + set -exuo pipefail + # No explicit check needed with "set -u". PYBOT_ARGS=("--loglevel" "TRACE") PYBOT_ARGS+=("--variable" "TOPOLOGY_PATH:${WORKING_TOPOLOGY}") @@ -282,8 +284,10 @@ function compose_pybot_arguments () { function copy_archives () { - set -exuo pipefail - + # Create additional archive if workspace variable is set. + # This way if script is running in jenkins all will be + # automatically archived to logs.fd.io. + # # Variables read: # - WORKSPACE - Jenkins workspace, copy only if the value is not empty. # Can be unset, then it speeds up manual testing. @@ -294,9 +298,8 @@ function copy_archives () { # Functions called: # - die - Print to stderr and exit. - # We will create additional archive if workspace variable is set. - # This way if script is running in jenkins all will be - # automatically archived to logs.fd.io. + set -exuo pipefail + if [[ -n "${WORKSPACE-}" ]]; then mkdir -p "${WORKSPACE}/archives/" || die "Archives dir create failed." cp -rf "${ARCHIVE_DIR}"/* "${WORKSPACE}/archives" || die "Copy failed." @@ -305,6 +308,7 @@ function copy_archives () { function deactivate_docker_topology () { + # Deactivate virtual vpp-device topology by removing containers. # # Variables read: @@ -316,9 +320,9 @@ function deactivate_docker_topology () { case_text="${NODENESS}_${FLAVOR}" case "${case_text}" in "1n_skx") - hostname=$(grep search /etc/resolv.conf | cut -d' ' -f3) + hostname=$(grep search /etc/resolv.conf | cut -d' ' -f3) || die ssh="ssh root@${hostname} -p 6022" - env_vars="$(env | grep CSIT_ | tr '\n' ' ' )" + env_vars=$(env | grep CSIT_ | tr '\n' ' ' ) || die ${ssh} "$(declare -f); deactivate_wrapper ${env_vars}" || { die "Topology cleanup via shim-dcr failed!" } @@ -337,6 +341,7 @@ function deactivate_docker_topology () { function die () { + # Print the message to standard error end exit with error code specified # by the second argument. # @@ -355,8 +360,6 @@ function die () { function die_on_pybot_error () { - set -exuo pipefail - # Source this fragment if you want to abort on any failed test case. # # Variables read: @@ -364,6 +367,8 @@ function die_on_pybot_error () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + if [[ "${PYBOT_EXIT_STATUS}" != "0" ]]; then die "Test failures are present!" "${PYBOT_EXIT_STATUS}" fi @@ -372,8 +377,6 @@ function die_on_pybot_error () { function generate_tests () { - set -exuo pipefail - # Populate ${GENERATED_DIR}/tests based on ${CSIT_DIR}/tests/. # Any previously existing content of ${GENERATED_DIR}/tests is wiped before. # The generation is done by executing any *.py executable @@ -387,8 +390,10 @@ function generate_tests () { # Directories replaced: # - ${GENERATED_DIR}/tests - Overwritten by the generated tests. - rm -rf "${GENERATED_DIR}/tests" - cp -r "${CSIT_DIR}/tests" "${GENERATED_DIR}/tests" + set -exuo pipefail + + rm -rf "${GENERATED_DIR}/tests" || die + cp -r "${CSIT_DIR}/tests" "${GENERATED_DIR}/tests" || die cmd_line=("find" "${GENERATED_DIR}/tests" "-type" "f") cmd_line+=("-executable" "-name" "*.py") file_list=$("${cmd_line[@]}") || die @@ -405,8 +410,6 @@ function generate_tests () { function get_test_code () { - set -exuo pipefail - # Arguments: # - ${1} - Optional, argument of entry script (or empty as unset). # Test code value to override job name from environment. @@ -417,6 +420,8 @@ function get_test_code () { # - NODENESS - Node multiplicity of desired testbed. # - FLAVOR - Node flavor string, usually describing the processor. + set -exuo pipefail + TEST_CODE="${1-}" || die "Reading optional argument failed, somehow." if [[ -z "${TEST_CODE}" ]]; then TEST_CODE="${JOB_NAME-}" || die "Reading job name failed, somehow." @@ -462,18 +467,18 @@ function get_test_code () { function get_test_tag_string () { - set -exuo pipefail - # Variables read: # - GERRIT_EVENT_TYPE - Event type set by gerrit, can be unset. # - GERRIT_EVENT_COMMENT_TEXT - Comment text, read for "comment-added" type. # - TEST_CODE - The test selection string from environment or argument. # Variables set: - # - TEST_TAG_STRING - The string following "perftest" in gerrit comment, - # or empty. + # - TEST_TAG_STRING - The string following trigger word in gerrit comment. + # May be empty, not set on event types not adding comment. # TODO: ci-management scripts no longer need to perform this. + set -exuo pipefail + trigger="" if [[ "${GERRIT_EVENT_TYPE-}" == "comment-added" ]]; then case "${TEST_CODE}" in @@ -509,8 +514,6 @@ function get_test_tag_string () { function reserve_and_cleanup_testbed () { - set -exuo pipefail - # Reserve physical testbed, perform cleanup, register trap to unreserve. # When cleanup fails, remove from topologies and keep retrying # until all topologies are removed. @@ -530,6 +533,8 @@ function reserve_and_cleanup_testbed () { # Traps registered: # - EXIT - Calls cancel_all for ${WORKING_TOPOLOGY}. + set -exuo pipefail + while [[ ${TOPOLOGIES[@]} ]]; do for topo in "${TOPOLOGIES[@]}"; do set +e @@ -600,8 +605,8 @@ function reserve_and_cleanup_testbed () { function run_pybot () { - set -exuo pipefail - + # Run pybot with options based on input variables. Create output_info.xml + # # Variables read: # - CSIT_DIR - Path to existing root of local CSIT git repository. # - ARCHIVE_DIR - Path to store robot result files in. @@ -612,6 +617,8 @@ function run_pybot () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + all_options=("--outputdir" "${ARCHIVE_DIR}" "${PYBOT_ARGS[@]}") all_options+=("--noncritical" "EXPECTED_FAILING") all_options+=("${EXPANDED_TAGS[@]}") @@ -628,24 +635,25 @@ function run_pybot () { all_options+=("--report" "none") all_options+=("--output" "${ARCHIVE_DIR}/output_info.xml") all_options+=("${ARCHIVE_DIR}/output.xml") - rebot "${all_options[@]}" + rebot "${all_options[@]}" || true popd || die "Change directory operation failed." } function select_tags () { - set -exuo pipefail - # Variables read: # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed. # - TEST_CODE - String affecting test selection, usually jenkins job name. # - TEST_TAG_STRING - String selecting tags, from gerrit comment. # Can be unset. # - TOPOLOGIES_DIR - Path to existing directory with available tpologies. + # - BASH_FUNCTION_DIR - Directory with input files to process. # Variables set: # - TAGS - Array of processed tag boolean expressions. + set -exuo pipefail + # NIC SELECTION start_pattern='^ TG:' end_pattern='^ \? \?[A-Za-z0-9]\+:' @@ -658,40 +666,44 @@ function select_tags () { reserved=$(sed "${sed_command}" "${WORKING_TOPOLOGY}" \ | grep -hoP "model: \K.*" | sort -u) # All topologies DUT NICs - Selected topology DUT NICs - exclude_nics=($(comm -13 <(echo "${reserved}") <(echo "${available}"))) + exclude_nics=($(comm -13 <(echo "${reserved}") <(echo "${available}"))) || { + die "Computation of excluded NICs failed." + } - # Select default NIC + # Select default NIC tag. case "${TEST_CODE}" in *"3n-dnv"* | *"2n-dnv"*) - DEFAULT_NIC='nic_intel-x553' + default_nic="nic_intel-x553" ;; *"3n-tsh"*) - DEFAULT_NIC='nic_intel-x520-da2' + default_nic="nic_intel-x520-da2" ;; *) - DEFAULT_NIC='nic_intel-x710' + default_nic="nic_intel-x710" ;; esac + # Tag file directory shorthand. + tfd="${BASH_FUNCTION_DIR}" case "${TEST_CODE}" in # Select specific performance tests based on jenkins job type variable. *"ndrpdr-weekly"* ) - readarray -t test_tag_array < "${BASH_FUNCTION_DIR}/mlr-weekly.txt" + readarray -t test_tag_array < "${tfd}/mlr-weekly.txt" || die ;; *"mrr-daily"* ) - readarray -t test_tag_array < "${BASH_FUNCTION_DIR}/mrr-daily.txt" + readarray -t test_tag_array < "${tfd}/mrr-daily.txt" || die ;; *"mrr-weekly"* ) - readarray -t test_tag_array < "${BASH_FUNCTION_DIR}/mrr-weekly.txt" + readarray -t test_tag_array < "${tfd}/mrr-weekly.txt" || die ;; * ) if [[ -z "${TEST_TAG_STRING-}" ]]; then # If nothing is specified, we will run pre-selected tests by # following tags. - test_tag_array=("mrrAND${DEFAULT_NIC}AND1cAND64bANDip4base" - "mrrAND${DEFAULT_NIC}AND1cAND78bANDip6base" - "mrrAND${DEFAULT_NIC}AND1cAND64bANDl2bdbase" - "mrrAND${DEFAULT_NIC}AND1cAND64bANDl2xcbase" + test_tag_array=("mrrAND${default_nic}AND1cAND64bANDip4base" + "mrrAND${default_nic}AND1cAND78bANDip6base" + "mrrAND${default_nic}AND1cAND64bANDl2bdbase" + "mrrAND${default_nic}AND1cAND64bANDl2xcbase" "!dot1q" "!drv_avf") else # If trigger contains tags, split them into array. @@ -701,6 +713,10 @@ function select_tags () { esac # Blacklisting certain tags per topology. + # + # Reasons for blacklisting: + # - ipsechw - Blacklisted on testbeds without crypto hardware accelerator. + # TODO: Add missing reasons here (if general) or where used (if specific). case "${TEST_CODE}" in *"2n-skx"*) test_tag_array+=("!ipsechw") @@ -763,7 +779,7 @@ function select_tags () { if [[ "${TEST_TAG_STRING-}" == *"nic_"* ]]; then prefix="${prefix}mrrAND" else - prefix="${prefix}mrrAND${DEFAULT_NIC}AND" + prefix="${prefix}mrrAND${default_nic}AND" fi fi for tag in "${test_tag_array[@]}"; do @@ -782,8 +798,6 @@ function select_tags () { function select_vpp_device_tags () { - set -exuo pipefail - # Variables read: # - TEST_CODE - String affecting test selection, usually jenkins job name. # - TEST_TAG_STRING - String selecting tags, from gerrit comment. @@ -791,6 +805,8 @@ function select_vpp_device_tags () { # Variables set: # - TAGS - Array of processed tag boolean expressions. + set -exuo pipefail + case "${TEST_CODE}" in # Select specific performance tests based on jenkins job type variable. * ) @@ -827,13 +843,13 @@ function select_vpp_device_tags () { function select_os () { - set -exuo pipefail - # Variables set: # - VPP_VER_FILE - Name of File in CSIT dir containing vpp stable version. # - IMAGE_VER_FILE - Name of File in CSIT dir containing the image name. # - PKG_SUFFIX - Suffix of OS package file name, "rpm" or "deb." + set -exuo pipefail + os_id=$(grep '^ID=' /etc/os-release | cut -f2- -d= | sed -e 's/\"//g') || { die "Get OS release failed." } @@ -858,8 +874,6 @@ function select_os () { function select_topology () { - set -exuo pipefail - # Variables read: # - NODENESS - Node multiplicity of testbed, either "2n" or "3n". # - FLAVOR - Node flavor string, currently either "hsw" or "skx". @@ -871,9 +885,12 @@ function select_topology () { # Functions called: # - die - Print to stderr and exit. + set -exuo pipefail + case_text="${NODENESS}_${FLAVOR}" case "${case_text}" in # TODO: Move tags to "# Blacklisting certain tags per topology" section. + # TODO: Double link availability depends on NIC used. "1n_vbox") TOPOLOGIES=( "${TOPOLOGIES_DIR}"/*vpp_device*.template ) TOPOLOGIES_TAGS="2_node_single_link_topo" @@ -919,6 +936,7 @@ function select_topology () { function untrap_and_unreserve_testbed () { + # Use this as a trap function to ensure testbed does not remain reserved. # Perhaps call directly before script exit, to free testbed for other jobs. # This function is smart enough to avoid multiple unreservations (so safe). @@ -957,10 +975,13 @@ function untrap_and_unreserve_testbed () { function warn () { + # Print the message to standard error. # # Arguments: # - ${@} - The text of the message. + set -exuo pipefail + echo "$@" >&2 } diff --git a/resources/libraries/bash/function/device.sh b/resources/libraries/bash/function/device.sh index 0b1a090fd6..0eda008abb 100644 --- a/resources/libraries/bash/function/device.sh +++ b/resources/libraries/bash/function/device.sh @@ -14,9 +14,11 @@ set -exuo pipefail # This library defines functions used by multiple entry scripts. +# Deliberately not depending on common.sh to allow standalone usage. # Keep functions ordered alphabetically, please. function activate_wrapper () { + # Acts as wrapper for activate docker topology. # # Variables read: @@ -37,6 +39,7 @@ function activate_wrapper () { function bind_interfaces_to_containers () { + # Bind linux network interface to container and create symlink for PCI # address in container. # @@ -83,6 +86,7 @@ function bind_interfaces_to_containers () { function bind_interfaces_to_driver () { + # Bind network interface specified by parameter to driver specified by # parameter. # @@ -90,9 +94,11 @@ function bind_interfaces_to_driver () { # - ADDR - PCI address of network interface. # - DRIVER - Kernel driver. + set -exuo pipefail + pci_path="/sys/bus/pci/devices/${ADDR}" drv_path="/sys/bus/pci/drivers/${DRIVER}" - vd="$(cat ${pci_path}/vendor ${pci_path}/device)" || { + vd=$(cat ${pci_path}/vendor ${pci_path}/device) || { die "Failed to retrieve interface details!" } set +e @@ -108,6 +114,7 @@ function bind_interfaces_to_driver () { function clean_environment () { + # Cleanup environment by removing topology containers and shared volumes # and binding interfaces back to original driver. # @@ -143,10 +150,13 @@ function clean_environment () { function clean_environment_on_exit () { + # Cleanup environment by removing topology containers and binding # interfaces back to original driver only if exit code is not 0. # This function acts as workaround as 'set -eu' does not trigger ERR trap. + set -exuo pipefail + if [ $? -ne 0 ]; then clean_environment || die fi @@ -154,6 +164,7 @@ function clean_environment_on_exit () { function deactivate_wrapper () { + # Acts as wrapper for deactivate docker topology. # # Variables read: @@ -169,9 +180,12 @@ function deactivate_wrapper () { function die () { + # Print the message to standard error end exit with error code specified # by the second argument. # + # Duplicate of common.sh function, as this file is also used standalone. + # # Hardcoded values: # - The default error message. # Arguments: @@ -186,6 +200,7 @@ function die () { function enter_mutex () { + # Enter mutual exclusion for protecting execution from starvation and # deadlock. @@ -208,6 +223,7 @@ function enter_mutex () { function exit_mutex () { + # Exit mutual exclusion. set -exuo pipefail @@ -222,11 +238,12 @@ function exit_mutex () { function get_available_interfaces () { + # Find and get available Virtual functions. # # Arguments: - # - ${1} - Node flavor string, usually describing the processor and node - # multiplicity of desired testbed, separated by underscore. + # - ${1} - Nodeness, as set by common.sh get_test_code. + # - ${2} - Flavor, as set by common.sh get_test_code. # Variables set: # - DUT1_NETDEVS - List of network devices allocated to DUT1 container. # - DUT1_PCIDEVS - List of PCI addresses allocated to DUT1 container. @@ -347,6 +364,7 @@ function get_available_interfaces () { function get_krn_driver () { + # Get kernel driver from linux network device name. # # Variables read: @@ -364,6 +382,7 @@ function get_krn_driver () { function get_mac_addr () { + # Get MAC address from linux network device name. # # Variables read: @@ -382,6 +401,7 @@ function get_mac_addr () { function get_pci_addr () { + # Get PCI address in :. format from linux network # device name. # @@ -405,34 +425,41 @@ function get_pci_addr () { function installed () { - set -exuo pipefail - # Check if the given utility is installed. Fail if not installed. # + # Duplicate of common.sh function, as this file is also used standalone. + # # Arguments: # - ${1} - Utility to check. # Returns: # - 0 - If command is installed. # - 1 - If command is not installed. + set -exuo pipefail + command -v "${1}" } function print_env_variables () { + # Get environment variables prefixed by CSIT_. set -exuo pipefail - env | grep CSIT_ + env | grep CSIT_ || true } function read_env_variables () { + # Read environment variables from parameters. # # Arguments: # - ${@} - Variables passed as an argument. + # Variables read, set or exported: Multiple, + # see the code for the current list. + # TODO: Do we need to list them and their meanings? set -exuo pipefail @@ -454,6 +481,7 @@ function read_env_variables () { function set_env_variables () { + # Set environment variables. # # Variables read: @@ -465,6 +493,7 @@ function set_env_variables () { # - TG_NETMACS - List of network devices MAC addresses of TG container. # - TG_PCIDEVS - List of PCI addresses of devices of TG container. # - TG_DRIVERS - List of interface drivers to TG container. + # Variables set: TODO. set -exuo pipefail @@ -502,6 +531,7 @@ function set_env_variables () { function start_topology_containers () { + # Starts csit-sut-dcr docker containers for TG/DUT1. # # Variables read: @@ -551,11 +581,11 @@ function start_topology_containers () { # Run TG and DUT1. As initial version we do support only 2-node. params=(${dcr_stc_params} --name csit-tg-$(uuidgen) ${dcr_image}) - DCR_UUIDS+=([tg]="$(docker run "${params[@]}")") || { + DCR_UUIDS+=([tg]=$(docker run "${params[@]}")) || { die "Failed to start TG docker container!" } params=(${dcr_stc_params} --name csit-dut1-$(uuidgen) ${dcr_image}) - DCR_UUIDS+=([dut1]="$(docker run "${params[@]}")") || { + DCR_UUIDS+=([dut1]=$(docker run "${params[@]}")) || { die "Failed to start DUT1 docker container!" } @@ -565,21 +595,21 @@ function start_topology_containers () { # Get Containers TCP ports. params=(${DCR_UUIDS[tg]}) - DCR_PORTS+=([tg]="$(docker port "${params[@]}")") || { + DCR_PORTS+=([tg]=$(docker port "${params[@]}")) || { die "Failed to get port of TG docker container!" } params=(${DCR_UUIDS[dut1]}) - DCR_PORTS+=([dut1]="$(docker port "${params[@]}")") || { + DCR_PORTS+=([dut1]=$(docker port "${params[@]}")) || { die "Failed to get port of DUT1 docker container!" } # Get Containers PIDs. params=(--format="{{ .State.Pid }}" ${DCR_UUIDS[tg]}) - DCR_CPIDS+=([tg]="$(docker inspect "${params[@]}")") || { + DCR_CPIDS+=([tg]=$(docker inspect "${params[@]}")) || { die "Failed to get PID of TG docker container!" } params=(--format="{{ .State.Pid }}" ${DCR_UUIDS[dut1]}) - DCR_CPIDS+=([dut1]="$(docker inspect "${params[@]}")") || { + DCR_CPIDS+=([dut1]=$(docker inspect "${params[@]}")) || { die "Failed to get PID of DUT1 docker container!" } } @@ -587,8 +617,12 @@ function start_topology_containers () { function warn () { # Print the message to standard error. # + # Duplicate of common.sh function, as this file is also used standalone. + # # Arguments: # - ${@} - The text of the message. + set -exuo pipefail + echo "$@" >&2 } diff --git a/resources/libraries/bash/function/gather.sh b/resources/libraries/bash/function/gather.sh index 56b148d1e1..1fc2d3e604 100644 --- a/resources/libraries/bash/function/gather.sh +++ b/resources/libraries/bash/function/gather.sh @@ -24,8 +24,6 @@ set -exuo pipefail function gather_build () { - set -exuo pipefail - # Variables read: # - TEST_CODE - String affecting test selection, usually jenkins job name. # - DOWNLOAD_DIR - Path to directory pybot takes the build to test from. @@ -43,6 +41,8 @@ function gather_build () { # TODO: Separate DUT-from-TEST_CODE from gather-for-DUT, # when the first one becomes relevant for per_patch. + set -exuo pipefail + pushd "${DOWNLOAD_DIR}" || die "Pushd failed." case "${TEST_CODE}" in *"hc2vpp"*) @@ -71,8 +71,6 @@ function gather_build () { function gather_dpdk () { - set -exuo pipefail - # Ensure latest DPDK archive is downloaded. # # Variables read: @@ -84,6 +82,8 @@ function gather_dpdk () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + dpdk_repo="https://fast.dpdk.org/rel" # Use downloaded packages with specific version if [[ "${TEST_CODE}" == *"daily"* ]] || \ @@ -117,8 +117,6 @@ function gather_dpdk () { function gather_ligato () { - set -exuo pipefail - # Build docker image (with vpp, ligato and vpp-agent), # and put it to ${DOWNLOAD_DIR}/. # @@ -155,6 +153,8 @@ function gather_ligato () { # so maybe it is not worth introducing fragments/functions for the blocks. # TODO: This fragment is too long anyway, split it up. + set -exuo pipefail + gather_vpp || die "The function should have died on error." mkdir -p /tmp/vpp && rm -f /tmp/vpp/* || { @@ -244,8 +244,6 @@ function gather_ligato () { function gather_vpp () { - set -exuo pipefail - # Variables read: # - BASH_FUNCTION_DIR - Bash directory with functions. # - TEST_CODE - The test selection string from environment or argument. @@ -268,6 +266,8 @@ function gather_vpp () { # - ${CSIT_DIR}/resources/tools/scripts/download_install_vpp_pkgs.sh # - Should download and extract requested files to ./. + set -exuo pipefail + case "${TEST_CODE}" in # Not csit-vpp as this code is re-used by ligato gathering. "csit-"*) diff --git a/resources/libraries/bash/function/per_patch.sh b/resources/libraries/bash/function/per_patch.sh index 27fa0cfb06..61de6f7ade 100644 --- a/resources/libraries/bash/function/per_patch.sh +++ b/resources/libraries/bash/function/per_patch.sh @@ -15,13 +15,10 @@ set -exuo pipefail # This library defines functions used mainly by per patch entry scripts. # Generally, the functions assume "common.sh" library has been sourced already. - # Keep functions ordered alphabetically, please. function archive_test_results () { - set -exuo pipefail - # Arguments: # - ${1}: Directory to archive to. Required. Parent has to exist. # Variable set: @@ -34,6 +31,8 @@ function archive_test_results () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory command failed." TARGET="$(readlink -f "$1")" mkdir -p "${TARGET}" || die "Directory creation failed." @@ -47,8 +46,6 @@ function archive_test_results () { function archive_parse_test_results () { - set -exuo pipefail - # Arguments: # - ${1}: Directory to archive to. Required. Parent has to exist. # Variables read: @@ -58,6 +55,8 @@ function archive_parse_test_results () { # - archive_test_results - Archiving results. # - parse_bmrr_results - See definition in this file. + set -exuo pipefail + archive_test_results "$1" || die parse_bmrr_results "${TARGET}" || { die "The function should have died on error." @@ -67,8 +66,6 @@ function archive_parse_test_results () { function build_vpp_ubuntu_amd64 () { - set -exuo pipefail - # This function is using Vagrant script to build VPP with all dependencies # that is ARCH/OS aware. VPP repo is SSOT for building mechanics and CSIT # is consuming artifacts. This way if VPP will introduce change in building @@ -82,6 +79,8 @@ function build_vpp_ubuntu_amd64 () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory command failed." echo 'Building using "make build-root/vagrant/build.sh"' build-root/vagrant/"build.sh" || die "Vagrant VPP build script failed." @@ -95,8 +94,6 @@ function build_vpp_ubuntu_amd64 () { function compare_test_results () { - set -exuo pipefail - # Variables read: # - VPP_DIR - Path to directory with VPP git repo (at least built parts). # - ARCHIVE_DIR - Path to where robot result files are created in. @@ -111,6 +108,8 @@ function compare_test_results () { # - 0 - If the comparison utility sees no regression (nor data error). # - 1 - If the comparison utility sees a regression (or data error). + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." # Reusing CSIT main virtualenv. pip install -r "${PYTHON_SCRIPTS_DIR}/perpatch_requirements.txt" || { @@ -123,8 +122,6 @@ function compare_test_results () { function download_builds () { - set -exuo pipefail - # This is mostly useful only for Sandbox testing, to avoid recompilation. # # Arguments: @@ -138,6 +135,8 @@ function download_builds () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." dirs=("build-root" "build_parent" "build_current" "archive" "csit_current") rm -rf ${dirs[@]} || { @@ -152,8 +151,6 @@ function download_builds () { function initialize_csit_dirs () { - set -exuo pipefail - # This could be in prepare_test, but download_builds also needs this. # # Variables read: @@ -164,6 +161,8 @@ function initialize_csit_dirs () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." rm -rf "csit_current" "csit_parent" || { die "Directory deletion failed." @@ -176,8 +175,6 @@ function initialize_csit_dirs () { function parse_bmrr_results () { - set -exuo pipefail - # Currently "parsing" is just two greps. # TODO: Re-use PAL parsing code, make parsing more general and centralized. # @@ -190,6 +187,8 @@ function parse_bmrr_results () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + rel_dir="$(readlink -e "${1}")" || die "Readlink failed." in_file="${rel_dir}/output.xml" out_file="${rel_dir}/results.txt" @@ -206,8 +205,6 @@ function parse_bmrr_results () { function select_build () { - set -exuo pipefail - # Arguments: # - ${1} - Path to directory to copy VPP artifacts from. Required. # Variables read: @@ -220,6 +217,8 @@ function select_build () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." source_dir="$(readlink -e "$1")" rm -rf "${DOWNLOAD_DIR}"/* || die "Cleanup of download dir failed." @@ -231,8 +230,6 @@ function select_build () { function set_aside_commit_build_artifacts () { - set -exuo pipefail - # Function is copying VPP built artifacts from actual checkout commit for # further use and clean git. # Variables read: @@ -245,6 +242,8 @@ function set_aside_commit_build_artifacts () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." rm -rf "build_current" || die "Remove operation failed." mkdir -p "build_current" || die "Directory creation failed." @@ -262,8 +261,6 @@ function set_aside_commit_build_artifacts () { function set_aside_parent_build_artifacts () { - set -exuo pipefail - # Function is copying VPP built artifacts from parent checkout commit for # further use. Checkout to parent is not part of this function. # Variables read: @@ -275,6 +272,8 @@ function set_aside_parent_build_artifacts () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + cd "${VPP_DIR}" || die "Change directory operation failed." rm -rf "build_parent" || die "Remove failed." mkdir -p "build_parent" || die "Directory creation operation failed." @@ -284,21 +283,19 @@ function set_aside_parent_build_artifacts () { function set_perpatch_dut () { - set -exuo pipefail - # Variables set: # - DUT - CSIT test/ subdirectory containing suites to execute. # TODO: Detect DUT from job name, when we have more than just VPP perpatch. + set -exuo pipefail + DUT="vpp" } function set_perpatch_vpp_dir () { - set -exuo pipefail - # Variables read: # - CSIT_DIR - Path to existing root of local CSIT git repository. # Variables set: @@ -306,6 +303,8 @@ function set_perpatch_vpp_dir () { # Functions called: # - die - Print to stderr and exit, defined in common.sh + set -exuo pipefail + # In perpatch, CSIT is cloned inside VPP clone. VPP_DIR="$(readlink -e "${CSIT_DIR}/..")" || die "Readlink failed." }