Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. #14252

coeuvre · 2021-11-10T07:06:20Z

When use BwtB, intermediate outputs are not downloaded. If a following build disables remote cache, a "file not found" error will be thrown if an action doesn't have its inputs downloaded in previous build because those files cannot be downloaded in this build.

This change fix this issue by:

Do not load remote metadata from action cache if --experimental_action_cache_store_output_metadata is set and remote cache is disabled.
Invalidate action nodes if previous build use BwtB and remote cache is changed from enabled to disabled.

Fixes #13882.

…enabled to disabled.

alexjski

I would recommend splitting that into 2 logically separate changes as in the description.

alexjski · 2021-11-10T17:04:05Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  # See https://github.com/bazelbuild/bazel/issues/13882.
+
+  mkdir -p a
+  cat > a/BUILD <<EOF


nit: you can use EOF and not need to escape the $ in the content.

alexjski · 2021-11-10T17:04:30Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  # Test that BwtB does cause build failure if remote cache is disabled in a following build.
+  # See https://github.com/bazelbuild/bazel/issues/13882.
+
+  mkdir -p a


supernit: -p not needed in this case

alexjski · 2021-11-10T17:05:07Z

src/test/shell/bazel/remote/remote_execution_test.sh

+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to populate the cache"
+
+  bazel clean || fail "Failed to clean"


super nit: bazel clean >& "${TEST_log}" || fail ...

alexjski · 2021-11-10T17:10:33Z

src/test/shell/bazel/remote/remote_execution_test.sh

+    --remote_download_toplevel \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to download outputs without remote metadata"
+  (! [[ -f bazel-bin/a/a.txt ]] && ! [[ -f bazel-bin/a/b.txt ]]) \


nit: can we reverse that to [[ -f bazel-bin/a/a.txt ]] || [[ -f bazel-bin/a/b.txt ]] && fail ...?

alexjski · 2021-11-10T17:14:18Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  (! [[ -f bazel-bin/a/a.txt ]] && ! [[ -f bazel-bin/a/b.txt ]]) \
+  || fail "Expected outputs of producer are not downloaded without remote metadata"
+
+  # build without remote cache without remote metadata


Is this the build which fails without your change? The reason I am asking is because this test may be merging 2 test cases in here. In general, it is a little hard to follow what is arrange-act-assert in here.

Meta-comment -- is there a way to run the remote tests in a Java test using BuildIntegrationTestCase? In those, it is much easier to parameterize etc.

Split into 2 test cases.

Meta-comment -- is there a way to run the remote tests in a Java test using BuildIntegrationTestCase? In those, it is much easier to parameterize etc.

Not sure. These remote tests run a remote worker process which is a real remote server. Do we prefer BuildIntegrationTestCase over shell in general? If so, I can try to make it work with remote tests and write new tests there in future PRs.

We generally do. I wouldn't tell you to rewrite these tests, but you may want to prefer Java-based ones when applicable in the future. Sent you a document talking about that offline.

coeuvre

Thanks for the comments. I will split this PR when importing.

coeuvre · 2021-11-11T06:06:25Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  # Test that BwtB does cause build failure if remote cache is disabled in a following build.
+  # See https://github.com/bazelbuild/bazel/issues/13882.
+
+  mkdir -p a


coeuvre · 2021-11-11T06:06:32Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  # See https://github.com/bazelbuild/bazel/issues/13882.
+
+  mkdir -p a
+  cat > a/BUILD <<EOF


coeuvre · 2021-11-11T06:06:39Z

src/test/shell/bazel/remote/remote_execution_test.sh

+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to populate the cache"
+
+  bazel clean || fail "Failed to clean"


coeuvre · 2021-11-11T06:18:55Z

src/test/shell/bazel/remote/remote_execution_test.sh

+    --remote_download_toplevel \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to download outputs without remote metadata"
+  (! [[ -f bazel-bin/a/a.txt ]] && ! [[ -f bazel-bin/a/b.txt ]]) \


coeuvre · 2021-11-11T06:25:08Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  (! [[ -f bazel-bin/a/a.txt ]] && ! [[ -f bazel-bin/a/b.txt ]]) \
+  || fail "Expected outputs of producer are not downloaded without remote metadata"
+
+  # build without remote cache without remote metadata


Split into 2 test cases.

Meta-comment -- is there a way to run the remote tests in a Java test using BuildIntegrationTestCase? In those, it is much easier to parameterize etc.

Not sure. These remote tests run a remote worker process which is a real remote server. Do we prefer BuildIntegrationTestCase over shell in general? If so, I can try to make it work with remote tests and write new tests there in future PRs.

alexjski · 2021-11-11T15:43:02Z

src/test/shell/bazel/remote/remote_execution_test.sh


