Skip to content
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.

Sign up for GitHub

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

Take care of manifest config blob #10759

Closed
wants to merge 4 commits into from
Closed

Take care of manifest config blob #10759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2f45fb
Remember image with matching config reference
Sep 1, 2016
ed473e3
Pullthrough blobs using Get() as well
Sep 1, 2016
b0a1cad
New e2e test: fetch manifest schema 2 with old client
Sep 2, 2016
ebc85ed
Image size needs to add a size of manifest config file
Sep 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion hack/util.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,9 @@ function install_registry() {
readonly -f install_registry

function wait_for_registry() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make sure the status has observed the current generation as well:

local generation=$(oc get dc/docker-registry -o jsonpath='{.metadata.generation}')
local onereplicajs='{.status.observedGeneration},{.status.replicas},{.status.updatedReplicas},{.status.availableReplicas}'
wait_for_command "oc get dc/docker-registry -o jsonpath='$onereplicajs' --config='${ADMIN_KUBECONFIG}' | grep '^${generation},1,1,1$'"  $((5*TIME_MIN))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: put a comment that all the registry re-deployments wipe out registry storage.

wait_for_command "oc get endpoints docker-registry --template='{{ len .subsets }}' --config='${ADMIN_KUBECONFIG}' | grep -q '[1-9][0-9]*'" $((5*TIME_MIN))
local generation=$(oc get dc/docker-registry -o jsonpath='{.metadata.generation}')
local onereplicajs='{.status.observedGeneration},{.status.replicas},{.status.updatedReplicas},{.status.availableReplicas}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quote expansions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, thats for the line above this

wait_for_command "oc get dc/docker-registry -o jsonpath='$onereplicajs' --config='${ADMIN_KUBECONFIG}' | grep '^$generation,1,1,1$'" $((5*TIME_MIN))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ${var} braces

local readyjs='{.items[*].status.conditions[?(@.type=="Ready")].status}'
wait_for_command "oc get pod -l deploymentconfig=docker-registry -o jsonpath='$readyjs' --config='${ADMIN_KUBECONFIG}' | grep -qi true" $TIME_MIN
}
Expand Down
10 changes: 7 additions & 3 deletions pkg/cmd/admin/top/images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,18 @@ func (o TopImagesOptions) imagesTop() []Info {

func getStorage(image *imageapi.Image) int64 {
storage := int64(0)
layerSet := sets.NewString()
blobSet := sets.NewString()
for _, layer := range image.DockerImageLayers {
if layerSet.Has(layer.Name) {
if blobSet.Has(layer.Name) {
continue
}
layerSet.Insert(layer.Name)
blobSet.Insert(layer.Name)
storage += layer.LayerSize
}
if len(image.DockerImageConfig) > 0 && !blobSet.Has(image.DockerImageMetadata.ID) {
blobSet.Insert(image.DockerImageMetadata.ID)
storage += int64(len(image.DockerImageConfig))
}
return storage
}

Expand Down
59 changes: 59 additions & 0 deletions pkg/cmd/admin/top/images_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,65 @@ func TestImagesTop(t *testing.T) {
},
},
},
"with metadata and image config": {
images: &imageapi.ImageList{
Items: []imageapi.Image{
{
ObjectMeta: kapi.ObjectMeta{Name: "image1"},
DockerImageLayers: []imageapi.ImageLayer{
{Name: "layer1", LayerSize: int64(512)},
{Name: "layer2", LayerSize: int64(512)},
},
DockerImageManifest: "non empty metadata",
DockerImageConfig: "raw image config",
DockerImageMetadata: imageapi.DockerImage{
ID: "manifestConfigID",
},
},
},
},
streams: &imageapi.ImageStreamList{},
pods: &kapi.PodList{},
expected: []Info{
imageInfo{
Image: "image1",
Metadata: true,
Parents: []string{},
Usage: []string{},
Storage: int64(1024 + len("raw image config")),
},
},
},
"with metadata and image config and some layers duplicated": {
images: &imageapi.ImageList{
Items: []imageapi.Image{
{
ObjectMeta: kapi.ObjectMeta{Name: "image1"},
DockerImageLayers: []imageapi.ImageLayer{
{Name: "layer1", LayerSize: int64(512)},
{Name: "layer2", LayerSize: int64(256)},
{Name: "layer1", LayerSize: int64(512)},
},
DockerImageManifest: "non empty metadata",
DockerImageConfig: "raw image config",
DockerImageMetadata: imageapi.DockerImage{
ID: "layer2",
},
},
},
},
streams: &imageapi.ImageStreamList{},
pods: &kapi.PodList{},
expected: []Info{
imageInfo{
Image: "image1",
Metadata: true,
Parents: []string{},
Usage: []string{},
Storage: int64(512 + 256),
},
},
},
"multiple tags": {
images: &imageapi.ImageList{
Items: []imageapi.Image{
Expand Down
10 changes: 7 additions & 3 deletions pkg/cmd/admin/top/imagestreams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func getImageStreamSize(g graph.Graph, node *imagegraph.ImageStreamNode) (int64,
storage := int64(0)
images := len(imageEdges)
layers := 0
layerSet := sets.NewString()
blobSet := sets.NewString()
for _, e := range imageEdges {
imageNode, ok := e.To().(*imagegraph.ImageNode)
if !ok {
Expand All @@ -154,12 +154,16 @@ func getImageStreamSize(g graph.Graph, node *imagegraph.ImageStreamNode) (int64,
layers += len(image.DockerImageLayers)
// we're counting only unique layers per the entire stream
for _, layer := range image.DockerImageLayers {
if layerSet.Has(layer.Name) {
if blobSet.Has(layer.Name) {
continue
}
layerSet.Insert(layer.Name)
blobSet.Insert(layer.Name)
storage += layer.LayerSize
}
if len(image.DockerImageConfig) > 0 && !blobSet.Has(image.DockerImageMetadata.ID) {
blobSet.Insert(image.DockerImageMetadata.ID)
storage += int64(len(image.DockerImageConfig))
}
}

return storage, images, layers
Expand Down
50 changes: 50 additions & 0 deletions pkg/cmd/admin/top/imagestreams_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,56 @@ func TestImageStreamsTop(t *testing.T) {
},
},
},
"multiple images with manifest config": {
images: &imageapi.ImageList{
Items: []imageapi.Image{
{
ObjectMeta: kapi.ObjectMeta{Name: "image1"},
DockerImageLayers: []imageapi.ImageLayer{{Name: "layer1", LayerSize: int64(1024)}},
DockerImageConfig: "raw image config",
DockerImageMetadata: imageapi.DockerImage{
ID: "manifestConfigID",
},
},
{
ObjectMeta: kapi.ObjectMeta{Name: "image2"},
DockerImageLayers: []imageapi.ImageLayer{
{Name: "layer1", LayerSize: int64(1024)},
{Name: "layer2", LayerSize: int64(128)},
},
DockerImageConfig: "raw image config",
DockerImageMetadata: imageapi.DockerImage{
ID: "manifestConfigID",
},
},
},
},
streams: &imageapi.ImageStreamList{
Items: []imageapi.ImageStream{
{
ObjectMeta: kapi.ObjectMeta{Name: "stream1", Namespace: "ns1"},
Status: imageapi.ImageStreamStatus{
Tags: map[string]imageapi.TagEventList{
"tag1": {
Items: []imageapi.TagEvent{{Image: "image1"}},
},
"tag2": {
Items: []imageapi.TagEvent{{Image: "image2"}},
},
},
},
},
},
},
expected: []Info{
imageStreamInfo{
ImageStream: "ns1/stream1",
Storage: int64(1152 + len("raw image config")),
Images: 2,
Layers: 3,
},
},
},
"multiple unreferenced images": {
images: &imageapi.ImageList{
Items: []imageapi.Image{
Expand Down
7 changes: 7 additions & 0 deletions pkg/dockerregistry/server/blobdescriptorservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,5 +203,12 @@ func imageHasBlob(
}
}

// only manifest V2 schema2 has docker image config filled where dockerImage.Metadata.id is its digest
if len(image.DockerImageConfig) > 0 && image.DockerImageMetadata.ID == blobDigest {
// remember manifest config reference of schema 2 as well
r.rememberLayersOfImage(image, cacheName)
return true
}

return false
}
34 changes: 32 additions & 2 deletions pkg/dockerregistry/server/pullthroughblobstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ func (r *pullthroughBlobStore) Stat(ctx context.Context, dgst digest.Digest) (di
return desc, err
}

return r.remoteStat(ctx, dgst)
}

// remoteStat attempts to find requested blob in candidate remote repositories and if found, it updates
// digestToRepository store. ErrBlobUnknown will be returned if not found.
func (r *pullthroughBlobStore) remoteStat(ctx context.Context, dgst digest.Digest) (distribution.Descriptor, error) {
// look up the potential remote repositories that this blob could be part of (at this time,
// we don't know which image in the image stream surfaced the content).
is, err := r.repo.getImageStream()
Expand Down Expand Up @@ -112,13 +118,13 @@ func (r *pullthroughBlobStore) ServeBlob(ctx context.Context, w http.ResponseWri

desc, err := store.Stat(ctx, dgst)
if err != nil {
context.GetLogger(ctx).Errorf("Failed to stat digest %q: %v", dgst.String(), err)
context.GetLogger(ctx).Errorf("failed to stat digest %q: %v", dgst.String(), err)
return err
}

remoteReader, err := store.Open(ctx, dgst)
if err != nil {
context.GetLogger(ctx).Errorf("Failure to open remote store for digest %q: %v", dgst.String(), err)
context.GetLogger(ctx).Errorf("failure to open remote store for digest %q: %v", dgst.String(), err)
return err
}
defer remoteReader.Close()
Expand All @@ -130,6 +136,30 @@ func (r *pullthroughBlobStore) ServeBlob(ctx context.Context, w http.ResponseWri
return nil
}

// Get attempts to fetch the requested blob by digest using a remote proxy store if necessary.
func (r *pullthroughBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
store, ok := r.digestToStore[dgst.String()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return early in the ok case and unindent the rest of the function?

if store, ok := r.digestToStore[dgst.String()]; ok {
  return store.Get(ctx, desc.Digest)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pullthroughBlobStore instances are per-request, right? just making sure we don't have to worry about locking/races on the digestToStore map, or about long-term growth/retention

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return early in the ok case and unindent the rest of the function?

good suggestion, I'll rewrite

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pullthroughBlobStore instances are per-request, right?

That's right.

if ok {
return store.Get(ctx, dgst)
}

data, originalErr := r.BlobStore.Get(ctx, dgst)
if originalErr == nil {
return data, nil
}

desc, err := r.remoteStat(ctx, dgst)
if err != nil {
context.GetLogger(ctx).Errorf("failed to stat blob %q in remote repositories: %v", dgst.String(), err)
return nil, originalErr
}
store, ok = r.digestToStore[desc.Digest.String()]
if !ok {
return nil, originalErr
}
return store.Get(ctx, desc.Digest)
}

// findCandidateRepository looks in search for a particular blob, referring to previously cached items
func (r *pullthroughBlobStore) findCandidateRepository(ctx context.Context, search map[string]*imageapi.DockerImageReference, cachedLayers []string, dgst digest.Digest, retriever importer.RepositoryRetriever) (distribution.Descriptor, error) {
// no possible remote locations to search, exit early
Expand Down
16 changes: 12 additions & 4 deletions pkg/dockerregistry/server/repositorymiddleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ func (r *repository) signedManifestFillImageMetadata(manifest *schema1.SignedMan

refs := manifest.References()

layerSet := sets.NewString()
blobSet := sets.NewString()
image.DockerImageMetadata.Size = int64(0)

blobs := r.Blobs(r.ctx)
Expand All @@ -473,11 +473,15 @@ func (r *repository) signedManifestFillImageMetadata(manifest *schema1.SignedMan
}
layer.LayerSize = desc.Size
// count empty layer just once (empty layer may actually have non-zero size)
if !layerSet.Has(layer.Name) {
if !blobSet.Has(layer.Name) {
image.DockerImageMetadata.Size += desc.Size
layerSet.Insert(layer.Name)
blobSet.Insert(layer.Name)
}
}
if len(image.DockerImageConfig) > 0 && !blobSet.Has(image.DockerImageMetadata.ID) {
blobSet.Insert(image.DockerImageMetadata.ID)
image.DockerImageMetadata.Size += int64(len(image.DockerImageConfig))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the size of the ID is not already included?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like a test around this size calculation

Copy link
Author

@miminar miminar Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the size of the ID is not already included?

The ID is a name of the config. It's actually a digest - similar to layer's digests. The size needs to be updated for the size of the config itself, not its ID.

Update of blobSet is actually a useless operation here, but I'd keep that for a sake of correctness in case we append another size updating block codes afterwards.

would like a test around this size calculation

I'm working on tests. Tests added.

}

return nil
}
Expand Down Expand Up @@ -544,7 +548,7 @@ func (r *repository) getImageStreamImage(dgst digest.Digest) (*imageapi.ImageStr

// rememberLayersOfImage caches the layer digests of given image
func (r *repository) rememberLayersOfImage(image *imageapi.Image, cacheName string) {
if len(image.DockerImageLayers) == 0 && len(image.DockerImageManifestMediaType) > 0 {
if len(image.DockerImageLayers) == 0 && len(image.DockerImageManifestMediaType) > 0 && len(image.DockerImageConfig) == 0 {
// image has no layers
return
}
Expand All @@ -553,6 +557,10 @@ func (r *repository) rememberLayersOfImage(image *imageapi.Image, cacheName stri
for _, layer := range image.DockerImageLayers {
r.cachedLayers.RememberDigest(digest.Digest(layer.Name), r.blobrepositorycachettl, cacheName)
}
// remember reference to manifest config as well for schema 2
if image.DockerImageManifestMediaType == schema2.MediaTypeManifest && len(image.DockerImageMetadata.ID) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we only do this if len(image.DockerImageConfig) > 0?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we only do this if len(image.DockerImageConfig) > 0?

To make it comply with the other conditions, I'll use this instead. image.DockerImageConfig is filled only for manifest schema 2.

r.cachedLayers.RememberDigest(digest.Digest(image.DockerImageMetadata.ID), r.blobrepositorycachettl, cacheName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is image.DockerImageMetadata.ID computed when the registry creates an image, or is it provided by the user?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is image.DockerImageMetadata.ID computed when the registry creates an image, or is it provided by the user?

For schema 2, it is parsed from manifest which is immutable:

origin/pkg/image/api/helper.go

Line 543 in e22f4b2

image.DockerImageMetadata.ID = manifest.Config.Digest

}
return
}

Expand Down
4 changes: 1 addition & 3 deletions pkg/image/api/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,17 +552,15 @@ func ImageWithMetadata(image *Image) error {
image.DockerImageMetadata.Architecture = config.Architecture
image.DockerImageMetadata.Size = int64(len(image.DockerImageConfig))

layerSet := sets.NewString(image.DockerImageMetadata.ID)
if len(image.DockerImageLayers) > 0 {
layerSet := sets.NewString()
for _, layer := range image.DockerImageLayers {
if layerSet.Has(layer.Name) {
continue
}
layerSet.Insert(layer.Name)
image.DockerImageMetadata.Size += layer.LayerSize
}
} else {
image.DockerImageMetadata.Size += config.Size
}
default:
return fmt.Errorf("unrecognized Docker image manifest schema %d for %q (%s)", manifest.SchemaVersion, image.Name, image.DockerImageReference)
Expand Down
12 changes: 11 additions & 1 deletion pkg/image/importer/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type mockRepository struct {

blobs *mockBlobStore

manifest *schema1.SignedManifest
manifest distribution.Manifest
tags map[string]string
}

Expand Down Expand Up @@ -79,6 +79,8 @@ func (r *mockRepository) Tags(ctx context.Context) distribution.TagService {
type mockBlobStore struct {
distribution.BlobStore

blobs map[digest.Digest][]byte

statErr, serveErr, openErr error
}

Expand All @@ -94,6 +96,14 @@ func (r *mockBlobStore) Open(ctx context.Context, dgst digest.Digest) (distribut
return nil, r.openErr
}

func (r *mockBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
b, exists := r.blobs[dgst]
if !exists {
return nil, distribution.ErrBlobUnknown
}
return b, nil
}

type mockTagService struct {
distribution.TagService

Expand Down
11 changes: 8 additions & 3 deletions pkg/image/importer/importer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,15 +310,15 @@ func formatRepositoryError(repository *importRepository, refName string, refID s
func (isi *ImageStreamImporter) calculateImageSize(ctx gocontext.Context, repo distribution.Repository, image *api.Image) error {
bs := repo.Blobs(ctx)

layerSet := sets.NewString()
blobSet := sets.NewString()
size := int64(0)
for i := range image.DockerImageLayers {
layer := &image.DockerImageLayers[i]

if layerSet.Has(layer.Name) {
if blobSet.Has(layer.Name) {
continue
}
layerSet.Insert(layer.Name)
blobSet.Insert(layer.Name)

if layerSize, ok := isi.digestToLayerSizeCache[layer.Name]; ok {
size += layerSize
Expand All @@ -335,6 +335,11 @@ func (isi *ImageStreamImporter) calculateImageSize(ctx gocontext.Context, repo d
size += desc.Size
}

if len(image.DockerImageConfig) > 0 && !blobSet.Has(image.DockerImageMetadata.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we only do this if image.DockerImageManifestMediaType == schema2.MediaTypeManifest?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we only do this if image.DockerImageManifestMediaType == schema2.MediaTypeManifest?

Both conditions should be equivalent:

(len(image.DockerImageConfig) > 0) == (image.DockerImageManifestMediaType == schema2.MediaTypeManifest)

I'll use the first everywhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using len(image.DockerImageConfig) > 0 everywhere now.

blobSet.Insert(image.DockerImageMetadata.ID)
size += int64(len(image.DockerImageConfig))
}

image.DockerImageMetadata.Size = size
return nil
}
Expand Down
Loading