Fixed cache issue with steps that output empty layer (#73)

* Fixing cache issue with steps that output empty layer

Steps like WORKDIR and USER that output no layers would never get pushed
to the cache/registry, and so the cache would always break on these
steps.

* Fixed for comments

lib/builder/build_node.go

@@ -105,13 +105,13 @@ func (n *buildNode) doCommit(cacheMgr cache.Manager, opts *buildOptions) error {
 		return fmt.Errorf("commit: %s", err)
 	}
 
-	// If the number of digestPairs is 0 or greater than 1 then we cannot push
+	// If the number of digestPairs is greater than 1 then we cannot push
 	// the resulting layer mappings to the distributed cache.
-	if len(n.digestPairs) != 1 {
+	if len(n.digestPairs) > 1 {
 		return nil
 	}
 
-	if err := n.pushCacheLayers(cacheMgr); err != nil {
+	if err := n.pushCacheLayer(cacheMgr); err != nil {
 		return fmt.Errorf("push cache: %s", err)
 	}
 	return nil
@@ -147,20 +147,28 @@ func (n *buildNode) applyLayer(digestPair *image.DigestPair, modifyfs bool) erro
 }
 
 // pushCacheLayers pushs cached layers for this node's digest pair(s).
-func (n *buildNode) pushCacheLayers(cacheMgr cache.Manager) error {
-	digestPair := n.digestPairs[0]
-	log.Infof("* Committed gzipped layer %s (%d bytes)",
-		digestPair.GzipDescriptor.Digest, digestPair.GzipDescriptor.Size)
+func (n *buildNode) pushCacheLayer(cacheMgr cache.Manager) error {
+	var digestPair *image.DigestPair
+	if len(n.digestPairs) != 0 {
+		digestPair = n.digestPairs[0]
+	}
+
+	if digestPair != nil {
+		log.Infof("* Committed gzipped layer %s (%d bytes)",
+			digestPair.GzipDescriptor.Digest, digestPair.GzipDescriptor.Size)
+	}
 	log.Infof("* Pushing with cache ID %s", n.CacheID())
 	return cacheMgr.PushCache(n.CacheID(), digestPair)
 }
 
-// pullCacheLayers pulls cached layers for this node's digest pair(s).
-func (n *buildNode) pullCacheLayers(cacheMgr cache.Manager) bool {
+// pullCacheLayer pulls cached layers for this node's digest pair(s).
+func (n *buildNode) pullCacheLayer(cacheMgr cache.Manager) bool {
 	digestPair, err := cacheMgr.PullCache(n.CacheID())
 	if err != nil {
 		log.Errorf("Failed to fetch intermediate layer with cache ID %s: %s", n.CacheID(), err)
 		return false
+	} else if digestPair == nil {
+		return true
 	}
 	n.digestPairs = []*image.DigestPair{digestPair}
 	return true

lib/builder/build_stage.go

@@ -245,8 +245,8 @@ func (stage *buildStage) pullCacheLayers(cacheMgr cache.Manager) {
 	// it gets executed.
 	for _, node := range stage.nodes[1:] {
 		// Stop once the cache chain is broken.
-		if node.HasCommit() {
-			if !node.pullCacheLayers(cacheMgr) {
+		if node.HasCommit() || stage.forceCommit {
+			if !node.pullCacheLayer(cacheMgr) {
 				return
 			}
 		}

lib/cache/cache_manager.go

@@ -29,6 +29,7 @@ import (
 )
 
 const _cachePrefix = "makisu_builder_cache_"
+const _cacheEmptyEntry = "MAKISU_CACHE_EMPTY"
 
 // Manager is the interface through which we interact with the cacheID -> image layer mapping.
 type Manager interface {
@@ -114,6 +115,10 @@ func (manager *registryCacheManager) PullCache(cacheID string) (*image.DigestPai
 	}
 	log.Infof("Found mapping in cacheID KVStore: %s => %s", cacheID, entry)
 
+	if entry == _cacheEmptyEntry {
+		return nil, nil
+	}
+
 	tarDigest, gzipDigest, err := parseEntry(entry)
 	if err != nil {
 		return nil, errors.Wrapf(ErrorLayerNotFound, "parse entry %s", entry)
@@ -149,12 +154,14 @@ func (manager *registryCacheManager) PushCache(cacheID string, digestPair *image
 		manager.Lock()
 		defer manager.Unlock()
 
-		if err := manager.registryClient.PushLayer(digestPair.GzipDescriptor.Digest); err != nil {
-			manager.pushErrors.Add(fmt.Errorf("push layer %s: %s", digestPair.GzipDescriptor.Digest, err))
-			return
+		if digestPair != nil {
+			if err := manager.registryClient.PushLayer(digestPair.GzipDescriptor.Digest); err != nil {
+				manager.pushErrors.Add(fmt.Errorf("push layer %s: %s", digestPair.GzipDescriptor.Digest, err))
+				return
+			}
 		}
 
-		entry := createEntry(digestPair.TarDigest, digestPair.GzipDescriptor.Digest)
+		entry := createEntry(digestPair)
 		if err := manager.cacheIDStore.Put(_cachePrefix+cacheID, entry); err != nil {
 			manager.pushErrors.Add(fmt.Errorf("store tag mapping (%s,%s): %s", cacheID, entry, err))
 			return
@@ -189,6 +196,9 @@ func parseEntry(entry string) (image.Digest, image.Digest, error) {
 	return image.Digest("sha256:" + split[0]), image.Digest("sha256:" + split[1]), nil
 }
 
-func createEntry(tarDigest, gzipDigest image.Digest) string {
-	return fmt.Sprintf("%s,%s", tarDigest.Hex(), gzipDigest.Hex())
+func createEntry(pair *image.DigestPair) string {
+	if pair == nil {
+		return _cacheEmptyEntry
+	}
+	return fmt.Sprintf("%s,%s", pair.TarDigest.Hex(), pair.GzipDescriptor.Digest.Hex())
 }

test/python/test_build.py

@@ -108,6 +108,24 @@ def test_build_go_from_scratch(registry1, storage_dir):
     assert code == 0, err
 
 
+def test_build_commit_empty_pair(registry1, storage_dir, cache_dir, tmpdir):
+    utils.registry_ensure_image('debian:8', registry1.addr)
+    new_image1 = new_image_name()
+    new_image2 = new_image_name()
+    context_dir = os.path.join(os.getcwd(), 'testdata/build-context/commit-empty-pair')
+    test_file = tmpdir.mkdir("d1").join("f1")
+    test_file.write("hello")
+
+    # First build, mount in test file.
+    additional_volumes = {test_file: '/mnt/f1'}
+    utils.makisu_build_image(new_image1, registry1.addr, context_dir, storage_dir, cache_dir, additional_volumes)
+
+    # Second build, without test file. It would fail if distributed cache doesn't work.
+    utils.makisu_build_image(new_image2, registry1.addr, context_dir, storage_dir, cache_dir)
+    code, err = utils.docker_run_image(registry1.addr, new_image2)
+    assert code == 0, err
+
+
 def test_build_with_cache(registry1, storage_dir, cache_dir):
     utils.registry_ensure_image('debian:8', registry1.addr)
     new_image1 = new_image_name()

testdata/build-context/commit-empty-pair/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN mkdir /test #!COMMIT
+WORKDIR /test #!COMMIT
+RUN cp /mnt/f1 f1 #!COMMIT
+RUN ls /test/f1