-  bazel clean || fail "Failed to clean"
+function test_download_toplevel_when_turn_remote_cache_off_with_metadata() {


These 2 test cases differ by 1 option only -- why not put the test case as a separate helper function and call that twice with different parameters (Bash tests parameterization). Example here.

alexjski · 2021-11-11T15:43:25Z

src/test/shell/bazel/remote/remote_execution_test.sh


-  # download top level outputs without remote metadata
+  # download top level outputs
  bazel build \
    --remote_cache=grpc://localhost:${worker_port} \
    --remote_download_toplevel \
    --verbose_failures \


supernit: --verbose_failures is unlikely needed/we could add that to the blazerc for the whole test if that aids debugging failures.

alexjski · 2021-11-11T15:45:01Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  (! [[ -f bazel-bin/a/a.txt ]] && ! [[ -f bazel-bin/a/b.txt ]]) \
+  || fail "Expected outputs of producer are not downloaded without remote metadata"
+
+  # build without remote cache without remote metadata


We generally do. I wouldn't tell you to rewrite these tests, but you may want to prefer Java-based ones when applicable in the future. Sent you a document talking about that offline.

alexjski · 2021-11-15T15:54:52Z

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -3286,8 +3250,7 @@ EOF
  bazel build \
    --remote_cache=grpc://localhost:${worker_port} \
    --remote_download_toplevel \
-    --experimental_action_cache_store_output_metadata \
-    --verbose_failures \
+    $extra_build_flags \


I suspect bash linter may not like that -- your best bet may be to use the "$@" array directly or declare and use extra_build_flags as an array and use like that (personally, I recommend "$@"):

local -r extra_build_flags=("$@") ... "${extra_build_flags[@]}"

…he is changed from enabled to disabled. Part of #14252. PiperOrigin-RevId: 410243297

…he is changed from enabled to disabled. Part of bazelbuild#14252. PiperOrigin-RevId: 410243297

…is disabled. Part of bazelbuild#14252. Closes bazelbuild#14252. PiperOrigin-RevId: 410448656

… from enabled to disabled. (#14321) * Remote: Invalidate actions if previous build used BwoB and remote cache is changed from enabled to disabled. Part of #14252. PiperOrigin-RevId: 410243297 * In `ArchivedTreeArtifact`, make the assumption that the relative output path is always a single segment, and use this to serialize less data. This assumption holds because the origin of the relative output path (i.e. `bazel-out`) is `BlazeDirectories#getRelativeOutputPath`, which always returns a single-segment string. Instead of passing that around, just extract it from the tree artifact's exec path. Additionally, the public `ArchivedTreeArtifact#create` method is removed in order to enforce a consistent naming pattern for all instances. The codec supports custom derived tree roots even though there is currently no such serialization in skyframe (all serialized instances have the default `:archived_tree_artifacts`, but it was easy enough to be flexible). PiperOrigin-RevId: 406382340 * Remote: Don't load remote metadata from action cache if remote cache is disabled. Part of #14252. Closes #14252. PiperOrigin-RevId: 410448656 Co-authored-by: jhorvitz <jhorvitz@google.com>

Remote: Fix "file not found" error when remote cache is changed from …

27eb756

…enabled to disabled.

coeuvre requested a review from a team as a code owner November 10, 2021 07:06

google-cla bot added the cla: yes label Nov 10, 2021

coeuvre requested review from philwo and alexjski November 10, 2021 07:22

alexjski suggested changes Nov 10, 2021

View reviewed changes

coeuvre commented Nov 11, 2021

View reviewed changes

coeuvre requested a review from alexjski November 11, 2021 06:26

Address review comments.

6147342

alexjski approved these changes Nov 11, 2021

View reviewed changes

Address review comments

32c6363

alexjski approved these changes Nov 15, 2021

View reviewed changes

Address review comments

1f2fd85

alexjski approved these changes Nov 16, 2021

View reviewed changes

bazel-io pushed a commit that referenced this pull request Nov 16, 2021

Remote: Invalidate actions if previous build used BwoB and remote cac…

ed68933

…he is changed from enabled to disabled. Part of #14252. PiperOrigin-RevId: 410243297

bazel-io closed this in c9b7e22 Nov 17, 2021

coeuvre added a commit to coeuvre/bazel that referenced this pull request Nov 24, 2021

Remote: Invalidate actions if previous build used BwoB and remote cac…

8ff02e1

…he is changed from enabled to disabled. Part of bazelbuild#14252. PiperOrigin-RevId: 410243297

coeuvre added a commit to coeuvre/bazel that referenced this pull request Nov 24, 2021

Remote: Don't load remote metadata from action cache if remote cache …

f927113

…is disabled. Part of bazelbuild#14252. Closes bazelbuild#14252. PiperOrigin-RevId: 410448656

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. #14252

Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. #14252

coeuvre commented Nov 10, 2021

alexjski left a comment

alexjski Nov 10, 2021

coeuvre Nov 11, 2021

alexjski Nov 10, 2021

coeuvre Nov 11, 2021

alexjski Nov 10, 2021

coeuvre Nov 11, 2021

alexjski Nov 10, 2021

coeuvre Nov 11, 2021

alexjski Nov 10, 2021

coeuvre Nov 11, 2021

alexjski Nov 11, 2021

coeuvre left a comment

coeuvre Nov 11, 2021

coeuvre Nov 11, 2021

coeuvre Nov 11, 2021

coeuvre Nov 11, 2021

coeuvre Nov 11, 2021

alexjski Nov 11, 2021

coeuvre Nov 15, 2021

alexjski Nov 11, 2021

coeuvre Nov 15, 2021

alexjski Nov 11, 2021

alexjski Nov 15, 2021


		bazel clean \|\| fail "Failed to clean"
		function test_download_toplevel_when_turn_remote_cache_off_with_metadata() {

Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. #14252

Remote: Fix "file not found" error when remote cache is changed from enabled to disabled. #14252

Conversation

coeuvre commented Nov 10, 2021

alexjski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coeuvre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment