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

Make stable validate_lvm_raid1 reboot #18827

Merged
merged 1 commit into from Mar 15, 2024

Conversation

manfredi
Copy link
Contributor

@manfredi manfredi commented Mar 7, 2024

  • Description: [sporadic] make stable validate_lvm_raid1 reboot
  • Related ticket: poo#154753
  • Verification run: overview

Copy link
Contributor

@rakoenig rakoenig left a comment

Choose a reason for hiding this comment

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

The title of the commit is confusing and doesn't seem related to the code change.
Besides that it looks good.

@jknphy
Copy link
Contributor

jknphy commented Mar 8, 2024

The title of the commit is confusing and doesn't seem related to the code change. Besides that it looks good.

it looks good as a draft, because at least we are trying to narrow the problem, but VR still are still not stable. Commit's first line and title should (most of the time) match and be descriptive in few characters. At the moment the commit is too short for example (to have some meaning to someone using git log.

@manfredi manfredi force-pushed the issues-154753 branch 4 times, most recently from e159194 to bcd44a5 Compare March 12, 2024 08:48
@manfredi manfredi changed the title [WIP] Make stable validate_lvm_raid1 reboot Make stable validate_lvm_raid1 reboot Mar 12, 2024
Copy link
Contributor

@sofiasyria sofiasyria left a comment

Choose a reason for hiding this comment

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

We could completely remove line 28. The rebooting and rechecking also might not be necessary. I think just validation of expected disks after first boot should suffice.

@manfredi
Copy link
Contributor Author

Removed the latest calls:

    _remove_raid_disk($config, $expected_num_devs);
    _reboot();
    $self->wait_boot;
    _check_raid_disks_after_reboot($config, $expected_num_devs);

Copy link
Contributor

@sofiasyria sofiasyria left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@b10n1k b10n1k left a comment

Choose a reason for hiding this comment

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

lgtm

See bsc#1172203
Also the rebooting and rechecking also are not necessary.
@lemon-suse
Copy link
Contributor

Remove unneeded steps make sense. Just a minor comments for the description for the test, only a summary while no test steps, not very friendly for reader. Maybe that's already out of the scope of your ticket, we can discuss such things later.

@manfredi
Copy link
Contributor Author

Remove unneeded steps make sense. Just a minor comments for the description for the test, only a summary while no test steps, not very friendly for reader. Maybe that's already out of the scope of your ticket, we can discuss such things later.

@lemon-suse the motivations are explained in the poo#154753 comments


sub _reboot {
record_info('system reboots');
power_action('reboot', textmode => 'textmode');
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the import on top.

@lemon-suse
Copy link
Contributor

Remove unneeded steps make sense. Just a minor comments for the description for the test, only a summary while no test steps, not very friendly for reader. Maybe that's already out of the scope of your ticket, we can discuss such things later.

@lemon-suse the motivations are explained in the poo#154753 comments

In fact, my comments only related with the test module that no test steps in description, and this is not only for this test module. Your PR looks good to me. I mean maybe later we can raise a new ticket to update test modules for test steps in description.

@lemon-suse lemon-suse merged commit d8f1717 into os-autoinst:master Mar 15, 2024
10 checks passed
@jknphy
Copy link
Contributor

jknphy commented Mar 15, 2024

@lemon-suse besides the approvals (before merging) please ensure that all the comments are addressed with answers or tickets.

@manfredi manfredi deleted the issues-154753 branch March 15, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants