Skip to content

Add support for REAPI execution from the sandbox#2014

Merged
juergbi merged 4 commits intomasterfrom
jbilleter/nested-reapi
Jun 14, 2025
Merged

Add support for REAPI execution from the sandbox#2014
juergbi merged 4 commits intomasterfrom
jbilleter/nested-reapi

Conversation

@juergbi
Copy link
Contributor

@juergbi juergbi commented May 23, 2025

Fixes #1985.

@juergbi juergbi force-pushed the jbilleter/nested-reapi branch 3 times, most recently from a045d16 to b52065e Compare May 23, 2025 16:15
@abderrahim
Copy link
Contributor

I tested this, and recc inside the sandbox is complaining

2025-05-27T08:22:40.382+0000 [218:140519756722176] [buildboxcommon_grpcretrier.cpp:134] [ERROR] ActionCache.GetActionResult() failed with: 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.383+0000 [218:140519756722176] [executioncontext.cpp:467] [ERROR] Error while querying action cache at "unix:/tmp/casd.sock": 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.385+0000 [218:140519756722176] [buildboxcommon_grpcretrier.cpp:134] [ERROR] Execution.Execute() failed with: 12: Execute: 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.386+0000 [218:140519756722176] [executioncontext.cpp:832] [ERROR] Error while calling `Execute()` on "unix:/tmp/casd.sock": 12: Execute: 12: Action Cache service is not enabled for instance ""

I dug a bit and it is due to the fact that I'm using a cache storage service. Disabling it works fine.

@gtristan
Copy link
Contributor

Generally like this patch overall... although it is still in draft, do we want this in 2.5 ?

Should we add an integration test for exercising recc ? Would be pretty cool if we had an example for this in the canonical format too ?

I'd be happy to add a tested example based on the recc demo to sweeten the pot if we think we're ready to go ahead with this...

@abderrahim
Copy link
Contributor

abderrahim commented May 29, 2025

I feel it's still not ready. While it's good enough, it depends on unreleased buildbox (buildbox was released earlier today, my bad) and there are a few corner cases such as #2014 (comment) above that need to be handled.

I'd say let's leave it till 2.6. I think that's also @juergbi's opinion as this is still marked as draft.

@juergbi
Copy link
Contributor Author

juergbi commented Jun 2, 2025

Leaving it till 2.6 is fine with me. I anyway expect typical use cases to require #1991 and that's also still a draft.

@juergbi juergbi force-pushed the jbilleter/nested-reapi branch from b52065e to 06e3efc Compare June 13, 2025 15:13
@juergbi
Copy link
Contributor Author

juergbi commented Jun 13, 2025

I dug a bit and it is due to the fact that I'm using a cache storage service. Disabling it works fine.

I've added a check for this to fail the build with a clear error message. In https://github.com/apache/buildstream/tree/jbilleter/action-cache-service, I'm adding support for action-cache-service, which then enables use of the REAPI socket with storage-service. That branch is not yet tested (or documented), though, and I think it's ok to first merge this and then the action-cache-service addition.

I've also added a minimal test to this branch.

CI is currently failing due to BuildBox being too old for the added test. #2023 updates the CI images based on https://gitlab.com/BuildStream/buildstream-docker-images/-/merge_requests/230.

@abderrahim
Copy link
Contributor

I'm adding support for action-cache-service, which then enables use of the REAPI socket with storage-service. That branch is not yet tested (or documented), though, and I think it's ok to first merge this and then the action-cache-service addition.

Yeah, this is fine. With the clear error message here, we can land this even if it doesn't support the cache storage-service.

I'll test the action-cache-service branch in the mean time

@juergbi juergbi force-pushed the jbilleter/nested-reapi branch from 06e3efc to a452797 Compare June 14, 2025 08:13
@juergbi juergbi marked this pull request as ready for review June 14, 2025 08:13
@juergbi juergbi force-pushed the jbilleter/nested-reapi branch from a452797 to c78a184 Compare June 14, 2025 08:52
_REQUIRED_CASD_MINOR = 0
_REQUIRED_CASD_MICRO = 58
_REQUIRED_CASD_MAJOR = 1
_REQUIRED_CASD_MINOR = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have some cleanups of fallback code paths now that we require buildbox 1.2. That shouldn't block this pull request, but it should be nice to have it done before the next release.

@juergbi juergbi merged commit d3c6448 into master Jun 14, 2025
17 checks passed
@juergbi juergbi deleted the jbilleter/nested-reapi branch June 14, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to expose remote execution services to the sandbox

3 participants