Remove Docker dependency for retrieving image filesystem #89

nkubala · 2017-09-18T18:27:50Z

This PR removes the dependency on the Docker API client for retrieving an image from a remote registry or a local docker daemon, and converting it to an in-memory filesystem. The Docker client is still needed to retrieve image history, and will be used automatically by the container-diff tool itself.

The main motivation for this is for code that uses container-diff as a library to convert both Docker images and tarballs into convenient representations of filesystems, without relying on a functional Docker daemon on the host machine.

By default, the Image Prepper in this code will now default to looking for images in the local Docker daemon first, followed by checking a cloud registry. Because this is not necessarily intended behavior when using the container-diff CLI (for example, if you wanted to compare both local and remote versions of an image), the container-diff CLI now requires the prefix daemon:// to specify an image contained in a local Docker daemon.

This also removes support for specifying images by their Docker "Image ID" (e.g. f5ad8aa3b589), since this is not supported by the containers/image library.

…ages in local daemon

…ion manually. fix up image prep

dlorenc · 2017-09-18T21:08:48Z

cmd/analyze.go

+	analyzeTypes, err := differs.GetAnalyzers(analyzerArgs)
+	if err != nil {
+		glog.Error(err.Error())
+		return errors.New("Could not perform image analysis")


Could you wrap the previous error instead of making a new one? No need to log there either.

dlorenc · 2017-09-18T21:10:26Z

pkg/util/image_prep_utils.go

 	if err != nil {
-		return "", err
+		errStr = "\nUnable to remove " + path


Again, wrap the error you get instead of making a new one.

dlorenc · 2017-09-18T21:11:38Z

cmd/diff.go

+				prepper = pkgutil.LOCAL
+				imageName = strings.Replace(imageName, "daemon://", "", -1)
+			} else if pkgutil.IsTar(imageName) {
+				prepper = pkgutil.TAR


Can we change this to pass an actual type or function instead of a string?

dlorenc · 2017-09-18T21:12:37Z

pkg/util/tar_utils.go

@@ -41,11 +41,21 @@ func unpackTar(tr *tar.Reader, path string) error {
 		if strings.Contains(header.Name, ".wh.") {
 			rmPath := filepath.Join(path, header.Name)
 			newName := strings.Replace(rmPath, ".wh.", "", 1)
-			err := os.Remove(rmPath)
+			if _, err := os.Stat(rmPath); err == nil {
+				err := os.Remove(rmPath)


nit: collapse this into the if

r2d4 · 2017-09-18T21:22:33Z

cmd/analyze.go

+	var prepper string
+	if strings.HasPrefix(imageName, "daemon://") {
+		// force local daemon if we have the corresponding prefix
+		prepper = pkgutil.LOCAL


just CamelCase for constants

r2d4 · 2017-09-19T17:09:23Z

cmd/diff.go

 	}
 	return nil
 }

+func processImageName(imageName string) string {


i would get rid of the nested negated conditions

if pkgutil.IsTar(imageName) || strings.HasPrefix(imageName, "daemon://") return imageName // default condition return "remote://" + imageName

or even

if pkgutil.IsTar(imageName) return ... if strings.HasPrefix(imageName, "daemon://") return ... return ...

r2d4 · 2017-09-19T17:22:46Z

pkg/util/image_prepper.go

@@ -54,17 +54,23 @@ func getImage(prepper Prepper, source string) (Image, error) {
 	}, nil
 }

-func (p ImagePrepper) GetImage() (Image, error) {
+func (p *ImagePrepper) GetImage() (Image, error) {


Its weird that GetImage() actually mutates the fields on the struct. Calling this twice on the same imagePrepper will break, right? I would refactor this. getImage already takes in redundant information since it takes prepper and prepper.ImagePrepper.Source

the reason I changed it to do this is to handle the prefixes that the image prepper respects to force a certain source (e.g. daemon://<image_path>). these need to be removed before actually trying to parse the image reference with the containers/image library. GetImage() only ever gets called one time per prepper, so there shouldn't be any risk here. getImage() doesn't need to take the source as parameter though, since it gets it from the prepper

r2d4 · 2017-09-19T17:27:19Z

pkg/util/image_prep_utils.go

-	"Cloud Registry": func(ip ImagePrepper) Prepper { return CloudPrepper{ImagePrepper: ip} },
-	"Tar":            func(ip ImagePrepper) Prepper { return TarPrepper{ImagePrepper: ip} },
+var sourceToPrepMap = map[string]func(ip *ImagePrepper) Prepper{
+	"Local Daemon":   func(ip *ImagePrepper) Prepper { return DaemonPrepper{ImagePrepper: ip} },


Where do these strings come from? I would get rid of them in favor of some function like

func NewPrepperForSource(source string) Prepper or func NewPrepperForSource(source ImageSourceType) Prepper

yeah this is getting refactored out in an upcoming PR. these are just here for logging at this point

r2d4 · 2017-09-19T17:28:19Z

pkg/util/image_prep_utils.go

-		if errMsg != "" {
-			glog.Error(errMsg)
+		if err := remove(image.FSPath, true); err != nil {
+			glog.Error(err.Error())


This can just be glog.Error(err)

Also, this won't exit or do anything besides print the error to stdout and the glog error file. Is that what you mean to do here?

not new code, but yeah this can fail without impacting the overall run. this should probably get knocked down to a warn though

r2d4 · 2017-09-19T17:29:49Z

pkg/util/image_prep_utils.go

-	}
-
-	var err error
+func remove(path string, dir bool) error {


Remove remove. os.RemoveAll tries to call remove before it deletes the directory. Just calling that directly will be fine

https://golang.org/src/os/path.go?s=1634:1667#L56

good call, done

r2d4 · 2017-09-19T17:29:56Z

pkg/util/image_prep_utils.go

-		errMsg := remove(image.FSPath, true)
-		if errMsg != "" {
-			glog.Error(errMsg)
+		if err := remove(image.FSPath, true); err != nil {


remove remove

r2d4 · 2017-09-19T17:36:24Z

cmd/diff.go

@@ -108,27 +98,34 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {

 	diffTypes, err := differs.GetAnalyzers(diffArgs)
 	if err != nil {
-		glog.Error(err.Error())
-		return errors.New("Could not perform image diff")
+		return fmt.Errorf("Could not perform image diff: %s", err.Error())


Here you usually want to just do something like fmt.Errorf("error msg: %s", err)
or fmt.Errorf("error msg: %v", err) if you might expect a typed error

nkubala · 2017-09-20T18:26:34Z

closing in favor of #97

nkubala added 8 commits September 14, 2017 13:52

update vendored code

1104153

wip; moved preppers to new package. removing docker dependency for im…

80676f4

…ages in local daemon

require 'daemon://' prefix for local daemon images. handle decompress…

cf93d47

…ion manually. fix up image prep

ignore tar files when checking image IDs. fix tests

e886877

only remove whiteout directory if it exists

f2f6423

Cleanup

95635cd

fix analyze entrypoint, strip daemon prefix before prepping

16b6e59

fix error message when no analzyers are provided

b0f5728

nkubala mentioned this pull request Sep 18, 2017

Implement tar driver for running file tests without Docker dependency GoogleCloudPlatform/runtimes-common#350

Merged

dlorenc reviewed Sep 18, 2017

View reviewed changes

r2d4 reviewed Sep 18, 2017

View reviewed changes

fix image name prefix handling, and error handling

f28aef6

r2d4 reviewed Sep 19, 2017

View reviewed changes

addressing review comments

96b40ad

This was referenced Sep 19, 2017

refactor preppers to their own files, and consolidate util methods #91

Merged

move image verification logic into preppers. replace string->prep map… #94

Merged

nkubala closed this Sep 20, 2017

Remove Docker dependency for retrieving image filesystem #89

Remove Docker dependency for retrieving image filesystem #89

Uh oh!

Conversation

nkubala commented Sep 18, 2017

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!

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!

nkubala commented Sep 20, 2017

Uh oh!

Uh oh!