[WIP] Replace ImageStreamMapping by ImageStreamImport #16475

dmage · 2017-09-21T14:24:49Z

We can decouple the integrated registry and OpenShift code if we change the process of uploading images. Instead of creating Image objects and pushing them to OpenShift, the integrated registry can ask OpenShift to import just uploaded images.

At the code level, it means using ImageStreamImport instead of ImageStreamMapping.

cc @miminar @legionus @bparees

Signed-off-by: Oleg Bulatov <obulatov@redhat.com>

openshift-merge-robot · 2017-09-21T14:24:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dmage
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/dockerregistry/OWNERS~~ [dmage]
pkg/image/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

smarterclayton · 2017-09-21T14:28:46Z

pkg/dockerregistry/server/tagservice.go

+					To:   &kapi.LocalObjectReference{Name: tag},
+
+					ImportPolicy: imageapiv1.TagImportPolicy{
+						Insecure: true,


@smarterclayton At the moment the registry does not know the correct value of this flag.

The registry knows if it's secure or not (for managed images). And source image streams have TagImportPolicy as well - it can be inherited for external images. It's not bulletproof though.

How does the registry know if it's secure or not? I'm pretty sure it doesn't.

How does the registry know if it's secure or not? I'm pretty sure it doesn't.

You're right. I took into consideration just passthrough tls termination where the certificate paths are passed to registry via environment variables. For edge tls termination, the registry cannot be sure for sure.

But that's just another reason for me for not replacing imagestreammapping with imagestreamimport.

smarterclayton · 2017-09-21T14:31:11Z

pkg/dockerregistry/server/tagservice.go

-		context.GetLogger(ctx).Errorf("error retrieving ImageStream %s/%s: %v", t.repo.namespace, t.repo.name, err)
-		return distribution.ErrRepositoryUnknown{Name: t.repo.Named().Name()}
-	}
+	dockerImageReference := fmt.Sprintf("%s/%s/%s@%s", t.repo.config.registryAddr, t.repo.namespace, t.repo.name, dgst.Digest.String())


I think this change bypasses the quota check. How would quota be checked?

How will access control to the image work? Does this require the api server to have full credentials to the registry?

What are the performance implications of this change? You're changing:

registry POST ImageStreamMapping to server

server write two objects to etcd

to

registry POST ImageStreamImport to server

server GET manifest from registry

registry GET ImageStream to prove client has access

server write two objects to etcd

Which seems like you're doing to double latency and number of API calls

I think this change bypasses the quota check. How would quota be checked?

The imagestreamimport checks the quota [1]. Also now we have a broken quota check at the time of blobs uploading.

[1]https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestreamimport/rest.go#L344

you're doing to double latency and number of API calls

My change adds one read from the registry (which will induce a get API call) to the write call. But in the whole it won't double number of API calls: manifest uploading is just a part of the image uploading process, and it has other steps too (layers verification, authentication).

Also, in the old code, ImageStreamMapping is created twice (from manifest service and from tag service). If it didn't not exist yet, then it would be created for the 3rd time. In the new code, only 1 API call will be preformed.

The new code gives the registry authority for untagged manifests, and solves #9550. It allows us to upload manifest lists to the integrated registry. The integrated registry becomes just a regular docker registry, and to support new types of manifests changes will be required only on API side (which seems more elegant).

This PR removes from the integrated registry responsibility for filling DockerImageMetadata. It makes the integrated registry a step closer to not using internal pkg/image/apis/image.

And in the end, the best code is no code. We can eventually remove ImageStreamMapping from master API without loosing functionality.

But if you don't like it, you can close the PR, though.

openshift-ci-robot · 2017-09-21T16:03:36Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/integration	`1096093`	link	`/test integration`
ci/openshift-jenkins/cmd	`1096093`	link	`/test cmd`
ci/openshift-jenkins/unit	`1096093`	link	`/test unit`
ci/openshift-jenkins/end_to_end	`1096093`	link	`/test end_to_end`
ci/openshift-jenkins/extended_conformance_install_update	`1096093`	link	`/test extended_conformance_install_update`

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.

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.

miminar · 2017-09-26T10:26:19Z

pkg/image/importer/credentials.go

