registry package rewrite #851

squaremo · 2017-11-21T14:56:05Z

Rewrite the registry code to remove some historical baggage and

Fixes Image metadata lookup credentials handling #836
Fixes Include imageId / image hash in /images response #839 (image.Info now includes an ImageID field and a Digest field; this will also help with Deal with multiple tags referring to the same image layer, and moving tags #822 and Detect image ID changes when determining whether or not to update #498 -- if we put image IDs in the image.Info, we'll be able to know which tags refer to the same image, though we still have to figure out what to do with that information)
Fixes lots of 'MANIFEST_UNKNOWN' errors in fluxd logs #838 (now uses docker distribution client library and thereby supports all content types)
Fixes Supply context to image fetch operation #713 (registry client methods now take a context argument; warmer aborts a fetch if the context is cancelled)
Fixes Implement image manifest request with HTTP rather than client #790 (we use the docker distribution client, which does the proper HTTPing for us)
Fixes Store whole image data in one place #710 (implemented the scheme of storing the whole result)

These two --

-- I am punting on, since they are to do with images for other OS/Arch combinations, and this needs more thought. Although we do handle manifestlist documents now (such as you get from nats:nanoserver), if there's no Linux/amd64 image it'll still result in an incomplete set of repo metadata.

registry/cache/memcached.go

-		Value:      val,
-		Expiration: expiry,
+		Value:      append(expiryBytes, v...),
+		Expiration: int32(expiry),


samb1729

Mostly LGTM. Some comments might be out of place because of files being moved after the commit I looked at.

registry/middleware/rate_limiter.go

