WIP: sfdisk implementation #1929

prestist · 2024-08-26T21:29:14Z

This is a WIP PR to add sfdisk support to ignition, Current state is iterating through failing tests and adding mapping to pretend to interface the same as before with sgdisk.

Resolves #1926

prestist · 2024-09-03T13:20:21Z

I think I might need a bit more for wipefs to work correctly. (looking into it but wanted to share)

Clean device using old way

[core@cosa-devsh ~]$ sudo sgdisk --zap-all /dev/vda
Found valid GPT with corrupt MBR; using GPT and will write new
protective MBR on save.
GPT data structures destroyed! You may now partition the disk using fdisk or
other utilities.

Write to device using command I will use again to verify it works

[core@cosa-devsh ~]$ sudo sgdisk --new=1:0:+65536 --change-name=1:create-partition --typecode=1:B921B045-1DF0-41C3-AF44-4C6F280D3FAE --partition-guid=1:05AE8178-224E-4744-862A-4F4B042662D0 /dev/vda
Creating new GPT entries in memory.
The operation has completed successfully.

Wipe device using wipefs to see if we can use a device that is wiped with it

[core@cosa-devsh ~]$ sudo wipefs -a -t mbr -t gpt -t pmbr /dev/vda
/dev/vda: 2 bytes were erased at offset 0x000001fe (PMBR): 55 aa
/dev/vda: calling ioctl to re-read partition table: Success

Verify disk is empty according to sfdisk

[core@cosa-devsh ~]$ sudo sfdisk --list /dev/vda
Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes

Try sgdisk command to create partition created above.

[core@cosa-devsh ~]$ sudo sgdisk --new=1:0:+65536 --change-name=1:create-partition --typecode=1:B921B045-1DF0-41C3-AF44-4C6F280D3FAE --partition-guid=1:05AE8178-224E-4744-862A-4F4B042662D0 /dev/vda
Found valid GPT with corrupt MBR; using GPT and will write new
protective MBR on save.
Could not create partition 1 from 67584 to 133119
Error encountered; not saving changes.

prestist · 2024-09-04T21:43:30Z

Hmm, I seem to be stumbling a bit, I ended up focusing directly on the pretend section today. I found a few odds and ends that were missed in the first pass.

I had been running against a creation test which only created one partition, however I stumbled into a test which makes two partitions, this is relevant because the nature of how the sfdisk is currently receiving my script input is painful. Currently I don't see a way to express multiple partitions when running commands in the echo <my script> | sudo sfdisk /dev/vda

using '\n' does not do what I would hope, so I think that leaves me two two imput ideas which I see

use cmd.stdin() after running cmd.execute(sfdisk /dev/vda)
create a file and read it in sfdisk /dev/vda < myfile.txt

I have been leaning towards option 1 as it seems the least invasive, but I dont think its working as I expect, and its difficult to debug blackbox tests.

Is option 2 even acceptable?

More on option 1:

Given ignition config 👇

            {
                        "ignition": {
                                "version": "3.3.0"
                        },
                        "storage": {
                                "disks": [{
                                        "device": "/dev/loop22",
                                        "partitions": [
                                        {
                                                "label": "newpart",
                                                "sizeMiB": 32
                                        },
                                        {
                                                "number": 1,
                                                "label": "foobar"
                                        }
                                        ]
                                }]
                        }
                }

the code expands it into an array of commands for sfdisk

{ "1 : start=2048 size=65536 name=foobar " ,
"size=65536 name=newpart "}

	cmd := exec.Command("sh", "-c", fmt.Sprintf("sudo %s %s", distro.SfdiskCmd(), op.dev))
	for _, s := range scripts {
		println("script %s", s)
		cmd.Stdin = bytes.NewBufferString(s)
	}
	cmd.Stdin = bytes.NewBufferString("quit")
	cmd.Stdin = bytes.NewBufferString("Y")

However then subsequently running a dump command I only find this std output

