Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

AC: use a RootDirectoryDigest for directory assets #40

Merged
merged 2 commits into from
Jul 29, 2024
Merged

AC: use a RootDirectoryDigest for directory assets #40

merged 2 commits into from
Jul 29, 2024

Conversation

harrysarson
Copy link

Which means we avoid needing to calculate TreeDigest`s whilst still
creating Actions/ActionResults that encode that an asset is a directory.

Note: the bb-remote-asset server does not validate that the
RootDirectoryDigest actually references a Directory proto because
bb-remote-asset treats the digest as an opaque digest.

This superseeds #34 based on the plan I wrote here #34 (comment).

Closes #33, #34

@EdSchouten
Copy link
Member

As mentioned in #34 (comment), this is not supported on the bb-storage side, due to it not permitting ActionResults that only have root_directory_digest set.

@harrysarson
Copy link
Author

I guess to fix the blobs-that-unmarshall-as-directories-but-are-not bug whilst still setting tree_digest we would use the changes to the Asset proto and the if data.Type == asset.Asset_DIRECTORY { bit but otherwise leave pkg/storage/action_cache_asset_store.go as it is on master.

Uploading an digest that does not reference a valid directory with PushDirectory would still fail (which maybe okay) and we action_cache_asset_store would still do the conversion from Directory to Tree.

@harrysarson
Copy link
Author

harrysarson commented Jul 16, 2024 •
edited
Loading

See the latest commit which adds back the TreeDigest logic

@harrysarson harrysarson force-pushed the harry/ac-dir-semantics branch 2 times, most recently from eb1f2ba to cdfd4e0 Compare July 17, 2024 10:03
tomcoldrick-ct
tomcoldrick-ct reviewed Jul 19, 2024
View reviewed changes
Copy link
Collaborator

@tomcoldrick-ct tomcoldrick-ct left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you squash the history down a little please? You should probably drop my commit altogether since this is essentially solving the problem entirely differently and just adds churn to the history.

pkg/storage/action_cache_asset_store.go Outdated Show resolved Hide resolved
@harrysarson harrysarson force-pushed the harry/ac-dir-semantics branch 4 times, most recently from 064a7d9 to fd11132 Compare July 25, 2024 07:47
@tomcoldrick-ct tomcoldrick-ct mentioned this pull request Jul 29, 2024
Closed
tomcoldrick-ct
tomcoldrick-ct reviewed Jul 29, 2024
View reviewed changes
Copy link
Collaborator

@tomcoldrick-ct tomcoldrick-ct left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably change the author on the first commit -- it should be you! Other than that, this looks good to me.

@harrysarson
AC: Use RootDirectoryDigest for Directories
08705e5 
bb-remote-asset does some cleverness to pop `Directory` objects into the
OutputDirectories field of the ActionResult for semantic purposes. This
commit cleans up that cleverness to allow blobs that look a bit like
directories to be uploaded as asset.

Prior to this commit we detected whether an object is a `Directory` by
fetching it from CAS and attempting to unmarshal it into a `Directory`
message. This commit requires all assets uploaded via `PushDirectory`
are `Directories` (and treats assets uploaded via `PushBlob` as blobs).

We still must convert Directories to a Tree to generate the `TreeDigest`
needed by the `ActionResult`, so the action cache asset store will fetch
from the CAS when putting a new directory. (The ActionResult needs a
`TreeDigest` to satisfy bb-storage's completeness checking.)

If the client decides to use `PushDirectory` to push something that
isn't a `Directory` (which isn't explicitly banned by the spec), then we
will
error. The client will need to use `PushBlob` for these assets instead.

Fixes buildbarn#33
Closes buildbarn#34
@harrysarson
give more info in caching_fetcher error messages
5f8b6c1
@harrysarson
Copy link
Author

You should probably change the author on the first commit

Done!

@tomcoldrick-ct tomcoldrick-ct merged commit df6e113 into buildbarn:master Jul 29, 2024
1 check passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@tomcoldrick-ct tomcoldrick-ct tomcoldrick-ct approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PushBlob broken on blobs that unmarshal as Directorys using the Action Cache asset store
3 participants
@harrysarson @EdSchouten @tomcoldrick-ct