search

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

Use GPT label when parted is available or sfdisk supports it #193

Open eburgueno opened 7 years ago

eburgueno commented 7 years ago

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.26 for consistency, but in the CentOS image only 2.23.2 is included so we need to make it an exception.

rh-atomic-bot commented 7 years ago

Can one of the admins verify this patch? I understand the following commands:

rhatdan commented 7 years ago

bot, add author to whitelist

rhvgoyal commented 7 years ago

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.

eburgueno commented 7 years ago

@rhvgoyal ok, I made some changes in https://github.com/projectatomic/docker-storage-setup/pull/193/commits/11302194379b53820da5239195414224f44361c2 that implement that.

eburgueno commented 7 years ago

I just realised the automated tests only run on a fedora/25/atomic image (which doesn't come with parted), and with a secondary disk of 10GB. So I did some manual testing for the following cases:

The question is, does your infrastructure allow using a centos/7/atomic image (or however you call it) as well? what about disk sizes of > 2TB? Happy to create new tests if so.

rhvgoyal commented 7 years ago

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.

eburgueno commented 7 years ago

@rhvgoyal You mean to add "Signed-off-by" to my commit message? who should I say signed it off? (you may need a CONTRIBUTING file in the repo to clarify these :) )

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.

rhvgoyal commented 7 years ago

@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.

rhvgoyal commented 7 years ago

@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.

Signed-off-by: Your Name <your email id>

rhatdan commented 7 years ago

Yes we should use the default git uses

git commit -a --sign

eburgueno commented 7 years ago

@rhvgoyal looks like re-writing history worked :)

rh-atomic-bot commented 7 years ago

:umbrella: The latest upstream changes (presumably abf5548) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot commented 7 years ago

Can one of the admins verify this patch? I understand the following commands:

rh-atomic-bot commented 7 years ago

:umbrella: The latest upstream changes (presumably 0d18761) made this pull request unmergeable. Please resolve the merge conflicts.