@@ -169,7 +169,7 @@ func (s *SecretCredentialStore) init() credentialprovider.DockerKeyring {
 func basicCredentialsFromKeyring(keyring credentialprovider.DockerKeyring, target *url.URL) (string, string) {
 	// TODO: compare this logic to Docker authConfig in v2 configuration
 	var value string
-	if len(target.Scheme) == 0 || target.Scheme == "https" {
+	if len(target.Scheme) == 0 || target.Scheme == "https" || target.Scheme == "http" {


Quoting @smarterclayton from (see original comment):

We don't want to send https credentials to http endpoints. You have to explicitly say you want to send credentials to an insecure endpoint.

If this is going to change, the comment below needs to be modified or removed.

credentialprovider.DockerKeyring doesn't know anything about schemas. We are using it wrong, it works with image names, not with URLs.

miminar · 2017-09-26T10:28:26Z

pkg/dockerregistry/server/tagservice.go

-	}
-
-	ism := imageapiv1.ImageStreamMapping{
+	isi := imageapiv1.ImageStreamImport{


How will the image stream import work if the image isn't tagged in any image stream yet?

The image is imported by the digest.

The imagestreamimport requires a source repository name. What if there is no corresponding image stream yet?

miminar · 2017-09-26T10:30:44Z

pkg/dockerregistry/server/tagservice.go

+					To:   &kapi.LocalObjectReference{Name: tag},
+
+					ImportPolicy: imageapiv1.TagImportPolicy{
+						Insecure: true,


The registry knows if it's secure or not (for managed images). And source image streams have TagImportPolicy as well - it can be inherited for external images. It's not bulletproof though.

miminar · 2017-09-26T10:46:22Z

pkg/dockerregistry/server/manifestservice.go


 	manifest, err := m.manifests.Get(withRepository(ctx, m.repo), dgst, options...)
 	switch err.(type) {
 	case distribution.ErrManifestUnknownRevision:
 		break
 	case nil:
-		m.repo.rememberLayersOfManifest(dgst, manifest, ref.Exact())
-		m.migrateManifest(withRepository(ctx, m.repo), image, dgst, manifest, true)


You may still want to remove manifest and config from the image object.

miminar · 2017-09-26T10:49:07Z

pkg/dockerregistry/server/manifestservice.go

@@ -47,38 +40,29 @@ func (m *manifestService) Exists(ctx context.Context, dgst digest.Digest) (bool,
 func (m *manifestService) Get(ctx context.Context, dgst digest.Digest, options ...distribution.ManifestServiceOption) (distribution.Manifest, error) {
 	context.GetLogger(ctx).Debugf("(*manifestService).Get")

-	image, _, _, err := m.repo.getStoredImageOfImageStream(dgst)


This moves the status of "source of truth" from etcd to registry's storage. Is that intentional?

Yes, it fixes #9550.

IMHO etcd should stay as a source of truth. Deleting an image using oc delete should prevent registry from serving it, which applies to deleting imagestreamtag or whole imagestream. Tagging the image into new image stream using oc tag should allow registry to serve it from the corresponding repository.

Moving the source of truth from etcd to storage implies a need for one-way synchronization of storage state to etcd. We don't have that in place yet.

OK, you are right, this code has a bug. Image streams should be the source of truth for tagged manifests. Though, image streams don't contain information about untagged manifests and can't be used as a source of truth for them.

Though, image streams don't contain information about untagged manifests and can't be used as a source of truth for them.

Well, we don't keep the history of image presence in image streams (something like untagged images). And I don't think it's desirable since image stream is already too fat.

The rejection to serve a manifest can be determined from the absence of image reference in the image stream.

I don't want to get a rejection, I want to pull it back even if I don't tag it.

openshift-merge-robot · 2017-09-28T00:27:15Z

@dmage PR needs rebase

openshift-bot · 2018-02-21T05:10:37Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2018-03-23T05:19:36Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2018-04-22T05:27:44Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Replace ImageStreamMapping by ImageStreamImport

1096093

Signed-off-by: Oleg Bulatov <obulatov@redhat.com>

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 Sep 21, 2017

openshift-merge-robot assigned soltysh and smarterclayton Sep 21, 2017

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2017

smarterclayton reviewed Sep 21, 2017

View reviewed changes

miminar suggested changes Sep 26, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2017

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 21, 2018

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 23, 2018

openshift-ci-robot closed this Apr 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Replace ImageStreamMapping by ImageStreamImport #16475

[WIP] Replace ImageStreamMapping by ImageStreamImport #16475

dmage commented Sep 21, 2017

openshift-merge-robot commented Sep 21, 2017

smarterclayton Sep 21, 2017

legionus Sep 21, 2017

miminar Sep 26, 2017

dmage Sep 27, 2017

miminar Sep 27, 2017

miminar Sep 27, 2017

smarterclayton Sep 21, 2017

smarterclayton Sep 21, 2017 •

edited

Loading

legionus Sep 21, 2017

dmage Sep 22, 2017

openshift-ci-robot commented Sep 21, 2017

miminar Sep 26, 2017

dmage Sep 26, 2017

miminar Sep 26, 2017

dmage Sep 27, 2017

miminar Sep 27, 2017

miminar Sep 26, 2017

miminar Sep 26, 2017

miminar Sep 26, 2017

dmage Sep 26, 2017

miminar Sep 27, 2017

dmage Sep 27, 2017

miminar Sep 27, 2017

dmage Sep 27, 2017

openshift-merge-robot commented Sep 28, 2017

openshift-bot commented Feb 21, 2018

openshift-bot commented Mar 23, 2018

openshift-bot commented Apr 22, 2018

[WIP] Replace ImageStreamMapping by ImageStreamImport #16475

[WIP] Replace ImageStreamMapping by ImageStreamImport #16475

Conversation

dmage commented Sep 21, 2017

openshift-merge-robot commented Sep 21, 2017

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 Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 21, 2017

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

Choose a reason for hiding this comment

openshift-merge-robot commented Sep 28, 2017

openshift-bot commented Feb 21, 2018

openshift-bot commented Mar 23, 2018

openshift-bot commented Apr 22, 2018

smarterclayton Sep 21, 2017 •

edited

Loading