Move Storage features in pkg/util from libpod to containers/storage #295

rhatdan · 2019-03-04T13:52:09Z

In an effort to remove cross vendoring, trying to fix buildah from importing from
libpod. I believe these libraries make more sense in containers/storage then in libpod.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2019-03-04T13:52:37Z

@giuseppe @nalind PTAL

rhatdan · 2019-03-04T13:54:49Z

@vrothberg PTAL

vrothberg

One nit regarding the env variables. We need to be careful with future changes to the package to not introduce regressions.

vrothberg · 2019-03-04T14:03:58Z

pkg/rootless/rootless_linux.c

+  if (pid)
+    return pid;
+
+  setenv ("_LIBPOD_USERNS_CONFIGURED", "init", 1);


Can we rename the identifiers to CONTAINERS ?

mheon · 2019-03-04T14:32:38Z

Why are we moving pkg/util over? That's general-purpose utility functions in Podman

nalind · 2019-03-04T15:23:51Z

The util package also depends on github.com/containers/image, which depends on this library.

rhatdan · 2019-03-04T15:28:32Z

@mheon Util package currently is used in buildah, libpod and eventually skopeo.

rhatdan · 2019-03-04T15:29:12Z

Util package also seems mainly concerned with setting up default containers storage.

rhatdan · 2019-03-04T15:30:39Z

@nalind Can we start moving parts of containers image over here if it is needed by both packages.

I would like to get to the point where we have a stack of tools that looks like this. But the vendoring only goes in one direction.

cri-o
libpod
buildah
image
storage

mheon · 2019-03-04T15:34:39Z

@rhatdan Not really? There's some storage in there, but that's where I tell people to stick general utility functions, and a fair bit of it will be libpod-specific

nalind · 2019-03-04T17:22:23Z

@nalind Can we start moving parts of containers image over here if it is needed by both packages.

The only dependency in that direction so far is a call to manifest.Digest, which we could probably change to a callback or a hook or something.

rhatdan · 2019-03-04T17:49:39Z

@mheon I will remove the parts of utils.go that have nothing to do with storage.

mheon · 2019-03-04T18:01:15Z

@rhatdan Ack, SGTM.

I mainly want to keep the package so I can keep telling @baude to put stuff there when nobody knows where to put it

wking · 2019-03-10T05:40:08Z

In an effort to remove cross vendoring, trying to fix buildah from importing from libpod.

I don't have a great handle on how libraries are distributed among these projects, so take this with a grain of salt, but I don't see a connection between the rootless stuff and storage, except for holding a package-level skipStorageSetup boolean. Maybe that package should instead live in buildah and libpod could pull it from there (since libpod already vendors buildah to support podman build)?

giuseppe · 2019-03-11T08:06:01Z

I agree with @wking. It might help with vendoring and avoid circular dependencies, but containers/storage doesn't seem the best place for pkg/rootless, it is very libpod centric and the libpod repository still seem like the best place for it.

We are only using a subset of the package from Buildah, we could probably avoid the circular dependency embedding the few changes inside Buildah itself and not requiring pkg/rootless.

rhatdan · 2019-03-11T14:02:13Z

I am moving pkg/rootless to buildah and just putting the StorageDefaults handling in this PR.
@nalind Requested that we pass in the rootless fields, into the storage calls to help it differentiate between the system storage versus rootless storage.

rhatdan · 2019-03-11T14:03:07Z

Opened containers/buildah#1400 to test this patch out and mv pkg/rootless to buildah.

rhatdan · 2019-03-11T17:44:23Z

@nalind PTAL

nalind · 2019-03-11T18:43:41Z

pkg/util/utils.go

+}
+
+// GetRootlessRuntimeDir returns the runtime directory when running as non root
+func GetRootlessRuntimeDir(rootlessUid int) (string, error) {


Does this need to be exported?

It is used all over libpod.

/home/dwalsh/libpod/libpod/runtime.go: rootlessRuntimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/runtime.go: val, err = util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/runtime.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/runtime.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir() /home/dwalsh/libpod/libpod/oci.go: runtimeDir, err := util.GetRootlessRuntimeDir()

nalind · 2019-03-11T18:43:53Z

pkg/util/utils.go

+		}
+	}
+	if runtimeDir == "" {
+		tmpDir := fmt.Sprintf("%s/user/%d", os.TempDir(), rootlessUid)


Why the "user" intermediate directory under what will likely be "/tmp"?

Because it was there in the libpod version. Removed.

nalind · 2019-03-11T18:43:59Z

pkg/util/utils.go

+
+// GetRootlessDirInfo returns the parent path of where the storage for containers and
+// volumes will be in rootless mode
+func GetRootlessDirInfo(rootlessUid int) (string, string, error) {


Does this need to be exported?

Nope, Fixed

nalind · 2019-03-11T18:44:02Z

pkg/util/utils.go

+}
+
+// GetRootlessStorageOpts returns the storage opts for containers running as non root
+func GetRootlessStorageOpts(rootlessUid int) (storage.StoreOptions, error) {


Does this need to be exported?

Nope Fixed.

nalind · 2019-03-11T18:44:05Z

pkg/util/utils.go

+}
+
+// GetDefaultStoreOptions returns the default storage ops for containers
+func GetDefaultStoreOptions(rootless bool, rootlessUid int) (storage.StoreOptions, error) {


What do we gain by hiding this in a subpackage? How should a caller know if it should call this function, GetRootlessStorageOpts(), or simply read from storage.DefaultStoreOptions?

Documentation?

Rewritten again to make these functions primary in storage and to remove links to the constants.
Now storage.DefaultStoreOptions is function.

nalind · 2019-03-11T18:44:13Z

pkg/util/utils.go

+}
+
+// StorageConfigFile returns the path to the storage config file used
+func StorageConfigFile(rootless bool) string {


How should a caller know if this is needed instead of trusting storage.DefaultConfigFile?

I could make DefaultConfigFile not be exposed.

Change DefaultConfigFile-> defaultConfigFile.
Now you need to call StorageFile()

nalind · 2019-03-11T18:44:15Z

pkg/util/utils.go

+// StorageConfigFile returns the path to the storage config file used
+func StorageConfigFile(rootless bool) string {
+	if rootless {
+		return filepath.Join(os.Getenv("HOME"), ".config/containers/storage.conf")


GetRootlessDirInfo() returns an error if $HOME is not set. Should this function be checking for that?

giuseppe · 2019-03-14T11:19:05Z

LGTM

nalind · 2019-03-18T14:06:15Z

utils.go

+	if subUIDMap != "" && subGIDMap != "" {
+		mappings, err := idtools.NewIDMappings(subUIDMap, subGIDMap)
+		if err != nil {
+			return nil, err


Wrap this with errors.Wrapf.

nalind · 2019-03-18T14:06:17Z

utils.go

+	}
+	parsedUIDMap, err := idtools.ParseIDMap(UIDMapSlice, "UID")
+	if err != nil {
+		return nil, err


Wrap this with errors.Wrapf.

nalind · 2019-03-18T14:06:19Z

utils.go

+	}
+	parsedGIDMap, err := idtools.ParseIDMap(GIDMapSlice, "GID")
+	if err != nil {
+		return nil, err


Wrap this with errors.Wrapf.

nalind · 2019-03-18T14:06:23Z

utils.go

+
+	if rootless {
+		if os.IsNotExist(err) {
+			os.MkdirAll(filepath.Dir(storageConf), 0755)


Check for an error here.

nalind · 2019-03-18T14:06:26Z

utils.go

+			if storageOpts.GraphRoot == "" {
+				storageOpts.GraphRoot = defaultRootlessGraphRoot
+			}
+		}


We should do something about non-nil errors that aren't IsNotExist errors.

Added handling for this.

nalind · 2019-03-18T14:06:40Z

Some nits around error handling, but otherwise LGTM.

rhatdan · 2019-03-18T16:10:12Z

@giuseppe PTAL I rewrote some of the GetRootlessRuntimeDir functions.

giuseppe · 2019-03-18T18:04:31Z

utils.go

+			return tmpDir, nil
+		}
+	}
+	tmpDir := fmt.Sprintf("%s/%d", os.TempDir(), rootlessUid)


filepath.Join?

That fails to build because rootlessUid is an int.

giuseppe · 2019-03-19T08:11:50Z

LGTM

TomSweeneyRedHat · 2019-03-19T11:59:49Z

utils.go

+	}
+	parsedUIDMap, err := idtools.ParseIDMap(UIDMapSlice, "UID")
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to create ParseIDMap UID=%s", UIDMapSlice)


to differentiate, could you s/ParseIDMap/ParseUidMap/ here and with ParseGidMap below at line 57?

TomSweeneyRedHat · 2019-03-19T12:05:46Z

LGTM other than two small nits.

Test errors are odd. All the go 1.11 tests pass, the others fail due to not being able to find md2man?

Building/Running tests
go get -u -tags " libdm_no_deferred_remove ostree "  github.com/cpuguy83/go-md2man
package github.com/cpuguy83/go-md2man/v2/md2man: cannot find package "github.com/cpuguy83/go-md2man/v2/md2man" in any of:
	/root/.gimme/versions/go1.10.linux.amd64/src/github.com/cpuguy83/go-md2man/v2/md2man (from $GOROOT)
	/root/go/src/github.com/cpuguy83/go-md2man/v2/md2man (from $GOPATH)
Makefile:85: recipe for target 'install.tools' failed
make: *** [install.tools] Error 1
+sudo rm -rf /tmp/storage_iCIjsD

…rage In an effort to remove cross vendoring, trying to fix buildah from importing from libpod. I beleive these libraries make more sense in containers/storage then in libpod. Signed-off-by: Daniel J Walsh <[email protected]>

Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg reviewed Mar 4, 2019

View reviewed changes

rhatdan force-pushed the rootless branch from 9390828 to 5fcaeac Compare March 4, 2019 17:50

rhatdan force-pushed the rootless branch from 5fcaeac to d343971 Compare March 4, 2019 18:32

rhatdan changed the title ~~Move pkg/rootless and pkg/util from libpod to containers/storage~~ [wip] Move pkg/rootless and pkg/util from libpod to containers/storage Mar 11, 2019

rhatdan force-pushed the rootless branch 5 times, most recently from 5d5054b to 7767c0b Compare March 11, 2019 12:48

rhatdan changed the title ~~[wip] Move pkg/rootless and pkg/util from libpod to containers/storage~~ Move pkg/rootless and pkg/util from libpod to containers/storage Mar 11, 2019

rhatdan force-pushed the rootless branch from 7767c0b to 4f11abc Compare March 11, 2019 17:43

nalind reviewed Mar 11, 2019

View reviewed changes

rhatdan force-pushed the rootless branch 2 times, most recently from d7908a2 to dcbc3f5 Compare March 12, 2019 14:08

rhatdan changed the title ~~Move pkg/rootless and pkg/util from libpod to containers/storage~~ Move Storage features in pkg/util from libpod to containers/storage Mar 13, 2019

rhatdan mentioned this pull request Mar 13, 2019

Support user containers-storage containers/skopeo#547

Closed

nalind reviewed Mar 18, 2019

View reviewed changes

rhatdan force-pushed the rootless branch 3 times, most recently from 13662fb to 7157a77 Compare March 18, 2019 16:09

giuseppe reviewed Mar 18, 2019

View reviewed changes

TomSweeneyRedHat reviewed Mar 19, 2019

View reviewed changes

rhatdan force-pushed the rootless branch from 7157a77 to 877c68c Compare March 19, 2019 12:33

Bump to latest golang for testing.

058f984

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan merged commit dca3d21 into containers:master Mar 19, 2019

Move Storage features in pkg/util from libpod to containers/storage #295

Move Storage features in pkg/util from libpod to containers/storage #295

Uh oh!

Conversation

rhatdan commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Mar 4, 2019

Uh oh!

nalind commented Mar 4, 2019

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

mheon commented Mar 4, 2019

Uh oh!

nalind commented Mar 4, 2019

Uh oh!

rhatdan commented Mar 4, 2019

Uh oh!

mheon commented Mar 4, 2019

Uh oh!

wking commented Mar 10, 2019

Uh oh!

giuseppe commented Mar 11, 2019

Uh oh!

rhatdan commented Mar 11, 2019

Uh oh!

rhatdan commented Mar 11, 2019

Uh oh!

rhatdan commented Mar 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Mar 4, 2019 •

edited

Loading

TomSweeneyRedHat Mar 19, 2019 •

edited

Loading