From 84ee6f07460ede5ae1e971efb1143ce8319351e9 Mon Sep 17 00:00:00 2001 From: kevinrizza Date: Fri, 1 May 2020 11:32:22 -0400 Subject: [PATCH 1/2] (bug) Always overwrite local db when copying Multiple runs of index add when --generate is specified because the db file cannot be cleaned up. To account for this, update the copyDatabaseTo function to not return when the parent folder already exists and overwrite the actual database when doing io.Copy() --- pkg/lib/indexer/indexer.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/lib/indexer/indexer.go b/pkg/lib/indexer/indexer.go index c7862a0af..5eb8aa093 100644 --- a/pkg/lib/indexer/indexer.go +++ b/pkg/lib/indexer/indexer.go @@ -71,7 +71,7 @@ func (i ImageIndexer) AddToIndex(request AddToIndexRequest) error { // this is in its own function context so that the deferred cleanup runs before we do a docker build // which prevents the full contents of the previous image from being in the build context var databasePath string - if err := func () error { + if err := func() error { tmpDir, err := ioutil.TempDir("./", tmpDirPrefix) if err != nil { @@ -152,7 +152,7 @@ func (i ImageIndexer) DeleteFromIndex(request DeleteFromIndexRequest) error { // this is in its own function context so that the deferred cleanup runs before we do a docker build // which prevents the full contents of the previous image from being in the build context var databasePath string - if err := func () error { + if err := func() error { tmpDir, err := ioutil.TempDir("./", tmpDirPrefix) if err != nil { @@ -228,7 +228,7 @@ func (i ImageIndexer) PruneFromIndex(request PruneFromIndexRequest) error { // this is in its own function context so that the deferred cleanup runs before we do a docker build // which prevents the full contents of the previous image from being in the build context var databasePath string - if err := func () error { + if err := func() error { tmpDir, err := ioutil.TempDir("./", tmpDirPrefix) if err != nil { @@ -339,7 +339,7 @@ func copyDatabaseTo(databaseFile, targetDir string) (string, error) { if err := os.MkdirAll(targetDir, 0777); err != nil { return "", err } - } else { + } else if err != nil { return "", err } @@ -353,7 +353,7 @@ func copyDatabaseTo(databaseFile, targetDir string) (string, error) { dbFile := path.Join(targetDir, defaultDatabaseFile) // define the path to copy to the database/index.db file - to, err := os.OpenFile(dbFile, os.O_RDWR|os.O_CREATE, 0666) + to, err := os.OpenFile(dbFile, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0666) if err != nil { return "", err } From d3c0563d82a64d7eaa11d52e43b818cbef9a828f Mon Sep 17 00:00:00 2001 From: kevinrizza Date: Fri, 1 May 2020 11:32:54 -0400 Subject: [PATCH 2/2] Split build and push command runners Enable daemonless pull by default on indexes. When no build happens, this makes opm index add daemonless --- cmd/opm/index/add.go | 66 +++++++++++++++++++++++++++++------ pkg/lib/indexer/indexer.go | 9 ++--- pkg/lib/indexer/interfaces.go | 20 ++++++----- test/e2e/opm_test.go | 4 +-- 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/cmd/opm/index/add.go b/cmd/opm/index/add.go index f6c8caad9..a4e4fed87 100644 --- a/cmd/opm/index/add.go +++ b/cmd/opm/index/add.go @@ -55,7 +55,9 @@ func addIndexAddCmd(parent *cobra.Command) { } indexCmd.Flags().Bool("skip-tls", false, "skip TLS certificate verification for container image registries while pulling bundles") indexCmd.Flags().StringP("binary-image", "i", "", "container image for on-image `opm` command") - indexCmd.Flags().StringP("container-tool", "c", "podman", "tool to interact with container images (save, build, etc.). One of: [docker, podman]") + indexCmd.Flags().StringP("container-tool", "c", "", "tool to interact with container images (save, build, etc.). One of: [docker, podman]") + indexCmd.Flags().StringP("build-tool", "u", "", "tool to build container images. One of: [docker, podman]. Defaults to podman. Overrides part of container-tool.") + indexCmd.Flags().StringP("pull-tool", "p", "", "tool to pull container images. One of: [none, docker, podman]. Defaults to none. Overrides part of container-tool.") indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built") indexCmd.Flags().Bool("permissive", false, "allow registry load errors") indexCmd.Flags().StringP("mode", "", "replaces", "graph update mode that defines how channel graphs are updated. One of: [replaces, semver, semver-skippatch]") @@ -96,15 +98,6 @@ func runIndexAddCmdFunc(cmd *cobra.Command, args []string) error { return err } - containerTool, err := cmd.Flags().GetString("container-tool") - if err != nil { - return err - } - - if containerTool == "none" { - return fmt.Errorf("none is not a valid container-tool for index add") - } - tag, err := cmd.Flags().GetString("tag") if err != nil { return err @@ -130,11 +123,19 @@ func runIndexAddCmdFunc(cmd *cobra.Command, args []string) error { return err } + pullTool, buildTool, err := getContainerTools(cmd) + if err != nil { + return err + } + logger := logrus.WithFields(logrus.Fields{"bundles": bundles}) logger.Info("building the index") - indexAdder := indexer.NewIndexAdder(containertools.NewContainerTool(containerTool, containertools.PodmanTool), logger) + indexAdder := indexer.NewIndexAdder( + containertools.NewContainerTool(buildTool, containertools.PodmanTool), + containertools.NewContainerTool(pullTool, containertools.NoneTool), + logger) request := indexer.AddToIndexRequest{ Generate: generate, @@ -155,3 +156,46 @@ func runIndexAddCmdFunc(cmd *cobra.Command, args []string) error { return nil } + +// getContainerTools returns the pull and build tools based on command line input +// to preserve backwards compatibility and alias the legacy `container-tool` parameter +func getContainerTools(cmd *cobra.Command) (string, string, error) { + buildTool, err := cmd.Flags().GetString("build-tool") + if err != nil { + return "", "", err + } + + if buildTool == "none" { + return "", "", fmt.Errorf("none is not a valid container-tool for index add") + } + + pullTool, err := cmd.Flags().GetString("pull-tool") + if err != nil { + return "", "", err + } + + containerTool, err := cmd.Flags().GetString("container-tool") + if err != nil { + return "", "", err + } + + // Backwards compatiblity mode + if containerTool != "" { + if pullTool == "" && buildTool == "" { + return containerTool, containerTool, nil + } else { + return "", "", fmt.Errorf("container-tool cannot be set alongside pull-tool or build-tool") + } + } + + // Check for defaults, then return + if pullTool == "" { + pullTool = "none" + } + + if buildTool == "" { + buildTool = "podman" + } + + return pullTool, buildTool, nil +} diff --git a/pkg/lib/indexer/indexer.go b/pkg/lib/indexer/indexer.go index 5eb8aa093..98735fa27 100644 --- a/pkg/lib/indexer/indexer.go +++ b/pkg/lib/indexer/indexer.go @@ -42,7 +42,8 @@ type ImageIndexer struct { RegistryAdder registry.RegistryAdder RegistryDeleter registry.RegistryDeleter RegistryPruner registry.RegistryPruner - ContainerTool containertools.ContainerTool + BuildTool containertools.ContainerTool + PullTool containertools.ContainerTool Logger *logrus.Entry } @@ -99,7 +100,7 @@ func (i ImageIndexer) AddToIndex(request AddToIndexRequest) error { Permissive: request.Permissive, Mode: request.Mode, SkipTLS: request.SkipTLS, - ContainerTool: i.ContainerTool, + ContainerTool: i.PullTool, } // Add the bundles to the registry @@ -292,13 +293,13 @@ func (i ImageIndexer) getDatabaseFile(workingDir, fromIndex string) (string, err var reg image.Registry var rerr error - switch i.ContainerTool { + switch i.PullTool { case containertools.NoneTool: reg, rerr = containerdregistry.NewRegistry(containerdregistry.WithLog(i.Logger)) case containertools.PodmanTool: fallthrough case containertools.DockerTool: - reg, rerr = execregistry.NewRegistry(i.ContainerTool, i.Logger) + reg, rerr = execregistry.NewRegistry(i.PullTool, i.Logger) } if rerr != nil { return "", rerr diff --git a/pkg/lib/indexer/interfaces.go b/pkg/lib/indexer/interfaces.go index 4918eec26..a9017ecc0 100644 --- a/pkg/lib/indexer/interfaces.go +++ b/pkg/lib/indexer/interfaces.go @@ -15,14 +15,15 @@ type IndexAdder interface { } // NewIndexAdder is a constructor that returns an IndexAdder -func NewIndexAdder(containerTool containertools.ContainerTool, logger *logrus.Entry) IndexAdder { +func NewIndexAdder(buildTool, pullTool containertools.ContainerTool, logger *logrus.Entry) IndexAdder { return ImageIndexer{ DockerfileGenerator: containertools.NewDockerfileGenerator(logger), - CommandRunner: containertools.NewCommandRunner(containerTool, logger), - LabelReader: containertools.NewLabelReader(containerTool, logger), + CommandRunner: containertools.NewCommandRunner(buildTool, logger), + LabelReader: containertools.NewLabelReader(pullTool, logger), RegistryAdder: registry.NewRegistryAdder(logger), - ImageReader: containertools.NewImageReader(containerTool, logger), - ContainerTool: containerTool, + ImageReader: containertools.NewImageReader(pullTool, logger), + BuildTool: buildTool, + PullTool: pullTool, Logger: logger, } } @@ -42,7 +43,8 @@ func NewIndexDeleter(containerTool containertools.ContainerTool, logger *logrus. LabelReader: containertools.NewLabelReader(containerTool, logger), RegistryDeleter: registry.NewRegistryDeleter(logger), ImageReader: containertools.NewImageReader(containerTool, logger), - ContainerTool: containerTool, + BuildTool: containerTool, + PullTool: containerTool, Logger: logger, } } @@ -59,7 +61,8 @@ func NewIndexExporter(containerTool containertools.ContainerTool, logger *logrus CommandRunner: containertools.NewCommandRunner(containerTool, logger), LabelReader: containertools.NewLabelReader(containerTool, logger), ImageReader: containertools.NewImageReader(containerTool, logger), - ContainerTool: containerTool, + BuildTool: containerTool, + PullTool: containerTool, Logger: logger, } } @@ -76,7 +79,8 @@ func NewIndexPruner(containerTool containertools.ContainerTool, logger *logrus.E LabelReader: containertools.NewLabelReader(containerTool, logger), RegistryPruner: registry.NewRegistryPruner(logger), ImageReader: containertools.NewImageReader(containerTool, logger), - ContainerTool: containerTool, + BuildTool: containerTool, + PullTool: containerTool, Logger: logger, } } diff --git a/test/e2e/opm_test.go b/test/e2e/opm_test.go index 729660913..453c5dde8 100644 --- a/test/e2e/opm_test.go +++ b/test/e2e/opm_test.go @@ -89,7 +89,7 @@ func buildIndexWith(containerTool string) error { bundleImage + ":" + bundleTag2, } logger := logrus.WithFields(logrus.Fields{"bundles": bundles}) - indexAdder := indexer.NewIndexAdder(containertools.NewContainerTool(containerTool, containertools.NoneTool), logger) + indexAdder := indexer.NewIndexAdder(containertools.NewContainerTool(containerTool, containertools.NoneTool), containertools.NewContainerTool(containerTool, containertools.NoneTool), logger) request := indexer.AddToIndexRequest{ Generate: false, @@ -109,7 +109,7 @@ func buildFromIndexWith(containerTool string) error { bundleImage + ":" + bundleTag3, } logger := logrus.WithFields(logrus.Fields{"bundles": bundles}) - indexAdder := indexer.NewIndexAdder(containertools.NewContainerTool(containerTool, containertools.NoneTool), logger) + indexAdder := indexer.NewIndexAdder(containertools.NewContainerTool(containerTool, containertools.NoneTool), containertools.NewContainerTool(containerTool, containertools.NoneTool), logger) request := indexer.AddToIndexRequest{ Generate: false,