Remote cache + disk cache requests incorrect action keys

[keith]

Description of the problem / feature request:

Using bazel 0.25.0rc2 with the new remote + disk cache feature #7512 the cache requests to the remote server are prefixed with ac_ when they should be the sha of the object being requested.

If the remote cache you're using validates this, it can result in a cache miss when it shouldn't.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

bazel build some-target --remote_http_cache=http://some.remote.cache --disk_cache=/some/disk/cache

Where the remote cache is https://github.com/buchgr/bazel-remote

In this case you get many warnings:

WARNING: Reading from Remote Cache:
400 Bad Request
Resource name must be a SHA256 hash in hex. Got '/cas/ac_2d1692c6da631a61480bd32a3d8ab8234bf9e5dcd3c3cfa78d388578e2a8b3cf'.

Since the cache is validating the format of the keys.

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 0.25.0rc2

Have you found anything relevant by searching the web?

The warning is coming from this logic: https://github.com/buchgr/bazel-remote/blob/8811a77bfd9f8a35e4c23e758c9c585edb76283d/server/http.go#L62-L67

[keith]

cc @aherrmann @buchgr

[aherrmann]

The OnDiskBlobStore does prepend the ac_ prefix for action cache items, while the http cache does not. Because of this difference in behavior, the CombinedDiskHttpBlobStore calls getActionResult separately on the on-disk or http blob store for action cache items. However, the OnDiskBlobStore's getActionResult calls get, which ends up being the get from CombinedDiskHttpBlobStore. This way, the http cache receives a request for the ac_ prefixed item. So, this is a bug in #7512.

@buchgr I think one possible fix would be to factor out the file access logic in OnDiskBlobStore.get into, say, OnDiskBlobStore.getFile, and have OnDiskBlobStore.getActionResult call that getFile. What do you think of that approach?

[keith]

@buchgr any thoughts here? It would be nice if we could get a fix in for 0.26.0 if possible

[buchgr]

@ishikhman can you take care of this please?

[ishikhman]

@ishikhman can you take care of this please?

yep, will have a look

[ishikhman]

@aherrmann yep, that's a bug in Combined cache.

I'd suggest another approach to fix this. CombinedDiskHttpBlobStore extends OnDiskBlobStore and uses HttpBlobStore, while logically CombinedDiskHttpBlobStore is a composition of both, but not an extension of one of them. Combined Store should use composition instead of inheritance and delegate operation to one of the two Stores based on the logic. This refactoring would automatically fix the bug and will make behaviour more controllable and transparent.

I'll create a PR in the next days.

[buchgr]

I'd suggest another approach to fix this. CombinedDiskHttpBlobStore extends OnDiskBlobStore and uses HttpBlobStore, while logically CombinedDiskHttpBlobStore is a composition of both, but not an extension of one of them

That's problematic because it would require us to do two copy operations instead of a move. I think instead the simplification that would make the most sense is to merge OnDiskBlobStore and CombinedDiskHttpBlobStore into one class and just support bsHttp to be null. That way we can keep it efficient and neither need composition nor inheritance. WDYT?

[aherrmann]

@ishikhman Thanks for looking into this. For details on why the implementation uses inheritance instead of composition see #7512 (review) #7512 (comment) #7512 (comment)

[ishikhman]

@buchgr @aherrmann thanks for getting me up-to-date with the implementation details!

I see what you mean, but I still believe that composition can be easily used here - we do not have to delegate everything, right? So we could keep almost everything that has been implemented initially, just changing the call chain a bit. Let me show you the draft: #8141

[ishikhman]

Just for the future references:
the bug is not only in the way we get the cached results, but also in how we put them. HttpBlobStore does not add a prefix to a key for an action result, but it does store and reads it differently (ac vs cas).

[ishikhman]

FYI: the changes are in master and will be included into 0.27

[dslomov]

Do we need a patch release for 0.25 as well?

[buchgr]

No we don't. This is not a regression as it never worked before :(. *Von: *Dmitry Lomov <[email protected]> *Datum: *Do., 9. Mai 2019, 14:24 *An: *bazelbuild/bazel *Cc: *Jakob Buchgraber, Mention Do we need a patch release for 0.25

…

<#7498> as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8052 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKS5E3DEFVBAXFAR4BSXPTPUQJZHANCNFSM4HGDU54A> .

[keith]

@ishikhman did this change also fix the issue mentioned somewhere else where you had to have --remote_upload_local_results=true to write to the disk cache?

[keith]

FYI this was cherry picked #7499 (comment)

[ishikhman]

@ishikhman did this change also fix the issue mentioned somewhere else where you had to have --remote_upload_local_results=true to write to the disk cache?

@keith if you mean #8216 then unfortunately no. Is that the issue you were referring to?

[keith]

Ah yes. Thanks for the link.

…

-- Keith Smiley

On May 9, 2019, at 08:20, Ira Shikhman ***@***.***> wrote: @ishikhman did this change also fix the issue mentioned somewhere else where you had to have --remote_upload_local_results=true to write to the disk cache? @keith if you mean #8216 then unfortunately no. Is that the issue you were referring to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.