Parallelize image mirroring and reuse mounted layers #19017

smarterclayton · 2018-03-19T02:12:54Z

Linear execution of large mirrors doesn't make sense. Create an execution plan that can be run in parallel and also supports dry run.

$ cat /tmp/file2
docker.io/openshift/origin:v3.9 registry.svc.ci.openshift.org/ci/test1:v3.9
docker.io/openshift/origin:v3.9 registry.svc.ci.openshift.org/ci/test2:v3.9
docker.io/openshift/origin:v3.10 registry.svc.ci.openshift.org/ci/test1:v3.10
docker.io/openshift/origin:v3.10 registry.svc.ci.openshift.org/ci/test2:v3.10
$ oc image mirror -f /tmp/file2
registry.svc.ci.openshift.org/
  ci/test1
    blobs:
      docker.io/openshift/origin sha256:514a97903be7e078bd811bf37fd5716ee8f24fe10a5ba7df4328d13c4cd112bb 262B
      docker.io/openshift/origin sha256:910e88f98b9f323081f8650f2f18f7173cb1dd3cc0d6e720968fcf72d14c89d8 262B
      docker.io/openshift/origin sha256:ce242a29d50867ad0bde94539b48b6f23c9c2757eaf7e6b7841498ae0019b7cb 2.639KiB
      docker.io/openshift/origin sha256:303a374f1b63bb18cb81008d151128fd2bea8bdd22a31ec0b7d328d6179cb48b 2.64KiB
      docker.io/openshift/origin sha256:963bdd7dfde6f82b11cb6e9216ab77b3ee874fb28008f38416bd1fb2b313ae6e 61.42MiB
      docker.io/openshift/origin sha256:92f5eb04e64965f468424081a7fc8d5b68b825d83246c09eb9bed12bd726f782 61.42MiB
      docker.io/openshift/origin sha256:5e35d10a3ebadf9d6ab606ce72e1e77f8646b2e2ff8dd3a60d4401c3e3a76f31 69.6MiB
      docker.io/openshift/origin sha256:1f6ecdfa27d59eeffd78055c9f2846444f1d24a5422789c20aa70adaf1deec4c 139.2MiB
      docker.io/openshift/origin sha256:fc90d30a5389d9f5e851664952d5aee2a828b3ffe6211e2bb16b21f9c15b6a93 182.4MiB
    manifests:
      sha256:004e7ef81f386f648f023c5838ef38562d07f5d25cc3dab87008f33c49a99412 -> v3.9
      sha256:0a0ba0705f0d7be46746619b705a138c71f74f6a3a6924d69d1517a504d85c6c -> v3.10
  ci/test2
    blobs:
      docker.io/openshift/origin sha256:514a97903be7e078bd811bf37fd5716ee8f24fe10a5ba7df4328d13c4cd112bb 262B
      docker.io/openshift/origin sha256:910e88f98b9f323081f8650f2f18f7173cb1dd3cc0d6e720968fcf72d14c89d8 262B
      docker.io/openshift/origin sha256:ce242a29d50867ad0bde94539b48b6f23c9c2757eaf7e6b7841498ae0019b7cb 2.639KiB
      docker.io/openshift/origin sha256:303a374f1b63bb18cb81008d151128fd2bea8bdd22a31ec0b7d328d6179cb48b 2.64KiB
      docker.io/openshift/origin sha256:963bdd7dfde6f82b11cb6e9216ab77b3ee874fb28008f38416bd1fb2b313ae6e 61.42MiB
      docker.io/openshift/origin sha256:92f5eb04e64965f468424081a7fc8d5b68b825d83246c09eb9bed12bd726f782 61.42MiB
      docker.io/openshift/origin sha256:5e35d10a3ebadf9d6ab606ce72e1e77f8646b2e2ff8dd3a60d4401c3e3a76f31 69.6MiB
      docker.io/openshift/origin sha256:1f6ecdfa27d59eeffd78055c9f2846444f1d24a5422789c20aa70adaf1deec4c 139.2MiB
      docker.io/openshift/origin sha256:fc90d30a5389d9f5e851664952d5aee2a828b3ffe6211e2bb16b21f9c15b6a93 182.4MiB
    manifests:
      sha256:004e7ef81f386f648f023c5838ef38562d07f5d25cc3dab87008f33c49a99412 -> v3.9
      sha256:0a0ba0705f0d7be46746619b705a138c71f74f6a3a6924d69d1517a504d85c6c -> v3.10
  stats: shared=9 unique=0 size=514.1MiB ratio=0.00

phase 0:
  registry.svc.ci.openshift.org ci/test1
    blobs=9 mounts=0 manifests=2 shared=9
phase 1:
  registry.svc.ci.openshift.org ci/test2
    blobs=9 mounts=9 manifests=2 shared=9

