canonical-ubuntu-qa team mailing list archive

[Paride Legovini <mp+445759@xxxxxxxxxxxxxxxxxx>]

I had a very cursory look at half of the script. Maybe I should know what problem it's trying to solve before actually reviewing.

I'm worried about the pollution of the keypair database.

Diff comments:

> diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-instances-common.sh b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-instances-common.sh
> new file mode 100644
> index 0000000..2046ad8
> --- /dev/null
> +++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-instances-common.sh
> @@ -0,0 +1,154 @@
> +#!/bin/bash
> +
> +# Common functions shared by scripts in this directory which create openstack instances
> +
> +get_random_string(){
> +    ran=$(echo $RANDOM | md5sum | head -c 20)

Why bother with md5sum? They will remain 32768 "random" strings.

> +    printf "%s" "${ran}"
> +}
> +
> +
> +make_and_load_key(){
> +    # $1 rc file
> +    # $2 name prefix
> +    key_id="${2}-key-$(get_random_string)"
> +    if ! openstack keypair list | grep "${key_id}" &>/dev/null; then

Not great to grep output meant for humans. Better to check the RC of `openstack keypair show` (or look for a way to get machine parseable output, in the specific case that would be --format).

> +            if ! openstack keypair create --public-key ~/.ssh/id_rsa.pub "${key_id}" &> /dev/null; then

Isn't this going to load many identical keys to openstack? Indeed we have many extra keys...

$ openstack keypair list -c Name
+-----------------------------------------------+
| Name                                          |
+-----------------------------------------------+
| mirror_speed_tester-key-083f5f1a6c9e41cc4e28  |
| mirror_speed_tester-key-18c1d57fbdd7b56b362e  |
| mirror_speed_tester-key-3695bdd04da08c33431c  |
| mirror_speed_tester-key-475358f828fae53741d3  |
| mirror_speed_tester-key-4e648da868bc9c46d14c  |
| mirror_speed_tester-key-59edfc702830b63dd2cb  |
| mirror_speed_tester-key-8168be5ef36a7eeb3a62  |
| mirror_speed_tester-key-945d41395fb58be39c07  |
| mirror_speed_tester-key-a053e245665de5d25a7d  |
| mirror_speed_tester-key-a645ee91b3feae5aa72d  |
| mirror_speed_tester-key-bcea594a2174115aeb4d  |
| testbed-juju-806ee7-stg-proposed-migration-77 |
| tim-test-key-37ce6a53e93c5e6193a8             |
| tim-test-key-44207b90100dc2119c53             |
| tim-test-key-865e3ab144d56fd02e68             |
| tim-test-key-d0f2e37d2855e5daf7ab             |
+-----------------------------------------------+

> +                    printf "keypair creation failed for %s, skipping" "${1}"
> +                    exit 1
> +            fi
> +    fi
> +    while ! openstack keypair list | grep "${key_id}" &>/dev/null; do

Poor openstack, let's at least sleep 1 somewhere.

retry(1) (from the 'retry' package) is handy in these cases.

> +            ctr=$((ctr+1))
> +            if [ $ctr -gt 9 ]; then
> +                    printf "key not loading for %s, exiting" "${1}"
> +                    exit 1
> +            fi
> +    done
> +    printf "%s" "${key_id}"
> +}
> +
> +get_arch(){
> +    # $1 rc file
> +    arch=$(echo "${1}" | awk -F[-.] '{print $2}')
> +    if [ -z "$arch" ]; then
> +            arch="amd64"
> +    elif [[ "${arch}" == "rc" ]]; then
> +            arch="amd64"
> +    fi
> +    printf "%s" "${arch}"
> +}
> +
> +get_image(){
> +    # $1 release
> +    # $2 arch
> +    image=$(openstack image list | grep "adt/ubuntu-${1}-${2}" | cut -d' ' -f2)
> +    printf "%s" "${image}"
> +}
> +
> +get_netid(){
> +    net=""
> +    if [[ "${OS_USERNAME}" =~ .*"stg".* ]]; then

-1 on this matching

> +            if openstack network list | grep 'net_stg_proposed-migration' &> /dev/null; then
> +                    net=$(openstack network list | grep 'net_stg_proposed-migration' | cut -d' ' -f2)
> +            elif openstack network list | grep 'net_stg-proposed-migration' &> /dev/null; then
> +                    net=$(openstack network list | grep 'net_stg-proposed-migration' | cut -d' ' -f2)
> +            else
> +                    echo "Network finding failure, skipping ${FILE}..."
> +                    exit 1
> +            fi
> +    elif [[ "${OS_USERNAME}" =~ .*"prod".* ]]; then
> +            net=$(openstack network list | grep 'net_prod-proposed-migration' | cut -d' ' -f2)
> +    fi
> +    printf "%s" "${net}"
> +}
> +
> +wait_for_server_to_build(){
> +    # $1 server name
> +    # $2 key
> +    while ! openstack server show "${1}" | grep status | grep ACTIVE &>/dev/null; do
> +            # echo "waiting for server to come up..."
> +            if openstack server show "${1}" | grep status | grep ERROR; then
> +                    openstack keypair delete "${key}"
> +                    echo "Server building failed, see below..."
> +                    openstack server show "${1}"
> +                    exit 1
> +            fi
> +            sleep 5
> +    done
> +}
> +
> +wait_for_server_connectivity(){
> +    # $1 server_name
> +    ip_address=$(openstack server list | grep "${1}" | cut -d'|' -f5 | cut -d'=' -f2)
> +    sleep 10
> +    while ! ssh -o StrictHostKeyChecking=no ubuntu@"${ip_address}" : &>/dev/null; do
> +            # echo "waiting for server to have connectivity..."
> +            sleep 3
> +    done
> +    printf "%s" "${ip_address}"
> +}
> +
> +create_server_and_wait_for_connectivity(){
> +    # maybe this changes and we just pass the file or something
> +    # need name prefix
> +    # new args
> +    # $1 release
> +    # $2 name_prefix
> +    # $3 rc file
> +    # $4 flavor (default is autopkgtest)
> +    # shellcheck disable=SC1090
> +    if ! . "${3}"; then
> +        printf "Sourcing file: %s failed, exiting...\n" "${3}"
> +        exit 1
> +    fi
> +
> +    key=$(make_and_load_key "${3}" "${2}")
> +    # printf "Key loaded: %s\n" "${key}"
> +    server_name="${2}-$(get_random_string)"
> +    # printf "Booting up instance %s for %s\n" "${server_name}" "${3}"
> +    if openstack server list | grep "${server_name}" &> /dev/null; then
> +        printf "Server %s already exists, exiting" "${server_name}"
> +        exit 1
> +    fi
> +    arch=$(get_arch "${3}")
> +    # printf "Architecture: %s\n" "${arch}"
> +    img=$(get_image "${1}" "${arch}")
> +    # printf "Using image: %s\n" "${img}"
> +    flavor="autopkgtest"
> +    if [ $# -gt 3 ]; then
> +        flavor="${4}"
> +    fi
> +    netid=$(get_netid)
> +    openstack server create --image "${img}" --flavor "${flavor}" --nic net-id="${netid}" -- "${server_name}" &> /dev/null
> +    wait_for_server_to_build "${server_name}" "${key}"
> +    # printf "Server built: %s\n" "${server_name}"
> +    ip=$(wait_for_server_connectivity "${server_name}")
> +    # printf "Server has connectivity: %s\n" "${server_name}"
> +    printf "%s|%s|%s" "${server_name}" "${key}" "${ip}"
> +}
> +
> +show_server_details(){
> +    # $1 server details
> +    name=$(printf "%s" "${1}" | cut -d'|' -f1)
> +    key=$(printf "%s" "${1}" | cut -d'|' -f2)
> +    ip=$(printf "%s" "${1}" | cut -d'|' -f3)
> +    printf "%s\n%s\n%s" "${name}" "${key}" "${ip}"
> +}
> +
> +get_server_ip(){
> +    # $1 server details
> +    printf "%s" "${1}" | cut -d'|' -f3
> +}
> +
> +kill_server(){
> +    # $1 server details
> +    # $2 file
> +    # shellcheck disable=SC1090
> +    source "${2}"
> +    printf "Killing server: \n%s" "$(show_server_details "${1}")"
> +    server_name=$(printf "%s" "${1}" | cut -d'|' -f1)
> +    key=$(printf "%s" "${1}" | cut -d'|' -f2)
> +    openstack server delete "${server_name}"
> +    openstack keypair delete "${key}"
> +}


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/445759
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:create-instances-refactor into autopkgtest-cloud:master.