Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Fix local images when daemon uses containerd storage #222

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67824e9
Fix local images when daemon uses containerd storage
natalieparellano Oct 25, 2023
9c78088
Update acceptance/reproducibility_test.go
natalieparellano Oct 25, 2023
dc9c424
Merge branch 'main' into fix/containerd-hack
natalieparellano Oct 30, 2023
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
6 changes: 6 additions & 0 deletions acceptance/reproducibility_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
natalieparellano marked this conversation as resolved.
Show resolved Hide resolved
cfg1.DockerVersion = ""
cfg2.DockerVersion = ""
Comment on lines +168 to +169
Copy link
Member Author

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

config = v1.Config{
but it seems to have no effect.

cfg1.Config.Image = ""
cfg2.Config.Image = ""
Comment on lines +170 to +171
Copy link
Member Author

Choose a reason for hiding this comment

The 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())
Expand Down
35 changes: 17 additions & 18 deletions local/local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,15 +1114,15 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
when("#Rebase", func() {
when("image exists", func() {
var (
repoName = newTestImageName()
oldBase, oldTopLayer, newBase, origID string
oldBaseLayer1DiffID string
oldBaseLayer2DiffID string
newBaseLayer1DiffID string
newBaseLayer2DiffID string
imgLayer1DiffID string
imgLayer2DiffID string
origNumLayers int
repoName = newTestImageName()
oldBase, oldTopLayer, newBase string
oldBaseLayer1DiffID string
oldBaseLayer2DiffID string
newBaseLayer1DiffID string
newBaseLayer2DiffID string
imgLayer1DiffID string
imgLayer2DiffID string
origNumLayers int
)

it.Before(func() {
Expand Down Expand Up @@ -1198,11 +1198,10 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
inspect, _, err = dockerClient.ImageInspectWithRaw(context.TODO(), repoName)
h.AssertNil(t, err)
origNumLayers = len(inspect.RootFS.Layers)
origID = inspect.ID
})

it.After(func() {
h.AssertNil(t, h.DockerRmi(dockerClient, repoName, oldBase, newBase, origID))
h.AssertNil(t, h.DockerRmi(dockerClient, repoName, oldBase, newBase))
})

it("switches the base", func() {
Expand Down Expand Up @@ -1604,6 +1603,8 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
repoName = newTestImageName()
prevLayer1SHA string
prevLayer2SHA string
layer1Path string
layer2Path string
)

it.Before(func() {
Expand All @@ -1616,13 +1617,11 @@ func testImage(t *testing.T, when spec.G, it spec.S) {
)
h.AssertNil(t, err)

layer1Path, err := h.CreateSingleFileLayerTar("/layer-1.txt", "old-layer-1", daemonOS)
layer1Path, err = h.CreateSingleFileLayerTar("/layer-1.txt", "old-layer-1", daemonOS)
h.AssertNil(t, err)
defer os.Remove(layer1Path)

layer2Path, err := h.CreateSingleFileLayerTar("/layer-2.txt", "old-layer-2", daemonOS)
layer2Path, err = h.CreateSingleFileLayerTar("/layer-2.txt", "old-layer-2", daemonOS)
h.AssertNil(t, err)
defer os.Remove(layer2Path)

h.AssertNil(t, prevImage.AddLayer(layer1Path))
h.AssertNil(t, prevImage.AddLayer(layer2Path))
Expand All @@ -1638,6 +1637,8 @@ func testImage(t *testing.T, when spec.G, it spec.S) {

it.After(func() {
h.AssertNil(t, h.DockerRmi(dockerClient, repoName, prevImageName))
h.AssertNil(t, os.RemoveAll(layer1Path))
h.AssertNil(t, os.RemoveAll(layer2Path))
})

it("reuses a layer", func() {
Expand Down Expand Up @@ -1869,7 +1870,7 @@ func testImage(t *testing.T, when spec.G, it spec.S) {

it.After(func() {
h.AssertNil(t, os.Remove(tarPath))
h.AssertNil(t, h.DockerRmi(dockerClient, repoName, origID))
h.AssertNil(t, h.DockerRmi(dockerClient, repoName))
})

it("saved image overrides image with new ID", func() {
Expand Down Expand Up @@ -1909,7 +1910,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) {

h.AssertEq(t, inspect.Created, imgutil.NormalizedDateTime.Format(time.RFC3339))
h.AssertEq(t, inspect.Container, "")
h.AssertEq(t, inspect.DockerVersion, "")

history, err := dockerClient.ImageHistory(context.TODO(), repoName)
h.AssertNil(t, err)
Expand Down Expand Up @@ -1940,7 +1940,6 @@ func testImage(t *testing.T, when spec.G, it spec.S) {

h.AssertEq(t, inspect.Created, expectedTime.Format(time.RFC3339))
h.AssertEq(t, inspect.Container, "")
h.AssertEq(t, inspect.DockerVersion, "")

history, err := dockerClient.ImageHistory(context.TODO(), repoName)
h.AssertNil(t, err)
Expand Down
147 changes: 93 additions & 54 deletions local/save.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io"
"os"
"path/filepath"
"strings"

"github.com/docker/docker/api/types"
"github.com/docker/docker/client"
Expand All @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The 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
}
Expand Down Expand Up @@ -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 {
Expand Down
63 changes: 8 additions & 55 deletions local/save_file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package local
import (
"archive/tar"
"context"
"crypto/sha256"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -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")
}
Expand All @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The 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()
Expand Down
Loading