Rework named volumes in DB #2774

mheon · 2019-03-26T19:36:20Z

I haven't done more than compile test this, but the code's finally done, so I figured I'd put it up for review.

This swaps the backend for named volumes, making them explicit in the database rather than intermixed with other container volumes. This has numerous advantages, especially with volumes that require mounts - we can ensure they're mounted when we start the container easily.

The biggest issue with this is the changes to the way the database tracks dependency containers for a volume. I've written things such that we don't need any changes to existing containers in the database - things should continue working as before on an upgrade. We won't know to remove container dependencies from the volume as it is removed, but the DB has been changed to gracefully handle this. The issue will come if you downgrade to an earlier Podman after using this patch - you may end up with volumes that can never be removed without upgrading back to a version with this patch. I don't have a good way around that.

openshift-ci-robot · 2019-03-26T19:36:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [mheon]

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

mheon · 2019-03-26T20:48:49Z

@rhatdan What would you say about removing podman volume rm --force?

I really don't like the concept of force-removing volumes. You end up with a bunch of containers that don't work anymore because a volume they need went and disappeared.

We should either require --force to remove the containers using the volume, or remove it entirely.

mheon · 2019-03-26T20:49:43Z

(That is why the tests are failing - I ripped out a lot of code related to volume rm --force because it violated DB conventions and might have been able to wedge containers into a state where they couldn't be removed)

rhatdan · 2019-03-27T11:17:45Z

podman volume rm --force should be removing any containers that rely on the volume.

giuseppe · 2019-03-27T11:44:59Z

pkg/spec/spec.go

@@ -126,7 +152,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint
 		g.RemoveMount("/dev/mqueue")
 		devMqueue := spec.Mount{
 			Destination: "/dev/mqueue",
-			Type:        "bind",
+			Type:        bindMount,


we could probably just drop the Type altogether. The OCI runtime looks at the options to see if it is a bind mount (either contains bind or rbind)

I'll leave it for now, it's in the official OCI spec.

giuseppe · 2019-03-27T11:50:17Z

pkg/spec/spec.go

+			continue
+		}
+		// Volumes that are not named volumes must be an absolute or relative path
+		if strings.Contains(mount.Source, "/") || strings.Contains(mount.Source, ".") {


it seems that Docker doesn't accept relative paths, but accept named volumes as foo.bar. If we want to extend it in a non breaking way, we could limit relative paths to having a prefix like ./, but raise an error on, e.g. -v .foo:/foo

What do you think?

Right now we're using the same name regex for volumes that we do for pods and containers - letters, numbers, underscores only. I need to check and see what Docker uses for their volume name regex.

I don't know if I mind requiring that volume names not allow . if it makes detecting/allowing relative paths easier. @rhatdan @giuseppe WDYT?

I don't like the idea of not allowing volume names that Docker does allow.

We can document that we allow paths that begin with . and / only.

Alright, looking at this, it seems like Docker allows . in volume and container names now. No other changes I could see.

Will modify our regex in a separate PR to handle this, then update this PR.

Fixed. We now use HasPrefix() to check only the first character which is guaranteed to be alphanumeric for names.

mheon · 2019-03-29T14:22:38Z

podman volume rm --force now removes all containers using the volume.

Only remaining question: should we force-remove those containers (stop them if they're running)?

baude · 2019-03-29T14:30:29Z

LGTM

mheon · 2019-03-29T14:30:59Z

@rhatdan @giuseppe @vrothberg @TomSweeneyRedHat PTAL

rh-atomic-bot · 2019-03-29T14:50:34Z

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

mheon · 2019-03-29T15:33:11Z

I apparently broke volumes-from here

mheon · 2019-03-29T17:08:14Z

Going to take a few hours to fix this, and it's not really release critical, so we'll go without it.

mheon · 2019-04-03T23:15:14Z

Finding heaps of bugs with the --mount flag at the same time I'm working on this. Going to try and get this in as-is, and rework the rest in a followup.

mheon · 2019-04-03T23:56:57Z

Alright, if this passes tests, I think it's ready. Diving into this, I have way more work to do on the volumes side - the --mount flag is EXTREMELY buggy right now, I want to move image volume handling into libpod, and named volumes need to regain their locks and gain Mount() and Unmount() functions (plus the ability to know when their dependent containers are using them).

mheon · 2019-04-04T03:11:38Z

35 minutes to build 12 commits? Wow.

Alright, squashing down further.

mheon · 2019-04-04T04:17:30Z

Add to our --mount woes: commit handling of volume mounts is definitely wrong. Unfortunately, we have tests that verify the incorrect behavior, which seem to be failing here.

rh-atomic-bot · 2019-04-04T16:23:45Z

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

Signed-off-by: Matthew Heon <[email protected]>

This swaps the previous handling (parse all volume mounts on the container and look for ones that might refer to named volumes) for the new, explicit named volume lists stored per-container. It also deprecates force-removing volumes that are in use. I don't know how we want to handle this yet, but leaving containers that depend on a volume that no longer exists is definitely not correct. Signed-off-by: Matthew Heon <[email protected]>

Replaces old functionality we used for handling image volumes. Signed-off-by: Matthew Heon <[email protected]>

Now that named volumes must be explicitly enumerated rather than passed in with all other volumes, we need to split normal and named volumes up before passing them into libpod. This PR does this. Signed-off-by: Matthew Heon <[email protected]>

Signed-off-by: Matthew Heon <[email protected]>

The flag should be substantially more durable, and no longer relies on the create artifact. This should allow it to properly handle our new named volume implementation. Signed-off-by: Matthew Heon <[email protected]>

We were never using it. It's actually a potentially quite sizable field (very expensive to decode an array of structs!). Removing it should do no harm. Signed-off-by: Matthew Heon <[email protected]>

The Commit test is blatantly wrong and testing buggy behavior. We should be commiting the destination, if anything - and more likely nothing at all. When force-removing volumes, don't remove the volumes of containers we need to remove. This can lead to a chicken and the egg problem where the container removes the volume before we can. When we re-add volume locks this could lead to deadlocks. I don't really want to deal with this, and this doesn't seem a particularly harmful quirk, so we'll let this slide until we get a bug report. Signed-off-by: Matthew Heon <[email protected]>

mheon · 2019-04-04T16:27:58Z

Rebased, tests ought to be fixed

mheon · 2019-04-04T17:20:22Z

Tests finally passing.
@rhatdan @baude @vrothberg @giuseppe @TomSweeneyRedHat PTAL

rhatdan · 2019-04-04T18:30:18Z

@mheon Nice work
LGTM

TomSweeneyRedHat · 2019-04-04T18:54:34Z

test/e2e/volume_rm_test.go

@@ -47,7 +47,7 @@ var _ = Describe("Podman volume rm", func() {
 		Expect(len(session.OutputToStringArray())).To(Equal(0))
 	})

-	It("podman rm with --force flag", func() {
+	It("podman volume rm with --force flag", func() {


Is there a test with a volume that is removed that's associated with multiple containers?

TomSweeneyRedHat

LGTM.
One testing question. Would like a head nod from either @giuseppe or @vrothberg

giuseppe · 2019-04-04T18:56:45Z

/lgtm

openshift-ci-robot added the size/L label Mar 26, 2019

openshift-ci-robot requested review from rhatdan and umohnani8 March 26, 2019 19:36

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2019

giuseppe reviewed Mar 27, 2019

View reviewed changes

mheon force-pushed the db_rework_named_volume branch from d6e2bd3 to 37ed099 Compare March 29, 2019 14:20

mheon force-pushed the db_rework_named_volume branch from 37ed099 to e4f637b Compare March 29, 2019 14:55

mheon force-pushed the db_rework_named_volume branch from e4f637b to 97deed5 Compare April 3, 2019 22:59

openshift-ci-robot added size/XL and removed size/L labels Apr 3, 2019

mheon force-pushed the db_rework_named_volume branch 4 times, most recently from a5d2490 to 6fc464a Compare April 4, 2019 01:23

mheon force-pushed the db_rework_named_volume branch from 6fc464a to 2de3f26 Compare April 4, 2019 03:13

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL labels Apr 4, 2019

openshift-ci-robot added size/XL and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 4, 2019

mheon mentioned this pull request Apr 4, 2019

WIP Support nfs local volume mounts #2415

Closed

mheon added the Release Notes 1.3.0 label Apr 4, 2019

mheon force-pushed the db_rework_named_volume branch from 7bd786c to 28d1f0e Compare April 4, 2019 16:11

mheon force-pushed the db_rework_named_volume branch from 28d1f0e to 1e82a03 Compare April 4, 2019 16:25

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2019

mheon added 8 commits April 4, 2019 12:26

Add named volumes for each container to database

11799f4

Signed-off-by: Matthew Heon <[email protected]>

Create non-existing named volumes at container create

ee770ad

Replaces old functionality we used for handling image volumes. Signed-off-by: Matthew Heon <[email protected]>

Volume force-remove now removed dependent containers

3e066e2

Signed-off-by: Matthew Heon <[email protected]>

Major rework of --volumes-from flag

42c95ee

The flag should be substantially more durable, and no longer relies on the create artifact. This should allow it to properly handle our new named volume implementation. Signed-off-by: Matthew Heon <[email protected]>

Drop LocalVolumes from our the database

1fdc89f

We were never using it. It's actually a potentially quite sizable field (very expensive to decode an array of structs!). Removing it should do no harm. Signed-off-by: Matthew Heon <[email protected]>

mheon force-pushed the db_rework_named_volume branch from 1e82a03 to 02c6110 Compare April 4, 2019 16:27

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2019

TomSweeneyRedHat reviewed Apr 4, 2019

View reviewed changes

openshift-ci-robot assigned giuseppe Apr 4, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2019

openshift-merge-robot merged commit e320efe into containers:master Apr 4, 2019

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 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework named volumes in DB #2774

Rework named volumes in DB #2774

mheon commented Mar 26, 2019

openshift-ci-robot commented Mar 26, 2019

mheon commented Mar 26, 2019

mheon commented Mar 26, 2019

rhatdan commented Mar 27, 2019

giuseppe Mar 27, 2019

mheon Mar 29, 2019

giuseppe Mar 27, 2019

mheon Mar 27, 2019

rhatdan Mar 28, 2019

mheon Mar 28, 2019

mheon Mar 29, 2019

mheon commented Mar 29, 2019

baude commented Mar 29, 2019

mheon commented Mar 29, 2019

rh-atomic-bot commented Mar 29, 2019

mheon commented Mar 29, 2019

mheon commented Mar 29, 2019

mheon commented Apr 3, 2019

mheon commented Apr 3, 2019

mheon commented Apr 4, 2019

mheon commented Apr 4, 2019

rh-atomic-bot commented Apr 4, 2019

mheon commented Apr 4, 2019

mheon commented Apr 4, 2019

rhatdan commented Apr 4, 2019

TomSweeneyRedHat Apr 4, 2019

TomSweeneyRedHat left a comment •

edited

Loading

giuseppe commented Apr 4, 2019

Rework named volumes in DB #2774

Rework named volumes in DB #2774

Conversation

mheon commented Mar 26, 2019

openshift-ci-robot commented Mar 26, 2019

mheon commented Mar 26, 2019

mheon commented Mar 26, 2019

rhatdan commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Mar 29, 2019

baude commented Mar 29, 2019

mheon commented Mar 29, 2019

rh-atomic-bot commented Mar 29, 2019

mheon commented Mar 29, 2019

mheon commented Mar 29, 2019

mheon commented Apr 3, 2019

mheon commented Apr 3, 2019

mheon commented Apr 4, 2019

mheon commented Apr 4, 2019

rh-atomic-bot commented Apr 4, 2019

mheon commented Apr 4, 2019

mheon commented Apr 4, 2019

rhatdan commented Apr 4, 2019

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment • edited Loading

Choose a reason for hiding this comment

giuseppe commented Apr 4, 2019

TomSweeneyRedHat left a comment •

edited

Loading