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

Add Second partition for storage-drive testing #3594

Merged
merged 1 commit into from
Aug 10, 2019

Conversation

cevich
Copy link
Member

@cevich cevich commented Jul 18, 2019

This is mainly/initially to support use of Cirrus-CI
in https://github.com/containers/storage since that setup
re-uses the VM images from this project. However, it also
opens doors here, if libpod ever needs/wants to do things
with a dedicated storage device and/or storage-drivers.

Depends on #3632

@cevich cevich changed the title Second partition WIP: Second partition Jul 18, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Jul 18, 2019
@cevich cevich force-pushed the second_partition branch 2 times, most recently from e436b79 to 1f07da1 Compare July 18, 2019 12:58
@TomSweeneyRedHat
Copy link
Member

Tests aren't hip @cevich

@cevich
Copy link
Member Author

cevich commented Jul 18, 2019

Oh no they're not, completely FUBARd. I'm on one of the VMs twidling bits trying to figure out to make this work :D

@cevich cevich force-pushed the second_partition branch 4 times, most recently from 6fa1242 to c2b366d Compare July 19, 2019 18:53
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3601) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2019
@cevich cevich force-pushed the second_partition branch 2 times, most recently from baa0572 to 4d1122b Compare July 23, 2019 17:41
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2019
@cevich cevich changed the title WIP: Second partition WIP: Add Second partition for storage-drive testing Jul 24, 2019
@cevich cevich force-pushed the second_partition branch 3 times, most recently from b882875 to 2468826 Compare July 25, 2019 14:28
@cevich cevich force-pushed the second_partition branch 2 times, most recently from 3f0cfd6 to 87f4677 Compare July 26, 2019 16:40
@cevich cevich changed the title WIP: Add Second partition for storage-drive testing Add Second partition for storage-drive testing Jul 26, 2019
@openshift-ci-robot openshift-ci-robot added size/L and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL labels Jul 26, 2019
@cevich cevich force-pushed the second_partition branch 3 times, most recently from f7e27de to 297e81b Compare July 26, 2019 17:58
@cevich
Copy link
Member Author

cevich commented Jul 26, 2019

@rhatdan @nalind PTAL, this is ultimately for buildah's benefit. Once this can merge, I'll work on bringing the capability over there.

@mheon @baude I'm assuming libpod CI doesn't currently need to test with a device-mapper storage driver or similar "special" storage setup (swap enabled?). Otherwise let me know.

@rhatdan
Copy link
Member

rhatdan commented Jul 29, 2019

LGTM
But we originally wanted this for containers/storage so that we could run lvm tests for it.

@cevich
Copy link
Member Author

cevich commented Jul 29, 2019

Oh woops. No matter, containers/storage reuses the images from this project also. Glad I didn't start the work on buildah yet 😄

@cevich
Copy link
Member Author

cevich commented Jul 31, 2019

@rhatdan this PR is the other reason I want to try using the libpod produced VM cache-images in c/storage and c/buildah. (First reason being, fixing the stupid dpkg is locked thing).

@mheon
Copy link
Member

mheon commented Jul 31, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019
@rhatdan
Copy link
Member

rhatdan commented Aug 1, 2019

@mheon @edsantiago @vrothberg @TomSweeneyRedHat @baude PTAL and lets get this merged.

@TomSweeneyRedHat
Copy link
Member

LGTM

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I'm pretty baffled by this, but that's because I have no understanding of the /dev/sda setup offered by the CI environment. It might be nice in this or a future PR to document that, explain how it is that parted can be relied upon to have that space available in the sda device. As long as this works, I guess it's ok, it's just confusing.

contrib/cirrus/add_second_partition.sh Outdated Show resolved Hide resolved
contrib/cirrus/lib.sh Outdated Show resolved Hide resolved
contrib/cirrus/setup_environment.sh Outdated Show resolved Hide resolved
@cevich
Copy link
Member Author

cevich commented Aug 1, 2019

explain how it is that parted can be relied upon to have that space available in the sda device. As long as this works, I guess it's ok, it's just confusing.

I'll add a comment about this. The "expansion space" is created because of the difference between base-image build time (20gig storage request) and the runtime request (200gig always otherwise gcloud issues warning). We have a unit-test in place to catch the 200gig case, and I tested manually to verify parted does fail safe:

If when the add_second_partition.sh runs, /dev/sda1 uses all the space, parted gives a message like: "Error, you asked for 50%, rounding up due to allocation. Refusing to grow partition from 100% to 100%" then exit's non-zero.

There are other ways this can break though, and no good solutions that are less complicated (I spent about two weeks trying - no joke 😦)

This is mainly/initially to support use of Cirrus-CI
in https://github.com/containers/buildah since that setup
re-uses the VM images from this project. However, it also
opens doors here, if libpod ever needs/wants to do things
with a dedicated storage device and/or storage-drivers.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the second_partition branch from 297e81b to 0a05af1 Compare August 1, 2019 18:08
@edsantiago
Copy link
Member

200gig always otherwise gcloud issues warning

That's right - I remember now. It's just that there are so many obscure trivial details to remember. Comments are helpful for that. Thanks.

@cevich
Copy link
Member Author

cevich commented Aug 2, 2019

It's just that there are so many obscure trivial details to remember.

Well I guess that makes me the king of obscure trivial details then (depending on my current level of caffeination) 🤣 I do really appreciate the feedback. There's nothing better than another pair of eyes to spot camouflaged assumptions.

@cevich
Copy link
Member Author

cevich commented Aug 2, 2019

(This PR does not need to go in until after release)

@cevich
Copy link
Member Author

cevich commented Aug 9, 2019

Okay, this one is ready @baude @mheon (when things quiet down a little).

@rhatdan
Copy link
Member

rhatdan commented Aug 10, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 926901d into containers:master Aug 10, 2019
@cevich cevich deleted the second_partition branch June 30, 2021 18:03
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants