From 49a9502312b6af391a10e1a5c3e05d245ad54899 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 14 Mar 2023 07:32:41 -0700 Subject: [PATCH] Clear all remote metadata if any of them are evicted from remote cache With TTL based discarding and upcoming lease extension, remote cache eviction error won't happen if remote cache can guarantee the TTL. However, if it happens, it usually means the remote cache is under high load and it could possibly evict more blobs that Bazel wouldn't aware of. Following builds could still fail for the same error (caused by different blobs). This PR changes to remove all remote metadata when the remove cache eviction error happens (which should be rare with the help from TTL based discarding and lease extension) to make sure next incremental build can success. Part of #16660. Closes #17747. PiperOrigin-RevId: 516519657 Change-Id: Ia99770b9d314ca62801b73dc96d09ed8ac2233f6 --- .../build/lib/actions/cache/ActionCache.java | 10 ++- .../cache/CompactPersistentActionCache.java | 10 +++ .../google/devtools/build/lib/remote/BUILD | 5 +- .../build/lib/remote/LeaseService.java | 70 ++++++++++--------- .../lib/actions/ActionCacheCheckerTest.java | 6 ++ .../actions/util/ActionCacheTestHelper.java | 4 ++ .../skyframe/TimestampBuilderTestCase.java | 5 ++ 7 files changed, 75 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index 8f91a182befc2c..6b020a472d85cc 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -45,6 +45,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Predicate; import javax.annotation.Nullable; /** @@ -76,6 +77,9 @@ public interface ActionCache { */ void remove(String key); + /** Removes entry from cache that matches the predicate. */ + void removeIf(Predicate predicate); + /** * An entry in the ActionCache that contains all action input and output * artifact paths and their metadata plus action key itself. @@ -246,7 +250,8 @@ public RemoteFileArtifactValue getOutputFile(Artifact output) { return outputFileMetadata.get(output.getExecPathString()); } - Map getOutputFiles() { + /** Gets metadata of all output files */ + public Map getOutputFiles() { return outputFileMetadata; } @@ -273,7 +278,8 @@ public SerializableTreeArtifactValue getOutputTree(SpecialArtifact output) { return outputTreeMetadata.get(output.getExecPathString()); } - Map getOutputTrees() { + /** Gets metadata of all output trees */ + public Map getOutputTrees() { return outputTreeMetadata; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index aac16acc51751e..d906498770e072 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -55,6 +55,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Predicate; import javax.annotation.Nullable; /** @@ -342,6 +343,10 @@ public ActionCache.Entry get(String key) { return null; } byte[] data = map.get(index); + return get(data); + } + + private ActionCache.Entry get(byte[] data) { try { return data != null ? decode(indexer, data) : null; } catch (IOException e) { @@ -381,6 +386,11 @@ public void remove(String key) { map.remove(indexer.getIndex(key)); } + @Override + public void removeIf(Predicate predicate) { + map.entrySet().removeIf(entry -> predicate.test(get(entry.getValue()))); + } + @ThreadSafety.ThreadHostile @Override public long save() throws IOException { diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 862216b2a87e4a..dca53cd3745a3b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -232,8 +232,11 @@ java_library( srcs = ["LeaseService.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/skyframe", "//third_party:jsr305", ], diff --git a/src/main/java/com/google/devtools/build/lib/remote/LeaseService.java b/src/main/java/com/google/devtools/build/lib/remote/LeaseService.java index 8479d787c99dc0..0e1d65d628e645 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/LeaseService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/LeaseService.java @@ -13,15 +13,13 @@ // limitations under the License. package com.google.devtools.build.lib.remote; -import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.ActionCacheUtils; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.ActionLookupValue; -import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.cache.ActionCache; +import com.google.devtools.build.lib.skyframe.ActionExecutionValue; +import com.google.devtools.build.lib.skyframe.SkyFunctions; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.skyframe.MemoizingEvaluator; -import java.util.HashMap; import java.util.Set; import javax.annotation.Nullable; @@ -41,36 +39,44 @@ public void handleMissingInputs(Set missingActionInputs) { return; } - var actions = new HashMap(); + // If any outputs are evicted, remove all remote metadata from skyframe and local action cache. + // + // With TTL based discarding and lease extension, remote cache eviction error won't happen if + // remote cache can guarantee the TTL. However, if it happens, it usually means the remote cache + // is under high load and it could possibly evict more blobs that Bazel wouldn't aware of. + // Following builds could still fail for the same error (caused by different blobs). - try { - for (ActionInput actionInput : missingActionInputs) { - if (actionInput instanceof Artifact.DerivedArtifact) { - Artifact.DerivedArtifact output = (Artifact.DerivedArtifact) actionInput; - ActionLookupData actionLookupData = output.getGeneratingActionKey(); - var actionLookupValue = - memoizingEvaluator.getExistingValue(actionLookupData.getActionLookupKey()); - if (actionLookupValue instanceof ActionLookupValue) { - Action action = - ((ActionLookupValue) actionLookupValue) - .getAction(actionLookupData.getActionIndex()); - actions.put(actionLookupData, action); + memoizingEvaluator.delete( + key -> { + if (key.functionName().equals(SkyFunctions.ACTION_EXECUTION)) { + try { + var value = memoizingEvaluator.getExistingValue(key); + return value instanceof ActionExecutionValue + && isRemote((ActionExecutionValue) value); + } catch (InterruptedException ignored) { + return false; + } } - } - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); + return false; + }); + + if (actionCache != null) { + actionCache.removeIf( + entry -> !entry.getOutputFiles().isEmpty() || !entry.getOutputTrees().isEmpty()); } + } - if (!actions.isEmpty()) { - var actionKeys = actions.keySet(); - memoizingEvaluator.delete(key -> key instanceof ActionLookupData && actionKeys.contains(key)); + private boolean isRemote(ActionExecutionValue value) { + return value.getAllFileValues().values().stream().anyMatch(FileArtifactValue::isRemote) + || value.getAllTreeArtifactValues().values().stream().anyMatch(this::isRemoteTree); + } - if (actionCache != null) { - for (var action : actions.values()) { - ActionCacheUtils.removeCacheEntry(actionCache, action); - } - } - } + private boolean isRemoteTree(TreeArtifactValue treeArtifactValue) { + return treeArtifactValue.getChildValues().values().stream() + .anyMatch(FileArtifactValue::isRemote) + || treeArtifactValue + .getArchivedRepresentation() + .map(ar -> ar.archivedFileValue().isRemote()) + .orElse(false); } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 50f7df78317148..2eea1d7e4d81ad 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -73,6 +73,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import javax.annotation.Nullable; import org.junit.After; import org.junit.Before; @@ -1255,6 +1256,11 @@ public void remove(String key) { delegate.remove(key); } + @Override + public void removeIf(Predicate predicate) { + delegate.removeIf(predicate); + } + @Override public long save() throws IOException { return delegate.save(); diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionCacheTestHelper.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionCacheTestHelper.java index 93fd34f15b3960..ae8f8312b2b4f6 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionCacheTestHelper.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionCacheTestHelper.java @@ -17,6 +17,7 @@ import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; import java.io.PrintStream; +import java.util.function.Predicate; /** * Utilities for tests that use the action cache. @@ -38,6 +39,9 @@ public Entry get(String fingerprint) { @Override public void remove(String key) {} + @Override + public void removeIf(Predicate predicate) {} + @Override public long save() { return -1; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 8d481e9526e72f..b4d8cd57768353 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -580,6 +580,11 @@ public synchronized void remove(String key) { actionCache.remove(key); } + @Override + public void removeIf(java.util.function.Predicate predicate) { + actionCache.values().removeIf(predicate); + } + public synchronized void reset() { actionCache.clear(); }