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

Jump to bottom

Implement private.ImageDestination in all transports, improve transport helpers #1589

Merged
merged 16 commits into from Jul 5, 2022
Merged

Implement private.ImageDestination in all transports, improve transport helpers #1589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0d8ca84
Beautify imagedestination.wrapped
mtrmac Jul 1, 2022
fdc8a5e
Introduce internal/private/ImageDestinationInternalOnly
mtrmac Jan 11, 2022
0caf422
Add internal/imagedestination.impl, and use it in storageImageDestina…
mtrmac Jan 11, 2022
bf3a059
Add internal/imagedestination/stubs.NoPutBlobPartial
mtrmac Jul 1, 2022
63a1360
Implement private.ImageDestination in dockerImageDestination
mtrmac Jan 11, 2022
e07ab58
Use BlobInfoCache2 instead of types.BlobInfoCache in internal/types
mtrmac Jan 11, 2022
1799b98
Only create dirImageDestination when all checks are done
mtrmac Jul 1, 2022
321d69a
Add a transportName parameter to docker/internal/tarfile.NewDestination
mtrmac Jul 1, 2022
5e75f67
Implement private.ImageDestination in non-forwarding transports
mtrmac Jul 1, 2022
5dd69be
Implement private.ImageDestination in oci/archive and openshift
mtrmac Jul 1, 2022
0e068a4
Add stubs.ImplementsPutBlobPartial
mtrmac Jul 1, 2022
bebd5b6
Add stubs.NoSignatures and stubs.AlwaysSupportsSignatures, use them i…
mtrmac Jul 1, 2022
eed518d
Add internal/imagedestination/impl.Properties, use it to reduce boile…
mtrmac Jul 1, 2022
52f1c33
Add imagedestination.impl.Properties.SupportedManifestMIMETypes
mtrmac Jul 1, 2022
c3e1251
Add imagedestination.impl.Properties.DesiredLayerCompression
mtrmac Jul 1, 2022
85db725
Add Imagedestination.impl.Properties.AcceptsForeignLayerURLs
mtrmac Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
104 changes: 43 additions & 61 deletions directory/directory_dest.go
View file
Open in desktop
Expand Up @@ -9,6 +9,9 @@ import (
"path/filepath"
"runtime"

"github.com/containers/image/v5/internal/imagedestination/impl"
"github.com/containers/image/v5/internal/imagedestination/stubs"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/internal/putblobdigest"
"github.com/containers/image/v5/types"
"github.com/opencontainers/go-digest"
Expand All @@ -23,12 +26,16 @@ const version = "Directory Transport Version: 1.1\n"
var ErrNotContainerImageDir = errors.New("not a containers image directory, don't want to overwrite important data")

type dirImageDestination struct {
ref dirReference
desiredLayerCompression types.LayerCompression
impl.Compat
impl.PropertyMethodsInitialize
stubs.NoPutBlobPartialInitialize
stubs.AlwaysSupportsSignatures
Comment on lines +29 to +32
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would very much appreciate an independent opinion. I’m really not sure whether using mix-ins like this is is more succinct and convenient, or surprising and inscrutable.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I found it exotic at the beginning but started to like it. I still have concerns how approachable the code is for new pairs of eyes though. I think that one central structure with boolean fields would be easier to digest. At the current state, properties are implicitly embedded into the structures.

In other words: I am completely OK to merge it as is. It's a dramatic improvement!

Copy link
Collaborator Author

@mtrmac mtrmac Jul 4, 2022 •
edited

Choose a reason for hiding this comment

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

I think that one central structure with boolean fields would be easier to digest. At the current state, properties are implicitly embedded into the structures.

I’m not quite sure what this refers to. My best guess is that this says that it’s very hard to find the SupportsPutBlobPartial value for a transport just by text search (quite true), and perhaps that it would be nice to have something broadly like

type private.ImageSource interface {
    HasThreadSafeGetBlob bool
    SupportsPutBlobPartial bool
}

…
func /*docker.*/newImageSource(…) private.ImageSource {
    return &dockerImageSource{
      HasThreadSafeGetBlob: true,
      SupportsPutBlobPartial: true,
    }
}

Is that what you mean? The immediate problem is that Go interfaces only contain methods, not fields. Secondarily, and less importantly, SupportsPutBlobPartial could get out of sync with the actual implementation.[1]

The immediate problem could be handled by making private.ImageSource a struct

type private.ImageSource struct {
    HasThreadSafeGetBlob bool
    SupportsPutBlobPartial bool
    BasicImageSourceOperations // an interface
    PutBlobPartialOperations // an interface, or even a *PutBlobPartialOperations and remove a separate SupportsPutBlobPartial
}

which is quite attractive in some respects, OTOH calling methods would be more annoying, and to an extent this is replacing compile-time interfaces with run-time dynamic function pointers. We would also have a separate private.ImageSource struct and some separate dockerImageSource struct carrying the actual transport’s state, so that doesn’t help tracing the relationship between transport’s properties and methods much.

With the mix-ins embedded into interface implementations, questions like “what implements $method for $transport” are compiler-visible and can be statically answered, though probably hard to follow with an IDE. With a struct pointing to separate interfaces, all of that data only exists dynamically at runtime.

[1] The primary motivation for the No/Implements… stubs is to make it a compile-time failure to forget to add a method, or to fail to keep up with an interface change. Without that, it’s IIRC possible to change the type of a parameter in an interface, and implementations suddenly don’t match without any notice, so a run-time check like source.(private.ImageDestinationWithPutBlobPartial) can fail. With the stubs, and a single compile-time type check against a private.ImageDestination, we can guarantee that one of the two stubs are present and that the type signatures are correct.

Admittedly this might be overthinking it — there are only 10 transports after all, and we could just do careful type checks in unit tests, and review API additions carefully. Then we wouldn’t the Supports… methods, and we could just do interface type checks.

WRT the non-Supports values, one possible alternative is

type Properties struct {
   HasThreadSafeGetBlob bool
   … 
}

type ImageSource interface {
    func Properties() Properties
}

and then we don’t have a PropertyMethodsInitialize, just a single method implemented in every transport (+ compat.Impl providing the single-value getters in the existing interface).

One downside to this is that actual users of the transport would be more clumsy — everything would be source.Properties().SomeValue. A larger downside is that it would be harder and more expensive to implement “forwarding” transports like oci/archive forwarding to oci/layout, or BlobCache, especially if the forwarding transport only wanted to override some of the values — and the forwarding transport would have to collect all of the values although the caller only wants one of them.

I’m fairly certain there is a better design to be found, and I’m very open to suggestions.

Copy link
Collaborator Author

@mtrmac mtrmac Jul 4, 2022 •
edited

Choose a reason for hiding this comment

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

(Also, the …Initialize convention kind of drives me nuts. It seems somewhat attractive to me: to move that to a run-time check (allow each of those stubs to check if it has been initialized as a private method, and run that check in a unit test of the transport):

type privateSourceInitializationChecks interface {
   initializedPutBlobPartialStub()
   initialized…
}

func (stub ImplementsPutBlobPartial) initializedPutBlobPartialStub() bool {}

func (stub NoPutBlobPartialInitialize) initializedPutBlobPartialStub() bool {
	if !initialized { panic() }
}

func ValidatePrivateSourceImplementation(src private.ImageSource) { // called from a unit test
   inits, ok := src.(privateSourceInitializationChecks)
   inits.initializedPutBlobPartialStub()
   inits.initialized…
}

That has some run-time cost, but much worse it only works for transports where we can ~successfully create the source/destination object in a unit test. Which is awkward for remote clients like docker-daemon.

Oh well, I suppose hating Go might count as a hobby…)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed elaboration, @mtrmac.

I am supportive of the idea - and think it's a really good one. I very much enjoy the fact that build tests can help review changes :)


ref dirReference
}

// newImageDestination returns an ImageDestination for writing to a directory.
func newImageDestination(sys *types.SystemContext, ref dirReference) (types.ImageDestination, error) {
func newImageDestination(sys *types.SystemContext, ref dirReference) (private.ImageDestination, error) {
desiredLayerCompression := types.PreserveOriginal
if sys != nil {
if sys.DirForceCompress {
Expand All @@ -42,28 +49,27 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (types.Imag
desiredLayerCompression = types.Decompress
}
}
d := &dirImageDestination{ref: ref, desiredLayerCompression: desiredLayerCompression}

// If directory exists check if it is empty
// if not empty, check whether the contents match that of a container image directory and overwrite the contents
// if the contents don't match throw an error
dirExists, err := pathExists(d.ref.resolvedPath)
dirExists, err := pathExists(ref.resolvedPath)
if err != nil {
return nil, perrors.Wrapf(err, "checking for path %q", d.ref.resolvedPath)
return nil, perrors.Wrapf(err, "checking for path %q", ref.resolvedPath)
}
if dirExists {
isEmpty, err := isDirEmpty(d.ref.resolvedPath)
isEmpty, err := isDirEmpty(ref.resolvedPath)
if err != nil {
return nil, err
}

if !isEmpty {
versionExists, err := pathExists(d.ref.versionPath())
versionExists, err := pathExists(ref.versionPath())
if err != nil {
return nil, perrors.Wrapf(err, "checking if path exists %q", d.ref.versionPath())
return nil, perrors.Wrapf(err, "checking if path exists %q", ref.versionPath())
}
if versionExists {
contents, err := os.ReadFile(d.ref.versionPath())
contents, err := os.ReadFile(ref.versionPath())
if err != nil {
return nil, err
}
Expand All @@ -75,22 +81,37 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (types.Imag
return nil, ErrNotContainerImageDir
}
// delete directory contents so that only one image is in the directory at a time
if err = removeDirContents(d.ref.resolvedPath); err != nil {
return nil, perrors.Wrapf(err, "erasing contents in %q", d.ref.resolvedPath)
if err = removeDirContents(ref.resolvedPath); err != nil {
return nil, perrors.Wrapf(err, "erasing contents in %q", ref.resolvedPath)
}
logrus.Debugf("overwriting existing container image directory %q", d.ref.resolvedPath)
logrus.Debugf("overwriting existing container image directory %q", ref.resolvedPath)
}
} else {
// create directory if it doesn't exist
if err := os.MkdirAll(d.ref.resolvedPath, 0755); err != nil {
return nil, perrors.Wrapf(err, "unable to create directory %q", d.ref.resolvedPath)
if err := os.MkdirAll(ref.resolvedPath, 0755); err != nil {
return nil, perrors.Wrapf(err, "unable to create directory %q", ref.resolvedPath)
}
}
// create version file
err = os.WriteFile(d.ref.versionPath(), []byte(version), 0644)
err = os.WriteFile(ref.versionPath(), []byte(version), 0644)
if err != nil {
return nil, perrors.Wrapf(err, "creating version file %q", d.ref.versionPath())
return nil, perrors.Wrapf(err, "creating version file %q", ref.versionPath())
}

d := &dirImageDestination{
PropertyMethodsInitialize: impl.PropertyMethods(impl.Properties{
SupportedManifestMIMETypes: nil,
DesiredLayerCompression: desiredLayerCompression,
AcceptsForeignLayerURLs: false,
MustMatchRuntimeOS: false,
IgnoresEmbeddedDockerReference: false, // N/A, DockerReference() returns nil.
HasThreadSafePutBlob: false,
}),
NoPutBlobPartialInitialize: stubs.NoPutBlobPartial(ref),

ref: ref,
}
d.Compat = impl.AddCompat(d)
return d, nil
}

Expand All @@ -105,51 +126,14 @@ func (d *dirImageDestination) Close() error {
return nil
}

func (d *dirImageDestination) SupportedManifestMIMETypes() []string {
return nil
}

// SupportsSignatures returns an error (to be displayed to the user) if the destination certainly can't store signatures.
// Note: It is still possible for PutSignatures to fail if SupportsSignatures returns nil.
func (d *dirImageDestination) SupportsSignatures(ctx context.Context) error {
return nil
}

func (d *dirImageDestination) DesiredLayerCompression() types.LayerCompression {
return d.desiredLayerCompression
}

// AcceptsForeignLayerURLs returns false iff foreign layers in manifest should be actually
// uploaded to the image destination, true otherwise.
func (d *dirImageDestination) AcceptsForeignLayerURLs() bool {
return false
}

// MustMatchRuntimeOS returns true iff the destination can store only images targeted for the current runtime architecture and OS. False otherwise.
func (d *dirImageDestination) MustMatchRuntimeOS() bool {
return false
}

// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
// and would prefer to receive an unmodified manifest instead of one modified for the destination.
// Does not make a difference if Reference().DockerReference() is nil.
func (d *dirImageDestination) IgnoresEmbeddedDockerReference() bool {
return false // N/A, DockerReference() returns nil.
}

// HasThreadSafePutBlob indicates whether PutBlob can be executed concurrently.
func (d *dirImageDestination) HasThreadSafePutBlob() bool {
return false
}

// PutBlob writes contents of stream and returns data representing the result (with all data filled in).
// PutBlobWithOptions writes contents of stream and returns data representing the result.
// inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents.
// inputInfo.Size is the expected length of stream, if known.
// May update cache.
// inputInfo.MediaType describes the blob format, if known.
// WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available
// to any other readers for download using the supplied digest.
// If stream.Read() at any time, ESPECIALLY at end of input, returns an error, PutBlob MUST 1) fail, and 2) delete any data stored so far.
func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) {
func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, options private.PutBlobOptions) (types.BlobInfo, error) {
blobFile, err := os.CreateTemp(d.ref.path, "dir-put-blob")
if err != nil {
return types.BlobInfo{}, err
Expand Down Expand Up @@ -200,16 +184,14 @@ func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp
return types.BlobInfo{Digest: blobDigest, Size: size}, nil
}

// TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination
// TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination
// (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree).
// info.Digest must not be empty.
// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input.
// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size, and may
// include CompressionOperation and CompressionAlgorithm fields to indicate that a change to the compression type should be
// reflected in the manifest that will be written.
// If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure.
// May use and/or update cache.
func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) {
func (d *dirImageDestination) TryReusingBlobWithOptions(ctx context.Context, info types.BlobInfo, options private.TryReusingBlobOptions) (bool, types.BlobInfo, error) {
if info.Digest == "" {
return false, types.BlobInfo{}, fmt.Errorf("Can not check for a blob with unknown digest")
}
Expand Down
3 changes: 3 additions & 0 deletions directory/directory_test.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"testing"

"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/manifest"
"github.com/containers/image/v5/pkg/blobinfocache/memory"
"github.com/containers/image/v5/types"
Expand All @@ -16,6 +17,8 @@ import (
"github.com/stretchr/testify/require"
)

var _ private.ImageDestination = (*dirImageDestination)(nil)

func TestDestinationReference(t *testing.T) {
ref, tmpDir := refToTempDir(t)

Expand Down
10 changes: 3 additions & 7 deletions docker/archive/dest.go
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ import (
"io"

"github.com/containers/image/v5/docker/internal/tarfile"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/types"
)

Expand All @@ -16,7 +17,7 @@ type archiveImageDestination struct {
writer io.Closer // May be nil if the archive is shared
}

func newImageDestination(sys *types.SystemContext, ref archiveReference) (types.ImageDestination, error) {
func newImageDestination(sys *types.SystemContext, ref archiveReference) (private.ImageDestination, error) {
if ref.sourceIndex != -1 {
return nil, fmt.Errorf("Destination reference must not contain a manifest index @%d", ref.sourceIndex)
}
Expand All @@ -35,7 +36,7 @@ func newImageDestination(sys *types.SystemContext, ref archiveReference) (types.
archive = tarfile.NewWriter(fh)
writer = fh
}
tarDest := tarfile.NewDestination(sys, archive, ref.ref)
tarDest := tarfile.NewDestination(sys, archive, ref.Transport().Name(), ref.ref)
if sys != nil && sys.DockerArchiveAdditionalTags != nil {
tarDest.AddRepoTags(sys.DockerArchiveAdditionalTags)
}
Expand All @@ -47,11 +48,6 @@ func newImageDestination(sys *types.SystemContext, ref archiveReference) (types.
}, nil
}

// DesiredLayerCompression indicates if layers must be compressed, decompressed or preserved
func (d *archiveImageDestination) DesiredLayerCompression() types.LayerCompression {
return types.Decompress
}

// Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent,
// e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects.
func (d *archiveImageDestination) Reference() types.ImageReference {
Expand Down
5 changes: 5 additions & 0 deletions docker/archive/dest_test.go
View file
Open in desktop
@@ -0,0 +1,5 @@
package archive

import "github.com/containers/image/v5/internal/private"

var _ private.ImageDestination = (*archiveImageDestination)(nil)
5 changes: 3 additions & 2 deletions docker/daemon/daemon_dest.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/containers/image/v5/docker/internal/tarfile"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/types"
"github.com/docker/docker/client"
perrors "github.com/pkg/errors"
Expand All @@ -28,7 +29,7 @@ type daemonImageDestination struct {
}

// newImageDestination returns a types.ImageDestination for the specified image reference.
func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daemonReference) (types.ImageDestination, error) {
func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daemonReference) (private.ImageDestination, error) {
if ref.ref == nil {
return nil, fmt.Errorf("Invalid destination docker-daemon:%s: a destination must be a name:tag", ref.StringWithinTransport())
}
Expand Down Expand Up @@ -58,7 +59,7 @@ func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daem
return &daemonImageDestination{
ref: ref,
mustMatchRuntimeOS: mustMatchRuntimeOS,
Destination: tarfile.NewDestination(sys, archive, namedTaggedRef),
Destination: tarfile.NewDestination(sys, archive, ref.Transport().Name(), namedTaggedRef),
archive: archive,
goroutineCancel: goroutineCancel,
statusChannel: statusChannel,
Expand Down
5 changes: 5 additions & 0 deletions docker/daemon/daemon_dest_test.go
View file
Open in desktop
@@ -0,0 +1,5 @@
package daemon

import "github.com/containers/image/v5/internal/private"

var _ private.ImageDestination = (*daemonImageDestination)(nil)