-
Notifications
You must be signed in to change notification settings - Fork 184
Use DPS UUIDs for root and boot partitions #4380
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
base: main
Are you sure you want to change the base?
Conversation
Use the Discoverable Partition Specification UUID for root and boot partitions We were already using DPS UUID for both partitions, but they were UUIDs for generic Linux Data Parition This lets us use `systemd-systemd-gpt-auto-generator` which is a good choice for LUKS decryption without UKI addons for composefs backend Use architecture specific DPS UUID for root partition, and use XBOOTLDR UUID for boot partition Related: coreos/fedora-coreos-tracker#2060 Closes: coreos/fedora-coreos-tracker#1038 Signed-off-by: Pragyan Poudyal <[email protected]>
ab9fbb8 to
a8f0ac0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly updates the partition type UUIDs for boot and root partitions across multiple architectures to use the Discoverable Partition Specification (DPS) values. This change is essential for enabling systemd-systemd-gpt-auto-generator. The review includes suggestions to define these new UUIDs as variables within the mpp-vars section of each manifest. This would improve maintainability and readability by replacing magic strings with named variables, a practice already in use for other UUIDs in these files.
| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: B921B045-1DF0-41C3-AF44-4C6F280D3FAE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and maintainability, consider defining these partition type UUIDs as variables in mpp-vars and referencing them here. This avoids using magic strings and makes their purpose clear. For example, you could add dps_boot_partition_type and dps_root_partition_type_aarch64 to mpp-vars.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_aarch64}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: B921B045-1DF0-41C3-AF44-4C6F280D3FAE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please apply the same variable-based approach for partition type UUIDs in the image4k definition.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_aarch64}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: C31C45E6-3F39-412E-80FB-4809C4980599 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and maintainability, consider defining these partition type UUIDs as variables in mpp-vars and referencing them here. This avoids using magic strings and makes their purpose clear. For example, you could add dps_boot_partition_type and dps_root_partition_type_ppc64le to mpp-vars.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_ppc64le}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: C31C45E6-3F39-412E-80FB-4809C4980599 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please apply the same variable-based approach for partition type UUIDs in the image4k definition.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_ppc64le}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 72EC70A6-CF74-40E6-BD49-4BDA08E8F224 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and maintainability, consider defining these partition type UUIDs as variables in mpp-vars and referencing them here. This avoids using magic strings and makes their purpose clear. For example, you could add dps_boot_partition_type and dps_root_partition_type_riscv64 to mpp-vars.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_riscv64}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 72EC70A6-CF74-40E6-BD49-4BDA08E8F224 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please apply the same variable-based approach for partition type UUIDs in the image4k definition.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_riscv64}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}" | ||
| partnum: 3 | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 5EEAD9A9-FE09-4A1E-A1D7-520D00531306 | ||
| partnum: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and maintainability, consider defining these partition type UUIDs as variables in mpp-vars and referencing them here. This avoids using magic strings and makes their purpose clear. For example, you could add dps_boot_partition_type and dps_root_partition_type_s390x to mpp-vars.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}"
partnum: 3
- name: root
type:
mpp-format-string: '{dps_root_partition_type_s390x}'
partnum: 4| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}" | ||
| partnum: 3 | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 5EEAD9A9-FE09-4A1E-A1D7-520D00531306 | ||
| partnum: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please apply the same variable-based approach for partition type UUIDs in the image4k definition.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}"
partnum: 3
- name: root
type:
mpp-format-string: '{dps_root_partition_type_s390x}'
partnum: 4| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability and maintainability, consider defining these partition type UUIDs as variables in mpp-vars and referencing them here. This avoids using magic strings and makes their purpose clear. For example, you could add dps_boot_partition_type and dps_root_partition_type_x86_64 to mpp-vars.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_x86_64}'| - name: boot | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: BC13C2FF-59E6-4262-A352-B275FD6F7172 | ||
| size: | ||
| mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}" | ||
| - name: root | ||
| type: 0FC63DAF-8483-4772-8E79-3D69D8477DE4 | ||
| type: 4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please apply the same variable-based approach for partition type UUIDs in the image4k definition.
- name: boot
type:
mpp-format-string: '{dps_boot_partition_type}'
size:
mpp-format-int: "{boot_size_mb * 1024 * 1024 / four_k_sector_size}"
- name: root
type:
mpp-format-string: '{dps_root_partition_type_x86_64}'
jbtrystram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM but could you add the reference spec in the commit message ?
Also, does it interfere with coreos-boot-generator ? I guess we would drop that entirely ?
|
@Johan-Liebert1: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Unfortunately we don't include that right now. It got re-introduced recently and broke stuff. If we were to make this change I think we'd need to make some more careful consideration. Would we want to do it everywhere immediately? Or would we want to phase this in? Right now changes here land downstream in cc @travier |
Use the Discoverable Partition Specification UUID for root and boot
partitions
We were already using DPS UUID for both partitions, but they were UUIDs
for generic Linux Data Parition
This lets us use
systemd-systemd-gpt-auto-generatorwhich is a goodchoice for LUKS decryption without UKI addons for composefs backend
Use architecture specific DPS UUID for root partition, and use XBOOTLDR
UUID for boot partition
Related: coreos/fedora-coreos-tracker#2060
Closes: coreos/fedora-coreos-tracker#1038