Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(FACT-3140) Include partition type uuid for GPT based systems #2511

Closed
wants to merge 1 commit into from

Conversation

jcpunk
Copy link
Contributor

@jcpunk jcpunk commented Aug 5, 2022

I've taken a start at getting GPT GUID types into facter.

I fear I'll need a bit of help getting it over the line, but it should be a workable starting point....

@jcpunk jcpunk requested a review from a team as a code owner August 5, 2022 16:31
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@@ -121,7 +121,7 @@ def execute_and_extract_blkid_info
def populate_from_lsblk(partition_name, blkid_and_lsblk)
return {} unless available?('lsblk', blkid_and_lsblk)

blkid_and_lsblk[:lsblk] ||= Facter::Core::Execution.execute('lsblk -fp', logger: log)
blkid_and_lsblk[:lsblk] ||= Facter::Core::Execution.execute('lsblk -p -P -o NAME,FSTYPE,LABEL,UUID,PARTTYPE', logger: log)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the command lsblk -p -P -o NAME,FSTYPE,LABEL,UUID,PARTTYPE on a centos-7-x86_64 VM and it errored out with:

lsblk: unknown column: PARTTYPE

Looks like on redhat-9-x86_64 it does work as expected:

lsblk -p -P -o NAME,FSTYPE,LABEL,UUID,PARTTYPE
NAME="/dev/sda" FSTYPE="" LABEL="" UUID="" PARTTYPE=""
NAME="/dev/sda1" FSTYPE="xfs" LABEL="" UUID="e6a7819d-3337-4d37-8789-d6a08ff87af8" PARTTYPE="0x83"
NAME="/dev/sda2" FSTYPE="LVM2_member" LABEL="" UUID="yhI4fj-F1mX-rstz-2D5w-MFtV-Myil-LW5pl7" PARTTYPE="0x8e"
NAME="/dev/mapper/rhel-root" FSTYPE="xfs" LABEL="" UUID="9ff66d86-abea-4244-b7c2-4547f15db06d" PARTTYPE=""
NAME="/dev/mapper/rhel-swap" FSTYPE="swap" LABEL="" UUID="22260cab-8b6e-4777-af64-9f7e849d58d2" PARTTYPE=""
NAME="/dev/mapper/rhel-home" FSTYPE="xfs" LABEL="" UUID="326a248f-4e90-4207-92d0-33baa143774b" PARTTYPE=""
NAME="/dev/sr0" FSTYPE="" LABEL="" UUID="" PARTTYPE=""

Is the PARTYPE that important? Could we possibly drop it so we don't have to add in some logic to determine if lsblk is the right version that will support PARTTYPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear PARTTYPE is what I was hopeful to capture with this patch set....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like PARTTYPE was added in util-linux/util-linux@4686ba1 and first released in 2.25. So we'd need to make this conditional based on the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do that....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to do something like what this resolver does

output = Facter::Core::Execution.execute("#{command} --version 2>&1", logger: log)

to detect the version and only include the new fact on more recent versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at implementation.

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023

CLA assistant check
All committers have signed the CLA.

@jcpunk
Copy link
Contributor Author

jcpunk commented May 24, 2024

Rebased off HEAD

@joshcooper
Copy link
Contributor

joshcooper commented Jul 26, 2024

@jcpunk you'll need to stub out the call to lsblk --version 2>&1 similar to

allow(Facter::Core::Execution).to receive(:execute)
.with('lsblk -fp', logger: an_instance_of(Facter::Log)).and_return(load_fixture('lsblk_output').read)

Also please add a test to test the fallback behavior when the version is older and verify facter omits the partition type.

@jcpunk
Copy link
Contributor Author

jcpunk commented Jul 26, 2024

Long term it might make more sense to just run the version with the extra options and have it catch the error if it isn't supported.

It may be a good while before I get back to this.

@mhashizume
Copy link
Contributor

mhashizume commented Aug 2, 2024

I've started digging into this and I think the way the partition resolver currently works, in most cases the resolver won't even use lsblk. The partitions resolver will always try to get the output from blkid first and will only use lsblk as a fallback:

part_info = populate_from_blkid(partition_name, blkid_and_lsblk)
return populate_from_lsblk(partition_name, blkid_and_lsblk) if part_info.empty?

However, it seems like lsblk can return all of the information that blkid can and more. It might make sense to prefer lsblk over blkid in the resolver and add PARTUUID to lsblk's output to reach parity with what we already return with blkid. (PARTUUID was added to lsblk in 2.22, so the version check you already have should be fine.)

Also a nit, but I think "GUID" is preferred over "UUID" in the context of partition types. The UEFI spec refers to what we're discussing here as "PartitionTypeGUID" (page 125): https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf

I can work more on this next week and put up my own PR if you can't get back to this for a while.

@jcpunk
Copy link
Contributor Author

jcpunk commented Aug 2, 2024

If you're able to put one together that would be great!

@mhashizume
Copy link
Contributor

Closing this in favor of #2745

@mhashizume mhashizume closed this Aug 5, 2024
@jcpunk jcpunk deleted the fact-3140 branch August 6, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants