Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

WIP: RoutingBlobAccess #196

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

minor-fixes
Copy link
Contributor

This change adds a config option for RoutingBlobAccess, which is
intended to hold all possible ways to rewrite instance names.

While it is possible to affect instance name by (ab)using
DemultiplexingBlobAccess, there are some sharp edges (e.g. using it to
add an instance name may not work as expected, as not only must the
instance name be valid on the inner blobstore, but it must be valid once
returned upwards as well). RoutingBlobAccess will attempt to fix that
by offering easier-to-use semantics around instance name rewriting.

Tested: builds only, never ran


This PR's current purpose is to:

  • give me an opportunity to familiarize myself with the codebase (OK if this is a bad idea and should die here - I learned things)
  • validate the proposal of adding a BlobAccess wrapper with these semantics (vs. e.g. bug fixes to DemultiplexingBlobAccess)
    • I still need to go back and collect specific details/minimal repro for issues found using instance renaming with DemultiplexingBlobAccess, to better make the case for this
  • shed light on any misguided assumptions I have about how digests/instance names propagate through various backends

This change adds a `.bazelversion` file which allows `bazelisk` users to
use the expected version of `bazel` when building. The version of bazel
specified matches that of the one used in Github Actions currently.

This is a small quality-of-life improvement, as otherwise the default of
"latest release" will download bazel 7, fail to build, and create
bzlmod-related files in the process.
This change adds a config option for `RoutingBlobAccess`, which is
intended to hold all possible ways to rewrite instance names.

While it is possible to affect instance name by (ab)using
`DemultiplexingBlobAccess`, there are some sharp edges (e.g. using it to
add an instance name may not work as expected, as not only must the
instance name be valid on the inner blobstore, but it must be valid once
returned upwards as well). `RoutingBlobAccess` will attempt to fix that
by offering easier-to-use semantics around instance name rewriting.
@minor-fixes minor-fixes marked this pull request as draft March 11, 2024 04:01
@EdSchouten
Copy link
Member

using it to add an instance name may not work as expected, as not only must the instance name be valid on the inner blobstore, but it must be valid once returned upwards as well

This I don't understand. Are you saying that this currently does not hold? Namely, that BlobAccess.FindMissing() returns digests that can't be translated back to the original instance name? If that's the case, then we should figure out why that is happening.

Copy link
Member

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

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

I don't think we should gain yet another backend for this. Can't the same be achieved using the existing DemultiplexedBlobAccess? If not, can't it be improved to support this?

From a configuration perspective, I guess all we'd need to change is something like this:

diff --git a/src/bb-storage/pkg/proto/configuration/blobstore/blobstore.proto b/src/bb-storage/pkg/proto/configuration/blobstore/blobstore.proto
index 6a1dfd76..671699c3 100644
--- a/src/bb-storage/pkg/proto/configuration/blobstore/blobstore.proto
+++ b/src/bb-storage/pkg/proto/configuration/blobstore/blobstore.proto
@@ -853,9 +853,15 @@ message DemultiplexedBlobAccessConfiguration {
   // The backend to which requests are forwarded.
   BlobAccessConfiguration backend = 1;
 
-  // Add a prefix to the instance name of all requests forwarded to this
-  // backend.
-  string add_instance_name_prefix = 2;
+  oneof translation {
+    // Add a prefix to the instance name of all requests forwarded to
+    // this backend.
+    string add_instance_name_prefix = 2;
+
+    // Set the instance name to an exact value for all requests
+    // forwarded to this backend.
+    string set_instance_name = 3;
+  }
 }
 
 message ActionResultExpiringBlobAccessConfiguration {

I guess that if we add something like this, we should also add symmetry on the remote execution side?

diff --git a/src/bb-storage/pkg/proto/configuration/builder/builder.proto b/src/bb-storage/pkg/proto/configuration/builder/builder.proto
index a5170bac..88779fe1 100644
--- a/src/bb-storage/pkg/proto/configuration/builder/builder.proto
+++ b/src/bb-storage/pkg/proto/configuration/builder/builder.proto
@@ -15,10 +15,15 @@ message SchedulerConfiguration {
   // The gRPC endpoint at which the scheduler can be reached.
   buildbarn.configuration.grpc.ClientConfiguration endpoint = 1;
 
-  // Add a prefix to the instance name of all requests forwarded to this
-  // scheduler. By default, the prefix that was used to match the
-  // request against a scheduler is stripped from the instance name.
-  // This option can be used to re-add that prefix in case perfect
-  // forwarding is necessary.
-  string add_instance_name_prefix = 2;
+  oneof translation  {
+    // Add a prefix to the instance name of all requests forwarded to
+    // this scheduler. By default, the prefix that was used to match the
+    // request against a scheduler is stripped from the instance name.
+    // This option can be used to re-add that prefix in case perfect
+    // forwarding is necessary.
+    string add_instance_name_prefix = 2;
+
+    // ...
+    string set_instance_name = 3;
+  }
 }

Once we start doing that, maybe smarter to factor this out into a common message?

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.

2 participants