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

tui: show the disk on which an existing installation or backup is found #20

Merged
merged 2 commits into from
Aug 16, 2023
Merged

tui: show the disk on which an existing installation or backup is found #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc45c60
New diskutil.getHumanDiskLabel to centralize formatting of disk name …
ydirson Dec 14, 2022
97052fc
tui: show the disk on which an existing installation or backup is found
ydirson Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions diskutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,12 @@ def getHumanDiskName(disk):
return disk[5:]
return disk

def getHumanDiskLabel(disk, short=False):
(vendor, model, size) = getExtendedDiskInfo(disk)
template = "{device} - {size} [{vendor} {model}]" if not short else "{device} - {size}"
return template.format(device=getHumanDiskName(disk), size=getHumanDiskSize(size),
vendor=vendor, model=model)

# given a list of disks, work out which ones are part of volume
# groups that will cause a problem if we install XE to those disks:
def findProblematicVGs(disks):
Expand Down
14 changes: 6 additions & 8 deletions tui/installer/screens.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,11 @@ def get_admin_interface_configuration(answers):
def get_installation_type(answers):
entries = []
for x in answers['upgradeable-products']:
entries.append(("Upgrade %s" % str(x), (x, x.settingsAvailable())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the design goal was so that you don't need to see internal device names when installing upgrading/restoring. That way it looks cleaner and doesn't show (unstable) Linux device names. You can press F5 to see the full details of the existing installation, including the disk it is installed on. Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it does give the information. However, the goal of this patch is to help a user avoid making wrong decisions without realizing it, and I'm not sure that relying on an optional display would help much.

An example use-case where it would help, very much tied to patch series we did not submit yet, is when a system has been installed on a software RAID device (we have in XCPng a patch allowing the user to build a RAID1 for use as primary disk). I'm working on a patch-series that makes RAID assembly "opt-in, off by default" (see #16), and after this behavior change such a RAID install will not be listed at first; then if any install is found on another disk, when the disk is not mentioned in the entry itself, the user might think the install listed is the one it expects to find, and I think we don't want that to happen.

So maybe I should just move this commit back into my RAID-improvements PR-to-be (which incidentally already exists as a PR against XCPng, and on which you're welcome to comment already), with better context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see you have a reason for doing this. In that case I think it is generally fine to show some extra information there.

I'd like to ensure that it shows some kind of stable identifier rather than something like /dev/sda and /dev/sdb which might switch around on successive boots. Does it do this?

It might also be useful to include the disk size since that is often an easy way of disambiguating the disks in a system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect we should in fact use the same label as is already used in the primary-disk selection menu, and it already includes disk size. It does not use anything more stable than /dev/sda though, but with size as disambiguation it should be fine, and if disks are always presented by the same code it will be easy to improve it when deemed necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really happy with the results on a small 80x25 textmode:
installer-longlines

With a longer description (faked here for testing), the screen overflow looks really bad:
installer-verylonglines

Unfortunately, that old newt library does not seem to support multiline list entries. Looks like for we'll have to be satisfied with just adding the size :/.

entries.append(("Upgrade %s on %s" % (x, diskutil.getHumanDiskLabel(x.primary_disk, short=True)),
(x, x.settingsAvailable())))
for b in answers['backups']:
entries.append(("Restore %s from backup" % str(b), (b, None)))
entries.append(("Restore %s from backup to %s" % (b, diskutil.getHumanDiskLabel(b.root_disk, short=True)),
(b, None)))

entries.append( ("Perform clean installation", None) )

Expand Down Expand Up @@ -556,9 +558,7 @@ def select_primary_disk(answers):
(boot, root, state, storage, logs) = diskutil.probeDisk(de)
if storage[0]:
target_is_sr[de] = True
(vendor, model, size) = diskutil.getExtendedDiskInfo(de)
stringEntry = "%s - %s [%s %s]" % (diskutil.getHumanDiskName(de), diskutil.getHumanDiskSize(size), vendor, model)
e = (stringEntry, de)
e = (diskutil.getHumanDiskLabel(de), de)
entries.append(e)

# we should have at least one disk
Expand Down Expand Up @@ -665,9 +665,7 @@ def select_guest_disks(answers):
# Make a list of entries: (text, item)
entries = []
for de in diskEntries:
(vendor, model, size) = diskutil.getExtendedDiskInfo(de)
entry = "%s - %s [%s %s]" % (diskutil.getHumanDiskName(de), diskutil.getHumanDiskSize(size), vendor, model)
entries.append((entry, de))
entries.append((diskutil.getHumanDiskLabel(de), de))

text = TextboxReflowed(54, "Which disks would you like to use for %s storage? \n\nOne storage repository will be created that spans the selected disks. You can choose not to prepare any storage if you wish to create an advanced configuration after installation." % BRAND_GUEST)
buttons = ButtonBar(tui.screen, [('Ok', 'ok'), ('Back', 'back')])
Expand Down