From 8e06304165ccf50418027f65605b237eaf14aca7 Mon Sep 17 00:00:00 2001
From: Vratko Polak <vrpolak@cisco.com>
Date: Wed, 13 Dec 2023 15:56:01 +0100
Subject: [PATCH] feat(bootstrap): refactor functions before bisect

Ticket: CSIT-1618

Change-Id: I0c17f1e8f09a3944032d44efa8ecd36fef2056a2
Signed-off-by: Vratko Polak <vrpolak@cisco.com>
---
 .../libraries/bash/entry/bootstrap_verify_perf.sh  |   3 -
 .../libraries/bash/entry/bootstrap_vpp_device.sh   |   3 -
 resources/libraries/bash/entry/per_patch_device.sh |  11 +-
 resources/libraries/bash/entry/per_patch_perf.sh   |  22 ++--
 resources/libraries/bash/function/common.sh        |  37 +++++++
 resources/libraries/bash/function/per_patch.sh     | 123 +++++++++------------
 6 files changed, 108 insertions(+), 91 deletions(-)

diff --git a/resources/libraries/bash/entry/bootstrap_verify_perf.sh b/resources/libraries/bash/entry/bootstrap_verify_perf.sh
index 84d690ca12..18dfd08c39 100755
--- a/resources/libraries/bash/entry/bootstrap_verify_perf.sh
+++ b/resources/libraries/bash/entry/bootstrap_verify_perf.sh
@@ -46,9 +46,6 @@ archive_tests || die
 prepare_topology || die
 select_topology || die
 reserve_and_cleanup_testbed || die
-select_tags || die
-compose_robot_arguments || die
-set_environment_variables || die
 run_robot || die
 move_archives || die
 untrap_and_unreserve_testbed || die
diff --git a/resources/libraries/bash/entry/bootstrap_vpp_device.sh b/resources/libraries/bash/entry/bootstrap_vpp_device.sh
index 31038b0643..fd6279f8c7 100755
--- a/resources/libraries/bash/entry/bootstrap_vpp_device.sh
+++ b/resources/libraries/bash/entry/bootstrap_vpp_device.sh
@@ -44,9 +44,6 @@ archive_tests || die
 prepare_topology || die
 select_topology || die
 activate_docker_topology || die
-select_tags || die
-compose_robot_arguments || die
-set_environment_variables || die
 run_robot || die
 move_archives || die
 die_on_robot_error || die
diff --git a/resources/libraries/bash/entry/per_patch_device.sh b/resources/libraries/bash/entry/per_patch_device.sh
index 950a52209d..88d7f13494 100644
--- a/resources/libraries/bash/entry/per_patch_device.sh
+++ b/resources/libraries/bash/entry/per_patch_device.sh
@@ -39,14 +39,15 @@ source "${BASH_FUNCTION_DIR}/per_patch.sh" || die "Source failed."
 common_dirs || die
 check_prerequisites || die
 set_perpatch_vpp_dir || die
-build_vpp_ubuntu_amd64 "CURRENT" || die
-set_aside_commit_build_artifacts || die
-initialize_csit_dirs || die
+git status || die
+git describe || die
+build_vpp_ubuntu "CURRENT" || die
+initialize_csit_dirs "current" || die
 get_test_code "${1-}" || die
 get_test_tag_string || die
 set_perpatch_dut || die
 select_arch_os || die
-select_build "build_current" || die
+select_build "build-root" || die
 check_download_dir || die
 activate_virtualenv "${VPP_DIR}" || die
 generate_tests || die
@@ -54,8 +55,6 @@ archive_tests || die
 prepare_topology || die
 select_topology || die
 activate_docker_topology || die
-select_tags || die
-compose_robot_arguments || die
 run_robot || die
 move_archives || die
 die_on_robot_error || die
