-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix local images when daemon uses containerd storage #222
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,12 @@ func compare(t *testing.T, img1, img2 string) { | |
cfg2, err := v1img2.ConfigFile() | ||
h.AssertNil(t, err) | ||
|
||
// images that were created locally may have `DockerVersion` equal to "dev" and missing `Config.Image` if the daemon uses containerd storage | ||
cfg1.DockerVersion = "" | ||
cfg2.DockerVersion = "" | ||
cfg1.Config.Image = "" | ||
cfg2.Config.Image = "" | ||
Comment on lines
+170
to
+171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems unspec'd so IDK if we care. |
||
|
||
h.AssertEq(t, cfg1, cfg2) | ||
|
||
h.AssertEq(t, ref1.Identifier(), ref2.Identifier()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"io" | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
|
||
"github.com/docker/docker/api/types" | ||
"github.com/docker/docker/client" | ||
|
@@ -24,10 +25,17 @@ func (i *Image) Save(additionalNames ...string) error { | |
} | ||
|
||
func (i *Image) SaveAs(name string, additionalNames ...string) error { | ||
// during the first save attempt some layers may be excluded. The docker daemon allows this if the given set | ||
// of layers already exists in the daemon in the given order | ||
inspect, err := i.doSaveAs(name) | ||
if err != nil { | ||
var ( | ||
inspect types.ImageInspect | ||
err error | ||
) | ||
canOmitBaseLayers := !usesContainerdStorage(i.docker) | ||
if canOmitBaseLayers { | ||
// During the first save attempt some layers may be excluded. | ||
// The docker daemon allows this if the given set of layers already exists in the daemon in the given order. | ||
inspect, err = i.doSaveAs(name) | ||
} | ||
if !canOmitBaseLayers || err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the sorriest part... it's going to lead to a huge performance penalty unless we can work around it. Maybe with containerd storage we could get individual layers from the daemon, IDK. If anyone has further context here it would be much appreciated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also couldn't find a way around this. The trick of giving a tar header with no bytes does not work so we always have to pay the price it seems. At least until moby/moby#44369 ships |
||
// populate all layer paths and try again without the above performance optimization. | ||
if err := i.downloadBaseLayersOnce(); err != nil { | ||
return err | ||
|
@@ -58,6 +66,14 @@ func (i *Image) SaveAs(name string, additionalNames ...string) error { | |
return nil | ||
} | ||
|
||
func usesContainerdStorage(docker DockerClient) bool { | ||
info, err := docker.Info(context.Background()) | ||
if err != nil { | ||
return false | ||
} | ||
return strings.Contains(info.Driver, "stargz") | ||
} | ||
|
||
func (i *Image) doSaveAs(name string) (types.ImageInspect, error) { | ||
ctx := context.Background() | ||
done := make(chan error) | ||
|
@@ -94,58 +110,11 @@ func (i *Image) doSaveAs(name string) (types.ImageInspect, error) { | |
}() | ||
|
||
tw := tar.NewWriter(pw) | ||
defer tw.Close() | ||
|
||
configFile, err := i.newConfigFile() | ||
if err != nil { | ||
return types.ImageInspect{}, errors.Wrap(err, "generating config file") | ||
} | ||
|
||
id := fmt.Sprintf("%x", sha256.Sum256(configFile)) | ||
if err := addTextToTar(tw, id+".json", configFile); err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
|
||
var blankIdx int | ||
var layerPaths []string | ||
for _, path := range i.layerPaths { | ||
if path == "" { | ||
layerName := fmt.Sprintf("blank_%d", blankIdx) | ||
blankIdx++ | ||
hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0} | ||
if err := tw.WriteHeader(hdr); err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
layerPaths = append(layerPaths, layerName) | ||
} else { | ||
layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) | ||
f, err := os.Open(filepath.Clean(path)) | ||
if err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
defer f.Close() | ||
if err := addFileToTar(tw, layerName, f); err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
f.Close() | ||
layerPaths = append(layerPaths, layerName) | ||
} | ||
} | ||
|
||
manifest, err := json.Marshal([]map[string]interface{}{ | ||
{ | ||
"Config": id + ".json", | ||
"RepoTags": []string{repoName}, | ||
"Layers": layerPaths, | ||
}, | ||
}) | ||
configHash, err := i.addImageToTar(tw, repoName) | ||
if err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
|
||
if err := addTextToTar(tw, "manifest.json", manifest); err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
defer tw.Close() | ||
|
||
tw.Close() | ||
pw.Close() | ||
|
@@ -154,13 +123,16 @@ func (i *Image) doSaveAs(name string) (types.ImageInspect, error) { | |
return types.ImageInspect{}, errors.Wrapf(err, "loading image %q. first error", i.repoName) | ||
} | ||
|
||
inspect, _, err := i.docker.ImageInspectWithRaw(context.Background(), id) | ||
inspect, _, err := i.docker.ImageInspectWithRaw(context.Background(), i.repoName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With containerd storage the image ID is the sha of the manifest, which technically we could compute but it would be hard. To avoid collision problems we can validate that the data in inspect matches the config that we expect. |
||
if err != nil { | ||
if client.IsErrNotFound(err) { | ||
return types.ImageInspect{}, errors.Wrapf(err, "saving image %q", i.repoName) | ||
} | ||
return types.ImageInspect{}, err | ||
} | ||
if err = i.validateInspect(inspect, configHash); err != nil { | ||
return types.ImageInspect{}, err | ||
} | ||
|
||
return inspect, nil | ||
} | ||
|
@@ -247,6 +219,73 @@ func (i *Image) downloadBaseLayers() error { | |
return nil | ||
} | ||
|
||
func (i *Image) addImageToTar(tw *tar.Writer, repoName string) (string, error) { | ||
configFile, err := i.newConfigFile() | ||
if err != nil { | ||
return "", errors.Wrap(err, "generating config file") | ||
} | ||
|
||
configHash := fmt.Sprintf("%x", sha256.Sum256(configFile)) | ||
if err := addTextToTar(tw, configHash+".json", configFile); err != nil { | ||
return "", err | ||
} | ||
|
||
var blankIdx int | ||
var layerPaths []string | ||
for _, path := range i.layerPaths { | ||
if path == "" { | ||
layerName := fmt.Sprintf("blank_%d", blankIdx) | ||
blankIdx++ | ||
hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0} | ||
if err := tw.WriteHeader(hdr); err != nil { | ||
return "", err | ||
} | ||
layerPaths = append(layerPaths, layerName) | ||
} else { | ||
layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) | ||
f, err := os.Open(filepath.Clean(path)) | ||
if err != nil { | ||
return "", err | ||
} | ||
defer f.Close() | ||
if err := addFileToTar(tw, layerName, f); err != nil { | ||
return "", err | ||
} | ||
f.Close() | ||
layerPaths = append(layerPaths, layerName) | ||
} | ||
} | ||
|
||
manifest, err := json.Marshal([]map[string]interface{}{ | ||
{ | ||
"Config": configHash + ".json", | ||
"RepoTags": []string{repoName}, | ||
"Layers": layerPaths, | ||
}, | ||
}) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
return configHash, addTextToTar(tw, "manifest.json", manifest) | ||
} | ||
|
||
func (i *Image) validateInspect(inspect types.ImageInspect, givenConfigHash string) error { | ||
foundConfig, err := v1Config(inspect, i.createdAt, i.history) | ||
if err != nil { | ||
return fmt.Errorf("failed to get config file from inspect: %w", err) | ||
} | ||
foundConfigFile, err := json.Marshal(foundConfig) | ||
if err != nil { | ||
return fmt.Errorf("failed to marshal config file: %w", err) | ||
} | ||
foundID := fmt.Sprintf("%x", sha256.Sum256(foundConfigFile)) | ||
if foundID != givenConfigHash { | ||
return fmt.Errorf("expected config hash %q; got %q", givenConfigHash, foundID) | ||
} | ||
return nil | ||
} | ||
|
||
// helpers | ||
|
||
func checkResponseError(r io.Reader) error { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ package local | |
import ( | ||
"archive/tar" | ||
"context" | ||
"crypto/sha256" | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
|
@@ -31,9 +30,10 @@ func (i *Image) SaveFile() (string, error) { | |
} | ||
}() | ||
|
||
// All layers need to be present here. Missing layers are either due to utilization of: (1) WithPreviousImage(), | ||
// or (2) FromBaseImage(). The former is only relevant if ReuseLayers() has been called which takes care of | ||
// resolving them. The latter case needs to be handled explicitly. | ||
// All layers need to be present here. Missing layers are either due to utilization of | ||
// (1) WithPreviousImage(), or (2) FromBaseImage(). | ||
// The former is only relevant if ReuseLayers() has been called which takes care of resolving them. | ||
// The latter case needs to be handled explicitly. | ||
if err := i.downloadBaseLayersOnce(); err != nil { | ||
return "", errors.Wrap(err, "failed to fetch base layers") | ||
} | ||
|
@@ -55,63 +55,16 @@ func (i *Image) SaveFile() (string, error) { | |
tw := tar.NewWriter(pw) | ||
defer tw.Close() | ||
|
||
config, err := i.newConfigFile() | ||
if err != nil { | ||
return errors.Wrap(err, "failed to generate config file") | ||
} | ||
|
||
configName := fmt.Sprintf("/%x.json", sha256.Sum256(config)) | ||
if err := addTextToTar(tw, configName, config); err != nil { | ||
return errors.Wrap(err, "failed to add config file to tar archive") | ||
} | ||
|
||
for _, path := range i.layerPaths { | ||
err := func() error { | ||
f, err := os.Open(filepath.Clean(path)) | ||
if err != nil { | ||
return errors.Wrapf(err, "failed to open layer path: %s", path) | ||
} | ||
defer f.Close() | ||
|
||
layerName := fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path))) | ||
if err := addFileToTar(tw, layerName, f); err != nil { | ||
return errors.Wrapf(err, "failed to add layer to tar archive from path: %s", path) | ||
} | ||
|
||
return nil | ||
}() | ||
|
||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
t, err := registryName.NewTag(i.repoName, registryName.WeakValidation) | ||
if err != nil { | ||
return errors.Wrap(err, "failed to create tag") | ||
} | ||
|
||
layers := make([]string, 0, len(i.layerPaths)) | ||
for _, path := range i.layerPaths { | ||
layers = append(layers, fmt.Sprintf("/%x.tar", sha256.Sum256([]byte(path)))) | ||
} | ||
// returns valid 'name:tag' appending 'latest', if missing tag | ||
repoName := t.Name() | ||
|
||
manifest, err := json.Marshal([]map[string]interface{}{ | ||
{ | ||
"Config": configName, | ||
"RepoTags": []string{t.Name()}, | ||
"Layers": layers, | ||
}, | ||
}) | ||
if err != nil { | ||
return errors.Wrap(err, "failed to create manifest") | ||
} | ||
|
||
if err := addTextToTar(tw, "/manifest.json", manifest); err != nil { | ||
return errors.Wrap(err, "failed to add manifest to tar archive") | ||
} | ||
|
||
return nil | ||
_, err = i.addImageToTar(tw, repoName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reduces some duplication but also without the refactor I got an "unexpected format" error when trying to load the image in the daemon. Which is weird because the code looks basically the same... |
||
return err | ||
}) | ||
|
||
err = errs.Wait() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried explicitly setting this about here:
imgutil/local/save_file.go
Line 210 in 67824e9