Fix Restore on UEFI hosts (#150) #151
Conversation
This fixes a regression introduced in e9d686a. This is not sufficient however to get Restore to work un UEFI, as setEfiBootEntry() assumes `mounts['efi']` is populated, which is not the case (only `backend.mountVolumes()` would set that). Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
ydirson
changed the title
Commit 29c7bb5 introduced assumption that `mounts` has a `esp` key, which was not the case when called from `restoreFromBackup`. This makes sure the key is created. Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
|
Thanks for working on this. I was wondering why not doing the same thing we do for On the other end the code is repartitioning the disk and mounting destination disk again... but won't the partitions be corrupted at that time? |
| if boot_config.src_fmt == 'grub2' and efi_boot: | ||
| branding = util.readKeyValueFile(os.path.join(backup_fs.mount_point, constants.INVENTORY_FILE)) | ||
| branding['product-brand'] = branding['PRODUCT_BRAND'] | ||
|
|
||
| # repartition if needed | ||
| backup_fs.unmount() |
There was a problem hiding this comment.
Maybe just move this line inside the below if ?
We are restoring MBR so we won't have EFI to restore.
There was a problem hiding this comment.
possibly, but that rather looks like some improvement that can wait for later, here I'm solely focused on unbreaking the nominal case
That's an interesting question. Just wondering about that seems enough to justify reading them before touching the partition table, that looks like the safest move. The code states:
Well, if that ever happens with any partition with data, the restored disk is as good as dead anyway. I'm quite puzzled of how this is managed (recreating a partition table and getting This whole repartitioning stuff I find pretty scary TBH - I don't think there is even a safeguard to prevent from attempting to restore from an upgrade made with a different installer version, in which case this code would have no clue it's dealing with something it potentially does not even interpret properly. |
| @@ -209,8 +214,7 @@ def restore_partitions(): | |||
| # restore boot loader | |||
| if boot_config.src_fmt == 'grub2': | |||
| if efi_boot: | |||
| branding = util.readKeyValueFile(os.path.join(backup_fs.mount_point, constants.INVENTORY_FILE)) | |||
| branding['product-brand'] = branding['PRODUCT_BRAND'] | |||
| mounts['esp'] = esp | |||
There was a problem hiding this comment.
As you pointed out on https://github.com/xenserver/host-installer/pull/161/files, these solve the same issue
I think adding esp to mounts should happen when the mounting occurs in case any future changes need to inspect what is mounted and rely on that mounts dictionary
With your change here, it would incorrectly appear as if esp is not mounted until this code is run
(Another improvement would be to use 'esp' in mounts instead of if efi_mounted but that's for another PR)
|
I've merged #172 which backports the fix from master, this PR will need to be updated to contain only the inventory changes to be considered for merging |
Read from backup inventory before unmounting fixes a regression introduced in e9d686a.
Moving the read before unmounting was not sufficient however to get Restore to work un UEFI, as setEfiBootEntry() assumes
mounts['esp']is populated, continuing thebackend.mountVolumes()emulation done for Restore.