Add force to umount to force the umount of a container image #198

rhatdan · 2018-07-13T15:44:12Z

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

rhatdan · 2018-07-13T16:07:15Z

nalind · 2018-07-16T15:46:35Z

layers.go

-	Unmount(id string) error
+	Unmount(id string, force bool) (bool, error)
+
+	// Mounted returns whether or not the layer is mounted


Missing a . at the end of the godoc.

nalind · 2018-07-16T15:46:38Z

layers.go

@@ -677,7 +691,7 @@ func (r *layerStore) Unmount(id string) error {
 		layer.MountPoint = ""
 		err = r.Save()
 	}
-	return err
+	return false, err


If err is not nil, this probably needs to be true, since we're leaving the recorded MountCount unchanged.

nalind · 2018-07-16T15:46:41Z

store.go

@@ -262,8 +262,8 @@ type Store interface {
 	Mount(id, mountLabel string) (string, error)

 	// Unmount attempts to unmount a layer, image, or container, given an ID, a
-	// name, or a mount path.
-	Unmount(id string) error
+	// name, or a mount path. Returns whether or not the layer is still mounted


Missing a . at the end of the godoc.

nalind · 2018-07-16T15:46:54Z

The new value that Unmount() returns doesn't seem to be consulted anywhere, and this needs a test.

rhatdan · 2018-07-16T20:45:22Z

Added test and handling of the umount return value.

rhatdan · 2018-07-16T20:45:33Z

@nalind PTAL

cmd/containers-storage/mount.go

+		optionsHelp: "LayerOrContainerNameOrID",
+		usage:       "Check if a file system is mounted",
+		minArgs:     1,
+		action:      mounted,
 		addFlags: func(flags *mflag.FlagSet, cmd *command) {
 			flags.BoolVar(&jsonOutput, []string{"-json", "j"}, jsonOutput, "Prefer JSON output")


nalind · 2018-07-17T13:59:02Z

layers.go

@@ -677,7 +691,7 @@ func (r *layerStore) Unmount(id string) error {
 		layer.MountPoint = ""
 		err = r.Save()
 	}
-	return err
+	return err != nil, err


If the Put() succeeds but we fail to save the updated mount count, we return true here. Is that the intention?

you tell me. The storage has unmounted the file system, but the database does not have the correct data. Does this mean it is mounted or not?

nalind · 2018-07-17T13:59:08Z

tests/mount-layer.bats

+	run storage --debug=false mount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:09Z

tests/mount-layer.bats

+	# Check it layer is mounted
+	run storage --debug=false mounted $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]


Can we compare to "$layer mounted" here?

nalind · 2018-07-17T13:59:12Z

tests/mount-layer.bats

+	run storage --debug=false unmount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:14Z

tests/mount-layer.bats

+	run storage --debug=false mount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:15Z

tests/mount-layer.bats

+	run storage --debug=false mounted $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Unmount the third layer.


This comment looks wrong.

nalind · 2018-07-17T13:59:17Z

tests/mount-layer.bats

+	run storage --debug=false unmount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" == "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:20Z

tests/mount-layer.bats

+	run storage --debug=false unmount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:22Z

tests/mount-layer.bats

+	[ "$output" == "" ]
+
+
+	# Mount the layer twice and force umount


Add a period at the end.

nalind · 2018-07-17T13:59:26Z

tests/mount-layer.bats

+	run storage --debug=false mount $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:29Z

tests/mount-layer.bats

+	run storage --debug=false mounted $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Unmount the third layer.


This comment looks wrong.

nalind · 2018-07-17T13:59:30Z

tests/mount-layer.bats

+	run storage --debug=false unmount --force $layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Check it layer is mounted


s/it/if, add a period at the end.

nalind · 2018-07-17T13:59:32Z

tests/mount-layer.bats

+	[ "$status" -eq 0 ]
+	[ "$output" == "" ]
+
+	# Try to delete the first layer, which should fail because it has children.


This comment doesn't agree with the lines below it.

nalind · 2018-07-17T14:02:47Z

This includes changes to layers.go, but updates images_ffjson.go's timestamp, so CI still tries to rebuild layers_ffjson.go.

rhatdan · 2018-07-17T15:42:16Z

WooHoo tests pass.
@nalind PTAL, we still have the one question on what to return on partial error.

nalind · 2018-07-17T17:30:25Z

cmd/containers-storage/mount.go

 		errText := ""
 		if err != nil {
 			errText = fmt.Sprintf("%v", err)
 			errors = true
 		}
+		if !mounted {
+			fmt.Printf("%s mountpoint unmounted\n", arg)
+		}


Non-blocker: in principle it seems... off to treat mounted as useful if err is set.

nalind · 2018-07-17T17:30:53Z

cmd/containers-storage/mount.go

+		}
+		if mounted {
+			fmt.Printf("%s mounted\n", arg)
+		}


Non-blocker: in principle it seems... off to treat mounted as useful if err is set.

nalind · 2018-07-17T17:31:20Z

A couple of nits about the CLI's interpretation of results from Unmount(), but otherwise it LGTM.

Add force to umount to force the umount of a container image Add an interface to indicate whether or not the layer is mounted Add a boolean return from unmount to indicate when the layer is really unmounted Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

This check has been wrongly removed with containers#198 The check must stay as it's now part of the Stop/Delete API so reintroduce it back This fixes containers#233 and the associated CRI-O issues This PR + cri-o/cri-o#1910 fully fix the issue I'm going to revendor c/storage in CRI-O to full fix crio after this is merged Signed-off-by: Antonio Murdaca <runcom@linux.com>

rhatdan force-pushed the umount branch from cf6a5b5 to 2d56a7f Compare July 13, 2018 16:04

nalind reviewed Jul 16, 2018

View reviewed changes

rhatdan force-pushed the umount branch from cc9ecef to 25806b7 Compare July 16, 2018 20:44

rhatdan force-pushed the umount branch from 25806b7 to 00d6ec1 Compare July 17, 2018 12:06

nalind reviewed Jul 17, 2018

View reviewed changes

tests/mount-layer.bats Outdated

[ "$output" == "" ]

# Mount the layer twice and force umount

Copy link

Member

nalind Jul 17, 2018

Choose a reason for hiding this comment

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

Add a period at the end.

nalind reviewed Jul 17, 2018

View reviewed changes

rhatdan force-pushed the umount branch 2 times, most recently from abe6f92 to 46f031b Compare July 17, 2018 14:59

rhatdan force-pushed the umount branch 2 times, most recently from 6515e04 to bbecea7 Compare July 17, 2018 17:17

nalind reviewed Jul 17, 2018

View reviewed changes

rhatdan force-pushed the umount branch from bbecea7 to 1075a73 Compare July 17, 2018 18:00

rhatdan merged commit 46a8a1c into containers:master Jul 17, 2018

This was referenced Nov 9, 2018

devmapper regression #233

Closed

layers: do not try to unmount if not mounted at all on delete #240

Merged

Add force to umount to force the umount of a container image #198

Add force to umount to force the umount of a container image #198

Conversation

rhatdan commented Jul 13, 2018

rhatdan commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Jul 16, 2018

rhatdan commented Jul 16, 2018

rhatdan commented Jul 16, 2018

This comment was marked as resolved.

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

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

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

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

nalind commented Jul 17, 2018

rhatdan commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Jul 17, 2018