[yast-devel] Continued discussion on queries to the Ruby wrapper
This is a follow-up on this discussion https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717 The mail is mainly targeted to Josef, so we continue the discussion in a more persistent and prominent place than a merged pull request. Don't feel guilty if you decide to ignore it. :-) Before introducing the Y2Storage wrapper classes on top of the Storage ones, we had a query API (already discussed in this list) based on the concept of Y2Storage::DevicesLists. I think we don't need that anymore with the wrapper. I would go for an approach based on pure Ruby arrays, now that we don't have to fear not-found exceptions or to care about downcasting. I would simply add the following convenience methods to Y2Storage::Device and that's it. https://github.com/yast/yast-storage-ng/pull/184/commits/0f733850786242cddf7... So, how would we query the devicegraph? Let's explain it by example. Most examples are based on real cases I have seen while porting code in other packages... others are there for the sake of the challenge ;) 1) The filesystem, encrypted or not, in a partition. That is, we want the filesystem and we don't care whether there is an encryption (LUKS) layer sitting between the partition and that filesystem. 2) The filesystem directly placed in a partition, nil if no filesystem or if the filesystem is not directly there (e.g. there is an encryption layer in between). 3) The encrypted filesystem in a partition, nil if the partition is not formatted or if its directly formatted (without the intermediate encryption layer) 4) The disk that match a disk name or partition name, only if it's GPT 5) Filesystems (encrypted or not) on logical partitions of a given disk 6) Non-encrypted filesystem on logical partitions of a given disk 7) Logical partitions for the disk that match a disk name or contains partition name. 8) From a given set of volume groups, physical volumes that are not directly into a disk (or an encrypted disk) (e.g. they sit in a partition or a RAID device) 9) Checking if a given partition contains a btrfs filesystem (encrypted or not) 10) Disks hosting a given filesystem either directly or through one of its partitions. "Disks" in plural because in our devicegraphs, a filesystem can be lay across several block devices. 11) Disks "supporting" the existence of a given filesystem, for whatever technology is used. For example, if the filesystem is in a LV (LVM) and that LV sits on a VG, we want all the disks hosting the PVs of that VG, either directly or through partitions or RAID devices (yes, this is one of the examples based on real code). 12) Disks of a filesystem, only if the plain disk is directly formatted (no partitions, RAID or encryption involved). Otherwise nil 13) Disks of a filesystem, only if the disk is directly formatted (no partitions), nil otherwise, but supporting encrypted filesystems. 14) Devices with a extX filesystem (encrypted or not) 15) Partitions with a extX filesystem (encrypted or not) How would we do that just with the wrapper and plain arrays? 1) the_partition.blk_filesystem 2) the_partition.direct_blk_filesystem 3) the_partition.encrypted? ? the_partition.blk_filesystem : nil 4) disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } (disk && disk.gpt?) ? disk : nil # or disk = Y2Storage::Disk.find_by_name_or_partition(devgraph, the_name) (disk && disk.gpt?) ? disk : nil 5) parts = the_disk.partitions.select {|p| p.type.is?(:logical) } parts.map {|p| p.blk_filesystem }.compact 6) parts = the_disk.partitions.select {|p| p.type.is?(:logical) } parts.map {|p| p.direct_blk_filesystem }.compact 7) disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } disk.partitions.select {|p| p.type.is?(:logical) } 8) all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.disk? } 9) !!(partition.filesystem && partition.filesystem.type.is?(:btrfs)) 10) # See notes below devgraph.disks.select do |d| d.filesystem == the_filesystem || d.partitions.any? {|p| p.filesystem == the_filesystem } end # Beware, this is not equivalent because it also returns filesystems in # partitions of a software RAID or any other partitionable device that # is not a disk the_filesystem.ancestors.select do |a| (a.disk? || a.partition?) && a.filesystem == the_filesystem end 11) the_filesystem.ancestors.select(&:disk?).uniq 12) filesystem.blk_devices.select(&:disk?) 13) filesystem.plain_blk_devices.select(&:disk?) 14) all_ext = devgraph.blk_filesystems.select {|fs| fs.type.is?(:ext) } all_ext.map {|fs| fs.plain_blk_devices }.flatten 15) devgraph.partitions.select do |p| p.blk_filesystem && p.blk_filesystem.type.is?(:ext) end Feel free to provide more "challenges" and/or test alternative API against the current ones. And yes, before you ask, it would make sense to add this to BlkDevice alias_method :filesystem, :blk_filesystem alias_method :direct_filesystem, :direct_blk_filesystem Moreover, if we find ourselves too often checking for something in a disk and all its partitions, we could add a convenience method Y2Storage::Partitionable#this_and_partitions. Then some examples would change a bit, like: 10) devgraph.disks.select do |d| d.this_and_partitions.any? {|i| i.filesystem == the_filesystem } end Looking forward alternatives and/or feedback. Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 03/30/2017 06:53 PM, Ancor Gonzalez Sosa wrote:
This is a follow-up on this discussion https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717
The mail is mainly targeted to Josef, so we continue the discussion in a more persistent and prominent place than a merged pull request. Don't feel guilty if you decide to ignore it. :-)
Before introducing the Y2Storage wrapper classes on top of the Storage ones, we had a query API (already discussed in this list) based on the concept of Y2Storage::DevicesLists.
I think we don't need that anymore with the wrapper. I would go for an approach based on pure Ruby arrays, now that we don't have to fear not-found exceptions or to care about downcasting.
So I decided to do another test to validate that idea. Let's take existing code using the DeviceLists superpowers and compare how it would look like with plain arrays of wrapped objects. 1) # current efi_partition = devgraph.blk_filesystems.with_mountpoint("/boot/efi").partitions.first # wrapper + plain arrays efi_partition = devgraph.partitions.detect do |p| p.blk_filesystem && p.blk_filesystem.mountpoint == "/boot/efi" end 2) # current mbr_disk = devgraph.disks.with(name: mbr_disk_name).first # wrapper + plain arrays mbr_disk = devgraph.disks.detect {|d| d.name == "mbr_disk_name" } 3) # current partitions = devgraph.disks.with(name: disk.name).partitions.reject do |part| [Storage::ID_SWAP, Storage::ID_GPT_BIOS].include?(part.id) end # wrapper + plain arrays partitions = devgraph.partitions.select |part| part.disk && part.disk.name == disk.name && ![:swap, :gpt_bios].include?(part.id.to_sym) end 4) # current def part_or_disk_at_mountpoint(mntpnt) fses = staging.filesystems.with_mountpoint(mntpnt) return nil if fses.empty? partitions = fses.partitions partitions = fses.lvm_vgs.partitions if partitions.empty? return partitions.first unless partitions.empty? disks = fses.disks disks = fses.lvm_vgs.disks if disks.empty? disks.first end # wrapper + plain arrays def part_or_disk_at_mountpoint(mntpnt) fs = staging.filesystems.detect {|f| f.mountpoint == "mntpnt"} return nil unless fs fs.ancestors.detect {|a| a.partition? } || fs.ancestors.detect {|a| a.disk? } end Those four examples should be enough as an exercise. In general it looks like more code because we don't have the #with shortcut (it was useful as a shortcut, but its real motivation was to deal with Storage un-rubistm), but I still think it's ok since now is straightforward common Ruby code with no magic. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa <ancor@suse.de> wrote:
This is a follow-up on this discussion https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717
The mail is mainly targeted to Josef, so we continue the discussion in a more persistent and prominent place than a merged pull request. Don't feel guilty if you decide to ignore it. :-)
Thanks for writing it.
Before introducing the Y2Storage wrapper classes on top of the Storage ones, we had a query API (already discussed in this list) based on the concept of Y2Storage::DevicesLists.
I think we don't need that anymore with the wrapper. I would go for an approach based on pure Ruby arrays, now that we don't have to fear not-found exceptions or to care about downcasting. I would simply add the following convenience methods to Y2Storage::Device and that's it. https://github.com/yast/yast-storage-ng/pull/184/commits/0f733850786242cddf7...
Well, my biggest problem with this approach is that ancestor have to know about all its kids to have comprehensive API, which is very wrong approach from my POV. It also have to be modified, when new child is introduced to have it on same level as others. It should be responsibility of caller to know specific child if he want it. Device responsibility is to provide methods that are same for all devices and makes sense for them like comparing devices, its display_name or its ancestors. Consider if ruby use this approach and Object have methods like `string?` or `integer?`, do you feel it is good approach? Object have `is_a?` which moves responsibility to know about child to caller. So I suggest to use common ruby stuff.
So, how would we query the devicegraph? Let's explain it by example. Most examples are based on real cases I have seen while porting code in other packages... others are there for the sake of the challenge ;)
1) The filesystem, encrypted or not, in a partition. That is, we want the filesystem and we don't care whether there is an encryption (LUKS) layer sitting between the partition and that filesystem.
2) The filesystem directly placed in a partition, nil if no filesystem or if the filesystem is not directly there (e.g. there is an encryption layer in between).
3) The encrypted filesystem in a partition, nil if the partition is not formatted or if its directly formatted (without the intermediate encryption layer)
4) The disk that match a disk name or partition name, only if it's GPT
5) Filesystems (encrypted or not) on logical partitions of a given disk
6) Non-encrypted filesystem on logical partitions of a given disk
7) Logical partitions for the disk that match a disk name or contains partition name.
8) From a given set of volume groups, physical volumes that are not directly into a disk (or an encrypted disk) (e.g. they sit in a partition or a RAID device)
9) Checking if a given partition contains a btrfs filesystem (encrypted or not)
10) Disks hosting a given filesystem either directly or through one of its partitions. "Disks" in plural because in our devicegraphs, a filesystem can be lay across several block devices.
11) Disks "supporting" the existence of a given filesystem, for whatever technology is used. For example, if the filesystem is in a LV (LVM) and that LV sits on a VG, we want all the disks hosting the PVs of that VG, either directly or through partitions or RAID devices (yes, this is one of the examples based on real code).
12) Disks of a filesystem, only if the plain disk is directly formatted (no partitions, RAID or encryption involved). Otherwise nil
13) Disks of a filesystem, only if the disk is directly formatted (no partitions), nil otherwise, but supporting encrypted filesystems.
14) Devices with a extX filesystem (encrypted or not)
15) Partitions with a extX filesystem (encrypted or not)
How would we do that just with the wrapper and plain arrays?
I write how it will look like, for examples where it makes difference ( I consider we are in Y2Storage namespace, otherwise it need to be prepended by it ):
1) the_partition.blk_filesystem
2) the_partition.direct_blk_filesystem
3) the_partition.encrypted? ? the_partition.blk_filesystem : nil
4) disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } (disk && disk.gpt?) ? disk : nil
# or
disk = Y2Storage::Disk.find_by_name_or_partition(devgraph, the_name) (disk && disk.gpt?) ? disk : nil
5) parts = the_disk.partitions.select {|p| p.type.is?(:logical) } parts.map {|p| p.blk_filesystem }.compact
6) parts = the_disk.partitions.select {|p| p.type.is?(:logical) } parts.map {|p| p.direct_blk_filesystem }.compact
7) disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } disk.partitions.select {|p| p.type.is?(:logical) }
8) all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.disk? }
all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.is_a?(Disk) }
9) !!(partition.filesystem && partition.filesystem.type.is?(:btrfs))
10) # See notes below devgraph.disks.select do |d| d.filesystem == the_filesystem || d.partitions.any? {|p| p.filesystem == the_filesystem } end
# Beware, this is not equivalent because it also returns filesystems in # partitions of a software RAID or any other partitionable device that # is not a disk the_filesystem.ancestors.select do |a| (a.disk? || a.partition?) && a.filesystem == the_filesystem end
the_filesystem.ancestors.select do |a| (a.is?(Disk) || a.is_a?(Partition) && a.filesystem == the_filesystem end
11) the_filesystem.ancestors.select(&:disk?).uniq
the_filesystem.ancestors.grep(Disk).uniq
12) filesystem.blk_devices.select(&:disk?)
filesystem.blk_devices.grep(Disk)
13) filesystem.plain_blk_devices.select(&:disk?)
filesystem.plain_blk_devices.grep(Disk)
14) all_ext = devgraph.blk_filesystems.select {|fs| fs.type.is?(:ext) } all_ext.map {|fs| fs.plain_blk_devices }.flatten
15) devgraph.partitions.select do |p| p.blk_filesystem && p.blk_filesystem.type.is?(:ext) end
Feel free to provide more "challenges" and/or test alternative API against the current ones.
My challenge for you what have to changed if we add new MD Raid? So how hard will be to add new child to Device. This is good metric for me how hard/easy is to extend it. Josef
And yes, before you ask, it would make sense to add this to BlkDevice alias_method :filesystem, :blk_filesystem alias_method :direct_filesystem, :direct_blk_filesystem
Moreover, if we find ourselves too often checking for something in a disk and all its partitions, we could add a convenience method Y2Storage::Partitionable#this_and_partitions. Then some examples would change a bit, like:
10) devgraph.disks.select do |d| d.this_and_partitions.any? {|i| i.filesystem == the_filesystem } end
Looking forward alternatives and/or feedback.
Cheers.
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 03/31/2017 08:42 AM, Josef Reidinger wrote:
On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa <ancor@suse.de> wrote:
This is a follow-up on this discussion https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717
The mail is mainly targeted to Josef, so we continue the discussion in a more persistent and prominent place than a merged pull request. Don't feel guilty if you decide to ignore it. :-)
Thanks for writing it.
Before introducing the Y2Storage wrapper classes on top of the Storage ones, we had a query API (already discussed in this list) based on the concept of Y2Storage::DevicesLists.
I think we don't need that anymore with the wrapper. I would go for an approach based on pure Ruby arrays, now that we don't have to fear not-found exceptions or to care about downcasting. I would simply add the following convenience methods to Y2Storage::Device and that's it. https://github.com/yast/yast-storage-ng/pull/184/commits/0f733850786242cddf7...
Well, my biggest problem with this approach is that ancestor have to know about all its kids to have comprehensive API, which is very wrong approach from my POV. It also have to be modified, when new child is introduced to have it on same level as others.
It should be responsibility of caller to know specific child if he want it. Device responsibility is to provide methods that are same for all devices and makes sense for them like comparing devices, its display_name or its ancestors. Consider if ruby use this approach and Object have methods like `string?` or `integer?`, do you feel it is good approach? Object have `is_a?` which moves responsibility to know about child to caller.
So I suggest to use common ruby stuff.
I'm also fine with that. Some cases will become a little bit more verbose, but I can live with it. As I said, I just wanted those method to shorten stuff (see below), not because they are crucial.
[...]
8) all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.disk? }
all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.is_a?(Disk) }
Well, actually in most cases it would be all_pvs.reject {|pv| pv.blk_device.plain_device.is_a?(Y2Storage::Disk)}
9) !!(partition.filesystem && partition.filesystem.type.is?(:btrfs))
10) # See notes below devgraph.disks.select do |d| d.filesystem == the_filesystem || d.partitions.any? {|p| p.filesystem == the_filesystem } end
# Beware, this is not equivalent because it also returns filesystems in # partitions of a software RAID or any other partitionable device that # is not a disk the_filesystem.ancestors.select do |a| (a.disk? || a.partition?) && a.filesystem == the_filesystem end
the_filesystem.ancestors.select do |a| (a.is?(Disk) || a.is_a?(Partition) && a.filesystem == the_filesystem end
Once again, it would be slightly longer. the_filesystem.ancestors.select do |a| (a.is_a?(Y2Storage::Disk) || a.is_a?(Y2Storage::Partition) && a.filesystem == the_filesystem end
11) the_filesystem.ancestors.select(&:disk?).uniq
the_filesystem.ancestors.grep(Disk).uniq
12) filesystem.blk_devices.select(&:disk?)
filesystem.blk_devices.grep(Disk)
13) filesystem.plain_blk_devices.select(&:disk?)
filesystem.plain_blk_devices.grep(Disk)
Same for those three. It would be grep(Y2Storage::Disk) As said, I'm fine with using plain #is_a?, just pointing we would need to qualify that with Y2Storage:: very often Just thinking aloud. Would it make sense to add the same kind of shortcuts but with a more correct approach like defining #device?(whatever) in the base class to always return false and then redefine it in descendant classes to allow stuff like this? all_pvs.reject {|pv| pv.blk_device.plain_device.device?(:disk)} # or the_filesystem.ancestors.select do |a| (a.device?(:disk) || a.device?(:partition) && a.filesystem == the_filesystem end If instead of #device? we call it #is?, it would look similar to the enum wrappers and would be even shorter (but maybe too similar to #is_a?).
Feel free to provide more "challenges" and/or test alternative API against the current ones.
My challenge for you what have to changed if we add new MD Raid? So how hard will be to add new child to Device. This is good metric for me how hard/easy is to extend it.
Since we don't need to keep ABI compatibility (just API one), I don't see it as a big challenge, neither using plain #is_a? or adding some convenience method to the base class. With DevicesLists:: we were adding some logic/magic on top of libstorage, which I admit was potentially dangerous regarding the addition of new classes. Now we are basically accessing "directly" the libstorage API (saving us from exceptions, downcasts and enums), which is hopefully well designed in that regard.
Josef
And yes, before you ask, it would make sense to add this to BlkDevice alias_method :filesystem, :blk_filesystem alias_method :direct_filesystem, :direct_blk_filesystem
Moreover, if we find ourselves too often checking for something in a disk and all its partitions, we could add a convenience method Y2Storage::Partitionable#this_and_partitions. Then some examples would change a bit, like:
10) devgraph.disks.select do |d| d.this_and_partitions.any? {|i| i.filesystem == the_filesystem } end
Looking forward alternatives and/or feedback.
Cheers.
Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Fri, 31 Mar 2017 10:16:34 +0200 Ancor Gonzalez Sosa <ancor@suse.de> wrote:
On 03/31/2017 08:42 AM, Josef Reidinger wrote:
On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa <ancor@suse.de> wrote:
[...] all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.is_a?(Disk) }
Well, actually in most cases it would be all_pvs.reject {|pv| pv.blk_device.plain_device.is_a?(Y2Storage::Disk)}
Yeah, I mentioned it on top.
9) !!(partition.filesystem && partition.filesystem.type.is?(:btrfs))
10) # See notes below devgraph.disks.select do |d| d.filesystem == the_filesystem || d.partitions.any? {|p| p.filesystem == the_filesystem } end
# Beware, this is not equivalent because it also returns filesystems in # partitions of a software RAID or any other partitionable device that # is not a disk the_filesystem.ancestors.select do |a| (a.disk? || a.partition?) && a.filesystem == the_filesystem end
the_filesystem.ancestors.select do |a| (a.is?(Disk) || a.is_a?(Partition) && a.filesystem == the_filesystem end
Once again, it would be slightly longer.
the_filesystem.ancestors.select do |a| (a.is_a?(Y2Storage::Disk) || a.is_a?(Y2Storage::Partition) && a.filesystem == the_filesystem
or it can be more generic like ` a.respond_to?(:filesystem) && a.filesystem == the_filesystem which will find even encrypted devices ;)
end
11) the_filesystem.ancestors.select(&:disk?).uniq
the_filesystem.ancestors.grep(Disk).uniq
12) filesystem.blk_devices.select(&:disk?)
filesystem.blk_devices.grep(Disk)
13) filesystem.plain_blk_devices.select(&:disk?)
filesystem.plain_blk_devices.grep(Disk)
Same for those three. It would be grep(Y2Storage::Disk)
As said, I'm fine with using plain #is_a?, just pointing we would need to qualify that with Y2Storage:: very often
Just thinking aloud. Would it make sense to add the same kind of shortcuts but with a more correct approach like defining #device?(whatever) in the base class to always return false and then redefine it in descendant classes to allow stuff like this?
all_pvs.reject {|pv| pv.blk_device.plain_device.device?(:disk)} # or the_filesystem.ancestors.select do |a| (a.device?(:disk) || a.device?(:partition) && a.filesystem == the_filesystem end
If instead of #device? we call it #is?, it would look similar to the enum wrappers and would be even shorter (but maybe too similar to #is_a?).
yes, it is fine approach from my POV. If it is is? and consistent with enums, even better. It is shorter and also keep responsibility on caller. maybe we can use method `type` so we can use `[:disk, :partition].include?(a.type)`
Feel free to provide more "challenges" and/or test alternative API against the current ones.
My challenge for you what have to changed if we add new MD Raid? So how hard will be to add new child to Device. This is good metric for me how hard/easy is to extend it.
Since we don't need to keep ABI compatibility (just API one), I don't see it as a big challenge, neither using plain #is_a? or adding some convenience method to the base class.
With DevicesLists:: we were adding some logic/magic on top of libstorage, which I admit was potentially dangerous regarding the addition of new classes. Now we are basically accessing "directly" the libstorage API (saving us from exceptions, downcasts and enums), which is hopefully well designed in that regard.
good, so I will look forward for adding it and commenting if changes looks too big :) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
participants (2)
-
Ancor Gonzalez Sosa -
Josef Reidinger