[6.2.0] Remove actionId from RemoteFileArtifactValue.

[tjgq]

actionId was added by #11236 to track download requests for input prefetches.

However, according to the REAPI spec, action_id is

An identifier that ties multiple requests to the same action. For example, multiple requests to the CAS, Action Cache, and Execution API are used in order to compile foo.cc.

The input prefetches are requested by local actions that depend on outputs of remote executed action (which is identified by the action_id). These requests are not part of generating action hence shouldn't be bound with the action_id.

We could include ActionExecutionMetadata of the local action for the requests for the tracking purpose, but that desires another CL.

PiperOrigin-RevId: 510430853
Change-Id: I8d64a4f7033320a394e02a2b11498f09a3d15310

[sluongng]

FYI @coeuvre this change broke a feature on our end that would enable our users to track the origin of the remote cache requests. It's unfortunate that it made into 6.2.0 release without the mentioned change

We could include ActionExecutionMetadata of the local action for the requests for the tracking purpose, but that desires another CL.

Could you please share the idea behind including ActionExecutionMetadata to the request? 🤔

[sluongng]

To clarify a bit further, our CAS and AC implementation use this action_id metadata to help correlate the request to the action / build target. This let our users to troubleshoot cache misses issue a lot easier by inspecting write requests' action_id.

Even if an action were to be executed locally, its result could still be cached remotely in AC and thus, could be used to correlate at the end. I can see this change matter for a very small subset of local actions where remote caching is disabled, but in those cases, I don't think there is much harm in just keeping the action ID as-is.

@coeuvre If possible, I would urge reverting this change for a 6.2.1 patch release 🙏

[coeuvre]

The idea is, instead of using action_id of generating action when fetch the blobs, we use action_id of local actions that need the blobs as inputs.

I tend to make the latter case work and cherrypick it into 6.2.1. Does that work for you?

[sluongng]

@coeuvre We continue to receive feedback from enterprise customers on how this change affected their troubleshooting of remote cache traffic negatively.
So I have been wanting to implement the change you mentioned yourself, but I am a bit hazy on the detail

On a highlevel, the interesting function is AbstractActionInputPrefetcher.doDownloadFile where the prefetcher value is hardcoded.

The call stack looks something like this:

# From master @ f75d14bcd92d537c03e88f1aba9c3fdc260e1463

90: buildMetadata(buildRequestId, commandId, "prefetcher", null)
src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
  81: doDownloadFile(reporter, tempPath, finalPath.relativeTo(execRoot), metadata, priority)
    src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
      447: downloadFileNoCheckRx(dirCtx, path, treeRoot, actionInput, metadata, priority)
        src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
          349: prefetchFile(dirCtx, metadataSupplier, input, priority)
            src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
              311: prefetchFiles(inputs, metadataSupplier, priority)

                src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
                  252: prefetchInputs()

                src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
                  605: finalizeAction(action, outputMetadataStore)

                src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
                  627: downloadFileIfRemote(path)

So it seems like we are not only using this code path to prefetch inputs for Spawns that are about
to be locally executed, we are also using it to fetch outputs of remotely executed actions that
was built without BwtB?

If that's true, I think the scope of impact of this patch is high and I would prefer either reverting
or a fix in the next minor release.

@coeuvre do you have any pointer on how to obtain the action_id for local Spawn, specifically for AbstractSpawnStrategy.prefetchInputs()?

Should we go through the same dance, as in RemoteExecutionService.java? There we turn a Spawn into an Action to compute the ActionKey and use that as actionId.

[sluongng]

I am taking a light crack at fixing this in #18625

Very much WIP.

[coeuvre]

I have a CL being review internally to fix this. Should be able to submit it next week.

[sluongng]

I will drop my terrible attempt 🙇
Thanks for looking into this.

[coeuvre]

Here you go: 3b39ab8

Before you ask, I will try to cherrypick it into 6.3.0.

[sluongng]

Ah ok, the fix was not what I expected initially:

• The action_id is still set to prefetcher in RequestMetadata

• Fields action_mnemonic, configuration_id and target_id are now set instead in RequestMetadata

This is much better than previously, we could conditionally check for prefetcher action_id to print more metadata to our users.

---

I could be wrong, but for AbstractActionInputPrefetcher.finalizeAction(), it's when we download outputs from the remote cache after execution. In such a case, I think there should be a way to propagate the action_id down there still, without having to use prefetcher.

[coeuvre]

Yep, for remote actions, we could find a way to propagate the action_id down to the prefetcher. But there is no action_id by definition for local actions that download remote outputs. Let me fix the former case.

[coeuvre]

I could be wrong, but for AbstractActionInputPrefetcher.finalizeAction(), it's when we download outputs from the remote cache after execution. In such a case, I think there should be a way to propagate the action_id down there still, without having to use prefetcher.

@tjgq is actually removing the finalizeAction and moving the downloads into spawn execution so these downloads will have the action_id automatically. However, that changes probably won't be able to be cherrypicked into 6.3.0.

Since you have to have a special case for "prefetcher" because of local actions, it might be fine to use it for remote actions that download their own outputs, for now?

[sluongng]

I think we could make do without that for now. Thanks so much for these fixes 🙇

[coeuvre]

The fix has been cherry-picked in #18656.