Add `oc image append` which adds layers to a schema1/2 image by smarterclayton · Pull Request #20027 · openshift/origin

smarterclayton · 2018-06-18T05:18:58Z

This command can take zero or more gzipped layer tars (in Docker layer
format) and append them to an existing image or a scratch image and then
push the new image to a registry. Layers in the existing image are pushed
as well. The caller can mutate the provided config as it goes.

Pushing will fall back to schema1 if schema2 is not available, but it will always prefer that form.

Command is experimental for 3.11. Will be considered as part of CI use cases for creating new images, potentially building release layers, and completing content.

smarterclayton · 2018-06-18T05:31:06Z

@bparees this is most of the logic from the previous "add layer via image API", but modeled as a CLI command, sharing code with mirror, and much simpler (since the surface area is just taking docker images from a registry and slicing them). It does not support v1 push or read. It handles mounting and reuse of layers, lets you almost completely rewrite the image metadata, downconversion and up conversion from schema1/2, and add arbitrary layers.

I would imagine having this as a utility command, and then refactoring the core and reusing with a builder step that would interact with container-snapshot. But this is useful on its own.

smarterclayton · 2018-06-25T21:02:58Z

/test all

smarterclayton · 2018-07-03T00:37:53Z

/retest

openshift-ci-robot · 2018-07-03T01:59:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [smarterclayton]
~~docs/man/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~pkg/image/OWNERS~~ [smarterclayton]
~~pkg/oc/OWNERS~~ [smarterclayton]
~~test/extended/OWNERS~~ [smarterclayton]

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

smarterclayton · 2018-07-05T16:51:52Z

/retest

smarterclayton · 2018-07-05T17:44:31Z

/retest

smarterclayton · 2018-07-06T23:27:14Z

/retest

bparees · 2018-07-09T21:18:10Z

pkg/image/apis/image/docker10/conversion.go

+		Author:          in.Author,
+		Architecture:    in.Architecture,
+		Size:            in.Size,
+		OS:              "linux",


i feel like this is going to bite us as soon as we try to do multiarch... do we not have the image's architecture info anywhere? should this at least be based on runtime.GOARCH like we do on manifestlist conversion?

If you don't have this data, the only thing it supports was linux (docker schema changed to allow this, but older images had no metadata).

bparees · 2018-07-09T21:37:24Z

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

+
+		Layers may be provided as arguments to the command and must each be a gzipped tar archive
+		representing a filesystem overlay to the inherited images. The archive may contain a "whiteout"
+		file (the prefix '.wh.' and the filename) which will hide files in the lower layers. All


is this your convention or docker's? is there an escape mechanism if i really want to prefix a file with .wh?

This is docker's (and I think was also one of the kernel filesystem's conventions).

bparees · 2018-07-09T21:39:08Z

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