diff --git a/resources/libraries/bash/entry/per_patch_perf.sh b/resources/libraries/bash/entry/per_patch_perf.sh
index aba899689e..9a825a007e 100644
--- a/resources/libraries/bash/entry/per_patch_perf.sh
+++ b/resources/libraries/bash/entry/per_patch_perf.sh
@@ -44,11 +44,16 @@ source "${BASH_FUNCTION_DIR}/ansible.sh" || die "Source failed."
 common_dirs || die
 check_prerequisites || die
 set_perpatch_vpp_dir || die
-build_vpp_ubuntu_amd64 "CURRENT" || die
-set_aside_commit_build_artifacts || die
-build_vpp_ubuntu_amd64 "PARENT" || die
-set_aside_parent_build_artifacts || die
-initialize_csit_dirs || die
+git status || die
+git describe || die
+build_vpp_ubuntu "CURRENT" || die
+set_aside_build_artifacts "current" || die
+git checkout "HEAD~" || die "Failed to checkout parent commit."
+git status || die
+git describe || die
+build_vpp_ubuntu "PARENT" || die
+set_aside_build_artifacts "parent" || die
+initialize_csit_dirs "parent" "current" || die
 get_test_code "${1-}" || die
 get_test_tag_string || die
 set_perpatch_dut || die
@@ -58,9 +63,6 @@ activate_virtualenv "${VPP_DIR}" || die
 generate_tests || die
 archive_tests || die
 reserve_and_cleanup_testbed || die
-select_tags || die
-compose_robot_arguments || die
-set_environment_variables || die
 # Support for interleaved measurements is kept for future.
 iterations=1 # 8
 for ((iter=0; iter<iterations; iter++)); do
@@ -73,13 +75,13 @@ for ((iter=0; iter<iterations; iter++)); do
     select_build "build_current" || die
     check_download_dir || die
     run_robot || die
-    archive_test_results "csit_current/${iter}" || die
+    move_test_results "csit_current/${iter}" || die
     # TODO: Use less heavy way to avoid apt remove failures.
     ansible_playbook "cleanup" || die
     select_build "build_parent" || die
     check_download_dir || die
     run_robot || die
-    archive_test_results "csit_parent/${iter}" || die
+    move_test_results "csit_parent/${iter}" || die
 done
 untrap_and_unreserve_testbed || die
 compare_test_results  # The error code becomes this script's error code.
diff --git a/resources/libraries/bash/function/common.sh b/resources/libraries/bash/function/common.sh
index acd3a4be0b..44149ca6e1 100644
--- a/resources/libraries/bash/function/common.sh
+++ b/resources/libraries/bash/function/common.sh
@@ -93,6 +93,12 @@ function activate_docker_topology () {
     cat ${WORKING_TOPOLOGY} | grep -v password || {
         die "Topology read failed!"
     }
+
+    # Subfunctions to update data that may depend on topology reserved.
+    set_environment_variables || die
+    select_tags || die
+    compose_robot_arguments || die
+
 }
 
 
