@philwo @janakdr @alexjski PTAL

Another thought: we can probably move "load metadata cache" and "update metadata cache" into OutputService and separate MetadataCache from ActionCache so that this change only affects Bazel's remote execution/cache.

General idea seems reasonable, will defer to @alexjski

Can you explain why we need separate files for the metadata as opposed to storing that with the existing structure? That could technically open possibilities of entries present in the metadata, but not in the cache and the other way round. Is the worry that we could read the metadata in case of a cache miss?

They are technically different type of maps: the action cache is a map of <Path of action's first output, Digest of action data> while the metadata cache is a map of <Path of a output file, Metadata>. Storing them into different files seems to be reasonable.

That could technically open possibilities of entries present in the metadata, but not in the cache and the other way round.

I think this is a valid case. If entries present in the metadata but not in the cache, Bazel will just ignore them. If the other way round, it is just the case before this change.

Is the worry that we could read the metadata in case of a cache miss?

Even if we store metadata into action cache files, a cache miss shouldn't remove the metadata.

However, we can change to store the metadata to the existing action cache file (by suffix the path of output file with :metadata for example) if you prefer this way.

Updated to merge two maps into one. For metadata of an output, the key is its execPath plus a suffix :metadata.

Updated to merge two maps into one. For metadata of an output, the key is its execPath plus a suffix :metadata.

That's good. I think I initially had more negative reaction to separate maps than it calls for (sorry) because I didn't realize the output fingerprint check is still present, which negates all of the risks. Anyway with merged map, we know they both are always in sink which is nice thing in itself.

Regarding #13604 (comment), I am thinking separating these metadata methods from ActionCache and let OutputService do the job (that's why it initially has a MetadataCache interface).

The goal of this PR is allowing Bazel to download remote outputs after action execution. To do that, we need:

1. Download metadata of remote outputs and save them during action execution.
2. When the remote output is downloaded, the remote metadata should be removed from cache (so we can re-construct metadata from local file system).

1 is implemented - during the step when we update action cache, we also check for metadata injected during action execution, and save those which is "remote".

2 is only implemented for downloads happened within action execution - if the metadata injected is not "remote", we remove the key from cache during updating the action cache.

Considering the case that the local action cache of a remote action is valid (hence action execution is skipped) but we need to download its outputs. With current implementation, it won't work.

We need a post action execution download procedure. Whichever module implements this feature, it needs to access ActionCache to remove metadata for downloaded outputs. Accessing ActionCache somewhere else is risky IMO.

If we move these metadata methods to the OutputService i.e. it is responsible for maintaining the metadata, then we will have following steps when executing an action:

1. Load metadata from OutputService (if it is not null) into OutputStore (this is essentially constructing an action-local metadata cache from a global one).
2. Check the ActionCache with OutputStore.
3. If cache is invalidated, execute the action and update metadata with method provided by OutputService (merge the action-local metadata cache into a global one).
4. Call the post action download method provided by the OutputService during which the metadata is updated by itself for downloaded outputs.

WDYT?

Regarding #13604 (comment), I am thinking separating these metadata methods from ActionCache and let OutputService do the job (that's why it initially has a MetadataCache interface).

The goal of this PR is allowing Bazel to download remote outputs after action execution. To do that, we need:

1. Download metadata of remote outputs and save them during action execution.
2. When the remote output is downloaded, the remote metadata should be removed from cache (so we can re-construct metadata from local file system).

1 is implemented - during the step when we update action cache, we also check for metadata injected during action execution, and save those which is "remote".

2 is only implemented for downloads happened within action execution - if the metadata injected is not "remote", we remove the key from cache during updating the action cache.

That may be a reason for 2 maps, so you can still have cache hits for local actions.

Considering the case that the local action cache of a remote action is valid (hence action execution is skipped) but we need to download its outputs. With current implementation, it won't work.

We need a post action execution download procedure. Whichever module implements this feature, it needs to access ActionCache to remove metadata for downloaded outputs. Accessing ActionCache somewhere else is risky IMO.

If we move these metadata methods to the OutputService i.e. it is responsible for maintaining the metadata, then we will have following steps when executing an action:

1. Load metadata from OutputService (if it is not null) into OutputStore (this is essentially constructing an action-local metadata cache from a global one).
2. Check the ActionCache with OutputStore.
3. If cache is invalidated, execute the action and update metadata with method provided by OutputService (merge the action-local metadata cache into a global one).
4. Call the post action download method provided by the OutputService during which the metadata is updated by itself for downloaded outputs.

WDYT?

My very first reaction is that I think we discussed that before and that was not agreed upon. I see 2 reasons to need to download the outputs:

1. Follow-up local actions.
2. Allow users to inspect the outptus.

Seems like that if you need those, your best bet may be a FUSE-based solution which tracks all outputs which would deem all of these changes unnecessary (see #12823).

Another thing to consider about downloading results, if you wanted to do that eagerly (I think if we can have FUSE, that would be strictly better) is a subscribing to events and looking at outputs. We already issue those for top-level actions (BEP), there also is CachedActionEvent (you might need to add the metadata to it though). I am not pressing too hard, but that is something to think about -- it looks like we may already have the desired interface there. For reference, we already look at the outputs right there:

This PR as which only tracks the metadata in action cache, to me can serve an interesting case for remote-only builds, which is something that we discussed before and it is indeed a nice thing to have.

My very first reaction is that I think we discussed that before and that was not agreed upon. I see 2 reasons to need to download the outputs:

1. Follow-up local actions.
2. Allow users to inspect the outptus.

Another case is we can just inject metadata for the outputs of an action and start executing dependent actions while keeping output downloads in the background (today, we have to wait for the download).

Seems like that if you need those, your best bet may be a FUSE-based solution which tracks all outputs which would deem all of these changes unnecessary (see #12823).

IIUC, we are experimenting with non-FUSE-based solution. The solution in my head is we can build artifacts as normal and use "outputs" flags to tell Bazel which outputs we want to download, e.g. no flags means we want all the outputs, --remote_download_minimal means we want a minimal set of outputs (we can also introduce additional "outputs" flags to tell Bazel in a more fine grained level).

If we need outputs that weren't downloaded during previous build, we can then "rebuild" with different "outputs" flags which will hit the local action cache but start downloading missing outputs.

The entire progress for executing an action remotely should be:

1. Check for local action cache. If hit, go to step 6.
2. Remove injected metadata and local outputs if any.
3. Check for remote cache. If hit, go to step 5.
4. Upload inputs and execute.
5. Download and inject metadata.
6. Only download outputs which is requested by the "outputs" flags in the background (remove injected metadata after download).
7. If there are more actions, go to step 1.
8. Wait for all downloads.

This works without FUSE but provides similar experience.

Another thing to consider about downloading results, if you wanted to do that eagerly (I think if we can have FUSE, that would be strictly better) is a subscribing to events and looking at outputs. We already issue those for top-level actions (BEP), there also is CachedActionEvent (you might need to add the metadata to it though). I am not pressing too hard, but that is something to think about -- it looks like we may already have the desired interface there. For reference, we already look at the outputs right there:

bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

Lines 657 to 664 in 4ccb2cc

	checkOutputs(
	action,
	metadataHandler,
	/*filesetOutputSymlinksForMetrics=*/ null,
	/*isActionCacheHitForMetrics=*/ true);
	if (!eventPosted) {
	eventHandler.post(new CachedActionEvent(action, actionStartTime));
	}

.

This is also a good place to consider for post execution downloading. One thing I mentioned above is that we need to remove remote metadata from cache after download. So we need to decide whether should we expose ActionCache to other modules or we keep ActionCache as it is and introduce a separate interface for metadata only.

This PR as which only tracks the metadata in action cache, to me can serve an interesting case for remote-only builds, which is something that we discussed before and it is indeed a nice thing to have.

Just chiming in, but macFUSE performance isn't the best, so any solution that requires FUSE might not be performant on macOS.

To be clear, I wasn't meant to replace FUSE-based (or any filesystem-based output service) solution but just wanted to improve "build withouts bytes".

Chi, what is the reason to avoid such a setup and centralize logic inside Bazel?

IIUC, we don't offer the setup for Bazel today. For users who care about network bandwidth, the only choice is "build without bytes". The ways moving forward are probably:

1. Improve "build without bytes".
2. Add support for FUSE like Remote Output Service: place bazel-out/ on a FUSE file system #12823.

I don't think these two ways are incompatible with each other.

I would like to go ahead with Chi's idea of having this logic in Bazel.

I think filesystem-based layers are probably the most user-friendly interface when they work, but they come with lots of challenges, too: They are platform-specific, they are often slower than non-filesystem-based APIs (e.g. see sandboxfs performance vs symlink-based sandbox performance), they require additional setup (e.g. install FUSE), they require having special privileges on the system (admin rights, ability to load a kernel extension, ability to mount a filesystem, ...) and they might not be available on all systems due to security policies. As we know from Google they can perform very well in a carefully managed IT environment, but I don't believe we will be able to provide a good out of the box experience for these solutions on typical user systems.

The two approaches should nicely complement each other though!

I would like to go ahead with Chi's idea of having this logic in Bazel.

I think filesystem-based layers are probably the most user-friendly interface when they work, but they come with lots of challenges, too: They are platform-specific, they are often slower than non-filesystem-based APIs (e.g. see sandboxfs performance vs symlink-based sandbox performance), they require additional setup (e.g. install FUSE), they require having special privileges on the system (admin rights, ability to load a kernel extension, ability to mount a filesystem, ...) and they might not be available on all systems due to security policies. As we know from Google they can perform very well in a carefully managed IT environment, but I don't believe we will be able to provide a good out of the box experience for these solutions on typical user systems.

The two approaches should nicely complement each other though!

There already was a discussion about that before and I also believe there was some effort put towards testing how NFS-based solutions perform. I think that in order to have most informed discussion, we should merge those and look at the results from the tests and them make design decisions. I believe that this code review may not be the best place for that.

The main thinking so far was that if we have a FUSE/NFS-based solution available, then the incentive of this may be low. What I was missing from the proposal before was the justification for why the added complexity is needed if we have FUSE/NFS solution. If there is none, I would strongly prefer to i

My very first reaction is that I think we discussed that before and that was not agreed upon. I see 2 reasons to need to download the outputs:

1. Follow-up local actions.
2. Allow users to inspect the outptus.

Another case is we can just inject metadata for the outputs of an action and start executing dependent actions while keeping output downloads in the background (today, we have to wait for the download).

Seems like that if you need those, your best bet may be a FUSE-based solution which tracks all outputs which would deem all of these changes unnecessary (see #12823).

IIUC, we are experimenting with non-FUSE-based solution. The solution in my head is we can build artifacts as normal and use "outputs" flags to tell Bazel which outputs we want to download, e.g. no flags means we want all the outputs, --remote_download_minimal means we want a minimal set of outputs (we can also introduce additional "outputs" flags to tell Bazel in a more fine grained level).

If we need outputs that weren't downloaded during previous build, we can then "rebuild" with different "outputs" flags which will hit the local action cache but start downloading missing outputs.

The entire progress for executing an action remotely should be:

1. Check for local action cache. If hit, go to step 6.
2. Remove injected metadata and local outputs if any.
3. Check for remote cache. If hit, go to step 5.
4. Upload inputs and execute.
5. Download and inject metadata.
6. Only download outputs which is requested by the "outputs" flags in the background (remove injected metadata after download).
7. If there are more actions, go to step 1.
8. Wait for all downloads.

This works without FUSE but provides similar experience.

Another thing to consider about downloading results, if you wanted to do that eagerly (I think if we can have FUSE, that would be strictly better) is a subscribing to events and looking at outputs. We already issue those for top-level actions (BEP), there also is CachedActionEvent (you might need to add the metadata to it though). I am not pressing too hard, but that is something to think about -- it looks like we may already have the desired interface there. For reference, we already look at the outputs right there:

bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

Lines 657 to 664 in 4ccb2cc

	checkOutputs(
	action,
	metadataHandler,
	/*filesetOutputSymlinksForMetrics=*/ null,
	/*isActionCacheHitForMetrics=*/ true);
	if (!eventPosted) {
	eventHandler.post(new CachedActionEvent(action, actionStartTime));
	}

.

This is also a good place to consider for post execution downloading. One thing I mentioned above is that we need to remove remote metadata from cache after download. So we need to decide whether should we expose ActionCache to other modules or we keep ActionCache as it is and introduce a separate interface for metadata only.

Why do we need to remove the metadata? Also, it may be useful what metadata you mean in this context -- the newly added metadata in action cache or the metadata stored in ActionExecutionValue (skyframe).

This PR as which only tracks the metadata in action cache, to me can serve an interesting case for remote-only builds, which is something that we discussed before and it is indeed a nice thing to have.

Why do we need to remove the metadata? Also, it may be useful what metadata you mean in this context -- the newly added metadata in action cache or the metadata stored in ActionExecutionValue (skyframe).

I was referring to the newly added metadata in action cache. This PR only save remote metadata and load them into OutputStore before checking the action cache. If we don't remove the remote metadata of one output from action cache after downloaded it, we will still use the remote metadata from action cache instead of re-calculating it from local file system next time when we check action cache (even if the local file content is changed).

Now I think it again, maybe we can change to storing all metadata of outputs into action cache (which helps #13566). Re-calculating all metadata of outputs for the action from local file system (we do it today as well) and compare them with those stored in the action cache. Then we don't need to remove metadata after downloads.

It looks like saving metadata directly in action cache is a preferred way. Then I would like to make it stored within one map by extending ActionCache.Entry.

Why do we need to remove the metadata? Also, it may be useful what metadata you mean in this context -- the newly added metadata in action cache or the metadata stored in ActionExecutionValue (skyframe).

I was referring to the newly added metadata in action cache. This PR only save remote metadata and load them into OutputStore before checking the action cache. If we don't remove the remote metadata of one output from action cache after downloaded it, we will still use the remote metadata from action cache instead of re-calculating it from local file system next time when we check action cache (even if the local file content is changed).

I agree, we need to take care of that. My thinking was that it may be a better API to just request a desired end state rather than have fine-grain manipulations. My preference would be to have an API like this:

Map<PathFragment, FileArtifactMetadata> newOutputs = ...;
cache.updateMetadata(key, newOutputs);

In fact, that could be a single call as part of updating the cache, which we already do.

over:

Map<PathFragment, FileArtifactMetadata> oldOutputs = ...;
Map<PathFragment, FileArtifactMetadata> newOutputs = ...;
oldOutputs.keySet().stream().filter(k -> !newOutputs.containsKey(k)).forEach(cache::removeMetadata);
newOutputs.forEach((k, m) -> cache.putMetadata(k, m);

In fact, then you can choose in the cache implementation to only track the remote ones and you don't have to worry about removing old entries since the operation resembles map = newMap vs manipulate(map).

Now I think it again, maybe we can change to storing all metadata of outputs into action cache (which helps #13566). Re-calculating all metadata of outputs for the action from local file system (we do it today as well) and compare them with those stored in the action cache. Then we don't need to remove metadata after downloads.

Not sure if we need that. My point was merely about the API seeming too fine grained. Logically it makes perfect sense.

It looks like saving metadata directly in action cache is a preferred way. Then I would like to make it stored within one map by extending ActionCache.Entry.

I agree, if we are doing that, the update(Map<...>) seems like the simplest choice.

Added a few tests. More are coming tomorrow.

Tests added. I think we can have another review round.