Correctly store Excluded manifests

registry/cache/warming.go

@@ -181,17 +181,21 @@ func (w *Warmer) warm(ctx context.Context, logger log.Logger, id image.Name, cre
 			errorLogger.Log("err", "empty tag in fetched tags", "tags", tags)
 			repo.LastError = "empty tag in fetched tags"
 			return // abort and let the error be written
-		case err != nil:
+		case err != nil: // by and large these are cache misses, but any error will do
 			missing++
 		case time.Until(expiry) < refreshWhenExpiryWithin:
 			expired++
 		case len(bytes) == 0:
-			errorLogger.Log("err", "empty byte array from cache", "tag", tag)
+			errorLogger.Log("warning", "empty result from cache", "tag", tag)
 			missing++
 		default:
-			var image image.Info
-			if err := json.Unmarshal(bytes, &image); err == nil {
-				newImages[tag] = image
+			var entry registry.ImageEntry
+			if err := json.Unmarshal(bytes, &entry); err == nil {
+				if entry.ExcludedReason == "" {
+					newImages[tag] = entry.Info
+				} else {
+					logger.Log("info", "excluded in cache", "ref", newID)
+				}
 				continue // i.e., no need to update this one
 			}
 			missing++
@@ -221,35 +225,35 @@ func (w *Warmer) warm(ctx context.Context, logger log.Logger, id image.Name, cre
 			go func(imageID image.Ref) {
 				defer func() { awaitFetchers.Done(); <-fetchers }()
 				// Get the image from the remote
-				img, err := client.Manifest(ctx, imageID.Tag)
+				entry, err := client.Manifest(ctx, imageID.Tag)
 				if err != nil {
 					if err, ok := errors.Cause(err).(net.Error); ok && err.Timeout() {
 						// This was due to a context timeout, don't bother logging
 						return
 					}
-					errorLogger.Log("err", errors.Wrap(err, "requesting manifests"))
+					errorLogger.Log("err", err, "ref", imageID)
 					return
 				}
-				if img.ExcludedReason != "" {
-					errorLogger.Log("excluded", img.ExcludedReason)
+				if entry.ExcludedReason != "" {
+					errorLogger.Log("excluded", entry.ExcludedReason, "ref", imageID.Tag)
 				}
 
-				key := NewManifestKey(img.ID.CanonicalRef())
+				key := NewManifestKey(imageID.CanonicalRef())
 				// Write back to memcached
-				val, err := json.Marshal(img)
+				val, err := json.Marshal(entry)
 				if err != nil {
-					errorLogger.Log("err", errors.Wrap(err, "serializing tag to store in cache"))
+					errorLogger.Log("err", err, "ref", imageID)
 					return
 				}
 				err = w.cache.SetKey(key, val)
 				if err != nil {
-					errorLogger.Log("err", errors.Wrap(err, "storing manifests in cache"))
+					errorLogger.Log("err", err, "ref", imageID)
 					return
 				}
 				successMx.Lock()
 				successCount++
-				if img.ExcludedReason == "" {
-					newImages[imageID.Tag] = img.Info
+				if entry.ExcludedReason == "" {
+					newImages[imageID.Tag] = entry.Info
 				}
 				successMx.Unlock()
 			}(imID)

registry/client.go

@@ -18,11 +18,43 @@ import (
 	"github.com/weaveworks/flux/image"
 )
 
-type ImageEntry struct {
-	image.Info     `json:",inline"`
+type Excluded struct {
 	ExcludedReason string `json:",omitempty"`
 }
 
+// ImageEntry represents a result from looking up an image ref in an
+// image registry. It's an either-or: either you get an image.Info, or
+// you get a reason that the image should be treated as unusable
+// (e.g., it's for the wrong architecture).
+type ImageEntry struct {
+	image.Info `json:",omitempty"`
+	Excluded
+}
+
+// MarshalJSON does custom JSON marshalling for ImageEntry values. We
+// need this because the struct embeds the image.Info type, which has
+// its own custom marshaling, which would get used otherwise.
+func (entry ImageEntry) MarshalJSON() ([]byte, error) {
+	// We can only do it this way because it's explicitly an either-or
+	// -- I don't know of a way to inline all the fields when one of
+	// the things you're inlining defines its own MarshalJSON.
+	if entry.ExcludedReason != "" {
+		return json.Marshal(entry.Excluded)
+	}
+	return json.Marshal(entry.Info)
+}
+
+// UnmarshalJSON does custom JSON unmarshalling for ImageEntry values.
+func (entry *ImageEntry) UnmarshalJSON(bytes []byte) error {
+	if err := json.Unmarshal(bytes, &entry.Info); err != nil {
+		return err
+	}
+	if err := json.Unmarshal(bytes, &entry.Excluded); err != nil {
+		return err
+	}
+	return nil
+}
+
 // 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,
@@ -136,7 +168,9 @@ interpret:
 				goto interpret
 			}
 		}
-		return ImageEntry{ExcludedReason: "no suitable manifest (linux amd64) in manifestlist"}, nil
+		entry := ImageEntry{}
+		entry.ExcludedReason = "no suitable manifest (linux amd64) in manifestlist"
+		return entry, nil
 	default:
 		t := reflect.TypeOf(manifest)
 		return ImageEntry{}, errors.New("unknown manifest type: " + t.String())

registry/imageentry_test.go

@@ -0,0 +1,60 @@
+package registry
+
+import (
+	"encoding/json"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/weaveworks/flux/image"
+)
+
+// Check that the ImageEntry type can be round-tripped via JSON.
+func TestImageEntryRoundtrip(t *testing.T) {
+
+	test := func(t *testing.T, entry ImageEntry) {
+		bytes, err := json.Marshal(entry)
+		assert.NoError(t, err)
+
+		var entry2 ImageEntry
+		assert.NoError(t, json.Unmarshal(bytes, &entry2))
+		assert.Equal(t, entry, entry2)
+	}
+
+	ref, err := image.ParseRef("quay.io/weaveworks/flux:1.0.0")
+	assert.NoError(t, err)
+
+	info := image.Info{
+		ID:        ref,
+		CreatedAt: time.Now().UTC(), // to UTC since we unmarshal times in UTC
+	}
+
+	entry := ImageEntry{
+		Info: info,
+	}
+	t.Run("With an info", func(t *testing.T) { test(t, entry) })
+	t.Run("With an excluded reason", func(t *testing.T) {
+		entry.Info = image.Info{}
+		entry.ExcludedReason = "just because"
+		test(t, entry)
+	})
+}
+
+// Check that existing entries, which are image.Info, will parse into
+// the ImageEntry struct.
+func TestImageInfoParsesAsEntry(t *testing.T) {
+	ref, err := image.ParseRef("quay.io/weaveworks/flux:1.0.0")
+	assert.NoError(t, err)
+	info := image.Info{
+		ID:        ref,
+		CreatedAt: time.Now().UTC(), // to UTC since we unmarshal times in UTC
+	}
+
+	bytes, err := json.Marshal(info)
+	assert.NoError(t, err)
+
+	var entry2 ImageEntry
+	assert.NoError(t, json.Unmarshal(bytes, &entry2))
+	assert.Equal(t, info, entry2.Info)
+}