@@ -257,6 +263,10 @@ function common_dirs () {
 
 function compose_robot_arguments () {
 
+    # This function is called by run_tests function.
+    # The reason is that some jobs (bisect) perform reservation multiple times,
+    # so WORKING_TOPOLOGY can be different each time.
+    #
     # Variables read:
     # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed.
     # - DUT - CSIT test/ subdirectory, set while processing tags.
@@ -274,6 +284,8 @@ function compose_robot_arguments () {
     ROBOT_ARGS=("--loglevel" "TRACE")
     ROBOT_ARGS+=("--variable" "TOPOLOGY_PATH:${WORKING_TOPOLOGY}")
 
+    # TODO: The rest does not need to be recomputed on each reservation.
+    #       Refactor TEST_CODE so this part can be called only once.
     case "${TEST_CODE}" in
         *"device"*)
             ROBOT_ARGS+=("--suite" "tests.${DUT}.device")
@@ -717,6 +729,9 @@ function reserve_and_cleanup_testbed () {
     # When cleanup fails, remove from topologies and keep retrying
     # until all topologies are removed.
     #
+    # Multiple other functions are called from here,
+    # as they set variables that depend on reserved topology data.
+    #
     # Variables read:
     # - TOPOLOGIES - Array of paths to topology yaml to attempt reservation on.
     # - PYTHON_SCRIPTS_DIR - Path to directory holding the reservation script.
@@ -785,6 +800,11 @@ function reserve_and_cleanup_testbed () {
         echo "Sleeping ${sleep_time}"
         sleep "${sleep_time}" || die "Sleep failed."
     done
+
+    # Subfunctions to update data that may depend on topology reserved.
+    set_environment_variables || die
+    select_tags || die
+    compose_robot_arguments || die
 }
 
 
@@ -792,12 +812,23 @@ function run_robot () {
 
     # Run robot with options based on input variables.
     #
+    # Testbed has to be reserved already,
+    # as some data may have changed between reservations,
+    # for example excluded NICs.
+    #
     # Variables read:
     # - CSIT_DIR - Path to existing root of local CSIT git repository.
     # - ARCHIVE_DIR - Path to store robot result files in.
     # - ROBOT_ARGS, EXPANDED_TAGS - See compose_robot_arguments.sh
     # - GENERATED_DIR - Tests are assumed to be generated under there.
+    # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed.
+    # - DUT - CSIT test/ subdirectory, set while processing tags.
+    # - TAGS - Array variable holding selected tag boolean expressions.
+    # - TOPOLOGIES_TAGS - Tag boolean expression filtering tests for topology.
+    # - TEST_CODE - The test selection string from environment or argument.
     # Variables set:
+    # - ROBOT_ARGS - String holding part of all arguments for robot.
+    # - EXPANDED_TAGS - Array of string robot arguments compiled from tags.
     # - ROBOT_EXIT_STATUS - Exit status of most recent robot invocation.
     # Functions called:
     # - die - Print to stderr and exit.
@@ -864,6 +895,9 @@ function select_arch_os () {
 
 function select_tags () {
 
+    # Only to be called from the reservation function,
+    # as resulting tags may change based on topology data.
+    #
     # Variables read:
     # - WORKING_TOPOLOGY - Path to topology yaml file of the reserved testbed.
     # - TEST_CODE - String affecting test selection, usually jenkins job name.
@@ -1276,6 +1310,9 @@ function set_environment_variables () {
     # Depending on testbed topology, overwrite defaults set in the
     # resources/libraries/python/Constants.py file
     #
+    # Only to be called from the reservation function,
+    # as resulting values may change based on topology data.
+    #
     # Variables read:
     # - TEST_CODE - String affecting test selection, usually jenkins job name.
     # Variables set:
diff --git a/resources/libraries/bash/function/per_patch.sh b/resources/libraries/bash/function/per_patch.sh
index cfd22e7826..b9680a1560 100644
--- a/resources/libraries/bash/function/per_patch.sh
+++ b/resources/libraries/bash/function/per_patch.sh
@@ -18,40 +18,14 @@ set -exuo pipefail
 # Generally, the functions assume "common.sh" library has been sourced already.
 # Keep functions ordered alphabetically, please.
 
-function archive_test_results () {
 
-    # Arguments:
-    # - ${1}: Directory to archive to. Required. Parent has to exist.
-    # Variable set:
-    # - TARGET - Target directory.
-    # Variables read:
-    # - ARCHIVE_DIR - Path to where robot result files are created in.
-    # - VPP_DIR - Path to existing directory, root for to relative paths.
-    # Directories updated:
-    # - ${1} - Created, and robot and parsing files are moved/created there.
-    # 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."
-    file_list=("output.xml" "log.html" "report.html" "tests")
-    for filename in "${file_list[@]}"; do
-        mv "${ARCHIVE_DIR}/${filename}" "${TARGET}/${filename}" || {
-            die "Attempt to move '${filename}' failed."
-        }
-    done
-}
-
-
-function build_vpp_ubuntu_amd64 () {
+function build_vpp_ubuntu () {
 
     # This function is using make pkg-verify 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
     # mechanics they will not be blocked by CSIT repo.
+    #
     # Arguments:
     # - ${1} - String identifier for echo, can be unset.
     # Variables read:
@@ -115,20 +89,50 @@ function initialize_csit_dirs () {
     # Variables read:
     # - VPP_DIR - Path to WORKSPACE, parent of created directories.
     # Directories created:
-    # - csit_current - Holding test results of the patch under test (PUT).
-    # - csit_parent - Holding test results of parent of PUT.
+    # - csit_{part} - See the caller what it is used for.
     # 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."
-    }
-    mkdir -p "csit_current" "csit_parent" || {
-        die "Directory creation failed."
-    }
+    while true; do
+        if [[ ${#} < 1 ]]; then
+            # All directories created.
+            break
+        fi
+        name_part="${1}" || die
+        shift || die
+        dir_name="csit_${name_part}" || die
+        rm -rf "${dir_name}" || die "Directory deletion failed."
+        mkdir -p "${dir_name}" || die "Directory creation failed."
+    done
+}
+
+
+function move_test_results () {
+
+    # Arguments:
+    # - ${1}: Directory to archive to. Required. Parent has to exist.
+    # Variable set:
+    # - TARGET - Target archival directory, equivalent to the argument.
+    # Variables read:
+    # - ARCHIVE_DIR - Path to where robot result files are created in.
+    # - VPP_DIR - Path to existing directory, root for to relative paths.
+    # Directories updated:
+    # - ${1} - Created, and robot and parsing files are moved/created there.
+    # 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."
+    file_list=("output.xml" "log.html" "report.html" "tests")
+    for filename in "${file_list[@]}"; do
+        mv "${ARCHIVE_DIR}/${filename}" "${TARGET}/${filename}" || die
+    done
 }
 
 
@@ -157,56 +161,37 @@ function select_build () {
 }
 
 
-function set_aside_commit_build_artifacts () {
+function set_aside_build_artifacts () {
 
-    # Function is copying VPP built artifacts from actual checkout commit for
-    # further use and clean git.
+    # Function used to save VPP .deb artifacts from currently finished build.
+    #
+    # After the artifacts are copied to the target directory,
+    # the main git tree is cleaned up to not interfere with next build.
+    #
+    # Arguments:
+    # - ${1} - String to derive the target directory name from. Required.
     # Variables read:
     # - VPP_DIR - Path to existing directory, parent to accessed directories.
     # Directories read:
     # - build-root - Existing directory with built VPP artifacts (also DPDK).
     # Directories updated:
     # - ${VPP_DIR} - A local git repository, parent commit gets checked out.
-    # - build_current - Old contents removed, content of build-root copied here.
+    # - build_${1} - Old contents removed, content of build-root copied here.
     # 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."
-    mv "build-root"/*".deb" "build_current"/ || die "Move operation failed."
+    dir_name="build_${1}" || die
+    rm -rf "${dir_name}" || die "Remove operation failed."
+    mkdir -p "${dir_name}" || die "Directory creation failed."
+    mv "build-root"/*".deb" "${dir_name}"/ || die "Move operation failed."
     # The previous build could have left some incompatible leftovers,
     # e.g. DPDK artifacts of different version (in build/external).
     # Also, there usually is a copy of dpdk artifact in build-root.
     git clean -dffx "build"/ "build-root"/ || die "Git clean operation failed."
-    # Finally, check out the parent commit.
-    git checkout HEAD~ || die "Git checkout operation failed."
-    # Display any other leftovers.
-    git status || die "Git status operation failed."
-}
-
-
-function set_aside_parent_build_artifacts () {
-
-    # Function is copying VPP built artifacts from parent checkout commit for
-    # further use. Checkout to parent is not part of this function.
-    # Variables read:
-    # - VPP_DIR - Path to existing directory, parent of accessed directories.
-    # Directories read:
-    # - build-root - Existing directory with built VPP artifacts (also DPDK).
-    # Directories updated:
-    # - build_parent - Old directory removed, build-root debs moved here.
-    # 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."
-    mv "build-root"/*".deb" "build_parent"/ || die "Move operation failed."
+    git status || die
 }
 
 
-- 
2.16.6