+type RateLimiters struct {
+	RPS, Burst int
+	perHost    map[string]*rate.Limiter
+	mx         sync.Mutex


registry/registry.go

@@ -96,13 +69,13 @@ func (reg *registry) tagsToRepository(client Client, id image.Name, tags []strin
 	toFetch := make(chan string, len(tags))
 	fetched := make(chan result, len(tags))

-	for i := 0; i < reg.connections; i++ {
+	for i := 0; i < 100; i++ {


registry/client.go

-		logger: logger,
+// GetImage gets the manifest of a specific image ref, from its
+// registry.
+func (c *Cache) GetImage(id image.Ref) (image.Info, error) {


registry/warming.go

@@ -43,7 +42,7 @@ type backlogItem struct {
 func (w *Warmer) Loop(stop <-chan struct{}, wg *sync.WaitGroup, imagesToFetchFunc func() ImageCreds) {
 	defer wg.Done()

-	if w.Logger == nil || w.ClientFactory == nil || w.Expiry == 0 || w.Writer == nil || w.Reader == nil {
+	if w.Logger == nil || w.ClientFactory == nil || w.Expiry == 0 || w.Cache == nil {


registry/warming.go

@@ -151,12 +146,12 @@ func (w *Warmer) warm(id image.Name, creds Credentials) {
 		// See if we have the manifest already cached
 		// We don't want to re-download a manifest again.
 		newID := id.ToRef(tag)
-		key, err := cache.NewManifestKey(newID.CanonicalRef())
+		key := cache.NewManifestKey(newID.CanonicalRef())
 		if err != nil {


registry/warming.go

+// value (show the images, but also indicate there's a problem, for
+// example).
+type ImageRepository struct {
+	LastError  string


registry/client.go

+
+	// TODO(michael): can we type switch? Not sure how dependable the
+	// underlying types are.
+	switch mt {


registry/mock.go

-type ManifestFunc func(id image.Ref) (image.Info, error)
-type TagsFunc func(id image.Name) ([]string, error)
+type ManifestFunc func(ref string) (image.Info, error)
+type TagsFunc func() ([]string, error)


tamarakaufler

No ground breaking thoughts just a possible idea for a tiny cleanup

registry/monitoring.go

 	remoteDuration.With(
 		LabelRequestKind, RequestKindMetadata,
 		fluxmetrics.LabelSuccess, strconv.FormatBool(err == nil),
 	).Observe(time.Since(start).Seconds())
 	return
 }

-func (m *instrumentedClient) Tags(id image.Name) (res []string, err error) {
+func (m *instrumentedClient) Tags(ctx context.Context) (res []string, err error) {


registry/client.go

+// Client is a remote registry client for a particular image
+// repository (e.g., for quay.io/weaveworks/flux). It is an interface
+// so we can wrap it in instrumentation, write fake implementations,
+// and so on.


* make rate limiters a struct rather than a global * remove some constructor procedures in favour of literals This is mainly to tidy some things up before making more drastic changes.

It used to be the case that flux used a pull-through cache for image metadata. The cache was practically, but not logically, necessary. It made sense then for there to be a registry Client interface with two implementations: the implementation that looked at the cache, and the implementation that went and fetched the metadata from a registry. But we changed this such that we only ever look in the cache when querying, and refresh from registries in the background. Querying can just use cache operations, and only refreshing needs to fetch metadata from registries. This commit removes some of the layers that existed only because of the erstwhile equivalence of the two clients. Specifically: it removes the registry struct, which used a (cache) client factory to get a (cache) Client which it then used to assemble image manifests. Instead, the cache client just assembles the manifests itself. Consequently, the cache client factory can also go. I have kept the remote client factory around for now, as the work it does to construct a client has to live somewhere.

It's not necessary to repackage results from the docker regsitry library.

No point in having separate fields (in fact, it'd be weird if you supplied two different implementations, or two clients pointed at different places.)

- we fetch the whole entry each time, so just return both of the expiry and value - don't encode with JSON, since the byte array will end up being encoded as `[byte0, byte1, byte2, ...]`

There are two components that read or write to the cache. The first is the cache warmer, which refreshes the list of tags for an image repository in the cache, and for each tag, writes the image metadata (the layer created date) to the cache; the second is `registry.Cache` (an implementation of `registry.Registry`), which assembles a full list of `image.Info` by first reading the tags from the cache, then for each tag, reading the metadata. There are two problems with this: - the `registry.Cache` does a lot of reads to re-assemble what the warmer already had all in one place; and, - if the warmer fails to fetch a single manifest, the Cache cannot assemble an answer at all. This commit changes the warmer so that it writes whole results, and `registry.Cache` so that it reads whole results. This solves the first problem. The writing is done as a ratchet: we only overwrite the result when we have managed to get all the metadata. This solves the second problem. A consequence is that we no longer need to store the tags separately, as they are an intermediate result.

This moves the "cache" (looking less like a cache every day) into its own package, with the memcached implementation a subpackage therein.

- remove `Stop` method from cache client interface; it is particular to this client - memcacheClient -> MemcacheClient so it can be returned as such - explain why the expiry goes in two places

The package github.com/docker/distribution/registry/client (now) has types and procedures for fetching image metadata. This means we can get rid of a lot of the workarounds we were using to patch `docker-registry-client` so that e.g., it works with quay.io. I have changed the interfaces around a bit, since we usually need to request image manifests "in a straight line", reusing the same authorisation, and it makes sense to construct a client for each such series of requests. There are a few things we can keep track of across series of requests: specifically, the challenges we've seen from each host. So it's still useful to have a "factory" to hold that information, as well as other commonalities like rate limiting.

Replace the `context.TODO()`s given to with a context passed in. Also: the values returned by the docker distribution registry client will be one of the types in github.com/docker/distribution/manifest/{schema1,schema2,manifestlist}, so instead of dispatching on the media type and doing the deserialisation ourselves, just dispatch on the value's type.

These fields help with detecting when - references with different tags point at the same image (e.g., `latest` vs whatever) - the same reference fetched at different times refer to different images (e.g., `latest` now vs `latest` before) NB I have not attempted to use the fields as above -- I've just made sure they are populated.

- rate_limiter_test.go tested that contexts were not shared between transports; but we no longer implement the transport under test. - the use of memcached has changed - removed some spurious cache warmer tests - move the mock registry objects (which are handy elsewhere) to registry/mock - remove the tests that check if the registry assembles manifests from individual cache entries; it no longer does that - check that the cache warmer populates the cache, then the registry can read the result - move and rewrite the registry integration test and a change made /en passant/: - supply the registry cache expiry argument to the cache and use it - and don't supply it to the warmer, which doesn't use it

The cache.Warmer struct has some mandatory fields and some optional. With a constructor we can better enforce this. In particular, the cache client implementation, which is now mandatory but was still described as optional in examples and help text. We can simplify (and in examples, omit) the default values by assuming memcached is in the same namespace.

tamarakaufler reviewed Nov 27, 2017

View reviewed changes

squaremo force-pushed the image-reg-caching branch 2 times, most recently from 321694c to 5514161 Compare November 30, 2017 18:16

squaremo changed the title ~~[WIP] registry rewrite~~ registry package rewrite Nov 30, 2017

This was referenced Dec 1, 2017

Supply a reason for "available images" being blank #861

Merged

Limit concurrent connections to memcached #384

Closed

samb1729 suggested changes Dec 17, 2017

View reviewed changes

squaremo force-pushed the image-reg-caching branch from 32ad780 to 757677f Compare December 18, 2017 12:23

tamarakaufler suggested changes Dec 18, 2017

View reviewed changes

squaremo force-pushed the image-reg-caching branch from 757677f to b24a8ad Compare December 18, 2017 13:03

tamarakaufler approved these changes Dec 18, 2017

View reviewed changes

squaremo and others added 13 commits December 18, 2017 15:41

Simplify registry rate limiting and construction

3b41b8c

* make rate limiters a struct rather than a global * remove some constructor procedures in favour of literals This is mainly to tidy some things up before making more drastic changes.

Remove compatibility wrapper

f01cfe5

It's not necessary to repackage results from the docker regsitry library.

Use cache.Client in warmer

1509aff

No point in having separate fields (in fact, it'd be weird if you supplied two different implementations, or two clients pointed at different places.)

Change cache entry encoding

a7729df

- we fetch the whole entry each time, so just return both of the expiry and value - don't encode with JSON, since the byte array will end up being encoded as `[byte0, byte1, byte2, ...]`

Move cache implementation to its own package

6048af5

This moves the "cache" (looking less like a cache every day) into its own package, with the memcached implementation a subpackage therein.

Tidy memcache client

3f7bad9

- remove `Stop` method from cache client interface; it is particular to this client - memcacheClient -> MemcacheClient so it can be returned as such - explain why the expiry goes in two places

squaremo force-pushed the image-reg-caching branch from b24a8ad to 1a06c22 Compare December 18, 2017 15:41

samb1729 approved these changes Dec 18, 2017

View reviewed changes

squaremo merged commit 5245a0c into master Dec 19, 2017

squaremo deleted the image-reg-caching branch December 19, 2017 11:28

samb1729 mentioned this pull request Dec 20, 2017

Fully qualified image names being written to yamls #877

Closed

squaremo mentioned this pull request Dec 22, 2017

GCR content-type errors #879

Closed

squaremo mentioned this pull request Jan 4, 2018

Need ability to add to known_hosts #724

Closed

squaremo mentioned this pull request Jan 15, 2018

Image timestamps are incorrect #891

Closed

This was referenced Mar 12, 2018

Deal with images for another arch/OS #999

Closed

Warmer can't handle images with non linux/amd64 tags #741

Closed

squaremo mentioned this pull request Mar 23, 2018

DockerHub tags include windows images, but manifests are missing #853

Closed

squaremo mentioned this pull request Apr 24, 2018

Flux automation released untagged image weaveworks/service#1905

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

registry package rewrite #851

registry package rewrite #851

squaremo commented Nov 21, 2017 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

samb1729 left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

tamarakaufler left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

registry package rewrite #851

registry package rewrite #851

Conversation

squaremo commented Nov 21, 2017 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

samb1729 left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

tamarakaufler left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

squaremo commented Nov 21, 2017 •

edited

Loading