SUSE / suse-migration-services

GNU General Public License v3.0
7 stars 10 forks source link

Mount only os entries #166

Closed jesusbv closed 4 years ago

jesusbv commented 4 years ago

Before, mount service mounted everything on /etc/fstab, now that service takes into account the entries that matches the disk that holds the fstab file.

This Fixes #165

rjschwei commented 4 years ago

Wondering out loud, just some food for thought. I am wondering if we should add more comprehension to handling of fstab?

We know which filesystems/mount-points we need to upgrade a system. Some of these are

/usr, 
/ ....```

There's probably a list of 10 of these. If we would look through fstab and find entries that match our "system directories" and then mount those then we'd catch everything we need, and as the "non cloud use case" we'd catch the case where `/usr` is on a different disk for example. The system directories in our list must be mountable, period. If they are not mountable the system will not run and migration is not possible. Meaning we can "blindly" mount any one of those system directories if they are listed in fstab and then complain if it cannot be mounted and exit.

If I manually verify the current implementation in Azure where we have separate boot partitions things fail:

From get_root_disk()

-> lsblk -r -n -o 'NAME,MOUNTPOINT' sda sda1 sda2 /boot/efi sda3 /boot sda4 / sdb sdb1 sr0 -> lsblk -n -o pkname /dev/sda4 sda

Note that I added '/dev' to the second call of lsblk, something the code does not do and an error in the PR.

And then we have, from is_on_root()

-> lsblk -n o pkname /dev/disk/by-uuid/a8e8b4ac-b2b7-4abe-8eac-2904ea39e5cf lsblk: o: not a block device lsblk: pkname: not a block device sda3 8:3 0 1G 0 part /boot

where the UUID is directly from fstab

-> cat /etc/fstab UUID=de852492-9570-432f-b45d-dbe503532a03 / xfs defaults 1 1 UUID=a8e8b4ac-b2b7-4abe-8eac-2904ea39e5cf /boot xfs defaults 0 0 UUID=8677-6368 /boot/efi vfat defaults 0 0

given that is_on_root() makes a straight comparison of the lsblk output to the previously determined root device I don't see how this would work.

Anyway if we'd loop through the fstab entries, which we already do anyway

for line in .... if mountpoint in required_system_dirs: system_dirs_to_mount.append(mountpoint)


and then we'd mount everything that's in `system_dirs_to_mount`. I realize I am simplifying things a bit.

Anyway, as far as I can tell the current proposal will not work. And by choosing a different approach we could capture an adjacent use case, i.e. supporting system mount points on different devices. Yes, the adjacent use-case is not our primary target for system migration support.
jesusbv commented 4 years ago

-> lsblk -n -o pkname /dev/sda4 sda


Note that I added '/dev' to the second call of lsblk, something the code does not do and an error in the PR.

True. I left it behind. Will fix it now.

And then we have, from is_on_root()



-> lsblk -n o pkname /dev/disk/by-uuid/a8e8b4ac-b2b7-4abe-8eac-2904ea39e5cf
lsblk: o: not a block device
lsblk: pkname: not a block device
sda3 8:3 0 1G 0 part /boot

It should be -o as in the previous lsblk call.

given that is_on_root() makes a straight comparison of the lsblk output to the previously determined root device I don't see how this would work.

The comparison checks that the disk the entry is/belongs to is root disk.

schaefi commented 4 years ago

Wondering out loud, just some food for thought. I am wondering if we should add more comprehension to handling of fstab?

The only comprehension I'd like to add in the Fstab class is the knowledge of the disk where the root filesystem lives. In the migration process we expect any components that belongs to the system we can upgrade to be on the disk that holds the root filesystem. Only those should be taken into account. Technically it's possible to have /boot on another disk than / but who in Gods name does that and in the cloud it's even impossible because we come from an image as anybody else does

We know which filesystems/mount-points we need to upgrade a system. Some of these are There's probably a list of 10 of these. If we would look through fstab

This is an assumption you can't be sure about. I think adding code that does a selection by a static list even though that list follows standards is an unstable design

If we do it right the additional code to the fstab class will be only a couple lines of code and clear in its intention. It will then solve any of the customers manually added/attached storage XYZ components and we can also write a good logging message that we do not handle filesystem XYZ because it's not on the rootfs disk. If we now found out there are cases where we need to refine this we can do so if needed

schaefi commented 4 years ago

But while we are talking about it I think we can solve this even simpler

The fstab class could check if the device exists at the level of the export() and get_devices() methods and only takes those into account that are present.

At the time of the fstab file reading all device activation code (e.g LVM) is already done. So this should work and is more straight forward

Thoughts ?

rjschwei commented 4 years ago

But while we are talking about it I think we can solve this even simpler

The fstab class could check if the device exists at the level of the export() and get_devices() methods and only takes those into account that are present.

At the time of the fstab file reading all device activation code (e.g LVM) is already done. So this should work and is more straight forward

Thoughts ?

Yes, looping trough the fstab entries and then checking if the reverenced device is present avoids the issue with the static list and will solve the "/usr" is on a different disk problem that would be a "non cloud usecase" I like the idea.

schaefi commented 4 years ago

Yes, looping trough the fstab entries and then checking if the reverenced device is present avoids the issue with the static list and will solve the "/usr" is on a different disk problem that would be a "non cloud usecase" I like the idea.

ok so I think it would also be the easier implementation. @jesusbv sorry but I think the code changes again :) sometimes a better solution is found while coding. Hope you don't mind ?

schaefi commented 4 years ago

closed in favor of #167