Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't check for unreliable grub2 needle #4652

Merged
merged 1 commit into from
Mar 20, 2018
Merged

Conversation

kravciak
Copy link
Contributor

@kravciak kravciak commented Mar 19, 2018

Fix for: https://openqa.suse.de/tests/1554587
Local run: http://dhcp165.suse.cz/tests/2095#step/first_boot/2

  • problem was not triggered so it's just normal run

@@ -20,7 +20,11 @@ use caasp 'process_reboot';

sub run {
# On VMX images bootloader_uefi eats grub2 needle
assert_screen 'grub2' unless is_caasp('VMX');
if (is_caasp 'DVD') {
if (!check_screen('grub2')) {
Copy link
Member

@okurz okurz Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you probably know I recommend to always prevent the use of check_screen with non-zero timeout. Also, a record_soft_failure should normally be only used for product issue tracking. If you do not need this check and do not even abort the waiting time within the grub bootloader why not just delete the whole section and just wait in the next step for linux-login-casp? What Panos mentioned in https://progress.opensuse.org/issues/28648#note-6 is that he does not want to change the autoyast profile to disable the grub timeout. Maybe you want to rethink this? In the other tests we are quite happy with the decision to disable the grub timeout and always explicitly check for the grub screen :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understood it right, the installation/autoyast.pm is going to check also for the linux-login-casp needle among others. That's great because this needle is the final stage which means the system has been installed and booted. Well, in case this happens, it means that first_boot has already been tested by the installation/autoyast.pm which makes itself a bit useless, but I am fine with it. I guess this is why you need to exclude the grub needle from the first_boot.pm, because it's too late for that. This makes sense, and it should work in case openQA gets stalled again.

PS: As for the record_soft_failure I would agree with Oliver. Please feel free to use record_info('poo#28648', "Stall prevented matching of BIOS needle");.

Verification run for maintenance: http://skyrim.qam.suse.de/tests/overview?build=4652-kravciak&version=2.0&distri=caasp&groupid=98

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -133,6 +133,8 @@ sub run {
= qw(bios-boot nonexisting-package reboot-after-installation linuxrc-install-fail scc-invalid-url warning-pop-up inst-betawarning autoyast-boot);
push @needles, 'autoyast-confirm' if get_var('AUTOYAST_CONFIRM');
push @needles, 'autoyast-postpartscript' if get_var('USRSCR_DIALOG');
# grub2 needle check is unreliable (stalls during timeout) - poo#28648
push @needles, 'linux-login-casp' if is_caasp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwx788 can you check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure why we need it here at all as we return after first stage and this tag is not explicitly used anywhere. So I guess this change should not be here. It's not related to the problem at least and we can create needle with reboot-after-installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwx788 - we don't have reboot after installation 10 timeout on CaaSP, system just reboots. If there is stall then autoyast/installation.pm does not notice installation is finished (bios-boot needle disappears during stall)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do not fully understand how it helps. Is it enough just to put extra needle into the array? I mean we don't use this tag anywhere later in the module. And regarding reboot-after-installation we have 3 tags which work as a condition to exit installer loop and return in case of caasp. Could you please explain how this particular change is related?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally understood your point - thanks :) I added linux-login-casp to exit conditions, is it ok like this?

@kravciak kravciak force-pushed the grub2 branch 5 times, most recently from 2681daa to 8749f2f Compare March 20, 2018 11:24
@@ -149,6 +151,8 @@ sub run {
mouse_hide(1);
check_screen \@needles, $check_time;
until (match_has_tag('reboot-after-installation') || match_has_tag('bios-boot') || match_has_tag('autoyast-stage1-reboot-upcoming')) {
last if match_has_tag('linux-login-casp');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just put in the line above. Other than that let's think about more scalable solution, which we can introduce later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rwx788
Copy link
Member

rwx788 commented Mar 20, 2018

Let's try it out. Maybe disabling grub timeout in the profile could resolve this kind of issues (not the first_boot). WDYT?

@rwx788 rwx788 merged commit f23b1d1 into os-autoinst:master Mar 20, 2018
@kravciak
Copy link
Contributor Author

We don't have static profiles, we use profiles generated on admin dashboard (parallel job):

@rwx788
Copy link
Member

rwx788 commented Mar 20, 2018

We still can modify it on server side then, so it's possible. And that's why I'm not saying we should do it now. But we with sle15 we don't have a single static profile, as need to inject reg info, etc.

@kravciak kravciak deleted the grub2 branch March 28, 2018 14:42
@kravciak kravciak restored the grub2 branch March 28, 2018 14:42
@kravciak kravciak deleted the grub2 branch March 28, 2018 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants