Use GPT label when parted is available or sfdisk supports it #193
Conversation
|
Can one of the admins verify this patch?
|
|
bot, add author to whitelist |
|
I think we should check the size of disk and if it is more than 2TB, create GPT table otherwise fallback to msdos type table. That way we will not have surprises, if any. |
|
I just realised the automated tests only run on a
The question is, does your infrastructure allow using a |
|
Can you put "Signed-off-by". Also I see 3 commits in your branch. Is that your intention? I thought only topmost commit is relevant. If that's the case why not create a separate branch and just put one commit there. |
|
@rhvgoyal You mean to add "Signed-off-by" to my commit message? who should I say signed it off? (you may need a As for the commits, each one of them introduced new changes so they're all relevant (I commit "early and often"). I can squash them into a single one, even on a separate branch if you prefer? Not sure how GitHub would deal with it though, I might need to open a new PR. |
|
@eburgueno if patch is small enough, then one patch is good. But if single patch is become too big then I prefer that it is broken down into multiple patches. In your case github is only showing top level commit. Generally it shows all the commits which are being pushed so that I can review all the patches. I think rebase your patches on top of latest and try pushing again and see if it works. Or try creating a new PR. |
|
@eburgueno W.r.t Signed-off-by, I am still trying to figure out what should be our conventions. For now if you can just put your signed off by, that would work.
|
|
Yes we should use the default git uses git commit -a --sign |
eburgueno
force-pushed
the
master
branch
2 times, most recently
from
b6fa70e to
1130219
Compare
|
@rhvgoyal looks like re-writing history worked :) |
docker-storage-setup.sh
Outdated
|
|
||
| #Determine partition size. If larger than 2TB use GPT instead | ||
| size=$( awk "\$4 ~ /"$( basename $dev )"/ { print \$3 }" /proc/partitions ) | ||
| if [ $size -gt 2147483648 ]; then label='gpt'; else label='msdos'; fi |
There was a problem hiding this comment.
In rest of the script, we are following,
if condition; then
statement
fi
can we stick to that.
docker-storage-setup.sh
Outdated
| local dev="$1" | ||
| parted $dev --script mklabel msdos mkpart primary 0% 100% set 1 lvm on | ||
| local dev="$1" label="$2" | ||
| parted $dev --script mklabel $label mkpart primary 0% 100% set 1 lvm on |
There was a problem hiding this comment.
If label is of type gpt then keyword primary is not partition type any more. mkpart will expect a partition name. I am assuming right now it is using string primary as partition name instead.
There was a problem hiding this comment.
Oh, I didn't know that. Interestingly parted doesn't say anything in interactive mode, but it does on the man pages:
(parted) help mkpart
mkpart PART-TYPE [FS-TYPE] START END make a partition
PART-TYPE is one of: primary, logical, extended
FS-TYPE is one of: btrfs, nilfs2, ext4, ext3, ext2, fat32, fat16, hfsx, hfs+, hfs, jfs, swsusp, linux-swap(v1), linux-swap(v0), ntfs, reiserfs, hp-ufs, sun-ufs, xfs, apfs2, apfs1, asfs, amufs5, amufs4,
amufs3, amufs2, amufs1, amufs0, amufs, affs7, affs6, affs5, affs4, affs3, affs2, affs1, affs0, linux-swap, linux-swap(new), linux-swap(old)
START and END are disk locations, such as 4GB or 10%. Negative values count from the end of the disk. For example, -1s specifies exactly the last sector.
'mkpart' makes a partition without creating a new file system on the partition. FS-TYPE may be specified to set an appropriate partition ID.
# man parted
(...)
mkpart [part-type name fs-type] start end
Create a new partition. part-type may be specified only with msdos and dvh partition tables, it should be one of "primary", "logical", or "extended". name is required for GPT partition tables and fs-type is optional. fs-type can be one of "btrfs", "ext2", "ext3", "ext4", "fat16", "fat32", "hfs", "hfs+", "linux-swap", "ntfs", "reiserfs", or "xfs".
So yes, the above would set "primary" as the partition name. Since a name is required, do you have any preferences for which name to use?
There was a problem hiding this comment.
On this note, when using sfdisk no name seems to be set:
(parted) print
Model: ATA VBOX HARDDISK (scsi)
Disk /dev/sdb: 8590MB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
1 1049kB 8590MB 8589MB
When trying to recreate the partition with parted 3.1 (included in CentOS 7) or parted 3.2 (Fedora 25/cloud-base), mkpart seems to expect a non-empty string for name but only when running in non-interactive mode:
(parted) rm 1
(parted) q
[root@localhost ~]# parted /dev/sdb --script mkpart "" 0% 100%
[root@localhost ~]# parted /dev/sdb
(parted) print
Model: ATA VBOX HARDDISK (scsi)
Disk /dev/sdb: 8590MB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
(parted) mkpart
Partition name? []?
File system type? [ext2]?
Start? 2048
End? -1
(parted) print
Model: ATA VBOX HARDDISK (scsi)
Disk /dev/sdb: 8590MB
Sector size (logical/physical): 512B/512B
Partition Table: gpt
Disk Flags:
Number Start End Size File system Name Flags
1 2048MB 8589MB 6541MB
(parted)
I find this inconsistency quite annoying, but it does mean that we will need a partition name when using parted.
docker-storage-setup.sh
Outdated
| if $(sfdisk --help|grep gpt >/dev/null) ; then | ||
| label='label: gpt' | ||
| type='' | ||
| size=$(( $size - 34 )) |
There was a problem hiding this comment.
where does this -34 come from?
There was a problem hiding this comment.
Well, 34 is the first usable LBA sector when using GPT tables. sfdisk complains about not enough sectors available if we simply use the total number of sectors:
-bash-4.3# size=$(( $( awk "\$4 ~ /"$( basename /dev/sdb )"/ { print \$3 }" /proc/partitions ) * 2 - 2048 ))
-bash-4.3# echo $size
16775168
-bash-4.3# cat <<EOF | sfdisk /dev/sdb
> unit: sectors
> label: gpt
>
> /dev/sdb1 : start= 2048, size= ${size}
> EOF
Checking that no-one is using this disk right now ... OK
Disk /dev/sdb: 8 GiB, 8589934592 bytes, 16777216 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
>>> Script header accepted.
>>> Script header accepted.
>>> Created a new GPT disklabel (GUID: 05700A90-D214-4CE9-BFFB-4251619AC4A5).
The last usable GPT sector is 16777182, but 16777215 is requested.
Failed to add partition: Invalid argument
Leaving.
-bash-4.3# size=$(( $size - 34 ))
-bash-4.3# echo $size
16775134
-bash-4.3# cat <<EOF | sfdisk /dev/sdb
unit: sectors
label: gpt
/dev/sdb1 : start= 2048, size= ${size}
EOF
Checking that no-one is using this disk right now ... OK
Disk /dev/sdb: 8 GiB, 8589934592 bytes, 16777216 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
>>> Script header accepted.
>>> Script header accepted.
>>> Created a new GPT disklabel (GUID: FC8D245F-4DA6-4E6D-9BED-BB17C4087FE8).
Created a new partition 1 of type 'Linux filesystem' and of size 8 GiB.
/dev/sdb2:
New situation:
Device Start End Sectors Size Type
/dev/sdb1 2048 16777181 16775134 8G Linux filesystem
The partition table has been altered.
Calling ioctl() to re-read partition table.
Syncing disks.
-bash-4.3#
I don't know why this is not needed for MBR though.
docker-storage-setup.sh
Outdated
| # does not support GPT then we are limited to 2TB | ||
| if [ $label = "gpt" ]; then | ||
| if $(sfdisk --help|grep gpt >/dev/null) ; then | ||
| label='label: gpt' |
There was a problem hiding this comment.
Is there a reason to use single quote as opposed to double quotes.
There was a problem hiding this comment.
No reason. Will update to use double quotes instead.
|
☔ The latest upstream changes (presumably abf5548) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can one of the admins verify this patch?
|
- Use parted by default if available, fallback to sfdisk - Use GPT only when disk is larger than 2TB - Limit partition size to 2TB if parted is not available and sfdisk does not support GPT Signed-off-by: Eric Burgueño <[email protected]>
|
☔ The latest upstream changes (presumably 0d18761) made this pull request unmergeable. Please resolve the merge conflicts. |
Now that you can use parted for managing the partitions (#191), would it be possible to switch to GPT instead of MBR? This would help addressing #114.
This will require at least
sfdisk 2.26for consistency, but in the CentOS image only2.23.2is included so we need to make it an exception.