Teach the FilesystemValueChecker about remote outputs #7269

buchgr · 2019-01-28T12:22:08Z

The FilesystemValueChecker is used by Bazel to detect modified
outputs before a command. This change teaches it about remote
outputs that don't exist in the output base. That is if
SkyFrame has metadata about a remote output file which does not
exist in the output base it will not invalidate the output. However,
if the file exists in the output base it will be taken as the source
of truth.

Progress towards #6862

ericfelly · 2019-02-05T23:09:55Z

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java

    for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
+      isRemote = isRemote && e.getValue().isRemote();


it seems like things get strange when some files within the tree are remote and others non-remote. can you add some comments about when that would happen and the expected semantics?

Can that happen? A TreeArtifact can only ever be created by one piece by an action.

Can it maybe happen that only parts of it are materialized?

Sure, I ll add some comments (and checks maybe). I think it should be either remote or local and not a mix of both.

janakdr · 2019-02-05T23:13:14Z

Can you comment on the need for this? How often is the output tree modified inter-build? Inside google, it feels like we're moving towards a world where we implicitly assume the output tree is immutable.

ericfelly

this is unused code at this point, right? there's nothing in bazel codebase creating these artifact values with isRemote() == true.

janakdr

We should make sure to add integration tests internally.

src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java

lberki · 2019-02-06T12:20:26Z

src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java

@@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act
        try {
          ArtifactFileMetadata fileMetadata =
              ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
+          FileArtifactValue fileValue = actionValue.getArtifactValue(file);
+          boolean lastSeenRemotely = fileValue != null && fileValue.isRemote();


When is fileValue null here? Is my understanding correct that this is for cases when we don't actually need to call stat() to get the metadata of the output of an action?

Yes. IIRC fileValue here is none null only for injected metadata.

lberki · 2019-02-06T12:21:12Z

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java

    for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
+      isRemote = isRemote && e.getValue().isRemote();


Can that happen? A TreeArtifact can only ever be created by one piece by an action.

Can it maybe happen that only parts of it are materialized?

lberki · 2019-02-06T12:22:05Z

src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java

@@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act
        try {
          ArtifactFileMetadata fileMetadata =
              ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
+          FileArtifactValue fileValue = actionValue.getArtifactValue(file);
+          boolean lastSeenRemotely = fileValue != null && fileValue.isRemote();
+          if (!fileMetadata.exists() && lastSeenRemotely) {


What are the expected semantics when:

A file is created remotely

The file is later materialized

The file is then deleted by rm

Is Bazel expected to notice the deletion in that case?

Is Bazel expected to notice the deletion in that case?

It wouldn't notice it as an ActionExecutionValue is immutable and we can't and shouldn't modify it after action execution. It's a valid point you are raising though and I think there are two ways forwards:

Output files that are materialized after action execution will remain in the output base and Bazel will think that they are actually stored remotely and won't notice the scenario you are describing.

The remote module keeps track of all outputs it's materializing after action execution and deletes them again at the end of a build. That way Bazel's in memory state would match what's in the output base after the build.

buchgr · 2019-02-25T15:21:07Z

@janakdr

Apologies for the delay. I was out of office.

Can you comment on the need for this?

The need for this arrives from not download all build outputs for remote builds but instead injecting metadata about build outputs. In Bazel remote execution we'd like to have it so that some build outputs are downloaded locally (i.e. top level) and others are not and only live remotely.

How often is the output tree modified inter-build?

It's my understanding that this is not a supported scenario by Bazel aka undefined behavior. Why are you asking?

buchgr · 2019-02-25T15:22:17Z

this is unused code at this point, right? there's nothing in bazel codebase creating these artifact values with isRemote() == true.

yep. I broke this change out from a larger patch set. I ll be injecting RemoteArtifactValues in the remote module in a follow up change.

benjaminp · 2019-02-28T05:58:08Z

How often is the output tree modified inter-build?

It's my understanding that this is not a supported scenario by Bazel aka undefined behavior. Why are you asking?

Isn't this code you're modifying in FilesystemValueChecker there explicitly to support that? It stats the outputs of every action in Skyframe before every build to see if the in-memory state is out of sync with reality. This behavior is controlled by --experimental_check_output_files. It's also not fool-proof. For example, runfiles trees aren't tracked.

buchgr · 2019-02-28T10:36:34Z

@benjaminp you are right. I shall learn to read better. In my head I mapped inter-build to mean during the build.

@janakdr I wouldn't be able to quantify inter-build modifications to the output-tree. Ideally these don't happen, but as Benjamin correctly wrote it's hard a best effort mechanism that isn't fool proof.

The FilesystemValueChecker is used by Bazel to detect modified outputs before a command. This change teaches it about remote outputs that don't exist in the output base. That is if SkyFrame has metadata about a remote output file which does not exist in the output base it will not invalidate the output. However, if the file exists in the output base it will be taken as the source of truth. Progress towards bazelbuild#6862

googlebot added the cla: yes label Jan 28, 2019

buchgr requested review from ericfelly, lberki and janakdr February 5, 2019 14:14

buchgr force-pushed the teach-remote-outputs branch from 3af20a5 to a714e8d Compare February 5, 2019 14:15

ericfelly reviewed Feb 5, 2019

View reviewed changes

janakdr reviewed Feb 6, 2019

View reviewed changes

src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java Outdated Show resolved Hide resolved

lberki reviewed Feb 6, 2019

View reviewed changes

jin added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Feb 19, 2019

jin assigned ericfelly Feb 19, 2019

buchgr force-pushed the teach-remote-outputs branch from a714e8d to 481db5e Compare March 1, 2019 10:48

buchgr added 3 commits March 6, 2019 13:12

address comment

995751e

address comments

a38b6c2

buchgr force-pushed the teach-remote-outputs branch from 481db5e to a38b6c2 Compare March 6, 2019 12:12

bazel-io closed this in 0a0c962 Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teach the FilesystemValueChecker about remote outputs #7269

Teach the FilesystemValueChecker about remote outputs #7269

buchgr commented Jan 28, 2019 •

edited

Loading

ericfelly Feb 5, 2019

lberki Feb 6, 2019

buchgr Feb 25, 2019

buchgr Mar 4, 2019

janakdr commented Feb 5, 2019

ericfelly left a comment

janakdr left a comment

lberki Feb 6, 2019

buchgr Mar 1, 2019

lberki Feb 6, 2019

lberki Feb 6, 2019

buchgr Mar 1, 2019 •

edited

Loading

buchgr commented Feb 25, 2019 •

edited

Loading

buchgr commented Feb 25, 2019

benjaminp commented Feb 28, 2019

buchgr commented Feb 28, 2019

		for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
		isRemote = isRemote && e.getValue().isRemote();

Teach the FilesystemValueChecker about remote outputs #7269

Teach the FilesystemValueChecker about remote outputs #7269

Conversation

buchgr commented Jan 28, 2019 • edited Loading

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

janakdr commented Feb 5, 2019

ericfelly left a comment

Choose a reason for hiding this comment

janakdr 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

buchgr Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

buchgr commented Feb 25, 2019 • edited Loading

buchgr commented Feb 25, 2019

benjaminp commented Feb 28, 2019

buchgr commented Feb 28, 2019

buchgr commented Jan 28, 2019 •

edited

Loading

buchgr Mar 1, 2019 •

edited

Loading

buchgr commented Feb 25, 2019 •

edited

Loading