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

distributedCache: allow local dir:// and container-storage:// as a cache transport #4879

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

distributedCache: allow local dir:// and container-storage:// as a cache transport #4879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions define/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"time"

nettypes "github.com/containers/common/libnetwork/types"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/types"
encconfig "github.com/containers/ocicrypt/config"
"github.com/containers/storage/pkg/archive"
Expand Down Expand Up @@ -141,10 +140,10 @@ type BuildOptions struct {
TransientMounts []string
// CacheFrom specifies any remote repository which can be treated as
// potential cache source.
CacheFrom []reference.Named
CacheFrom []types.ImageReference
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking the API yes.

// CacheTo specifies any remote repository which can be treated as
// potential cache destination.
CacheTo []reference.Named
CacheTo []types.ImageReference
// CacheTTL specifies duration, if specified using `--cache-ttl` then
// cache intermediate images under this duration will be considered as
// valid cache sources and images outside this duration will be ignored.
Expand Down
4 changes: 2 additions & 2 deletions imagebuildah/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ var builtinAllowedBuildArgs = map[string]bool{
// interface. It coordinates the entire build by using one or more
// StageExecutors to handle each stage of the build.
type Executor struct {
cacheFrom []reference.Named
cacheTo []reference.Named
cacheFrom []types.ImageReference
cacheTo []types.ImageReference
cacheTTL time.Duration
containerSuffix string
logger *logrus.Logger
Expand Down
52 changes: 38 additions & 14 deletions imagebuildah/stage_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/containers/buildah/util"
config "github.com/containers/common/pkg/config"
cp "github.com/containers/image/v5/copy"
imagedirectory "github.com/containers/image/v5/directory"
imagedocker "github.com/containers/image/v5/docker"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/manifest"
Expand Down Expand Up @@ -985,10 +986,16 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
}
// logCachePulled produces build log for cases when `--cache-from`
// is used and a valid intermediate image is pulled from remote source.
logCachePulled := func(cacheKey string, remote reference.Named) {
logCachePulled := func(cacheKey string, remote types.ImageReference) {
if !s.executor.quiet {
cachePullMessage := "--> Cache pulled from remote"
fmt.Fprintf(s.executor.out, "%s %s\n", cachePullMessage, fmt.Sprintf("%s:%s", remote.String(), cacheKey))
repo := ""
if remote.Transport().Name() == imagedocker.Transport.Name() {
repo = remote.DockerReference().String()
} else {
repo = remote.StringWithinTransport()
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is supposed to represent arbitrary transports, please use transports.ImageName(), not a single-transport-scoped name.

}
Comment on lines +993 to +997
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really value in special-casing docker:// here, enough to be worth the ambiguity?

fmt.Fprintf(s.executor.out, "%s %s\n", cachePullMessage, fmt.Sprintf("%s:%s", repo, cacheKey))
}
}
// logCachePush produces build log for cases when `--cache-to`
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn’t this also need updating?

Expand Down Expand Up @@ -1782,18 +1789,29 @@ func (s *StageExecutor) generateCacheKey(ctx context.Context, currNode *parser.N

// cacheImageReference is internal function which generates ImageReference from Named repo sources
// and a tag.
func cacheImageReferences(repos []reference.Named, cachekey string) ([]types.ImageReference, error) {
func cacheImageReferences(repos []types.ImageReference, cachekey string) ([]types.ImageReference, error) {
var result []types.ImageReference
for _, repo := range repos {
tagged, err := reference.WithTag(repo, cachekey)
if err != nil {
return nil, fmt.Errorf("failed generating tagged reference for %q: %w", repo, err)
}
dest, err := imagedocker.NewReference(tagged)
if err != nil {
return nil, fmt.Errorf("failed generating docker reference for %q: %w", tagged, err)
if repo.Transport().Name() == imagedocker.Transport.Name() {
ref := repo.DockerReference()
tagged, err := reference.WithTag(ref, cachekey)
if err != nil {
return nil, fmt.Errorf("failed generating tagged reference for %q: %w", repo, err)
}
dest, err := imagedocker.NewReference(tagged)
if err != nil {
return nil, fmt.Errorf("failed generating docker reference for %q: %w", tagged, err)
}
result = append(result, dest)
} else if repo.Transport().Name() == imagedirectory.Transport.Name() {
dirPath := repo.StringWithinTransport()
tagged := filepath.Join(dirPath, cachekey)
dest, err := imagedirectory.NewReference(tagged)
if err != nil {
return nil, fmt.Errorf("failed generating directory reference for %q: %w", tagged, err)
}
result = append(result, dest)
Comment on lines +1806 to +1813
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m fairly unhappy with using the transport:image-name syntax, and especially the types.ImageReference values, to represent things that are not a single image. This would not be the first instance where that happens but I’m unhappy every time :)

Admittedly c/image has no “repo/collection of images” abstraction… and that’s because, just like this code shows, there’s just too little the transports have in common to build a meaningful abstraction like that.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

else… this silently drops unsupported input without telling the user?!

result = append(result, dest)
}
return result, nil
}
Expand Down Expand Up @@ -1833,7 +1851,7 @@ func (s *StageExecutor) pushCache(ctx context.Context, src, cacheKey string) err
// or a newer version of cache was found in the upstream repo. If new
// image was pulled function returns image id otherwise returns empty
// string "" or error if any error was encontered while pulling the cache.
func (s *StageExecutor) pullCache(ctx context.Context, cacheKey string) (reference.Named, string, error) {
func (s *StageExecutor) pullCache(ctx context.Context, cacheKey string) (types.ImageReference, string, error) {
srcList, err := cacheImageReferences(s.executor.cacheFrom, cacheKey)
if err != nil {
return nil, "", err
Expand All @@ -1851,14 +1869,20 @@ func (s *StageExecutor) pullCache(ctx context.Context, cacheKey string) (referen
ReportWriter: nil,
PullPolicy: define.PullIfNewer,
}
id, err := buildah.Pull(ctx, src.DockerReference().String(), options)
srcString := ""
if src.Transport().Name() == imagedocker.Transport.Name() {
srcString = src.DockerReference().String()
} else if src.Transport().Name() == imagedirectory.Transport.Name() {
srcString = "dir://" + src.StringWithinTransport()
Copy link
Contributor

Choose a reason for hiding this comment

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

// is not a part of the dir: syntax (and, theory corner, in POSIX, paths starting with "//" have, hypothetically special meaning, some UNIX systems IIRC accepted a "//nfs.host.name/path" at the kernel level).

This should just unconditionally use transports.ImageName; the called function uses alltransports.ParseImageName (before starting with heuristics), so this should use transports.ImageName to create a clearly unambiguous input.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

else… what??

id, err := buildah.Pull(ctx, srcString, options)
if err != nil {
logrus.Debugf("failed pulling cache from source %s: %v", src, err)
continue // failed pulling this one try next
//return "", fmt.Errorf("failed while pulling cache from %q: %w", src, err)
}
logrus.Debugf("successfully pulled cache from repo %s: %s", src, id)
return src.DockerReference(), id, nil
return src, id, nil
}
return nil, "", fmt.Errorf("failed pulling cache from all available sources %q", srcList)
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/cli/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/containers/buildah/pkg/parse"
"github.com/containers/buildah/pkg/util"
"github.com/containers/common/pkg/auth"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/types"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -296,8 +295,8 @@ func GenBuildOptions(c *cobra.Command, inputArgs []string, iopts BuildOptions) (
iopts.Quiet = true
}
}
var cacheTo []reference.Named
var cacheFrom []reference.Named
var cacheTo []types.ImageReference
var cacheFrom []types.ImageReference
cacheTo = nil
cacheFrom = nil
if c.Flag("cache-to").Changed {
Expand Down
15 changes: 8 additions & 7 deletions pkg/parse/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/containers/buildah/pkg/sshagent"
"github.com/containers/common/pkg/config"
"github.com/containers/common/pkg/parse"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/transports/alltransports"
"github.com/containers/image/v5/types"
"github.com/containers/storage/pkg/idtools"
"github.com/containers/storage/pkg/unshare"
Expand Down Expand Up @@ -53,16 +53,17 @@ const (
)

// RepoNamesToNamedReferences parse the raw string to Named reference
func RepoNamesToNamedReferences(destList []string) ([]reference.Named, error) {
var result []reference.Named
func RepoNamesToNamedReferences(destList []string) ([]types.ImageReference, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name and comment are now incorrect.

var result []types.ImageReference
for _, dest := range destList {
named, err := reference.ParseNormalizedNamed(dest)
if t := alltransports.TransportFromImageName(dest); t == nil {
// assume no default transport provided in such case append docker
dest = "docker://" + dest
Copy link
Contributor

Choose a reason for hiding this comment

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

If this remains: There are already over half a dozen copies of a similar fallback code, in many different variants. That should be centralized.

(For a start, there is util.DefaultTransport.)

}
named, err := alltransports.ParseImageName(dest)
if err != nil {
return nil, fmt.Errorf("invalid repo %q: must contain registry and repository: %w", dest, err)
}
if !reference.IsNameOnly(named) {
return nil, fmt.Errorf("repository must contain neither a tag nor digest: %v", named)
}
result = append(result, named)
}
return result, nil
Expand Down