+
+	example = templates.Examples(`
+# Remove the entrypoint on the mysql:latest image
+%[1]s --from mysql:latest --to myregistry.com/myimage:latest --image {"Entrypoint":null}


does this get merged w/ the other metadata fields? or the whole config becomes empty except for Entrypoint?

nm looks like it's a patch per the help below.

bparees · 2018-07-09T21:41:35Z

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

+	}
+
+	flag := cmd.Flags()
+	flag.BoolVar(&o.DryRun, "dry-run", o.DryRun, "Print the actions that would be taken and exit without writing to the destinations.")


should be destination (singular)? Doesn't look like you're allowing multiple destinations?

Yeah, copy pasta

bparees · 2018-07-09T21:44:11Z

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

+}
+
+// schema2ManifestOnly specifically requests a manifest list first
+var schema2ManifestOnly = distribution.WithManifestMediaTypes([]string{


schema2ManifestListOnly? schema2Only?

bparees · 2018-07-09T21:53:15Z

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

+)
+
+// TDOO: remove when quay.io switches to v2 schema
+func loadPrivateKey() (libtrust.PrivateKey, error) {


this can't be shared w/ the implementation in append?

It probably can. I didn't want it to be too generic at this point.

bparees · 2018-07-09T21:55:50Z

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

+	"github.com/golang/glog"
+)
+
+type workQueue struct {


can't share w/ the the append workqueue?

I plan to refactor soon - it's a little too specialized, so I was concerned about generalizing too early.

bparees · 2018-07-09T21:59:49Z

test/extended/images/append.go

+	// best effort to get the format string for the release
+	router, err := cli.AdminAppsClient().Apps().DeploymentConfigs("default").Get("router", metav1.GetOptions{})
+	if err != nil {
+		g.Skip(fmt.Sprintf("Unable to find router in order to query format string: %v", err))


this seems fragile and likely to lead to this test being skipped w/o anyone noticing.

We do this in other tests, I agree it's fragile, but we also need the e2e tests to work against a wider variety of servers. I could fail it, I guess.

bparees · 2018-07-09T22:01:22Z

test/extended/images/append.go

+					VolumeMounts: []kapiv1.VolumeMount{
+						{
+							Name:      "pull-secret",
+							MountPath: "/secret/.dockercfg",


.docker/config.json? (just to avoid confusion) or is our SA secret really still in .dockercfg format?

Still in that format.

smarterclayton · 2018-07-11T14:53:22Z

/retest

smarterclayton · 2018-07-11T17:18:55Z

/retest

smarterclayton · 2018-07-11T18:51:39Z

/retest

bparees

minor questions, depending on the answers, lgtm.

bparees · 2018-07-11T19:11:57Z

pkg/image/dockerlayer/add/add.go

+	return manifest, nil
+}
+
+/*


how dead is this commented out code? should it be removed or TODOed?

Totally, dead, accident. Will remove and repush

bparees · 2018-07-11T19:13:51Z

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

+	flag.StringVar(&o.FilterByOS, "filter-by-os", o.FilterByOS, "A regular expression to control which images are mirrored. Images will be passed as '<platform>/<architecture>[/<variant>]'.")
+
+	flag.StringVar(&o.From, "from", o.From, "The image to use as a base. If empty, a new scratch image is created.")
+	flag.StringVar(&o.To, "to", o.To, "The Docker repository tag to upload the appended image to.")


thoughts on whether this should support imagestreamtags as a "to" target in the future? (either using the --to flag, or introducing an new flag like --to-imagestreamtag ?)

(also --from for that matter)

So for both mirror and append I was thinking no, because all clusters should be exposing a public registry URL via the router, and we have oc registry login. So you should be able to easily get the external name and then use it.

Also, you need registry api access anyway to use these commands, so not a burden

Regressed when we started using our own logic.

Unify location of some base layer constants

These files will be copied for now and then refactored later into reusable packages.

system:serviceaccount:blah:blah is incorrectly encoded into base64 for authorization, causing the successive login to fail. Instead, encode it as system-serviceaccount-namespace-name which is ignored by the registry. Also add an insecure flag to skip-check that will bypass validating TLS certs during the skip-check.

These are moves of existing code in the same set, or new, simple dependency packages that bring in no major subtrees.

This command can take zero or more gzipped layer tars (in Docker layer format) and append them to an existing image or a scratch image and then push the new image to a registry. Layers in the existing image are pushed as well. The caller can mutate the provided config as it goes.

openshift-bot · 2018-07-12T02:33:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2018-07-12T04:04:32Z

@smarterclayton: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/verify	`b2be113`	link	`/test verify`
ci/openshift-jenkins/gcp	`b2be113`	link	`/test gcp`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

smarterclayton · 2018-07-12T14:23:17Z

/retest

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

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 18, 2018

smarterclayton changed the title ~~Add oc image append which adds layers to a schema1/2 image~~ WIP: Add oc image append which adds layers to a schema1/2 image Jun 18, 2018

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

openshift-ci-robot requested review from enj and miminar June 18, 2018 05:19

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2018

smarterclayton force-pushed the layer branch from 13322c7 to aa7a362 Compare June 18, 2018 05:28

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

smarterclayton force-pushed the layer branch from aa7a362 to b2be113 Compare June 18, 2018 17:32

smarterclayton force-pushed the layer branch 2 times, most recently from 1293ecd to f22d059 Compare June 28, 2018 19:52

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

smarterclayton force-pushed the layer branch from f22d059 to 768a4f8 Compare June 28, 2018 20:03

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 28, 2018

smarterclayton force-pushed the layer branch from 768a4f8 to 4ffde92 Compare July 2, 2018 02:47

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

smarterclayton force-pushed the layer branch from 4ffde92 to 322f59d Compare July 2, 2018 03:11

smarterclayton force-pushed the layer branch from 322f59d to 0753a94 Compare July 3, 2018 01:59

smarterclayton changed the title ~~WIP: Add oc image append which adds layers to a schema1/2 image~~ Add oc image append which adds layers to a schema1/2 image Jul 3, 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 Jul 3, 2018

smarterclayton force-pushed the layer branch from 0753a94 to 20ded5e Compare July 5, 2018 14:53

smarterclayton force-pushed the layer branch from 20ded5e to 4a454b9 Compare July 5, 2018 18:41

smarterclayton force-pushed the layer branch 4 times, most recently from a8014a7 to 5d58c39 Compare July 6, 2018 17:57

smarterclayton force-pushed the layer branch 3 times, most recently from 11a0b76 to c319b26 Compare July 9, 2018 19:05

bparees requested changes Jul 9, 2018

View reviewed changes

bparees self-assigned this Jul 9, 2018

smarterclayton force-pushed the layer branch 3 times, most recently from ec8f573 to 6bb87b3 Compare July 11, 2018 04:55

bparees reviewed Jul 11, 2018

View reviewed changes

smarterclayton added 7 commits July 11, 2018 17:13

Allow the -delete-namespace flag to work on origin tests

3dd13d9

Regressed when we started using our own logic.

Track OS information for images, prune status tags

fb2ae7f

Unify location of some base layer constants

Split fundamental docker types

54636b8

Split private mirror functions for reuse

da471b8

These files will be copied for now and then refactored later into reusable packages.

Update import-restrictions.json with new packages

1346deb

These are moves of existing code in the same set, or new, simple dependency packages that bring in no major subtrees.

smarterclayton force-pushed the layer branch from 6bb87b3 to 2721b3b Compare July 11, 2018 21:13

smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

openshift-merge-robot merged commit fec789b into openshift:master Jul 13, 2018

Conversation

smarterclayton commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Jun 25, 2018

Uh oh!

smarterclayton commented Jul 3, 2018

Uh oh!

openshift-ci-robot commented Jul 3, 2018

Uh oh!

smarterclayton commented Jul 5, 2018

Uh oh!

smarterclayton commented Jul 5, 2018

Uh oh!

smarterclayton commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Jul 11, 2018

Uh oh!

smarterclayton commented Jul 11, 2018

Uh oh!

smarterclayton commented Jul 11, 2018

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Jul 12, 2018

Uh oh!

openshift-ci-robot commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Jul 12, 2018

smarterclayton commented Jun 18, 2018 •

edited

Loading

smarterclayton commented Jun 18, 2018 •

edited

Loading

openshift-ci-robot commented Jul 12, 2018 •

edited

Loading