rockstor / rockstor-core

Linux/BTRFS based Network Attached Storage(NAS)
http://rockstor.com/docs/contribute_section.html
GNU General Public License v3.0
553 stars 137 forks source link

Un special-case system drive btrfs-in-partition treatment #2824

Closed phillxnet closed 5 months ago

phillxnet commented 6 months ago

In the olden days, pre:

we managed only whole disk btrfs, and special cased our system drive as it was partitioned. This is no longer necessary and represents an inconsistency that adds complexity - bringing with it a maintenance burden.

It is proposed that we leverage the work in the interim years re our data dives to apply similarly to our system drive (drives in the future). With the aim of removing legacy special treatment that was originally intended solely to present a partitioned system pool member as if it was a whole disk: the only entity we understood at that time.

I.e. our low-level internal representation from system.osi.scan_disks() for the system disk is presented below. The following are older real-system test data from when our OS pool was labelled rockstor_rockstor but otherwise similar. Test reference: https://github.com/rockstor/rockstor-core/blob/4d60a2c683140aaa10a8e5b201157163cc9a898b/src/rockstor/system/tests/test_osi.py#L1072-L1076

Disk(
                    name="/dev/sda3",
                    model="QEMU HARDDISK",
                    serial="QM00005",
                    size=7025459,
                    transport="sata",
                    vendor="ATA",
                    hctl="2:0:0:0",
                    type="part",
                    fstype="btrfs",
                    label="rockstor_rockstor",
                    uuid="355f53a4-24e1-465e-95f3-7c422898f542",
                    parted=True,
                    root=True,
                    partitions={},
                )

Where-as the equivalent btrfs-in-partition pool member would be represented as follows:

                # Note partitions entry within vda, consistent with cli prep.
                Disk(
                    name="/dev/vda",
                    model=None,
                    serial="serial-1",
                    size=4194304,
                    transport=None,
                    vendor="0x1af4",
                    hctl=None,
                    type="disk",
                    fstype="btrfs",
                    label="btrfs-in-partition",
                    uuid="55284332-af66-4ca0-9647-99d9afbe0ec5",
                    parted=True,
                    root=False,
                    partitions={"/dev/vda1": "vfat", "/dev/vda2": "btrfs"},
                )

I.e. normalise on our btrfs-in-partition internal representation and remove our now inconsistent special treatment of the system drive. So that all drives are referenced, at the scan_disk() level, by their base canonical name, not a partition reference (sda, not sda3 in above) and that the system disk, as per all data disks now, carries a partitions reference: which in turn would also normalise the parted=True reference as pertaining to the canonically named "Disk" entry.

We still have the potentially inconsistent fstype=... element: when dealing with partitioned devices, but this is consistent with our single btrfs partition per device specification/limit.

The primary aim here is to simplify our scan_disks() and normalise its output across both system and data drives. This is turn should ease ongoing maintenance (scan_disks() has excessive cyclomatic complexity) and help to accommodate future enhancements such as btrfs multi-device "ROOT" pool capability. It should also help with normalising our pool management at all higher levels in the code: given the OS pool member/s will be akin to our current data pool members.

Check list of proposed approach:

phillxnet commented 6 months ago

A 'special case' element of how we currently process the root drive within scan_disks(), that significantly complicates our process, is as follows.

There are caveats here however:

Note that fstype, uuid info etc are held by the direct container (the partition if btrfs-in-partition), not the parent. however we still gain simplification if we treat all drives similarly.

phillxnet commented 5 months ago

Closing as: Fixed by #2835