Project

General

Profile

Actions

action #130096

closed

openqa-clone-custom-git-refspec modifies PRODUCTDIR setting

Added by rainerkoenig 11 months ago. Updated 11 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Feature requests
Target version:
Start date:
2023-05-30
Due date:
% Done:

0%

Estimated time:

Description

Observation

Tried to run a VR with the following command

$ openqa-clone-custom-git-refspec https://github.com/rakoenig/os-autoinst-distri-opensuse/tree/yast_cc_container https://openqa.opensuse.org/tests/3313986

This leds to a failure in the cloned job:

[2023-05-30T11:15:42.330782+02:00] [warn] [pid:32320] !!! main: PRODUCTDIR 'os-autoinst-distri-opensuseproducts/alp' invalid, could not be found at /usr/lib/os-autoinst/OpenQA/Isotovideo/Utils.pm line 269.

So I modified the script to ouput the cmd issued at the end and see what happens to PRODUCTDIR on varius clone (dry runs):

test url old_product_dir productdir
https://openqa.opensuse.org/tests/3313986 "products/alp" "os-autoinst-distri-opensuseproducts/alp"
https://openqa.opensuse.org/tests/3323239 "opensuse/products/opensuse" "os-autoinst-distri-opensuse/products/opensuse"
https://openqa.suse.de/tests/11215764 "sle/products/sle" os-autoinst-distri-opensuse/products/sle"

So in the first attempt we run into that error because we're missing a / after os-autoinst-distri-opensuse. Nevertheless: The resulting productdir setting is never the same as it is stored in the vars.json of the original job.

Additinal info

