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

Guard better against RaidChooser's selected_level being None #440

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Jun 5, 2024

This happens more often since blivet dropped linear from mdraid in storaged-project/blivet#1236 . It was the only RAID level supported for mdraid that had min_members of 1. When you add the first member for a new mdraid set, of course, there is only one member.

RaidChooser's update method checks the min_members on each level and only adds it to the liststore of available levels if we currently have at least that many members. So, before that blivet change, when there was only one member, "linear" was the only level in the liststore, and was necessarily the selected_level. Now linear is gone, when there is only one member of an mdraid set, there are no levels in the liststore, which means selected_level winds up being None.

There are three places I can see in add_dialog.py which don't guard against selected_level being None. This fixes each of them.

This happens more often since blivet dropped linear from mdraid
in storaged-project/blivet#1236 . It was
the only RAID level supported for mdraid that had min_members of
1. When you add the first member for a new mdraid set, of course,
there is only one member.

RaidChooser's update method checks the `min_members` on each
level and only adds it to the liststore of available levels if we
currently have at least that many members. So, before that blivet
change, when there was only one member, "linear" was the only
level in the liststore, and was necessarily the selected_level.
Now linear is gone, when there is only one member of an mdraid
set, there are *no* levels in the liststore, which means
selected_level winds up being None.

There are three places I can see in add_dialog.py which don't
guard against selected_level being None. This fixes each of them.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor Author

AdamWill commented Jun 5, 2024

I tested this locally; I can reproduce the immediate crash by selecting "Software RAID" as the type for a new device (with python3-blivet-3.10.0-2.fc41.noarch), and this patch does fix it.

@StorageGhoul
Copy link

Can one of the admins verify this patch?

@vojtechtrefny
Copy link
Member

Jenkins, ok to test.

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Thank you!

@vojtechtrefny
Copy link
Member

I have great tests 🤦‍♂️

test_raid_type (blivetgui_tests.add_dialog_test.AddDialogTest.test_raid_type) ... Traceback (most recent call last):
  File "/home/vtrefny/blivet-gui/blivetgui/dialogs/add_dialog.py", line 926, in on_devices_combo_changed
    self.add_advanced_options()
  File "/home/vtrefny/blivet-gui/blivetgui/dialogs/add_dialog.py", line 866, in add_advanced_options
    if device_type == "mdraid" and self._raid_chooser.selected_level.name == "raid1":
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'name'
ok

@vojtechtrefny vojtechtrefny merged commit 6ad9055 into storaged-project:main Jun 6, 2024
11 checks passed
@AdamWill
Copy link
Contributor Author

AdamWill commented Jun 6, 2024

Yeah, I initially looked at adding a test for this then saw that one and thought "I'm pretty sure that one should hit this" :D I guess the missing piece is just "remember to run the blivet-gui tests when changing blivet"...

edit: oh, is it one of those "failing tests don't fail properly" cases? Hah, I love those.

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

Successfully merging this pull request may close these issues.

3 participants