Enable staging repos when DNF_CONTENTDIR is specified #169

2023-03-30T20:29:03Z

akatch commented

2023-03-30 20:29:03 +00:00

(Migrated from github.com)

This PR adds no new functionality but re-enables some logic to enable staging repositories when DNF_CONTENTDIR is supplied.

tcooper (Migrated from github.com) reviewed 2023-03-30 20:29:03 +00:00

lumarel (Migrated from github.com) reviewed 2023-03-30 20:29:03 +00:00

AlanMarshall (Migrated from github.com) reviewed 2023-03-31 14:50:37 +00:00

AlanMarshall (Migrated from github.com) left a comment

I have tested this using:

openqa-cli api -X POST isos ISO=Rocky-8.8-20230330.t.2-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.8 CURRREL=8 DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://download.rockylinux.org/stg/rocky/8.8-BETA/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.8-beta-boot-iso

Result: Both tests fail in _staging_repos_enable.pm line 11

Test incomplete:
Reason: backend died: No map for '' at /usr/lib/os-autoinst/consoles/VNC.pm line 660.

I have tested this using: ``` openqa-cli api -X POST isos ISO=Rocky-8.8-20230330.t.2-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.8 CURRREL=8 DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://download.rockylinux.org/stg/rocky/8.8-BETA/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.8-beta-boot-iso ``` Result: Both tests fail in _staging_repos_enable.pm line 11 Test incomplete: Reason: backend died: No map for '' at /usr/lib/os-autoinst/consoles/VNC.pm line 660.

AlanMarshall commented

2023-04-01 13:56:15 +00:00

(Migrated from github.com)

This is not looking good. I'm getting exact same incomplete tests with 8.7 stg.
Tested using:

openqa-cli api -X POST isos ISO=Rocky-8.7-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.7 CURRREL=8 DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://dl.rockylinux.org/stg/rocky/8.7/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.7-stg-boot-iso

Investigation is continuing...

This is not looking good. I'm getting exact same incomplete tests with 8.7 stg. Tested using: ``` openqa-cli api -X POST isos ISO=Rocky-8.7-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.7 CURRREL=8 DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://dl.rockylinux.org/stg/rocky/8.7/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.7-stg-boot-iso ``` Investigation is continuing...

tcooper commented

2023-04-02 23:47:55 +00:00

(Migrated from github.com)

@AlanMarshall Don't spend more time debugging this error issue other than to simply solve a puzzle. I forgot how this contentdir change is implemented and this PR isn't required after all. @akatch please leave this PR open until we discuss as it serves as a good example of how to test this in the production instance.

For reference (unless they get deleted) here are two tests of interest...

...and the code that is currently in place to use supplied contentdir without the need to run tests/_staging_repos_enable.pm which bombs terribly.

Likely a better PR will remove the conditional from main.pm and also remove tests/_staging_repos_enable.pm and tests/_staging_repos_disable.pm after verifying they are dead code paths.

@AlanMarshall Don't spend more time debugging this error issue other than to simply solve a puzzle. I forgot how this contentdir change is implemented and this PR isn't required after all. @akatch please leave this PR open until we discuss as it serves as a good example of how to test this in the production instance. For reference (unless they get deleted) here are two tests of interest... - https://openqa.rockylinux.org/tests/17301 - https://openqa.rockylinux.org/tests/17302 ...and [the code that is currently in place to use supplied `contentdir`](https://github.com/akatch/os-autoinst-distri-rocky/blob/enable_contentdir/tests/_do_install_and_reboot.pm#L143) without the need to run `tests/_staging_repos_enable.pm` which bombs terribly. Likely a better PR will remove the conditional from `main.pm` and also remove `tests/_staging_repos_enable.pm` and `tests/_staging_repos_disable.pm` after verifying they are dead code paths.

AlanMarshall commented

2023-04-03 00:49:50 +00:00

(Migrated from github.com)

Yes, thanks @tcooper from tests I am well on the way to a very similar conclusion and was about to report as such.

👍 1

AlanMarshall commented

2023-04-03 11:50:33 +00:00

(Migrated from github.com)

@tcooper I confirm that your results are exactly what my tests showed. The failure in script_run sed -i... was due to having no map for key #0001 which is probably ascii SOH. The failure occurs only after reboot when sed (or rsync) try to replace /etc/yum.repos.d/Rocky-* and does not occur when the same commands are run at the end of the install phase. However, /etc/yum.repos.d/Rocky-* can be read after reboot without any failure occurring.
@akatch I found that having a modified tests/_staging_repos_enable.pm was very useful in working out what was going on here and I therefore suggest retaining and extending PR #169 to provide an ongoing debug facility that only operates when the stg repos are being used via DNF_CONTENTDIR=stg. After all we have this existing code that can be modified and used to advantage for development rather than just deleting it as redundant.
Does this seem reasonable to everyone?

@tcooper I confirm that your results are exactly what my tests showed. The failure in ```script_run sed -i...``` was due to having no map for key ```#0001``` which is probably ascii SOH. The failure occurs only after reboot when sed (or rsync) try to replace ```/etc/yum.repos.d/Rocky-*``` and does not occur when the same commands are run at the end of the install phase. However, ```/etc/yum.repos.d/Rocky-*``` can be read after reboot without any failure occurring. @akatch I found that having a modified ```tests/_staging_repos_enable.pm``` was very useful in working out what was going on here and I therefore suggest retaining and extending PR #169 to provide an ongoing debug facility that only operates when the stg repos are being used via ```DNF_CONTENTDIR=stg```. After all we have this existing code that can be modified and used to advantage for development rather than just deleting it as redundant. Does this seem reasonable to everyone?

AlanMarshall commented

2023-04-04 11:21:41 +00:00

(Migrated from github.com)

As a suggestion, this is what I'm thinking of for now:

use base "installedtest";
use strict;
use testapi;
use utils;

sub run {
    my $self = shift;
    $self->root_console(tty => 4);
    script_run 'ls -alh /etc/yum.repos.d/';
    if (get_version_major() < 9) {
        script_run 'cat /etc/yum.repos.d/Rocky-BaseOS.repo';
    } else {
        script_run 'cat /etc/yum.repos.d/rocky-BaseOS.repo';
    }
    # Check dnf pointing at staging repos
    script_run 'cat /etc/dnf/vars/contentdir';
    script_run 'dnf clean all';
    # Check enabled repos
    script_run 'dnf repoinfo';
}

sub test_flags {
    return {fatal => 1};
}

The results appear as a row in the openqa run details page.
The dnf repoinfo result has identified the postinstall enabled repos as including:
http://dl.rockylinux.org/stg/rocky/8/BaseOS/x86_64/os/

and similar for AppStream and extras whereas the enabled repos should be:
http://dl.rockylinux.org/stg/rocky/8.8-Beta/BaseOS/x86_64/os/

I tried various combinations of VERSION=8.8-Beta and CURREL=8 with same result. Maybe there is another parameter that I haven't discovered yet... If not then this is a bug that needs fixing.
One of the commands that I tried was:

openqa-cli api -X POST isos ISO=Rocky-8.8-20230330.t.2-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.8-Beta CURREL=8.8-Beta DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://dl.rockylinux.org/stg/rocky/8.8-Beta/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.8-beta-boot-iso

Result always: http://dl.rockylinux.org/stg/rocky/8/BaseOS/x86_64/os/

I would also rename _staging_repos_enable.pm to something more appropriate.

As a suggestion, this is what I'm thinking of for now: ``` use base "installedtest"; use strict; use testapi; use utils; sub run { my $self = shift; $self->root_console(tty => 4); script_run 'ls -alh /etc/yum.repos.d/'; if (get_version_major() < 9) { script_run 'cat /etc/yum.repos.d/Rocky-BaseOS.repo'; } else { script_run 'cat /etc/yum.repos.d/rocky-BaseOS.repo'; } # Check dnf pointing at staging repos script_run 'cat /etc/dnf/vars/contentdir'; script_run 'dnf clean all'; # Check enabled repos script_run 'dnf repoinfo'; } sub test_flags { return {fatal => 1}; } ``` The results appear as a row in the openqa run details page. The ```dnf repoinfo``` result has identified the postinstall enabled repos as including: ```http://dl.rockylinux.org/stg/rocky/8/BaseOS/x86_64/os/``` and similar for ```AppStream``` and ```extras``` whereas the enabled repos should be: ```http://dl.rockylinux.org/stg/rocky/8.8-Beta/BaseOS/x86_64/os/``` I tried various combinations of VERSION=8.8-Beta and CURREL=8 with same result. Maybe there is another parameter that I haven't discovered yet... If not then this is a bug that needs fixing. One of the commands that I tried was: ``` openqa-cli api -X POST isos ISO=Rocky-8.8-20230330.t.2-x86_64-boot.iso ARCH=x86_64 DISTRI=rocky FLAVOR=boot-iso VERSION=8.8-Beta CURREL=8.8-Beta DNF_CONTENTDIR=stg GRUBADD=inst.repo=https://dl.rockylinux.org/stg/rocky/8.8-Beta/BaseOS/x86_64/os BUILD=-"$(date +%Y%m%d.%H%M%S).0"-8.8-beta-boot-iso ``` Result always: ```http://dl.rockylinux.org/stg/rocky/8/BaseOS/x86_64/os/``` I would also rename ```_staging_repos_enable.pm``` to something more appropriate.

tcooper commented

2023-04-04 14:56:21 +00:00

(Migrated from github.com)

@AlanMarshall I like this in principle. My suggestions...

I believe if you change the commands to assert_script_run("command | grep <expected_content}") the test can retain it's pass / fail nature.
Since we need the Rocky repos to be configured to stg/rocky and not mirrorlist type we should also double-check that to help us catch early any change in the mirrorlist format that sed pattern doesn't work for (unexpected but possible).
I prefer a hard fail here over a soft fail but will defer to the group.
I agree file should be renamed (replaced) to avoid confusion of the intent. Maybe... _staging_repos_verify.pm? Note a change in main.pm is also required if the test name is changed.

Good work 👍🏻

@AlanMarshall I like this in principle. My suggestions... - I believe if you change the commands to `assert_script_run("command | grep <expected_content}")` the test can retain it's pass / fail nature. - Since we need the Rocky repos to be configured to `stg/rocky` **and** not mirrorlist type we should also double-check that to help us catch early any change in the mirrorlist format that `sed` pattern doesn't work for (unexpected but possible). - I prefer a hard fail here over a soft fail but will defer to the group. - I agree file should be renamed (replaced) to avoid confusion of the intent. Maybe... `_staging_repos_verify.pm`? Note a change in `main.pm` is also required if the test name is changed. Good work 👍🏻

AlanMarshall commented

2023-04-04 15:00:57 +00:00

(Migrated from github.com)

@tcooper Thanks Trevor. I've just found a fix for the bug in the postinstall repos that I referred to earlier. I'll give you the details soon. It's a serious bug so needs addressed sooner rather than later. I'll PR when I get it all together.
The bug is that at the moment, postinstall, the repos are configured to stg/rocky/8 whereas they should be to stg/rocky/8.8-BETA as Louis defined a few days ago. I now have a fix for it.

@tcooper Thanks Trevor. I've just found a fix for the bug in the postinstall repos that I referred to earlier. I'll give you the details soon. It's a serious bug so needs addressed sooner rather than later. I'll PR when I get it all together. The bug is that at the moment, postinstall, the repos are configured to stg/rocky/8 whereas they should be to stg/rocky/8.8-BETA as Louis defined a few days ago. I now have a fix for it.