label: gpt
label-id: E800E3D5-7AE6-46ED-AC03-AD8E17F81947
device: /run/ignition/dev_aliases/dev/loop22
unit: sectors
first-lba: 34
last-lba: 133119
sector-size: 512

Which does not have any partitions listed, acting out these commands manually does; I wonder if cmd.stdin works like I am using it.

prestist · 2024-09-05T20:10:33Z

Ok, in the latest push a few things have happened.

Pulled in some code from sgdisk into sfdisk to allow for easier testing aka sgdiskcommit() and sgdiskbuildoptions()
Focused on pretend, it looks like sfdisk is now pretending correctly based on the scripts provided, which is good, as I was stuck. The answer was "%s" my script so the new line characters would work properly.
Found out that the code added for wipe is not sufficient, as it results in a currupt partition table. Spoke to @jlebon and I am going to look into how --zap-all works with sgdisk.

Next steps:

Found that partitions is doing some interpreting based on the pretend and it has an understanding of sgdisk today, need to expand that for sfdisk.
Proceed into commit
Fix wipe, as sgdisk --zap-all is functional and the code in sfdisk using wipefs results in a corrupt partition table.

jlebon · 2024-09-12T19:40:12Z

internal/sfdisk/sfdisk.go

+	// If wipe we need to reset the partition table
+	if op.wipe {
+		// sgdisk --zap-all /dev/sda destroys the partition table, let's do the same with wipefs
+		// Found out that wipefs is not sufficient, as it results in a corrupt partition table, look into sgdisk's way of doing --zap-all


Looks like util-linux/util-linux#3191 is an outstanding RFE to have an sgdisk --zap-all like option for sfdisk. We should verify the sgdisk code, but according to util-linux/util-linux#3191 (comment), it should be sufficient to zero out the first and last 34 sectors of the disk.

Hmm but actually, testing locally I can't get the corrupted partition table message to happen after using wipefs -a. It also seems like wipefs -a does not delete filesystem data hosted at the partitions as I previously thought.

Thank you; Yeah -a seems like the right combo for our needs. Thank you thank you.

internal/sfdisk/sfdisk.go

prestist · 2024-09-16T20:46:43Z

Latest push includes the parsing for sfdisk's pretend. Additionally added a few unit tests to sanity check the parsing. Will likely need to expand with examples but so far thats passing.

Additionally adjusted the wipe script as per @jlebon 's comment.

Next steps (from last comment):

✅ Found that partitions is doing some interpreting based on the pretend and it has an understanding of sgdisk today, need to expand that for sfdisk.
☑️ Proceed into commit
✅ Fix wipe, as sgdisk --zap-all is functional and the code in sfdisk using wipefs results in a corrupt partition table.

prestist · 2024-09-17T21:09:04Z

internal/exec/stages/disks/partitions.go

-)
+func getDeviceManager(logger *log.Logger, dev string) device_managers.DeviceManager {
+	// To be replaced with build tag support or something similar.
+	if false {


Note: will be dynamic based on arguments later just hard coded switch atm.

prestist · 2024-09-17T21:11:31Z

Update: added interface for device_managers (name could be better not sure what would be good; feedback wanted lol), thus abstracting implementation from partitions.go, additionally lifted and placed parse responsibility on the implementation rather then the caller.

Still need to work against commit.

This is a copy of sgdisk, with mapping around sfdisk tooling. Update the buildOptions to understand sfdisk requirements and focus on adding support for the pretend function. Fixes: https://issues.redhat.com/browse/COS-2837

Add an interface to abstract implementation details for partition management, allowing tooling to pivot between implementations. TBD: argument/tag reading for triggering the pivot Fixes: https://issues.redhat.com/browse/COS-2930

jlebon · 2024-09-24T19:12:58Z

internal/device_managers/deviceManagers.go

@@ -0,0 +1,28 @@
+package device_managers


How about partitioners?

Yeah thats a better name; will do!

jlebon · 2024-09-24T19:16:45Z