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

Test yast2 kdump in unsupported environment #11723

Merged
merged 1 commit into from Jan 13, 2021

Conversation

mloviska
Copy link
Contributor

@mloviska mloviska commented Jan 11, 2021

assert_screen([qw(yast2-kdump-disabled yast2-kdump-enabled)], 200);
my @initial_tags = qw(yast2-kdump-disabled yast2-kdump-enabled);
push(@initial_tags,
(is_sle('>=15-sp3')) ? 'yast2-kdump-not-supported' : 'yast2-kdump-cannot-read-mem') if (is_xen_pv);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does version check makes some difference at this point? I see check on line 143, which has or logic, so i think you can push both needles to initial_tags without version check.

Copy link
Contributor Author

@mloviska mloviska Jan 11, 2021

Choose a reason for hiding this comment

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

Not supported pop-up has been introduced with newer version of yast2-kdump, currently present in sle15sp3 only.
YaST2 would continue with the initialization process and later on reports memory read error as expected. Thus, we have 2 different flows depending on package version.

Yup, I have chosen the easy way instead of doing version check of the yast2-kdump package, which I admit is not ideal.

@dgdavid Do you plan to backport environment detection and related warnings for older sles as well? Or IOW will the new package be available for older sle releases?

Copy link

Choose a reason for hiding this comment

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

Hi there!

@mloviska, I guess you mean to backport yast/yast-kdump#118 and yast/yast-yast2#1120. If so, I'm not sure if it is a good idea. I'll discuss with the team anyway to see if it is possible in a safe way (I mean, without introducing unexpected behaviors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey David!

Yup, exactly. As you mentioned it requires yast2 >= 4.3.45, thus not sure what are the upgrade plans for older release for YaST2. Ok, let's see :)

Thanks for your help!

Copy link

@dgdavid dgdavid Jan 11, 2021

Choose a reason for hiding this comment

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

Hi again!

Changes mentioned before (yast-kdump 4.3.3 and yast2 4.3.45) depends on a bigger one done in the core YaST package few months ago for SP3 too (see yast2 4.3.6) to fix the Xen Host/Guest detection while adapting storage-ng for not displaying the missing grub partition warning when running in a XEN guest.

For that reason, we think that backporting all of them could be a little bit risky and we wouldn't like to break anything relying in the previous Yast::Arch XEN detection in oldest SLE releases.

I really sorry.

Have a good day!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the contrary, you have assured us to keep clear separation between the flows. As we won't expect the new warning alert on older release hence older yast2 packages.

Thanks for clarification, have a nice day as well!

activate_kdump;
if (activate_kdump == 16) {
return 16;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

return 16 if activate_kdump == 16; could be better

YaST2 currently detects the environment and alerts the user in case there is an attempt to configure
kdump in Xen DomU system which is not supported.
Copy link
Contributor

@grisu48 grisu48 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@jlausuch jlausuch merged commit efe005d into os-autoinst:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants