Merge pull request #3456 from nalind/platforms3

imagebuildah: handle --manifest directly

cmd/buildah/bud.go

@@ -378,7 +378,10 @@ func budCmd(c *cobra.Command, inputArgs []string, iopts budOptions) error {
 		options.ReportWriter = ioutil.Discard
 	}
 
-	_, _, err = imagebuildah.BuildDockerfiles(getContext(), store, options, dockerfiles...)
+	id, ref, err := imagebuildah.BuildDockerfiles(getContext(), store, options, dockerfiles...)
+	if err == nil && options.Manifest != "" {
+		logrus.Debugf("manifest list id = %q, ref = %q", id, ref.String())
+	}
 	return err
 }
 

cmd/buildah/commit.go

@@ -75,8 +75,8 @@ func init() {
 	flags.StringVar(&opts.creds, "creds", "", "use `[username[:password]]` for accessing the registry")
 	flags.BoolVarP(&opts.disableCompression, "disable-compression", "D", true, "don't compress layers")
 	flags.StringVarP(&opts.format, "format", "f", defaultFormat(), "`format` of the image manifest and metadata")
-	flags.StringVar(&opts.manifest, "manifest", "", "create image with as part of the specified manifest list. Creates manifest if it does not exist")
-	flags.StringVar(&opts.iidfile, "iidfile", "", "Write the image ID to the file")
+	flags.StringVar(&opts.manifest, "manifest", "", "adds created image to the specified manifest list. Creates manifest list if it does not exist")
+	flags.StringVar(&opts.iidfile, "iidfile", "", "write the image ID to the file")
 	flags.BoolVar(&opts.omitTimestamp, "omit-timestamp", false, "set created timestamp to epoch 0 to allow for deterministic builds")
 	flags.Int64Var(&opts.timestamp, "timestamp", 0, "set created timestamp to epoch seconds to allow for deterministic builds, defaults to current time")
 	flags.BoolVarP(&opts.quiet, "quiet", "q", false, "don't output progress information when writing images")

cmd/buildah/mount.go

@@ -61,7 +61,6 @@ func init() {
 }
 
 func mountCmd(c *cobra.Command, args []string, opts mountOptions) error {
-
 	if err := buildahcli.VerifyFlagsArgsOrder(args); err != nil {
 		return err
 	}

cmd/buildah/umount.go

@@ -10,7 +10,6 @@ import (
 )
 
 func init() {
-
 	umountCommand := &cobra.Command{
 		Use:     "umount",
 		Aliases: []string{"unmount"},

docs/buildah-bud.md

@@ -354,7 +354,7 @@ environment variable. `export BUILDAH_LAYERS=true`
 Log output which would be sent to standard output and standard error to the
 specified file instead of to standard output and standard error.
 
-**--manifest** "manifest"
+**--manifest** "listName"
 
 Name of the manifest list to which the built image will be added.  Creates the
 manifest list if it does not exist.  This option is useful for building multi
@@ -795,14 +795,18 @@ buildah bud --dns-search=example.com --dns=223.5.5.5 --dns-option=use-vc .
 
 buildah bud -f Containerfile.in -t imageName .
 
-### Building an multi-architecture image using a --manifest option (Requires emulation software)
+### Building an multi-architecture image using the --manifest option (requires emulation software)
 
 buildah bud --arch arm --manifest myimage /tmp/mysrc
 
 buildah bud --arch amd64 --manifest myimage /tmp/mysrc
 
 buildah bud --arch s390x --manifest myimage /tmp/mysrc
 
+buildah bud --platform linux/s390x,linux/ppc64le,linux/amd64 --manifest myimage /tmp/mysrc
+
+buildah bud --platform linux/arm64 --platform linux/amd64 --manifest myimage /tmp/mysrc
+
 ### Building an image using a URL
 
   This will clone the specified GitHub repository from the URL and use it as context. The Containerfile or Dockerfile at the root of the repository is used as the context of the build. This only works if the GitHub repository is a dedicated repository.

docs/buildah-commit.md

@@ -69,9 +69,9 @@ environment variable.  `export BUILDAH\_FORMAT=docker`
 
 Write the image ID to the file.
 
-**--manifest** "manifest"
+**--manifest** "listName"
 
-Name of the manifest list to which the image will be added. Creates the manifest list
+Name of the manifest list to which the built image will be added. Creates the manifest list
 if it does not exist. This option is useful for building multi architecture images.
 
 **--quiet**, **-q**
@@ -130,7 +130,7 @@ This example commits the container to the image on the local registry using cred
 This example saves an image based on the container, but stores dates based on epoch time.
 `buildah commit --timestamp=0 containerID newImageName`
 
-### Building an multi-architecture image using a --manifest option (Requires emulation software)
+### Building an multi-architecture image using the --manifest option (requires emulation software)
 
 ```
 #!/bin/sh

docs/buildah-source-add.md

@@ -15,6 +15,7 @@ Note that the buildah-source command and all its subcommands are experimental
 and may be subject to future changes
 
 ## OPTIONS
+
 **--annotation** *key=value*
 
 Add an annotation to the layer descriptor in the source-image manifest.  The input format is `key=value`.

imagebuildah/build.go

@@ -11,18 +11,20 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/containers/buildah/define"
-	"github.com/containers/buildah/manifests"
 	"github.com/containers/buildah/util"
 	"github.com/containers/common/libimage"
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/manifest"
+	istorage "github.com/containers/image/v5/storage"
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/hashicorp/go-multierror"
+	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/openshift/imagebuilder"
 	"github.com/openshift/imagebuilder/dockerfile/parser"
@@ -91,7 +93,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 		var data io.ReadCloser
 
 		if strings.HasPrefix(dfile, "http://") || strings.HasPrefix(dfile, "https://") {
-			logrus.Debugf("reading remote Dockerfile %q", dfile)
+			logger.Debugf("reading remote Dockerfile %q", dfile)
 			resp, err := http.Get(dfile)
 			if err != nil {
 				return "", nil, err
@@ -120,7 +122,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 			if dinfo.Mode().IsDir() {
 				for _, file := range []string{"Containerfile", "Dockerfile"} {
 					f := filepath.Join(dfile, file)
-					logrus.Debugf("reading local %q", f)
+					logger.Debugf("reading local %q", f)
 					contents, err = os.Open(f)
 					if err == nil {
 						break
@@ -170,28 +172,14 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 		options.JobSemaphore = semaphore.NewWeighted(int64(*options.Jobs))
 	}
 
-	if options.Manifest != "" && len(options.Platforms) > 0 {
-		// Ensure that the list's ID is known before we spawn off any
-		// goroutines that'll want to modify it, so that they don't
-		// race and create two lists, one of which will rapidly become
-		// ignored.
-		names, err := util.ExpandNames([]string{options.Manifest}, options.SystemContext, store)
-		if err != nil {
-			return "", nil, errors.Wrapf(err, "while expanding manifest list name %q", options.Manifest)
-		}
-		rt, err := libimage.RuntimeFromStore(store, nil)
-		if err != nil {
-			return "", nil, err
-		}
-		_, err = rt.LookupManifestList(options.Manifest)
-		if err != nil && errors.Cause(err) == storage.ErrImageUnknown {
-			list := manifests.Create()
-			_, err = list.SaveToImage(store, "", names, manifest.DockerV2ListMediaType)
-		}
-		if err != nil {
-			return "", nil, err
-		}
+	manifestList := options.Manifest
+	options.Manifest = ""
+	type instance struct {
+		v1.Platform
+		ID string
 	}
+	var instances []instance
+	var instancesLock sync.Mutex
 
 	var builds multierror.Group
 	if options.SystemContext == nil {
@@ -227,6 +215,16 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 				return err
 			}
 			id, ref = thisID, thisRef
+			instancesLock.Lock()
+			instances = append(instances, instance{
+				ID: thisID,
+				Platform: v1.Platform{
+					OS:           platformContext.OSChoice,
+					Architecture: platformContext.ArchitectureChoice,
+					Variant:      platformContext.VariantChoice,
+				},
+			})
+			instancesLock.Unlock()
 			return nil
 		})
 	}
@@ -237,17 +235,71 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options define.B
 		}
 		return "", nil, merr.ErrorOrNil()
 	}
-	if options.Manifest != "" {
+
+	if manifestList != "" {
 		rt, err := libimage.RuntimeFromStore(store, nil)
 		if err != nil {
 			return "", nil, err
 		}
-		list, err := rt.LookupManifestList(options.Manifest)
+		// Create the manifest list ourselves, so that it's not in a
+		// partially-populated state at any point if we're creating it
+		// fresh.
+		list, err := rt.LookupManifestList(manifestList)
+		if err != nil && errors.Cause(err) == storage.ErrImageUnknown {
+			list, err = rt.CreateManifestList(manifestList)
+		}
 		if err != nil {
 			return "", nil, err
 		}
+		// Add each instance to the list in turn.
+		storeTransportName := istorage.Transport.Name()
+		for _, instance := range instances {
+			instanceDigest, err := list.Add(ctx, storeTransportName+":"+instance.ID, nil)
+			if err != nil {
+				return "", nil, err
+			}
+			err = list.AnnotateInstance(instanceDigest, &libimage.ManifestListAnnotateOptions{
+				Architecture: instance.Architecture,
+				OS:           instance.OS,
+				Variant:      instance.Variant,
+			})
+			if err != nil {
+				return "", nil, err
+			}
+		}
 		id, ref = list.ID(), nil
+		// Put together a canonical reference
+		storeRef, err := istorage.Transport.NewStoreReference(store, nil, list.ID())
+		if err != nil {
+			return "", nil, err
+		}
+		imgSource, err := storeRef.NewImageSource(ctx, nil)
+		if err != nil {
+			return "", nil, err
+		}
+		defer imgSource.Close()
+		manifestBytes, _, err := imgSource.GetManifest(ctx, nil)
+		if err != nil {
+			return "", nil, err
+		}
+		manifestDigest, err := manifest.Digest(manifestBytes)
+		if err != nil {
+			return "", nil, err
+		}
+		img, err := store.Image(id)
+		if err != nil {
+			return "", nil, err
+		}
+		for _, name := range img.Names {
+			if named, err := reference.ParseNamed(name); err == nil {
+				if r, err := reference.WithDigest(reference.TrimNamed(named), manifestDigest); err == nil {
+					ref = r
+					break
+				}
+			}
+		}
 	}
+
 	return id, ref, nil
 }
 

imagebuildah/executor.go

@@ -438,7 +438,7 @@ func (b *Executor) buildStage(ctx context.Context, cleanupStages map[int]*StageE
 	}
 
 	if err != nil {
-		logrus.Debugf("Build(node.Children=%#v)", node.Children)
+		logrus.Debugf("buildStage(node.Children=%#v)", node.Children)
 		return "", nil, err
 	}
 

imagebuildah/stage_executor.go

@@ -919,7 +919,6 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string,
 		// determining if a cached layer with the same build args already exists
 		// and that is done in the if block below.
 		if checkForLayers && step.Command != "arg" {
-
 			cacheID, err = s.intermediateImageExists(ctx, node, addedContentSummary, s.stepRequiresLayer(step))
 			if err != nil {
 				return "", nil, errors.Wrap(err, "error checking if cached image exists from a previous build")

pkg/cli/common.go

@@ -205,7 +205,7 @@ func GetBudFlags(flags *BudResults) pflag.FlagSet {
 	if err := fs.MarkHidden("rusage-logfile"); err != nil {
 		panic(fmt.Sprintf("error marking the rusage-logfile flag as hidden: %v", err))
 	}
-	fs.StringVar(&flags.Manifest, "manifest", "", "add the image to the specified manifest list. Creates manifest if it does not exist")
+	fs.StringVar(&flags.Manifest, "manifest", "", "add the image to the specified manifest list. Creates manifest list if it does not exist")
 	fs.BoolVar(&flags.NoCache, "no-cache", false, "Do not use existing cached images for the container build. Build from the start with a new set of cached layers.")
 	fs.String("os", runtime.GOOS, "set the OS to the provided value instead of the current operating system of the host")
 	fs.BoolVar(&flags.Pull, "pull", true, "pull the image from the registry if newer or not present in store, if false, only pull the image if not present")

pkg/overlay/overlay.go

@@ -20,7 +20,6 @@ import (
 
 // TempDir generates an overlay Temp directory in the container content
 func TempDir(containerDir string, rootUID, rootGID int) (string, error) {
-
 	contentDir := filepath.Join(containerDir, "overlay")
 	if err := idtools.MkdirAllAs(contentDir, 0700, rootUID, rootGID); err != nil {
 		return "", errors.Wrapf(err, "failed to create the overlay %s directory", contentDir)
@@ -36,7 +35,6 @@ func TempDir(containerDir string, rootUID, rootGID int) (string, error) {
 
 // GenerateStructure generates an overlay directory structure for container content
 func GenerateStructure(containerDir, containerID, name string, rootUID, rootGID int) (string, error) {
-
 	contentDir := filepath.Join(containerDir, "overlay-containers", containerID, name)
 	if err := idtools.MkdirAllAs(contentDir, 0700, rootUID, rootGID); err != nil {
 		return "", errors.Wrapf(err, "failed to create the overlay %s directory", contentDir)
@@ -47,7 +45,6 @@ func GenerateStructure(containerDir, containerID, name string, rootUID, rootGID
 
 // generateOverlayStructure generates upper, work and merge directory structure for overlay directory
 func generateOverlayStructure(containerDir string, rootUID, rootGID int) (string, error) {
-
 	upperDir := filepath.Join(containerDir, "upper")
 	workDir := filepath.Join(containerDir, "work")
 	if err := idtools.MkdirAllAs(upperDir, 0700, rootUID, rootGID); err != nil {

pkg/parse/parse_unix.go

@@ -27,7 +27,6 @@ func DeviceFromPath(device string) (define.ContainerDevices, error) {
 	}
 
 	if !srcInfo.IsDir() {
-
 		dev, err := devices.DeviceFromPath(src, permissions)
 		if err != nil {
 			return nil, errors.Wrapf(err, "%s is not a valid device", src)

run_linux.go

@@ -1782,7 +1782,6 @@ func (b *Builder) cleanupTempVolumes() {
 }
 
 func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, optionMounts []specs.Mount, rootUID, rootGID, processUID, processGID int) (mounts []specs.Mount, Err error) {
-
 	// Make sure the overlay directory is clean before running
 	containerDir, err := b.store.ContainerDirectory(b.ContainerID)
 	if err != nil {
@@ -1844,7 +1843,6 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string,
 
 			overlayMount, err := overlay.Mount(contentDir, host, container, rootUID, rootGID, b.store.GraphOptions())
 			if err == nil {
-
 				b.TempVolumes[contentDir] = true
 			}
 

tests/bud.bats

@@ -3328,3 +3328,9 @@ _EOF
   test -n "$d2"
   test "$d1" != "$d2"
 }
+
+@test "bud-multiple-platform-no-partial-manifest-list" {
+  outputlist=localhost/testlist
+  run_buildah 1 bud --signature-policy ${TESTSDIR}/policy.json --platform=linux/arm,linux/amd64 --manifest $outputlist -f ${TESTSDIR}/bud/multiarch/Dockerfile.fail ${TESTSDIR}/bud/multiarch
+  run_buildah 125 manifest inspect $outputlist
+}

tests/bud/multiarch/Dockerfile.fail

@@ -0,0 +1,5 @@
+# This build should fail if we're building with at least one non-amd64 platform
+# either because we can't execute this test binary, or because it executed fine
+# but returned an error
+FROM alpine
+RUN test `arch` = x86_64