projectatomic / container-storage-setup

Service to set up storage for Docker and other container systems
Apache License 2.0
153 stars 77 forks source link

Add support for GPT partition table for disks in size bigger than 2TB #266

Closed rhvgoyal closed 6 years ago

rhvgoyal commented 6 years ago

Right now we create MSDOS partition tables by default and that woks only till partition/disk size of 2TB. If somebody passes in a bigger disk, both sfdisk and parted fail and container storage setup fails. This patch series detects if disk is bigger than 2TB and if it is, then it switches to using gpt format partition table.

rhvgoyal commented 6 years ago

cc @rhatdan @cgwalters

rhatdan commented 6 years ago

Any reason you can't use gpt to build both images?

rhvgoyal commented 6 years ago

In rhel, util-linux is old and sfdisk does not support gpt. So if I switch to gpt even for sfdisk, that will be broekn in rhel. I think parted did support gpt even back then. So parted probably will be fine.

Other reason is just being little conservative and not break anything working until and unless we have to. I am thinking of doing another PR and introduce a config option say CSS_USE_GPT=yes. That will force usage of GPT irrespective of disk size. And then we can make it default at some point of time in future where rhel has newer util-linux.

rhvgoyal commented 6 years ago

IOW, if things are working well with MSDOS table for disks upto 2TB, I really don't have a good reason to change that. It has downside of breaking something without any significant upside.

cgwalters commented 6 years ago

One thing that was noted as part of today's Fedora Atomic WG meeting is that we're investigating Ignition, which should take over the role of making disks for these types of things I think.

cgwalters commented 6 years ago

This looks reasonable to me though at first glance. It seems like it shouldn't be too hard to CI test this by using a sparse loopback disk right?

rhvgoyal commented 6 years ago

@cgwalters Sure if Ignition takes over, that's not a problem. We should be able to write a test for this on loop device. Will do.

rhvgoyal commented 6 years ago

@cgwalters @rhatdan I added the tests. PTAL.

rhatdan commented 6 years ago

LGTM Is this something we will need to modify the spec file to make sure that gpt is installed or is in the same package as the msdos version?

rhvgoyal commented 6 years ago

Old sfdisk does not handle GPT. But old parted does. So we need to make sure "parted" is installed. I think we had a bug where we made docker/container-storage-setup requiring parted. Let me check and see if that change finally made it in or not.

rhvgoyal commented 6 years ago

@rhatdan, In fedora and rhel container-storage-setup, I see it has "Requires: parted". So I think we should be good and this should not require any spec file changes.

cgwalters commented 6 years ago

@rh-atomic-bot r+ c019f15

rh-atomic-bot commented 6 years ago

:zap: Test exempted: pull fully rebased and already tested.