-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Sync images from one image stream to another registry #14471
Sync images from one image stream to another registry #14471
Conversation
0bd3ca6
to
48a1d5b
Compare
pkg/image/api/types.go
Outdated
@@ -306,6 +307,8 @@ type TagEventList struct { | |||
Items []TagEvent | |||
// Conditions is an array of conditions that apply to the tag event list. | |||
Conditions []TagEventCondition | |||
// PendingCopy is set if there is a copy waiting for a layer upload on this image stream tag. | |||
PendingCopy *ImageStreamTagCopy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't be this condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For two step copy (definition then binary layer upload) we need a place to store the pending image metadata. Since the API is tag focused, it's ok to not allow lots of pending images. I don't want to put too much metadata in condition
pkg/image/api/types.go
Outdated
// Standard object's metadata. | ||
metav1.ObjectMeta | ||
|
||
// from is an optional reference to an existing image stream tag or image to copy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
godoc: uppercase (also the other)
pkg/image/api/types.go
Outdated
From *kapi.ObjectReference | ||
// image is metadata that will replace the existing metadata of from, or if from | ||
// is empty, will be used to create a new scratch image. | ||
Image *ImageCopyMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't To
better here? (or at least I would expect from->to ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure - this is more declarative than destination. so if you specify it it's "this is what I want".
@@ -66,6 +66,9 @@ const ( | |||
// TemplateInstance backend for most of the heavy lifting. | |||
InfraTemplateServiceBrokerServiceAccountName = "template-service-broker" | |||
TemplateServiceBrokerControllerRoleName = "system:openshift:template-service-broker" | |||
|
|||
InfraRegistryManagementControllerServiceAccountName = "registry-management-controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will make my life in #14317 harder :-) is it possible to base this on top of the refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't for 3.6, just putting it up while i iterate.
pkg/cmd/server/origin/master.go
Outdated
|
||
// lazyServiceAccountSecretBasicAuthStore reads the service account token from a service account | ||
// and uses it as basic auth credentials. | ||
type lazyServiceAccountSecretBasicAuthStore struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this master.go good place for this to live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the only place it's used right now, I don't think of this as final because it's a really inelegant way for the master to prove auth to the registry. Unfortunately, we have no other way to auth to the registry.
imageStreamRegistry: imageStreamRegistry, | ||
sarRegistry: sarRegistry, | ||
defaultRegistry: defaultRegistry, | ||
gr: schema.GroupResource{Resource: "imagestreamtags", Group: "image.openshift.io"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageapi.Resource("imagestreamtags")
?
@@ -321,6 +339,7 @@ func newISTag(tag string, imageStream *imageapi.ImageStream, image *imageapi.Ima | |||
event := imageapi.LatestTaggedImage(imageStream, tag) | |||
if event == nil || len(event.Image) == 0 { | |||
if !allowEmptyEvent { | |||
glog.V(4).Infof("did not find tag %s in image stream status tags: %#v", tag, imageStream.Status.Tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wish we have some pretty-printer for tags (we print the tags in several places and the Status.Tags is pretty big arr)
images/dockerregistry/config.yml
Outdated
@@ -27,7 +27,7 @@ middleware: | |||
repository: | |||
- name: openshift | |||
options: | |||
acceptschema2: false | |||
acceptschema2: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already done in #13428
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i needed it to test because this only supports pushing as v2. That's temporary.
pkg/cmd/infra/pusher/pusher.go
Outdated
|
||
var ( | ||
longDesc = templates.LongDesc(` | ||
Push an image to a new location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide some examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still draft, but the use case is going to be a controller that runs a pod in your namespace that syncs your images to other repositories. So this is just the first part, still need to add some other bits:
- make it easy to inject pull secrets that can be used
- the controller itself
- figure out how to batch this - I kind of think we want to batch all of the "syncs" in a namespace all at once into one job.
@@ -119,6 +119,9 @@ func (r *repositoryRetriever) Repository(ctx gocontext.Context, registry *url.UR | |||
t = r.context.InsecureTransport | |||
} | |||
src := *registry | |||
if len(src.Scheme) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miminar check
@smarterclayton would be nice to provide some examples of this and extended test. |
pkg/image/importer/credentials.go
Outdated
// try removing the canonical ports | ||
if strings.HasSuffix(target.Host, ":443") || strings.HasSuffix(target.Host, ":80") { | ||
host := strings.SplitN(target.Host, ":", 2)[0] | ||
glog.V(5).Infof("Being asked for %s, trying %s without port", target, host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should :80 port be stripped for https?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A secret that is only served over http doesn't make much sense, but in this case it's checking whether you defined a more generic secret (i.e. you have "foo.com" but not "foo.com:443" - we should try "foo.com").
It's more the declarative "I want this image". From could be "base for
layers" or "base for metadata"
On Jun 5, 2017, at 3:34 AM, Michal Fojtik <notifications@github.com> wrote:
*@mfojtik* commented on this pull request.
------------------------------
In pkg/image/api/types.go
<#14471 (comment)>:
@@ -406,6 +409,40 @@ type ImageStreamImage struct {
Image Image
}
+// ImageStreamTagCopy allows a client to create a copy of an existing
image that changes
+// metadata or adds a new layer. It also allows the client to create
a new image (i.e.
+// FROM scratch). The resulting image is stored as a tag on the stream.
+type ImageStreamTagCopy struct {
+ metav1.TypeMeta
+ // Standard object's metadata.
+ metav1.ObjectMeta
+
+ // from is an optional reference to an existing image stream tag or
image to copy.
+ // If from is not set, this is assumed to create a new scratch image.
+ From *kapi.ObjectReference
+ // image is metadata that will replace the existing metadata of
from, or if from
+ // is empty, will be used to create a new scratch image.
+ Image *ImageCopyMetadata
Isn't To better here? (or at least I would expect from->to ;-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14471 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pxKvALZjSrSa6uiIlx9qNkcyBu0Xks5sA695gaJpZM4Nvjzj>
.
|
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smarterclayton No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smarterclayton No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smarterclayton No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@@ -239,6 +248,9 @@ func (th *tokenHandler) getToken(params map[string]string, additionalScopes ...s | |||
} | |||
var addedScopes bool | |||
for _, scope := range additionalScopes { | |||
if hasScope(scopes, scope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you not upstreaming this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now upstream and merged.
// load the manifest | ||
srcDigest := digest.Digest(srcDigestString) | ||
// var contentDigest digest.Digest / client.ReturnContentDigest(&contentDigest), | ||
srcManifest, err := manifests.Get(ctx, digest.Digest(srcDigest), schema2ManifestOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema1 is served regardless of accept header or WithManifestMediaTypes
option if the desired tag points to it.
glog.V(4).Infof("Manifest exists in %s, no need to copy layers without --force", dst.ref) | ||
} | ||
} | ||
if mustCopyLayers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this go to a function/method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a good candidate for parallelization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually prefer the reusing of moby's blob upload code.
But that doesn't handle s3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely don't want to take any more references to moby code. If the distribution client is insufficient containers/image should take it.
for _, blob := range srcManifest.References() { | ||
// tagging within the same registry is a no-op | ||
if src.ref.Registry == dst.ref.Registry && canonicalFrom.String() == canonicalTo.String() { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be moved before the blobs loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will change and make clearer
} | ||
|
||
// if the destination tag already has this manifest, do nothing | ||
var mustCopyLayers bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be set to true
if source and destination repository are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, mustCopyLayers should only be true when the user requests it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I actually meant the opposite. If the source and destination repository are the same, no copy is necessary.
// upload the each manifest | ||
for _, srcManifest := range srcManifests { | ||
switch srcManifest.(type) { | ||
case *schema2.DeserializedManifest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see a difference between handling schema2 and schema1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forcing function to stop propagation of schema1. It's easy enough to add later.
|
||
var options []distribution.BlobCreateOption | ||
if !o.SkipMount { | ||
options = append(options, client.WithMountFrom(blobSource), WithDescriptor(blob)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MountFrom makes sense only when src and dst registries are the same.
As an optimization, pushed/skipped blobs to the same registry could be remembered and then used for MountFrom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registries must ignore this field when it's not valid, so it doesn't cost us anything to always send it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put there at least a comment that this isn't supposed to work in most cases.
// mount successful | ||
if ebm, ok := err.(distribution.ErrBlobMounted); ok { | ||
glog.V(5).Infof("Blob mounted %#v", blob) | ||
if ebm.From.Digest() != blob.Digest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's worth it. Docker daemon doesn't check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are checking it because in a mirroring case we may have incorrect data, thus an admin may need to run --force.
} | ||
return srcManifests, srcDigest.Algorithm().FromBytes(body), nil | ||
default: | ||
return append(srcManifests, srcManifest), srcDigest, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manifest itself needs to be modified to omit all the references filtered out. Otherwise the manifest Put will fail due to not existing references in the destination repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manifest is modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manifest list is modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was confused by the name srcManifest
which is the input parameter. Having different name for the processed manifest list wouldn't hurt.
|
||
// upload and tag the manifest | ||
for _, tag := range dst.tags { | ||
toDigest, err := toManifests.Put(ctx, srcManifest, distribution.WithTag(tag)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't help. Manifest must not include references that don't exist in the destination repository. Otherwise, verification will fail.
Not on a registry that does dynamic transformation. |
Not sure what you are referring to? |
Ideally we'd have code for schema1 -> 2 that we would use here, then admins could use this as a migration function. Followup |
More comments? |
%[1]s myregistry.com/myimage:latest s3://s3.amazonaws.com/<region>/<bucket>/image | ||
|
||
# Copy image to multiple locations | ||
%[1]s myregistry.com/myimage:latest docker.io/myrepository/myimage:{stable,dev} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎 for the bash syntax. It's not obvious even to regular bash users.
} | ||
|
||
// if the destination tag already has this manifest, do nothing | ||
var mustCopyLayers bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I actually meant the opposite. If the source and destination repository are the same, no copy is necessary.
|
||
var options []distribution.BlobCreateOption | ||
if !o.SkipMount { | ||
options = append(options, client.WithMountFrom(blobSource), WithDescriptor(blob)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put there at least a comment that this isn't supposed to work in most cases.
} | ||
return srcManifests, srcDigest.Algorithm().FromBytes(body), nil | ||
default: | ||
return append(srcManifests, srcManifest), srcDigest, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was confused by the name srcManifest
which is the input parameter. Having different name for the processed manifest list wouldn't hurt.
pkg/oc/cli/cmd/image/mirror/s3.go
Outdated
func (r *s3Repository) Named() reference.Named { | ||
name, err := reference.ParseNamed(r.repoName) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can s3Driver.Repository()
attempt to parse the repoName
and error out to prevent this panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only valid repo names are allowed. I could pass a named reference I suppose.
Didn't know there was such a registry. Do you have a doc at hand?
Nevermind, you've already answered. |
Schema1 can't be upgraded to schema2. Distribution can dynamically transform schema2 into schema1 for backward compatibility: old clients (which support only schema1 manifests) are able to pull images with schema2 manifests. |
What is missing from schema1 that can't be recreated? |
Updated with latest comments, I also fixed issues with s3 and manifest lists (the wrong file was being pushed). |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
pattern = "^" + pattern | ||
} | ||
if !strings.HasSuffix(pattern, "$") { | ||
pattern = pattern + "$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find this intuitive. I'd expect --filter-by-os=/amd64/
to match. I can always anchor it if I want to match whole string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
// if we aren't forcing upload, skip the blob copy | ||
if !o.Force { | ||
_, err := toBlobs.Stat(ctx, blob.Digest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To save a few queries, a set of blobs uploaded to particular repo could be created. I'd expect some shared blobs among different manifests of a single manifest list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think when we parallelize this we'll want to do it.
Allows tokens that cover the scope to be reused
When Docker-Content-Digest is not specified on the response, fall back from HEAD to GET so that we can calculate the digest from the manifest content.
A new command `oc image mirror` uses the Docker registry API to read from one or more images and copy them to remote registries (without locally storing those images). The command can also copy to an S3 bucket, creating the necessary structure to form a read-only registry.
de91b17
to
e04b165
Compare
Applied labels, will be adding extended tests after I cut a beta release |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Automatic merge from submit-queue. |
Deprecating the previous --command-os to get consistency with other image handling. --filter-by-os originally landed in openshift/origin@e04b16527b (cli: Mirror images across registries or to S3, 2017-06-04, openshift/origin#14471). The wrapping in: o.FilterOptions.FilterByOS = fmt.Sprintf("^%s/", o.CommandOperatingSystem) guards against the unlikely case that a given --command-os value is a valid prefix for a longer OS, or matches an arch or varient or some such. Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Deprecating the previous --command-os to get consistency with other image handling. --filter-by-os originally landed in openshift/origin@e04b16527b (cli: Mirror images across registries or to S3, 2017-06-04, openshift/origin#14471). The wrapping in: o.FilterOptions.FilterByOS = fmt.Sprintf("^%s/", o.CommandOperatingSystem) guards against the unlikely case that a given --command-os value is a valid prefix for a longer OS, or matches an arch or varient or some such. Also teach oc to extract from standard locations e949088 (Enable all Linux arches in cli-artifacts, 2019-11-07, openshift#153) to avoid choking on hardlinks [1]: $ oc adm release extract --tools registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: image did not contain usr/share/openshift/mac/oc Teaching extractTarget about architectures avoids conflicts with multiple architectures trying to use the no-longer-specific-enough oc-linux in targetsByName [2]: $ ./oc adm release extract --command=oc --command-os='*' registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-11-20-121416 error: unable to iterate over layer sha256:13caf755813923e69e25ff1a28cc65766d6dcaa12676e5e274db9ec828e55d71 from registry.svc.ci.openshift.org/ocp/4.3-2019-11-20-121416@sha256:6497d5cb7102903baf98bbc0e07144f04827a21dcd800ff61360d25228a2d2ce: unable to find target with mapping name openshift-client-linux-4.3.0-0.ci-2019-11-20-121416.tar.gz Adding --command-arch on top of that allows us to set currentArch to avoid extracting 'oc' for multiple architectures all into the same file (making it unlikely that you get the architecture you want ;). Updated the completions with: $ make build $ hack/update-generated-completions.sh [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642 [2]: openshift#172 (comment)
Creates a command that can be used to bulk promote images from one registry to another, and accepts injected pull secrets.