info: Planning completed in 1.78s
sha256:0a0ba0705f0d7be46746619b705a138c71f74f6a3a6924d69d1517a504d85c6c registry.svc.ci.openshift.org/ci/test1:v3.10
sha256:004e7ef81f386f648f023c5838ef38562d07f5d25cc3dab87008f33c49a99412 registry.svc.ci.openshift.org/ci/test1:v3.9
sha256:004e7ef81f386f648f023c5838ef38562d07f5d25cc3dab87008f33c49a99412 registry.svc.ci.openshift.org/ci/test2:v3.9
sha256:0a0ba0705f0d7be46746619b705a138c71f74f6a3a6924d69d1517a504d85c6c registry.svc.ci.openshift.org/ci/test2:v3.10
info: Mirroring completed in 3.01s

smarterclayton · 2018-03-22T03:44:40Z

@bparees pretty raw but getting close. I'm working through some quay bugs / inconsistencies right now.

soltysh · 2018-03-22T09:29:39Z

pkg/oc/cli/cmd/registry/info/info.go

+		Example: fmt.Sprintf(loginExample, name+" login"),
+		Run: func(c *cobra.Command, args []string) {
+			o.Out = out
+			o.ErrOut = errOut


Move these up:

o := &Options{Out: out, ErrOut: errOut}

soltysh · 2018-03-22T09:32:03Z

pkg/oc/cli/cmd/registry/info/info.go

+	if err != nil {
+		return err
+	}
+	o.Namespaces = []string{ns, "openshift"}


Shouldn't that be configurable with openshift being the default?

soltysh · 2018-03-22T09:32:56Z

pkg/oc/cli/cmd/registry/info/info.go

+			o.Out = out
+			o.ErrOut = errOut
+			kcmdutil.CheckErr(o.Complete(f, args))
+			kcmdutil.CheckErr(o.Run())


You're missing

kcmdutil.CheckErr(o.Validate())

soltysh · 2018-03-22T09:34:02Z

pkg/oc/cli/cmd/registry/info/info.go

+	if o.Check {
+		ctx := apirequest.NewContext()
+		if !public && !o.ShowInternal {
+			fmt.Fprintf(o.ErrOut, "info: Registry does not have a public hostname registered\n")


s/info/warning

It's not a warning, you can run this inside a cluster successfully.

soltysh · 2018-03-22T09:37:45Z

pkg/oc/cli/cmd/registry/login/login.go

+`)
+)
+
+type Credentials struct {


Wouldn't it be better to use k8s.io/kubernetes/pkg/credentialprovider.DockerConfigJson instead? Although it contains only reading bits for dockerfile it's still relevant, imho.

This is about holding credentials from interior code to output, but not actually serializing the data. We direct modify JSON to avoid dropping fields.

soltysh · 2018-03-22T09:37:58Z

pkg/oc/cli/cmd/registry/login/login.go

+		Example: fmt.Sprintf(loginExample, name+" login"),
+		Run: func(c *cobra.Command, args []string) {
+			o.Out = out
+			o.ErrOut = errOut


Same as before.

soltysh · 2018-03-22T09:38:12Z

pkg/oc/cli/cmd/registry/login/login.go

+		Run: func(c *cobra.Command, args []string) {
+			o.Out = out
+			o.ErrOut = errOut
+			kcmdutil.CheckErr(o.Complete(f, args))


Again missing Validate call.

soltysh · 2018-03-22T09:39:30Z

test/cmd/image.sh

+
+os::test::junit::declare_suite_start "cmd/registry/info"
+os::cmd::expect_success "oc registry info"
+os::cmd::expect_failure_and_text "oc registry info --internal --public" "only one of --internal or --public"


I'd expect a bit more tests, to be honest ;)

These are all in a different PR, but registry info is difficult to test with our existing framework because it requires master reconfig

Comment on #18930

Allows tokens that cover the scope to be reused

smarterclayton · 2018-03-22T22:42:51Z

/test cross

smarterclayton · 2018-03-23T01:24:05Z

/retest
/test cross

Move to #18930

soltysh

Left you some comments.

soltysh · 2018-03-23T08:35:50Z

pkg/oc/cli/cmd/image/mirror/mirror.go

-	flag.BoolVar(&o.Force, "force", o.Force, "If true, attempt to write all contents.")
+	flag.BoolVar(&o.Force, "force", o.Force, "Attempt to write all layers and manifests even if they exist in the remote repository.")
+	flag.IntVar(&o.MaxRegistry, "max-registry", 4, "Number of concurrent registries to connect to at any one time.")
+	flag.IntVar(&o.MaxPerRegistry, "max-per-registry", 6, "Number of concurrent requests allowed per registry.")


req-per-registry? The current naming is confusing with max-registry imo.

soltysh · 2018-03-23T08:57:19Z

pkg/oc/cli/cmd/image/mirror/mirror.go

-}
-
-func (o *pushOptions) Repository(ctx apirequest.Context, context *registryclient.Context, creds auth.CredentialStore, t DestinationType, ref imageapi.DockerImageReference) (distribution.Repository, error) {
+func (o *pushOptions) Repository(ctx apirequest.Context, context *registryclient.Context, t DestinationType, ref imageapi.DockerImageReference) (distribution.Repository, error) {


This looks like a helper function can you make it private?

It's on a private type, which means it's not visible outside the package.

soltysh · 2018-03-23T08:58:57Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+	tabw := tabwriter.NewWriter(o.ErrOut, 0, 0, 1, ' ', 0)
+	for i := range work.phases {
+		phase := &work.phases[i]
+		fmt.Fprintf(o.ErrOut, "phase %d:\n", i)


Debug? If so please use glog.

No, deliberate output for a human that is exceptional (not the key output)

soltysh · 2018-03-23T08:59:29Z

pkg/oc/cli/cmd/image/mirror/mirror.go

+		tabw.Flush()
+	}
+	fmt.Fprintln(o.ErrOut)
+	fmt.Fprintf(o.ErrOut, "info: Planning completed in %s\n", time.Now().Sub(start).Round(10*time.Millisecond))


Why you're using ErrOut for majority of the outputs?

Because these are informational, not structured output intended for the tool.

To offer the best possible bulk upload story, we should look for mount opportunities when we evaluate our mirror targets and send them in order. Also use paralellization and add dry run support.

soltysh

/lgtm

openshift-ci-robot · 2018-03-30T13:47:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 19, 2018

openshift-ci-robot requested review from juanvallejo and soltysh March 19, 2018 02:13

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 19, 2018

smarterclayton force-pushed the plan branch from 90c61df to b91c635 Compare March 19, 2018 20:13

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2018

smarterclayton force-pushed the plan branch from b91c635 to fc3ff42 Compare March 20, 2018 04:48

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Mar 20, 2018

smarterclayton force-pushed the plan branch 3 times, most recently from b9fbba2 to 1a15970 Compare March 22, 2018 03:38

smarterclayton changed the title ~~WIP - Create an execution plan for mirroring in parallel~~ Create an execution plan for mirroring in parallel Mar 22, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2018

soltysh previously requested changes Mar 22, 2018

View reviewed changes

smarterclayton force-pushed the plan branch from 1a15970 to 5f1d680 Compare March 22, 2018 17:14

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2018

smarterclayton force-pushed the plan branch from 5f1d680 to bb8362e Compare March 22, 2018 20:28

UPSTREAM: docker/distribution: 2382: Don't double add scopes

e1c7ac6

Allows tokens that cover the scope to be reused

smarterclayton force-pushed the plan branch from bb8362e to 6c34182 Compare March 22, 2018 21:12

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2018

smarterclayton force-pushed the plan branch from 6c34182 to 974a76c Compare March 22, 2018 22:39

smarterclayton changed the title ~~Create an execution plan for mirroring in parallel~~ Parallelize image mirroring and reuse mounted layers Mar 22, 2018

soltysh reviewed Mar 23, 2018

View reviewed changes

Mirroring should take advantage of mount opportunities for binaries

932a57d

To offer the best possible bulk upload story, we should look for mount opportunities when we evaluate our mirror targets and send them in order. Also use paralellization and add dry run support.

smarterclayton force-pushed the plan branch from 974a76c to 932a57d Compare March 23, 2018 18:22

openshift-ci-robot assigned soltysh Mar 30, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2018

soltysh approved these changes Mar 30, 2018

View reviewed changes

openshift-merge-robot merged commit b68ddd7 into openshift:master Mar 30, 2018

liggitt mentioned this pull request Mar 31, 2018

update docker/distribution sha #19167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallelize image mirroring and reuse mounted layers #19017

Parallelize image mirroring and reuse mounted layers #19017

smarterclayton commented Mar 19, 2018 •

edited

Loading

smarterclayton commented Mar 22, 2018

soltysh Mar 22, 2018

soltysh Mar 22, 2018

soltysh Mar 22, 2018

soltysh Mar 22, 2018

smarterclayton Mar 23, 2018

soltysh Mar 22, 2018

smarterclayton Mar 23, 2018

soltysh Mar 22, 2018

soltysh Mar 22, 2018

soltysh Mar 22, 2018

smarterclayton Mar 23, 2018

smarterclayton Mar 23, 2018

smarterclayton commented Mar 22, 2018

smarterclayton commented Mar 23, 2018

soltysh left a comment

soltysh Mar 23, 2018

soltysh Mar 23, 2018

smarterclayton Mar 26, 2018

soltysh Mar 23, 2018

smarterclayton Mar 26, 2018

soltysh Mar 23, 2018

smarterclayton Mar 26, 2018

soltysh left a comment

openshift-ci-robot commented Mar 30, 2018

Parallelize image mirroring and reuse mounted layers #19017

Parallelize image mirroring and reuse mounted layers #19017

Conversation

smarterclayton commented Mar 19, 2018 • edited Loading

smarterclayton commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 22, 2018

smarterclayton commented Mar 23, 2018

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 30, 2018

smarterclayton commented Mar 19, 2018 •

edited

Loading