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

Introduce LVM thin volume test #5520

Merged
merged 1 commit into from Aug 8, 2018

Conversation

@@ -165,6 +165,9 @@ sub addlv {
send_key 'ret'; # create logical volume
assert_screen 'partition-lv-type';
type_string $args{name};

send_key($args{thinpool} ? 'alt-t' : $args{thinvolume} ? 'alt-i' : 'alt-o');
Copy link
Member

Choose a reason for hiding this comment

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

So now in case it's not thin pool or thin volume we additionally press alt-o. Can it have any impact on existing tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert_script_run 'lvs -a';
assert_script_run 'pvs -a';
# thin volume does not exceed thin pool size in our tests
#my $thin_volume_size = script_output q[lvs | grep Vwi | awk '{print $(NF-1)}' | cut -d'.' -f 1];
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 not leave any commented code.


foreach (@volumes) {
chomp;
assert_script_run "lvdisplay $_";
Copy link
Member

Choose a reason for hiding this comment

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

What's about adding some more validation, except return code of the command?

use partition_setup qw(create_new_partition_table addpart addlv);
use version_utils 'is_storage_ng';

sub addvg {
Copy link
Member

Choose a reason for hiding this comment

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

What's about moving it to lib/partition_setup.pm so if someone needs same method she/he can find it easier?

type_string $args{name};
wait_screen_change { send_key 'tab' } for (0 .. 1);
wait_screen_change { send_key 'spc' };
assert_screen 'partition-select-first-from-top';
Copy link
Member

Choose a reason for hiding this comment

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

JFYI, as long as we don't run it yet in textmode, we could use assert_and_click, which is a bit more scalable in case of UI changes.

@@ -398,6 +398,7 @@ sub load_reboot_tests {
}
}
loadtest "installation/first_boot";
loadtest "installation/lvm_thin_check" if get_var('LVM_THIN_LV');
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we should move this to console tests, as running consoletest_setup is definitely not a bad idea. I also don't find it wrong if we run only this test module after we finish installation, but it doesn't belong here. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No probs at all. This is just a mere suggestion where it can be placed, besides it saved a bit waiting time for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm...but on the other thought, we would need to run console tests after installation, which does not happen always.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, so basically I would say that it would be enough just to run this test module (with consoletest_setup in front). Or put it in console tests and execute same set of tests we run on cryptlvm (e.g. https://openqa.suse.de/tests/1896209).

@rwx788
Copy link
Member

rwx788 commented Aug 6, 2018

Needles are merged.

sub run {
create_new_partition_table;
# create boot and 2 lvm partitions
addpart(role => 'raw', fsid => 'bios-boot', size => 2);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please update ticket for SLE to adjust this line for other architectures? We need prep boot on ppc64, boot/zipl for s390x and efi for UEFI (aarch64).

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
rwx788 previously approved these changes Aug 6, 2018
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

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

LGTM


foreach (@volumes) {
chomp;
assert_script_run "lvdisplay $_";
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, please wait with the merge. I am still working on extra check over here.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I've added WIP label just for the case. Thanks for the reminder =)

@rwx788 rwx788 dismissed their stale review August 6, 2018 11:22

More changes to come

@rwx788 rwx788 added the WIP Work in progress label Aug 6, 2018
@mloviska
Copy link
Contributor Author

mloviska commented Aug 7, 2018

I do not have rights to remove WIP label...

Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

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

Some minor comments, but could you also squash or change commit messages?

chomp;
my $lvdisp_out = script_output "lvdisplay $volume";
foreach (keys %{$lv_stats}) {
die "Value of $_" unless ($lvdisp_out =~ /(?<tested_string>$lv_stats->{$_})/);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please put a bit more details in the die message, so reason is more obvious.

}
}

sub post_fail_hook {
Copy link
Member

Choose a reason for hiding this comment

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

I guess standard set of logs could be helpful here too, so what's about calling SUPER postfail hook too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@rwx788 rwx788 merged commit 5865a13 into os-autoinst:master Aug 8, 2018
@mloviska
Copy link
Contributor Author

mloviska commented Aug 8, 2018

We are missing needles:
os-autoinst/os-autoinst-needles-opensuse#408

@okurz
Copy link
Member

okurz commented Aug 8, 2018

done

@rwx788
Copy link
Member

rwx788 commented Aug 8, 2018

Ah, somehow got a feeling that already merged them. Thanks! ;)

@mloviska
Copy link
Contributor Author

mloviska commented Aug 8, 2018

@rwx788, That is my fault! Sorry! I should let you know that with the assert_and_click 'partition-select-first-from-top'; change I had to replace one needle.

@rwx788
Copy link
Member

rwx788 commented Aug 8, 2018

@mloviska no worries, you did a great job by reminding to merge ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants