Skip to content
This repository was archived by the owner on Mar 27, 2024. It is now read-only.

Handle error gracefully when we can't retrieve an image #251

Merged
merged 6 commits into from
Jul 30, 2018
Merged

Handle error gracefully when we can't retrieve an image #251

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8621bc5
Handle error gracefully when we can't retrieve an image
nkubala Jul 26, 2018
ee51ff8
fix channel read
nkubala Jul 26, 2018
6a7a550
improve error handling logic when retrieving images
nkubala Jul 27, 2018
7024d3b
move error combining logic into helper function
nkubala Jul 27, 2018
265b622
add comments for functions
nkubala Jul 27, 2018
9c47911
add tests
nkubala Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cmd/analyze.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ limitations under the License.
package cmd

import (
"errors"
"fmt"
"os"

"github.com/GoogleContainerTools/container-diff/cmd/util/output"
"github.com/GoogleContainerTools/container-diff/differs"
pkgutil "github.com/GoogleContainerTools/container-diff/pkg/util"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -56,27 +56,27 @@ func checkAnalyzeArgNum(args []string) error {
func analyzeImage(imageName string, analyzerArgs []string) error {
analyzeTypes, err := differs.GetAnalyzers(analyzerArgs)
if err != nil {
return err
return errors.Wrap(err, "getting analyzers")
}

image, err := getImageForName(imageName)
if err != nil {
return err
return errors.Wrapf(err, "error retrieving image %s", imageName)
}

if noCache && !save {
defer pkgutil.CleanupImage(image)
}
if err != nil {
return fmt.Errorf("Error processing image: %s", err)
return fmt.Errorf("error processing image: %s", err)
}

req := differs.SingleRequest{
Image: image,
AnalyzeTypes: analyzeTypes}
analyses, err := req.GetAnalysis()
if err != nil {
return fmt.Errorf("Error performing image analysis: %s", err)
return fmt.Errorf("error performing image analysis: %s", err)
}

logrus.Info("retrieving analyses")
Expand Down
64 changes: 46 additions & 18 deletions cmd/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,16 @@ limitations under the License.
package cmd

import (
"errors"
"fmt"
"os"
"strings"
"sync"

"github.com/GoogleContainerTools/container-diff/cmd/util/output"
"github.com/GoogleContainerTools/container-diff/differs"
pkgutil "github.com/GoogleContainerTools/container-diff/pkg/util"
"github.com/GoogleContainerTools/container-diff/util"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -69,33 +70,60 @@ func checkFilenameFlag(_ []string) error {
return errors.New("Please include --types=file with the --filename flag")
}

func processImage(imageName string, imageMap map[string]*pkgutil.Image, wg *sync.WaitGroup, errChan chan<- error) {
Copy link

Choose a reason for hiding this comment

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

Is there a name for this function that might be a bit more specific to what this function is doing? I wasn't sure what processImage meant when I was reading this.

It seems like the purpose of this function is to look up sometthing about the image (im not sure what tho - are we actually getting the bytes of the image itself, or looking up some metadata or something else?) and put that something into imageMap

So some other ideas:

  • AddImageToMap
  • GetMetadataForImage
  • FindImageSource
  • DownloadImage

(You might have some other name ideas tho since you probably know what exactly getImageForName is doing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is essentially just a concurrent wrapper around getImageForName, which retrieves a reference to an image and a reader for the actual bytes of the image. I can change this to getImage (the imageMap isn't really a useful struct here so I don't want to refer to it), but the reason I called it processImage is because getImageForName actually does a bit of image processing, i.e. unpacks the image filesystem to the local filesystem. I should probably just add some comments explaining what each function does :)

defer wg.Done()
image, err := getImageForName(imageName)
if image.Image == nil {
Copy link

Choose a reason for hiding this comment

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

2 questions:

  1. do you want to propagate the error to errChan anytime err is not nil? e.g. if err != nil || image.Image == nil
  2. what if image.Image is nil AND err is nil too? (calling err.Error() on it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok this logic isn't great so I'm gonna rework it a bit. Digging into the go-containerregistry code, if we get an error returned back from any of the three sources (tar/daemon/remote), the image returned will always be nil. So it's equivalent to just change this back to a vanilla if err != nil check.

The reason I was checking for image.Image == nil is to avoid putting the image into the returned map so we didn't attempt to clean up an empty path, but that logic is already happening in pkgutil.CleanupImage so this is safe. I moved the deferred calls up before we return the errors, and we're good.

TLDR answers for your questions: 1) yes, and 2) not possible

errChan <- fmt.Errorf("error retrieving image %s: %s", imageName, err.Error())
Copy link

Choose a reason for hiding this comment

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

I'm interested in why call err.Error() in this case vs. passing err to be formatted as '%s'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically the format types don't match (string vs error), but this seems to work so I'll change it back

return
}
if err != nil {
logrus.Warningf("diff may be inaccurate: %s", err)
}
imageMap[imageName] = &image
}

func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
diffTypes, err := differs.GetAnalyzers(diffArgs)
if err != nil {
return err
return errors.Wrap(err, "getting analyzers")
}

var wg sync.WaitGroup
wg.Add(2)

logrus.Infof("starting diff on images %s and %s, using differs: %s\n", image1Arg, image2Arg, diffArgs)

imageMap := map[string]*pkgutil.Image{
image1Arg: {},
image2Arg: {},
imageMap := map[string]*pkgutil.Image{}
errChan := make(chan error, 2)

go processImage(image1Arg, imageMap, &wg, errChan)
go processImage(image2Arg, imageMap, &wg, errChan)

wg.Wait()
close(errChan)

errs := []string{}
for {
err, ok := <-errChan
if !ok {
break
}
errs = append(errs, err.Error())
}

if len(errs) > 0 {
return errors.New(strings.Join(errs, "\n"))
}
Copy link

Choose a reason for hiding this comment

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

this logic to collect together errors into one error seems like a good candidate for being moved into a separate function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, done


img1, ok := imageMap[image1Arg]
if !ok {
return fmt.Errorf("cannot find image %s", image1Arg)
}
// TODO: fix error handling here
for imageArg := range imageMap {
go func(imageName string, imageMap map[string]*pkgutil.Image) {
defer wg.Done()
image, err := getImageForName(imageName)
imageMap[imageName] = &image
if err != nil {
logrus.Warningf("Diff may be inaccurate: %s", err)
}
}(imageArg, imageMap)
img2, ok := imageMap[image2Arg]
if !ok {
return fmt.Errorf("cannot find image %s", image2Arg)
}
Copy link

Choose a reason for hiding this comment

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

what would it mean if processImage returned for both images without errors, but one or both of these checks failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed these since this logic is duplicated elsewhere

wg.Wait()

if noCache && !save {
defer pkgutil.CleanupImage(*imageMap[image1Arg])
Expand All @@ -104,8 +132,8 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {

logrus.Info("computing diffs")
req := differs.DiffRequest{
Image1: *imageMap[image1Arg],
Image2: *imageMap[image2Arg],
Image1: *img1,
Image2: *img2,
DiffTypes: diffTypes}
diffs, err := req.GetDiff()
if err != nil {
Expand Down
30 changes: 17 additions & 13 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"github.com/GoogleContainerTools/container-diff/util"
"github.com/google/go-containerregistry/pkg/v1"
homedir "github.com/mitchellh/go-homedir"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand Down Expand Up @@ -139,17 +140,17 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
start := time.Now()
img, err = tarball.ImageFromPath(imageName, nil)
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "retrieving tar from path")
}
elapsed := time.Now().Sub(start)
logrus.Infof("retrieving image from tar took %f seconds", elapsed.Seconds())
logrus.Infof("retrieving image ref from tar took %f seconds", elapsed.Seconds())
} else if strings.HasPrefix(imageName, DaemonPrefix) {
// remove the daemon prefix
imageName = strings.Replace(imageName, DaemonPrefix, "", -1)

ref, err := name.ParseReference(imageName, name.WeakValidation)
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "parsing image reference")
}

start := time.Now()
Expand All @@ -158,28 +159,28 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
Buffer: true,
})
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "retrieving image from daemon")
}
elapsed := time.Now().Sub(start)
logrus.Infof("retrieving image from daemon took %f seconds", elapsed.Seconds())
logrus.Infof("retrieving local image ref took %f seconds", elapsed.Seconds())
} else {
// either has remote prefix or has no prefix, in which case we force remote
imageName = strings.Replace(imageName, RemotePrefix, "", -1)
ref, err := name.ParseReference(imageName, name.WeakValidation)
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "parsing image reference")
}
auth, err := authn.DefaultKeychain.Resolve(ref.Context().Registry)
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "resolving auth")
}
start := time.Now()
img, err = remote.Image(ref, auth, http.DefaultTransport)
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "retrieving remote image")
}
elapsed := time.Now().Sub(start)
logrus.Infof("retrieving remote image took %f seconds", elapsed.Seconds())
logrus.Infof("retrieving remote image ref took %f seconds", elapsed.Seconds())
}

// create tempdir and extract fs into it
Expand All @@ -188,7 +189,7 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
start := time.Now()
imgLayers, err := img.Layers()
if err != nil {
return pkgutil.Image{}, err
return pkgutil.Image{}, errors.Wrap(err, "getting image layers")
}
for _, layer := range imgLayers {
layerStart := time.Now()
Expand All @@ -197,12 +198,12 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
if err != nil {
return pkgutil.Image{
Layers: layers,
}, err
}, errors.Wrap(err, "getting extract path for layer")
}
if err := pkgutil.GetFileSystemForLayer(layer, path, nil); err != nil {
return pkgutil.Image{
Layers: layers,
}, err
}, errors.Wrap(err, "getting filesystem for layer")
}
layers = append(layers, pkgutil.Layer{
FSPath: path,
Expand All @@ -215,12 +216,15 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
}

path, err := getExtractPathForImage(imageName, img)
if err != nil {
return pkgutil.Image{}, err
}
// extract fs into provided dir
if err := pkgutil.GetFileSystemForImage(img, path, nil); err != nil {
return pkgutil.Image{
FSPath: path,
Layers: layers,
}, err
}, errors.Wrap(err, "getting filesystem for image")
}
return pkgutil.Image{
Image: img,
Expand Down
12 changes: 6 additions & 6 deletions differs/differs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@ func (req DiffRequest) GetDiff() (map[string]util.Result, error) {
if diff, err := differ.Diff(img1, img2); err == nil {
results[differ.Name()] = diff
} else {
logrus.Errorf("Error getting diff with %s: %s", differ.Name(), err)
logrus.Errorf("error getting diff with %s: %s", differ.Name(), err)
}
}

var err error
if len(results) == 0 {
err = fmt.Errorf("Could not perform diff on %v and %v", img1, img2)
err = fmt.Errorf("could not perform diff on %v and %v", img1, img2)
} else {
err = nil
}
Expand All @@ -87,13 +87,13 @@ func (req SingleRequest) GetAnalysis() (map[string]util.Result, error) {
if analysis, err := analyzer.Analyze(img); err == nil {
results[analyzeName] = analysis
} else {
logrus.Errorf("Error getting analysis with %s: %s", analyzeName, err)
logrus.Errorf("error getting analysis with %s: %s", analyzeName, err)
}
}

var err error
if len(results) == 0 {
err = fmt.Errorf("Could not perform analysis on %v", img)
err = fmt.Errorf("could not perform analysis on %v", img)
} else {
err = nil
}
Expand All @@ -107,11 +107,11 @@ func GetAnalyzers(analyzeNames []string) ([]Analyzer, error) {
if a, exists := Analyzers[name]; exists {
analyzeFuncs = append(analyzeFuncs, a)
} else {
return nil, fmt.Errorf("Unknown analyzer/differ specified: %s", name)
return nil, fmt.Errorf("unknown analyzer/differ specified: %s", name)
}
}
if len(analyzeFuncs) == 0 {
return nil, fmt.Errorf("No known analyzers/differs specified")
return nil, fmt.Errorf("no known analyzers/differs specified")
}
return analyzeFuncs, nil
}