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.

Sign up for GitHub

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

Jump to bottom

storage: Unified overview panel #19197

Closed
wants to merge 6 commits into from
Closed

storage: Unified overview panel #19197

wants to merge 6 commits into from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Aug 14, 2023 •
edited
Loading

See #16440

Demos:

Todo

  • LVM2 thin pools
  • iSCSI sessions
  • ...

Polish

  • Use Stratis pool names in the breadcrumb instead of UUID
  • Don't say "Stratis filesystem" when listing them for a pool
  • Navigation after deleting things

Notes

  • We need to handle "too many" rows on the overview. We can cut them off like we do with the "Drives" panel and show a "Show all NNN rows" button. We can also omit the content rows (filesystems, etc), and make them expandable.
  • Sorting like with the Filesystems panel would be really nice. How do we handle the hierachy?
  • Top-level things that have content directly on them are a bit confusing, like a drive or loop back device that directly has a filesystem on it. Technically it makes sense to have two rows for them, one of type "Drive", the other of type "xfs Filesystem". But in the users mind, both rows represent the same object, so it is maybe confusing to see two of them... hmm.
  • We should be able to add and remove physical volumes from vgroups already on the overview, but I think we need to change the approach for this. Instead of hanging the actions on the volume group, they should be on the block devices that will become/are the physical volumes. So this should probably be a followup.
  • We should (finally) distinguish between "empty" and "unrecognized".

@mvollmer mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Aug 14, 2023
@mvollmer mvollmer force-pushed the storage-unified-overview branch 9 times, most recently from a5fefa4 to c2d96db Compare August 21, 2023 11:41
@mvollmer mvollmer force-pushed the storage-unified-overview branch 4 times, most recently from fdcff0f to 74a0c8b Compare August 24, 2023 11:51
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 24, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 25, 2023
View reviewed changes
pkg/storaged/client.js Fixed Show fixed Hide fixed
@mvollmer mvollmer force-pushed the storage-unified-overview branch 3 times, most recently from ed6b0fa to 93236d6 Compare August 30, 2023 12:14
@mvollmer mvollmer force-pushed the storage-unified-overview branch 2 times, most recently from 3d5224e to 261de82 Compare October 3, 2023 12:45
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 3, 2023
View reviewed changes
pkg/storaged/block-details.jsx Fixed Show fixed Hide fixed
pkg/storaged/block-details.jsx Fixed Show fixed Hide fixed
pkg/storaged/side-panel.jsx Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 4, 2023
View reviewed changes
pkg/storaged/utils.js Fixed Show fixed Hide fixed
pkg/storaged/block-details.jsx Fixed Show fixed Hide fixed
pkg/storaged/block-details.jsx Fixed Show fixed Hide fixed
@mvollmer
storage: Keep order in SelectSpaces
55c6149 
Otherwise the order of PVs passed to calls like
CreatePlainVolumeWithLayout depends on in which order the PVs have
been selected in the dialog, which would not be wrong, but is
unexpected.
@mvollmer
storage: Let Stratis use the regular warnings mechanism for pool alerts
da2766b 
So that extending this to "errors" in addition to "warnings" later on
becomes slightly cleaner.
@mvollmer
test: Retry dropdown menu in storage tests
b5773f4 
It gets spuriously closed sometimes.
@mvollmer
storage: Support LVM2 RAID configurations
eeb31be
@mvollmer
storage: Stop kebab toggles from bubbling
6472deb 
This would otherwise interfere with clicks on table rows.
@jelly
Copy link
Member

jelly commented Oct 5, 2023

Feedback for the last demo:

overview

The overview page is very extensive, maybe a bit too much data is shown. If I want to find information about my system's disks I usually look at fdisk -l in a terminal. Everything important is highlighted in bold white letters with the device name and size being prominently displayed. For me that works quite well or I got used to it, but I think I identify disks by device name and size (then again I don't have too many disks installed anywhere).

For reference, gnome disks seems to do the same:

image

However, I believe there is work being done on re-designing gnome disks. So maybe this design can also serve as some inspiration.

empty vs unrecognized

Regarding unrecognized data, I agree that the current implementation can be a bit confusing. We should probably not show empty disks as that now seems to clutter the UI. And formalize if it will be "free" or "udisks does not know how to deal with fancy log based FS XYZ".

actions

I think we need to improve the discoverability of actions such as making a stratis pool, RAID area. Maybe instead of a kebab this should be a dropdown or Menu? @garrett probably can provide a better suggestion, but I think overall it is not very easy for someone new to find that in the current design.

breadcrumbs

When you click on:
image

Should it not show: "raid array -> $raid member" instead of overview -> /dev/foo?

@mvollmer
Copy link
Member Author

Continued in #19472

@mvollmer mvollmer closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run,
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvollmer @jelly