The lines to be investigated start here:

        local old_productdir
        old_productdir=$(echo "$json_data" | jq -r '.PRODUCTDIR') || throw_json_error "$json_url" "$json_data"
        local old_casedir
        old_casedir=$(echo "$json_data" | jq -r '.CASEDIR') || throw_json_error "$json_url" "$json_data"
        if [[ ${old_productdir:0:1} == "/" ]]; then
            local productdir="${productdir:-"${repo_name##*/}${old_productdir##"$old_casedir"}"}"
        else
            local productdir="${productdir:-"${repo_name##*/}${old_productdir#*"${old_casedir##*/}"}"}"
        fi

The documentation for PRODUCTDIR says:

Absolute or relative path to the optional "product directory" which includes the test schedule entry point main.pm as well as a needles subdirectory with the needles to load. Defaults to CASEDIR. Relative paths are considered relative to CASEDIR.

So it looks like if [[ ${old_productdir:0:1} == "/" ]]; then tries to distinguish between absolute and relative paths. I just don't understand the use of it. We're cloning from an existing job that did this already and so from my understanding it would be enough just to copy the PRODUCTDIR from the original job.

Even a line like local productdir="${productdir:-"${repo_name##*/}${old_productdir#*"${old_casedir##*/}"}"}" does not seem to make sens because the initial check ${productdir:- will result in false, since we just declared productdir as a local setting and so it should be empty, isn't it?

Proposed action

Think about the argements above and fix it in a way that does not make cloned tests fail.

Actions #1

Updated by livdywan 11 months ago

  • Category set to Feature requests
  • Target version set to future

I assume it's not super critical so I'm not adding it to the backlog right away.

Actions #2

Updated by okurz 11 months ago

  • Target version changed from future to Ready

We can try to address it now

Actions #3

Updated by okurz 11 months ago

  • Assignee set to okurz
Actions #4

Updated by okurz 11 months ago

rainerkoenig wrote:

So it looks like if [[ ${old_productdir:0:1} == "/" ]]; then tries to distinguish between absolute and relative paths. I just don't understand the use of it. We're cloning from an existing job that did this already and so from my understanding it would be enough just to copy the PRODUCTDIR from the original job.

  1. No, we need to change productdir to refer to the git checked out version. the purpose of overwriting PRODUCTDIR is that we actually load the schedule from the git checked out test distribution, not the one from /var/lib/openqa/share/tests . 2. Since I created the script 5 years ago behaviour in openQA has changed significantly but I think we actually still need this line.

Even a line like local productdir="${productdir:-"${repo_name##*/}${old_productdir#*"${old_casedir##*/}"}"}" does not seem to make sens because the initial check ${productdir:- will result in false, since we just declared productdir as a local setting and so it should be empty, isn't it?

The inner ${productdir:- gets the value from a variable $productdir from an outer scope if defined, yielding an empty string otherwise. The local productdir is a new locally scoped variable shadowing the potentially already defined outer scope variable.

I just tried it out to be sure with a script

foo=42
f() {
    local foo=${foo:-43}
    echo "foo in f(): $foo"
}
f
echo "foo in outer scope: $foo"
foo=
f
echo "foo in outer scope undefined: $foo"

which yields

foo in f(): 42
foo in outer scope: 42
foo in f(): 43
foo in outer scope undefined:

as expected where the first invocation of f() assigns the outer scope value 42 to the local variable of same name. The second output line shows that the value in outer scope is still unaltered 42. After unassigning foo the function assigns the fallback value of 43 and shows that in the output while in the outer scope the variable has no value as shown by the last line of output.

Another important point is that PRODUCTDIR is never specified in settings and shouldn't be specified because isotovideo tries to look for main.pm in "$casedir/products/$distri" if not found in "$casedir". The interesting part is why that comes to a different conclusion for alp.

I don't understand which could would compute a product dir of "products/alp". In openQA OpenQA::Utils::productdir we have

 146     my $dir = testcasedir($distri, $version, $rootfortests);
 147     return $dir . "/products/$distri" if -e "$dir/products/$distri";

so I guess line 147 is not effective here as we have a string that starts with no "/". And os-autoinst doesn't know anything about the name part "products/", it just assumes that the main.pm is in PRODUCTDIR and PRODUCTDIR is set to CASEDIR if not set.

I added an extra debug worker instance on o3 worker openqaworker7, instance 42 and triggered special tests:

openqa-clone-job --within-instance https://openqa.opensuse.org/tests/3313986 TEST+=okurz-poo130096 _GROUP=0 BUILD= WORKER_CLASS=okurz_debug _EXIT_AFTER_SCHEDULE=1
1 job has been created:
 - alp-bedrock-0.1-Default-x86_64-Build5.1-yast_1@uefi -> https://openqa.opensuse.org/tests/3340338

And with some custom debugging logs from the worker start:

Jun 06 10:56:29 openqaworker7 worker[28823]: PRODUCTDIR before:  at /usr/share/openqa/script/../lib/OpenQA/Worker/Engines/isotovideo.pm line 398.
evaluated default productdir: /var/lib/openqa/cache/openqa1-opensuse/tests/alp/products/alp at /usr/share/openqa/script/../lib/OpenQA/Worker/Engines/isotovideo.pm line 400.
casedir ne target_name, PRODUCTDIR before:  at /usr/share/openqa/script/../lib/OpenQA/Worker/Engines/isotovideo.pm line 411.
casedir ne target_name, PRODUCTDIR after products/alp at /usr/share/openqa/script/../lib/OpenQA/Worker/Engines/isotovideo.pm line 414.

The relevant code of concern seems to be:

$vars->{PRODUCTDIR} //= $absolute_paths && !$has_custom_dir ? $default_productdir : abs2rel($default_productdir, $default_casedir);

for the second branch when product dir is not set so effectively:

$vars->{PRODUCTDIR} = abs2rel($default_productdir, $default_casedir);

in the specific case:

abs2rel('/var/lib/openqa/cache/openqa1-opensuse/tests/alp/products/alp', '/var/lib/openqa/cache/openqa1-opensuse/tests/alp') -> `products/alp`

however for a standard opensuse tests when also using a custom git casedir that happens to be the same:

openqa-clone-job --within-instance https://openqa.opensuse.org/tests/3323239 TEST+=okurz-poo130096 _GROUP=0 BUILD= WORKER_CLASS=okurz_debug _EXIT_AFTER_SCHEDULE=1 _SKIP_CACHE_SYNC_TESTS=1 HDD_1= HDD_2= CASEDIR=https://github.com/os-autoinst/os-autoinst-distri-opensuse/

yielding:

casedir ne target_name, abs2rel(/var/lib/openqa/cache/openqa1-opensuse/tests/opensuse/products/opensuse, /var/lib/openqa/cache/openqa1-opensuse/tests/opensuse): products/opensuse

so I assume everything is actually fine regarding the generation of product dir in openQA worker and we should try to fix the concatenation in openqa-clone-custom-git-refspec

I tried out what a correct product dir should look like. I called

bash -ex $(which openqa-clone-custom-git-refspec) -n https://github.com/rakoenig/os-autoinst-distri-opensuse/tree/yast_cc_container https://openqa.opensuse.org/tests/3313986

and took over the openqa-clone-job command line and only added a slash in productdir and scheduled with WORKER_CLASS=okurz_debug so that I could monitor how the pool dir looks like:

openqa-clone-job --skip-chained-deps --parental-inheritance --within-instance https://openqa.opensuse.org 3313986 _GROUP=0 TEST+=@rakoenig/os-autoinst-distri-opensuse#yast_cc_container BUILD=rakoenig/os-autoinst-distri-opensuse#yast_cc_container CASEDIR=https://github.com/rakoenig/os-autoinst-distri-opensuse.git#yast_cc_container PRODUCTDIR=os-autoinst-distri-opensuse/products/alp NEEDLES_DIR=needles WORKER_CLASS=okurz_debug

https://openqa.opensuse.org/tests/3340773 looks like it could properly find the schedule file.

Actions #5

Updated by okurz 11 months ago

  • Due date set to 2023-06-20
  • Status changed from New to Feedback
Actions #6

Updated by okurz 11 months ago

https://github.com/os-autoinst/openQA/pull/5192 merged.

@rainerkoenig can you try if the new version of openqa-clone-custom-git-refspec fixes tthe problem as you observed them? You can download the script directly from https://raw.githubusercontent.com/os-autoinst/openQA/master/script/openqa-clone-custom-git-refspec or wait some more minutes to have the package ready from devel:openQA or some days to have the updated package in Tumbleweed :)

Actions #7

Updated by okurz 11 months ago

  • Due date deleted (2023-06-20)
  • Status changed from Feedback to Resolved

No response, assuming resolved

Actions

Also available in: Atom PDF