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.

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!

I know renames can be confusing in prs, so i did some commit juggling and it has been renamed; I hope I did it in a way that made sense.

internal/device_managers/sfdisk/sfdisk.go

jlebon · 2024-09-24T19:20:20Z

internal/device_managers/sfdisk/sfdisk.go

+
+	op.logger.Info("running sfdisk with script: %v", script)
+	exec.Command("sh", "-c", fmt.Sprintf("echo label: gpt | sudo %s %s", distro.SfdiskCmd(), op.dev))
+	cmd := exec.Command("sh", "-c", fmt.Sprintf("echo \"%s\" | sudo %s %s", script, distro.SfdiskCmd(), op.dev))


Same comments here.

Probably cleanest is to make a helper function that actually calls sfdisk and takes the script as an argument that it feeds to its stdin?

I gave it a swing

jlebon · 2024-09-24T19:30:56Z

internal/device_managers/sfdisk/sfdisk.go

+// ParseOutput takes the output from sfdisk. Similarly to sgdisk
+// it then uses regex to parse the output into understood values like 'start' 'size' and attempts
+// to catch any failures and wrap them to return to the caller.
+func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (map[int]device_managers.Output, error) {


OK yeah, this is pretty unfortunate. I was expecting --json or -d to also work in combination with --no-act, but that doesn't seem the case. Let's at least file an RFE upstream for now?

This has a high chance of regression if the output of sfdisk changes. We'll indeed want to make sure we have good coverage for this.

Yeah; 100%, Ok will do and tag it here once done.

jlebon · 2024-09-24T19:32:04Z

internal/device_managers/sfdisk/sfdisk.go

+	}
+
+	// Prepare the data, and a regex for matching on partitions
+	partitionRegex := regexp.MustCompile(`^/dev/\S+\s+\S*\s+(\d+)\s+(\d+)\s+\d+\s+\S+\s+\S+\s+\S+.*$`)


This deserves some explanation.

Also, I feel like we should ignore anything before the "New situation:" line.

Added some details

prestist · 2024-10-08T21:55:08Z

Ugh; I accidentally rebased too far back and effected external commits sigh; I will fix that.

edit: fixed

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

prestist force-pushed the sgdisk-fdisk branch from 4ce4905 to dd8f0b9 Compare August 30, 2024 15:07

prestist force-pushed the sgdisk-fdisk branch from dd8f0b9 to bba6095 Compare September 5, 2024 20:03

jlebon reviewed Sep 12, 2024

View reviewed changes

internal/sfdisk/sfdisk.go Outdated Show resolved Hide resolved

prestist force-pushed the sgdisk-fdisk branch from bba6095 to aaff066 Compare September 16, 2024 20:40

prestist commented Sep 17, 2024

View reviewed changes

prestist force-pushed the sgdisk-fdisk branch from 034d49f to 88e9194 Compare September 20, 2024 16:52

jlebon reviewed Sep 24, 2024

View reviewed changes

prestist force-pushed the sgdisk-fdisk branch 3 times, most recently from 1a20248 to 8e7dc92 Compare October 8, 2024 21:49

prestist force-pushed the sgdisk-fdisk branch from 8e7dc92 to 74f6842 Compare October 9, 2024 15:12

prestist added 2 commits October 16, 2024 15:35

distro: add sfdisk cmd

4269ad3

sfdisk: add sfdisk module with 'pretend' support

075a844

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

prestist force-pushed the sgdisk-fdisk branch 3 times, most recently from d152f1c to 32b61d7 Compare October 16, 2024 19:54

prestist force-pushed the sgdisk-fdisk branch from 32b61d7 to 99e632a Compare October 16, 2024 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: sfdisk implementation #1929

WIP: sfdisk implementation #1929

prestist commented Aug 26, 2024 •

edited

Loading

prestist commented Sep 3, 2024

prestist commented Sep 4, 2024 •

edited

Loading

prestist commented Sep 5, 2024

jlebon Sep 12, 2024

jlebon Sep 12, 2024

prestist Sep 12, 2024

prestist commented Sep 16, 2024

prestist Sep 17, 2024

prestist commented Sep 17, 2024

jlebon Sep 24, 2024

prestist Sep 25, 2024

prestist Oct 8, 2024

jlebon Sep 24, 2024

prestist Oct 8, 2024

jlebon Sep 24, 2024 •

edited by travier

Loading

prestist Sep 25, 2024

jlebon Sep 24, 2024

prestist Oct 8, 2024

prestist commented Oct 8, 2024 •

edited

Loading

WIP: sfdisk implementation #1929

Are you sure you want to change the base?

WIP: sfdisk implementation #1929

Conversation

prestist commented Aug 26, 2024 • edited Loading

prestist commented Sep 3, 2024

prestist commented Sep 4, 2024 • edited Loading

prestist commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestist commented Sep 16, 2024

Choose a reason for hiding this comment

prestist commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon Sep 24, 2024 • edited by travier Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestist commented Oct 8, 2024 • edited Loading

prestist commented Aug 26, 2024 •

edited

Loading

prestist commented Sep 4, 2024 •

edited

Loading

jlebon Sep 24, 2024 •

edited by travier

Loading

prestist commented Oct 8, 2024 •

edited

Loading