From c39fbd6735ff10876629a14e114696633b887180 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Tue, 16 Jan 2024 09:09:34 +1100 Subject: [PATCH] Add remote_provider = "..." option, replacing scheme-look-ups (#20240) This replaces the over-complicated address URI scheme-based look-up from #19827 for deciding on which provider to use with just a normal option `[GLOBAL].remote_provider`. This defaults to `reapi`, to preserve the current (stable) behaviour. That is, now, one can set: ```toml [GLOBAL] remote_provider = "experimental-file" remote_store_address = "file:///..." ``` Or similar. Another item in #19694 The commits are somewhat usefully broken up for review. --- .../remote-caching.mdx | 6 +- .../pants/engine/internals/scheduler.py | 6 +- src/python/pants/option/global_options.py | 342 +++++++----------- .../pants/option/global_options_test.py | 220 +++++------ src/rust/engine/Cargo.lock | 2 + src/rust/engine/fs/brfs/src/main.rs | 3 +- src/rust/engine/fs/fs_util/src/main.rs | 5 +- src/rust/engine/fs/store/src/lib.rs | 2 +- src/rust/engine/fs/store/src/remote_tests.rs | 4 +- src/rust/engine/fs/store/src/tests.rs | 5 +- .../remote/src/remote_cache_tests.rs | 5 +- .../remote/src/remote_tests.rs | 3 +- src/rust/engine/process_executor/src/main.rs | 6 +- .../src/action_cache_tests.rs | 3 +- .../src/byte_store_tests.rs | 3 +- .../src/action_cache_tests.rs | 3 +- .../src/byte_store_tests.rs | 3 +- .../remote_provider_traits/Cargo.toml | 2 + .../remote_provider_traits/src/lib.rs | 11 + src/rust/engine/remote_provider/src/lib.rs | 94 +++-- src/rust/engine/src/context.rs | 4 +- src/rust/engine/src/externs/interface.rs | 3 + 22 files changed, 353 insertions(+), 382 deletions(-) diff --git a/docs/docs/using-pants/remote-caching-&-execution/remote-caching.mdx b/docs/docs/using-pants/remote-caching-&-execution/remote-caching.mdx index 57a0a3dda9b..7a0e50f37e5 100644 --- a/docs/docs/using-pants/remote-caching-&-execution/remote-caching.mdx +++ b/docs/docs/using-pants/remote-caching-&-execution/remote-caching.mdx @@ -54,7 +54,7 @@ The values of the `ACTIONS_CACHE_URL` and `ACTIONS_RUNTIME_TOKEN` environment va uses: actions/github-script@v6 with: script: | - core.exportVariable('PANTS_REMOTE_STORE_ADDRESS', 'experimental:github-actions-cache+' + (process.env.ACTIONS_CACHE_URL || '')); + core.exportVariable('PANTS_REMOTE_STORE_ADDRESS', process.env.ACTIONS_CACHE_URL); core.exportVariable('PANTS_REMOTE_OAUTH_BEARER_TOKEN', process.env.ACTIONS_RUNTIME_TOKEN); ``` @@ -65,6 +65,7 @@ Once the GitHub values are configured, Pants will read the environment variables ```toml [GLOBAL] # GitHub Actions cache URL and token are set via environment variables +remote_provider = "experimental-github-actions-cache" remote_cache_read = true remote_cache_write = true ``` @@ -85,7 +86,8 @@ To read and write the cache to `/path/to/cache`, you will need to configure `pan ```toml [GLOBAL] -remote_store_address = "experimental:file:///path/to/cache" +remote_store_provider = "experimental-file" +remote_store_address = "file:///path/to/cache" remote_cache_read = true remote_cache_write = true ``` diff --git a/src/python/pants/engine/internals/scheduler.py b/src/python/pants/engine/internals/scheduler.py index 7483cb39d89..8f3145ee8c3 100644 --- a/src/python/pants/engine/internals/scheduler.py +++ b/src/python/pants/engine/internals/scheduler.py @@ -68,7 +68,6 @@ LOCAL_STORE_LEASE_TIME_SECS, ExecutionOptions, LocalStoreOptions, - normalize_remote_address, ) from pants.util.contextutil import temporary_file_path from pants.util.logging import LogLevel @@ -179,6 +178,7 @@ def __init__( parsed_javascript_deps_result=NativeParsedJavascriptDependencies, ) remoting_options = PyRemotingOptions( + provider=execution_options.remote_provider.value, execution_enable=execution_options.remote_execution, store_headers=execution_options.remote_store_headers, store_chunk_bytes=execution_options.remote_store_chunk_bytes, @@ -193,8 +193,8 @@ def __init__( execution_headers=execution_options.remote_execution_headers, execution_overall_deadline_secs=execution_options.remote_execution_overall_deadline_secs, execution_rpc_concurrency=execution_options.remote_execution_rpc_concurrency, - store_address=normalize_remote_address(execution_options.remote_store_address), - execution_address=normalize_remote_address(execution_options.remote_execution_address), + store_address=execution_options.remote_store_address, + execution_address=execution_options.remote_execution_address, execution_process_cache_namespace=execution_options.process_execution_cache_namespace, instance_name=execution_options.remote_instance_name, root_ca_certs_path=execution_options.remote_ca_certs_path, diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index c94b4a3c86d..dc13ae09e97 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -15,6 +15,8 @@ from pathlib import Path, PurePath from typing import Any, Callable, Type, TypeVar, cast +from typing_extensions import assert_never + from pants.base.build_environment import ( get_buildroot, get_default_pants_config_file, @@ -75,227 +77,117 @@ class DynamicUIRenderer(Enum): _G = TypeVar("_G", bound="_GlobMatchErrorBehaviorOptionBase") -_EXPERIMENTAL_SCHEME = "experimental:" +class RemoteProvider(Enum): + """Which remote provider to use.""" -def normalize_remote_address(addr: str | None) -> str | None: - if addr is None: - return None - return addr.removeprefix(_EXPERIMENTAL_SCHEME) + reapi = "reapi" + experimental_file = "experimental-file" + experimental_github_actions_cache = "experimental-github-actions-cache" + def _supports_execution(self) -> bool: + return self is RemoteProvider.reapi -@dataclass(frozen=True) -class _RemoteAddressScheme: - schemes: tuple[str, ...] - supports_execution: bool - experimental: bool - description: str + def _supported_schemes(self) -> list[str]: + if self is RemoteProvider.reapi: + return ["grpc", "grpcs"] + elif self is RemoteProvider.experimental_file: + return ["file"] + elif self is RemoteProvider.experimental_github_actions_cache: + return ["http", "https"] - def rendered_schemes(self) -> tuple[str, ...]: - """Convert the schemes into what the user needs to write. + assert_never(self) - For example: `experimental:some-scheme://` if experimental, or `some-scheme://` if not. + def _matches_scheme(self, addr: str) -> bool: + return any(addr.startswith(f"{scheme}://") for scheme in self._supported_schemes()) - This includes the :// because that's clearer in docs etc, even if it's not 'technically' - part of the scheme. - """ - # `experimental:` is used as a prefix-scheme, riffing on `view-source:https://...` in some - # web browsers. This ensures the experimental status is communicated right where a user is - # opting-in to using it. - experimental_prefix = _EXPERIMENTAL_SCHEME if self.experimental else "" - return tuple(f"{experimental_prefix}{scheme}://" for scheme in self.schemes) + def _human_readable_schemes(self) -> str: + return ", ".join(f"`{s}://`" for s in sorted(self._supported_schemes())) - @staticmethod - def _validate_address( - schemes: tuple[_RemoteAddressScheme, ...], - addr: str, - require_execution: bool, - context_for_diagnostics: str, - ) -> None: - addr_is_experimental = addr.startswith(_EXPERIMENTAL_SCHEME) - experimentalless_addr = addr.removeprefix(_EXPERIMENTAL_SCHEME) - - matching_scheme = next( - ( - (scheme_str, scheme) - for scheme in schemes - for scheme_str in scheme.schemes - if experimentalless_addr.startswith(f"{scheme_str}://") - ), - None, - ) - - if matching_scheme is None: - # This an address that doesn't seem to have a scheme we understand. - supported_schemes = ", ".join( - f"`{rendered}`" for scheme in schemes for rendered in scheme.rendered_schemes() - ) - raise OptionsError( - softwrap( - f""" - {context_for_diagnostics} has invalid value `{addr}`: it does not have a - supported scheme. + def validate_address(self, addr: str, address_source: str, provider_source: str) -> None: + if self._matches_scheme(addr): + # All good! The scheme matches this provider. + return - The value must start with one of: {supported_schemes} - """ - ) + other_providers = [ + provider for provider in RemoteProvider if provider._matches_scheme(addr) + ] + if other_providers: + rendered = ", ".join(f"`{p.value}`" for p in other_providers) + provider_did_you_mean = ( + f"to use a provider that does support this scheme ({rendered}) or " ) + else: + provider_did_you_mean = "" - scheme_str, scheme = matching_scheme + schemes_did_you_mean = ( + f"to use a scheme that is supported by this provider ({self._human_readable_schemes()})" + ) - if scheme.experimental and not addr_is_experimental: - # This is a URL like `some-scheme://` for a scheme that IS experimental, so let's tell - # the user they need to specify it as `experimental:some-scheme://`. - raise OptionsError( - softwrap( - f""" - {context_for_diagnostics} has invalid value `{addr}`: the scheme `{scheme_str}` - is experimental and thus must include the `{_EXPERIMENTAL_SCHEME}` prefix to - opt-in to this less-stable Pants feature. + raise OptionsError( + softwrap( + f""" + Value `{addr}` from {address_source} is invalid: it doesn't have a scheme that is + supported by provider `{self.value}` from {provider_source}. - Specify the value as `{_EXPERIMENTAL_SCHEME}{addr}`, with the - `{_EXPERIMENTAL_SCHEME}` prefix. - """ - ) + Did you mean {provider_did_you_mean}{schemes_did_you_mean}? + """ ) + ) - if not scheme.experimental and addr_is_experimental: - # This is a URL like `experimental:some-scheme://...` for a scheme that's NOT experimental, - # so let's tell the user to fix it up as `some-scheme://...`. It's low importance (we - # can unambigiously tell what they mean), so a warning is fine. - logger.warning( - softwrap( - f""" - {context_for_diagnostics} has value `{addr}` including `{_EXPERIMENTAL_SCHEME}` - prefix, but the scheme `{scheme_str}` is not experimental. - - Specify the value as `{experimentalless_addr}`, without the `{_EXPERIMENTAL_SCHEME}` - prefix. - """ - ) - ) + def validate_execution_supported(self, provider_source: str, execution_implied_by: str) -> None: + if self._supports_execution(): + # All good! Execution is supported by this provider. + return - if require_execution and not scheme.supports_execution: - # The address is being used for remote execution, but the scheme doesn't support it. - supported_execution_schemes = ", ".join( - f"`{rendered}`" - for scheme in schemes - if scheme.supports_execution - for rendered in scheme.rendered_schemes() - ) - raise OptionsError( - softwrap( - f""" - {context_for_diagnostics} has invalid value `{addr}`: the scheme `{scheme_str}` - does not support remote execution. + supported_execution_providers = ", ".join( + f"`{provider.value}`" for provider in RemoteProvider if provider._supports_execution() + ) + raise OptionsError( + softwrap( + f""" + Value `{self.value}` from {provider_source} is invalid: it does not support remote + execution, but remote execution is required due to {execution_implied_by}. - Either remove the value (and disable remote execution), or use an address for a - server does support remote execution, starting with one of: - {supported_execution_schemes} """ - ) + Either disable remote execution, or use a provider that does support remote + execution: {supported_execution_providers} + """ ) - - # Validated, all good! - - @staticmethod - def validate_address(addr: str, require_execution: bool, context_for_diagnostics: str) -> None: - _RemoteAddressScheme._validate_address( - _REMOTE_ADDRESS_SCHEMES, - addr=addr, - require_execution=require_execution, - context_for_diagnostics=context_for_diagnostics, ) @staticmethod - def address_help(context: str, extra: str, requires_execution: bool) -> Callable[[object], str]: - def render_list_item(scheme_strs: tuple[str, ...], description: str) -> str: - schemes = ", ".join(f"`{s}`" for s in scheme_strs) - return f"- {schemes}: {description}" + def provider_help() -> Callable[[object], str]: + def provider_list_item(p: RemoteProvider) -> str: + if p is RemoteProvider.reapi: + description = "a server using the Remote Execution API (https://github.com/bazelbuild/remote-apis)" + elif p is RemoteProvider.experimental_github_actions_cache: + description = "the GitHub Actions caching service" + elif p is RemoteProvider.experimental_file: + description = "a directory mapped on the current machine" + else: + assert_never(p) + + return f"- `{p.value}`: {description} (supported schemes for URIs: {p._human_readable_schemes()})" def renderer(_: object) -> str: - supported_schemes = [ - (scheme.rendered_schemes(), scheme.description) - for scheme in _REMOTE_ADDRESS_SCHEMES - if not requires_execution or (requires_execution and scheme.supports_execution) - ] - if requires_execution: - # If this is the help for remote execution, still include the schemes that don't - # support it, but mark them as such. - supported_schemes.append( - ( - tuple( - scheme_str - for scheme in _REMOTE_ADDRESS_SCHEMES - if not scheme.supports_execution - for scheme_str in scheme.rendered_schemes() - ), - "Remote execution is not supported.", - ) - ) - - schemes = "\n\n".join( - render_list_item(scheme_strs, description) - for scheme_strs, description in supported_schemes - ) - extra_inline = f"\n\n{extra}" if extra else "" + list_items = "\n\n".join(provider_list_item(p) for p in RemoteProvider) return softwrap( f""" - The URI of a server/entity used as a {context}.{extra_inline} + The type of provider to use, if using a remote cache and/or remote execution, See + {doc_url('remote-caching-execution')} for details. + + Each provider supports different `remote_store_address` and (optional) + `remote_execution_address` URIs. - Supported schemes: + Supported values: - {schemes} + {list_items} """ ) return renderer -# This duplicates logic/semantics around choosing a byte store/action cache (and, even, technically, -# remote execution) provider: it'd be nice to have it in one place, but huonw thinks we do the -# validation before starting the engine, and, in any case, we can refactor our way there (the remote -# providers aren't configured in one place yet) -_REMOTE_ADDRESS_SCHEMES = ( - _RemoteAddressScheme( - schemes=("grpc", "grpcs"), - supports_execution=True, - experimental=False, - description=softwrap( - """ - Use a [Remote Execution API](https://github.com/bazelbuild/remote-apis) remote - caching/execution server. `grpcs` uses TLS while `grpc` does not. Format: - `grpc[s]://$host:$port`. - """ - ), - ), - _RemoteAddressScheme( - schemes=("file",), - supports_execution=False, - experimental=True, - description=softwrap( - """ - Use a local directory as a 'remote' store, for testing, debugging, or potentially an NFS - mount. Format: `file://$path`. For example: `file:///tmp/remote-cache-example/` will - store within the `/tmp/remote-cache-example/` directory, creating it if necessary. - """ - ), - ), - _RemoteAddressScheme( - schemes=("github-actions-cache+http", "github-actions-cache+https"), - supports_execution=False, - experimental=True, - description=softwrap( - f""" - Use the GitHub Actions Cache for fine-grained caching. This requires extracting - `ACTIONS_CACHE_URL` (passing it in `PANTS_REMOTE_STORE_ADDRESS`) and - `ACTIONS_RUNTIME_TOKEN` (passing it in `PANTS_REMOTE_OAUTH_BEARER_TOKEN`). See - {doc_url('remote-caching#github-actions-cache')} for more details. - """ - ), - ), -) - - @dataclass(frozen=True) class _GlobMatchErrorBehaviorOptionBase: """This class exists to have dedicated types per global option of the `GlobMatchErrorBehavior` @@ -379,6 +271,7 @@ class AuthPluginResult: state: AuthPluginState store_headers: dict[str, str] execution_headers: dict[str, str] + provider: RemoteProvider = RemoteProvider.reapi store_address: str | None = None execution_address: str | None = None instance_name: str | None = None @@ -390,16 +283,21 @@ def __post_init__(self) -> None: plugin_context = f"in `AuthPluginResult` returned from `[GLOBAL].remote_auth_plugin` {name}" if self.store_address: - _RemoteAddressScheme.validate_address( + self.provider.validate_address( self.store_address, - require_execution=False, - context_for_diagnostics=f"`store_address` {plugin_context}", + address_source=f"`store_address` {plugin_context}", + provider_source="`provider` in same result", ) + if self.execution_address: - _RemoteAddressScheme.validate_address( + self.provider.validate_execution_supported( + provider_source=f"`provider` {plugin_context}", + execution_implied_by="`execution_address` in same result", + ) + self.provider.validate_address( self.execution_address, - require_execution=True, - context_for_diagnostics=f"`execution_address` {plugin_context}", + address_source=f"`execution_address` {plugin_context}", + provider_source="`provider` in same result", ) @property @@ -411,6 +309,7 @@ def is_available(self) -> bool: class DynamicRemoteOptions: """Options related to remote execution of processes which are computed dynamically.""" + provider: RemoteProvider execution: bool cache_read: bool cache_write: bool @@ -475,6 +374,7 @@ def __post_init__(self) -> None: @classmethod def disabled(cls) -> DynamicRemoteOptions: return cls( + provider=DEFAULT_EXECUTION_OPTIONS.remote_provider, execution=False, cache_read=False, cache_write=False, @@ -506,6 +406,7 @@ def _use_oauth_token(cls, bootstrap_options: OptionValueContainer) -> DynamicRem ) token_header = {"authorization": f"Bearer {oauth_token}"} + provider = cast(RemoteProvider, bootstrap_options.remote_provider) execution = cast(bool, bootstrap_options.remote_execution) cache_read = cast(bool, bootstrap_options.remote_cache_read) cache_write = cast(bool, bootstrap_options.remote_cache_write) @@ -521,6 +422,7 @@ def _use_oauth_token(cls, bootstrap_options: OptionValueContainer) -> DynamicRem execution_headers.update(token_header) store_headers.update(token_header) return cls( + provider=provider, execution=execution, cache_read=cache_read, cache_write=cache_write, @@ -586,6 +488,7 @@ def from_options( @classmethod def _use_no_auth(cls, bootstrap_options: OptionValueContainer) -> DynamicRemoteOptions: + provider = cast(RemoteProvider, bootstrap_options.remote_provider) execution = cast(bool, bootstrap_options.remote_execution) cache_read = cast(bool, bootstrap_options.remote_cache_read) cache_write = cast(bool, bootstrap_options.remote_cache_write) @@ -599,6 +502,7 @@ def _use_no_auth(cls, bootstrap_options: OptionValueContainer) -> DynamicRemoteO cache_rpc_concurrency = cast(int, bootstrap_options.remote_cache_rpc_concurrency) execution_rpc_concurrency = cast(int, bootstrap_options.remote_execution_rpc_concurrency) return cls( + provider=provider, execution=execution, cache_read=cache_read, cache_write=cache_write, @@ -622,6 +526,7 @@ def _use_auth_plugin( prior_result: AuthPluginResult | None, remote_auth_plugin_func: Callable, ) -> tuple[DynamicRemoteOptions, AuthPluginResult | None]: + provider = cast(RemoteProvider, bootstrap_options.remote_provider) execution = cast(bool, bootstrap_options.remote_execution) cache_read = cast(bool, bootstrap_options.remote_cache_read) cache_write = cast(bool, bootstrap_options.remote_cache_write) @@ -658,6 +563,7 @@ def _use_auth_plugin( logger.debug( f"Remote auth plugin `{plugin_name}` succeeded. Remote caching/execution will be attempted." ) + provider = auth_plugin_result.provider execution_headers = auth_plugin_result.execution_headers store_headers = auth_plugin_result.store_headers plugin_provided_opt_log = "Setting `[GLOBAL].remote_{opt}` is not needed and will be ignored since it is provided by the auth plugin: {plugin_name}." @@ -681,6 +587,7 @@ def _use_auth_plugin( execution_address = auth_plugin_result.execution_address opts = cls( + provider=provider, execution=execution, cache_read=cache_read, cache_write=cache_write, @@ -713,6 +620,8 @@ class ExecutionOptions: allowing Subsystems to be consumed before the Scheduler has been created). """ + remote_provider: RemoteProvider + remote_execution: bool remote_cache_read: bool remote_cache_write: bool @@ -760,6 +669,7 @@ def from_options( dynamic_remote_options: DynamicRemoteOptions, ) -> ExecutionOptions: return cls( + remote_provider=dynamic_remote_options.provider, # Remote execution strategy. remote_execution=dynamic_remote_options.execution, remote_cache_read=dynamic_remote_options.cache_read, @@ -846,6 +756,7 @@ def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions: DEFAULT_EXECUTION_OPTIONS = ExecutionOptions( + remote_provider=RemoteProvider.reapi, # Remote execution strategy. remote_execution=False, remote_cache_read=False, @@ -1576,6 +1487,12 @@ class BootstrapOptions: """ ), ) + + remote_provider = EnumOption( + default=RemoteProvider.reapi, + help=RemoteProvider.provider_help(), + ) + remote_execution = BoolOption( default=DEFAULT_EXECUTION_OPTIONS.remote_execution, help=softwrap( @@ -1707,10 +1624,11 @@ class BootstrapOptions: remote_store_address = StrOption( advanced=True, default=cast(str, DEFAULT_EXECUTION_OPTIONS.remote_store_address), - help=_RemoteAddressScheme.address_help( - "remote file store", - extra="", - requires_execution=False, + help=softwrap( + """ + The URI of a server/entity used as a remote file store. The supported URIs depends on + the value of the `remote_provider` option. + """ ), ) remote_store_headers = DictOption( @@ -1779,10 +1697,13 @@ class BootstrapOptions: remote_execution_address = StrOption( advanced=True, default=cast(str, DEFAULT_EXECUTION_OPTIONS.remote_execution_address), - help=_RemoteAddressScheme.address_help( - "remote execution scheduler", - extra="You must also set `[GLOBAL].remote_store_address`, which will often be the same value.", - requires_execution=True, + help=softwrap( + """ + The URI of a server/entity used as a remote execution scheduler. The supported URIs depends on + the value of the `remote_provider` option. + + You must also set `[GLOBAL].remote_store_address`, which will often be the same value. + """ ), ) remote_execution_headers = DictOption( @@ -2068,17 +1989,22 @@ def validate_instance(cls, opts): ) ) + provider_source = "the `[GLOBAL].remote_provider` option" if opts.remote_execution_address: - _RemoteAddressScheme.validate_address( + address_source = "the `[GLOBAL].remote_execution_address` option" + opts.remote_provider.validate_execution_supported( + provider_source=provider_source, execution_implied_by=address_source + ) + opts.remote_provider.validate_address( opts.remote_execution_address, - require_execution=True, - context_for_diagnostics="The `[GLOBAL].remote_execution_address` option", + address_source=address_source, + provider_source=provider_source, ) if opts.remote_store_address: - _RemoteAddressScheme.validate_address( + opts.remote_provider.validate_address( opts.remote_store_address, - require_execution=False, - context_for_diagnostics="The `[GLOBAL].remote_store_address` option", + address_source="the `[GLOBAL].remote_store_address` option", + provider_source=provider_source, ) # Ensure that remote headers are ASCII. diff --git a/src/python/pants/option/global_options_test.py b/src/python/pants/option/global_options_test.py index e8d05fd0b68..432b07cba92 100644 --- a/src/python/pants/option/global_options_test.py +++ b/src/python/pants/option/global_options_test.py @@ -4,27 +4,24 @@ from __future__ import annotations import sys -from collections import Counter from pathlib import Path from textwrap import dedent +from typing import ContextManager import pytest from pants.base.build_environment import get_buildroot from pants.engine.env_vars import CompleteEnvironmentVars +from pants.engine.internals.native_engine import PyRemotingOptions from pants.engine.internals.scheduler import ExecutionError from pants.engine.unions import UnionMembership from pants.init.options_initializer import OptionsInitializer from pants.option.errors import OptionsError -from pants.option.global_options import ( - _REMOTE_ADDRESS_SCHEMES, - DynamicRemoteOptions, - GlobalOptions, - _RemoteAddressScheme, -) +from pants.option.global_options import DynamicRemoteOptions, GlobalOptions, RemoteProvider from pants.option.options_bootstrapper import OptionsBootstrapper from pants.testutil import rule_runner from pants.testutil.option_util import create_options_bootstrapper +from pants.testutil.pytest_util import no_exception from pants.util.dirutil import safe_mkdir_for @@ -163,114 +160,129 @@ def test_invalidation_globs() -> None: assert suffix not in glob -def _scheme( - schemes: tuple[str, ...] = ("foo",), - supports_execution: bool = False, - experimental: bool = False, -) -> _RemoteAddressScheme: - return _RemoteAddressScheme( - schemes=schemes, - supports_execution=supports_execution, - experimental=experimental, - description="DESCRIPTION", - ) - - @pytest.mark.parametrize( - "address", + ("provider", "address", "expect_raises"), [ - "experimental:foo://", - "experimental:foo://host:123", - "experimental:foos://path/here", - "bar://", - "bar://user@host:123/path?query#fragment", + (RemoteProvider.reapi, "grpc://example", no_exception()), + (RemoteProvider.reapi, "grpcs://example", no_exception()), + ( + RemoteProvider.reapi, + "http://example", + pytest.raises( + OptionsError, + match=r"(?is)Value `http://example` from ADDRESS is invalid: it doesn't have a scheme that is supported by provider `reapi` from PROVIDER.*Did you mean to use a provider that does support this scheme \(`experimental-github-actions-cache`\) or to use a scheme that is supported by this provider \(`grpc://`, `grpcs://`\)\?", + ), + ), + ( + RemoteProvider.reapi, + "https://example", + pytest.raises( + OptionsError, + match=r"(?s)Value `https://example` from ADDRESS is invalid.*scheme.*supported", + ), + ), + ( + RemoteProvider.reapi, + "file://example", + pytest.raises( + OptionsError, + match=r"(?s)Value .* from ADDRESS is invalid.*scheme.*supported.* provider `reapi` from PROVIDER.*Did you mean .* provider .* scheme \(`experimental-file`\) .* provider \(`grpc://`, `grpcs://`\)", + ), + ), + ( + RemoteProvider.reapi, + "grpc-example", + pytest.raises( + OptionsError, + match=r"(?s)Value .* from ADDRESS is invalid.*scheme.*supported.* provider `reapi` from PROVIDER.*Did you mean to use a scheme that is supported by this provider \(`grpc://`, `grpcs://`\)", + ), + ), + (RemoteProvider.experimental_file, "file://example", no_exception()), + ( + RemoteProvider.experimental_file, + "http://example", + pytest.raises( + OptionsError, + match="(?s)Value .* from ADDRESS is invalid.*scheme.*supported", + ), + ), + (RemoteProvider.experimental_github_actions_cache, "http://example", no_exception()), + (RemoteProvider.experimental_github_actions_cache, "https://example", no_exception()), + ( + RemoteProvider.experimental_github_actions_cache, + "file://example", + pytest.raises( + OptionsError, + match="(?si)Value .* from ADDRESS is invalid.*scheme.*supported", + ), + ), ], ) -@pytest.mark.parametrize("execution", [False, True]) -def test_remote_schemes_validate_address_should_pass_for_various_good_addresses_without_execution( - address: str, execution: bool +def test_remote_provider_validate_address_should_match_table( + provider: RemoteProvider, address: str, expect_raises: ContextManager ) -> None: - _RemoteAddressScheme._validate_address( - ( - _scheme(schemes=("foo", "foos"), experimental=True, supports_execution=execution), - # (smoke test require_execution=False supports_execution=True) - _scheme(schemes=("bar",), supports_execution=True), - ), - address, - require_execution=execution, - context_for_diagnostics="CONTEXT", - ) + with expect_raises: + provider.validate_address(address, address_source="ADDRESS", provider_source="PROVIDER") @pytest.mark.parametrize( - "address", - ["", "foo", "foo:", "foo:/", "FOO://", "foo:bar://", "fooextra://", "baz://", "bars://"], -) -def test_remote_schemes_validate_address_should_error_when_bad_address(address: str) -> None: - with pytest.raises( - OptionsError, - match=f"(?s)CONTEXT has invalid value `{address}`: it does not have a supported scheme.*start with one of: `foo://`, `foos://`, `bar://`", - ): - _RemoteAddressScheme._validate_address( - ( - _scheme(schemes=("foo", "foos")), - _scheme(schemes=("bar",)), + ("provider", "expect_raises"), + [ + (RemoteProvider.reapi, no_exception()), + ( + RemoteProvider.experimental_file, + pytest.raises( + OptionsError, + match="(?s)Value `experimental-file` from PROVIDER is invalid: it does not support remote execution, but remote execution is required due to IMPLIED BY.*Either disable remote execution, or use a provider that does support remote execution: `reapi`", ), - address, - require_execution=False, - context_for_diagnostics="CONTEXT", - ) - - -def test_remote_schemes_validate_address_should_error_when_missing_experimental() -> None: - with pytest.raises( - OptionsError, - match="(?s)CONTEXT has invalid value `foo://bar`: the scheme `foo` is experimental.*Specify the value as `experimental:foo://bar`", - ): - _RemoteAddressScheme._validate_address( - (_scheme(experimental=True),), - "foo://bar", - require_execution=False, - context_for_diagnostics="CONTEXT", - ) - - -def test_remote_schemes_validate_address_should_warn_when_unnecessary_experimental(caplog) -> None: - with caplog.at_level("WARNING"): - _RemoteAddressScheme._validate_address( - (_scheme(experimental=False),), - "experimental:foo://bar", - require_execution=False, - context_for_diagnostics="CONTEXT", - ) - - assert "CONTEXT has value `experimental:foo://bar`" in caplog.text - assert "the scheme `foo` is not experimental" in caplog.text - assert "Specify the value as `foo://bar`" in caplog.text - - -def test_remote_schemes_validate_address_should_error_when_execution_required_but_not_supported() -> ( - None -): - with pytest.raises( - OptionsError, - match="(?s)CONTEXT has invalid value `foo://bar`: the scheme `foo` does not support remote execution.*starting with one of: `bar://`", - ): - _RemoteAddressScheme._validate_address( - ( - _scheme(supports_execution=False), - _scheme(schemes=("bar",), supports_execution=True), + ), + ( + RemoteProvider.experimental_github_actions_cache, + pytest.raises( + OptionsError, + match="(?si)Value `experimental-github-actions-cache` from PROVIDER is invalid.*remote execution.*IMPLIED BY", ), - "foo://bar", - require_execution=True, - context_for_diagnostics="CONTEXT", + ), + ], +) +def test_remote_provider_validate_execution_supported_should_match_table( + provider: RemoteProvider, expect_raises: ContextManager +) -> None: + with expect_raises: + provider.validate_execution_supported( + provider_source="PROVIDER", execution_implied_by="IMPLIED BY" ) -def test_remote_schemes_should_have_unique_schemes(): - # the raw schemes supported for remoting (not with experimental: prefix, etc.) should be unique, - # so there's no accidental ambiguity about, for instance, `http://` configured more than once - counts = Counter( - scheme_str for scheme in _REMOTE_ADDRESS_SCHEMES for scheme_str in scheme.schemes +@pytest.mark.parametrize("provider", RemoteProvider) +def test_remote_provider_matches_rust_enum( + provider: RemoteProvider, +) -> None: + PyRemotingOptions( + # the string should be converted to the Rust-side enum successfully, i.e. Python matches + # Rust + provider=provider.value, + # all the other fields aren't relevant to this test + execution_enable=False, + store_headers={}, + store_chunk_bytes=0, + store_rpc_retries=0, + store_rpc_concurrency=0, + store_rpc_timeout_millis=0, + store_batch_api_size_limit=0, + cache_warnings_behavior="ignore", + cache_content_behavior="validate", + cache_rpc_concurrency=0, + cache_rpc_timeout_millis=0, + execution_headers={}, + execution_overall_deadline_secs=0, + execution_rpc_concurrency=0, + store_address=None, + execution_address=None, + execution_process_cache_namespace=None, + instance_name=None, + root_ca_certs_path=None, + client_certs_path=None, + client_key_path=None, + append_only_caches_base_path=None, ) - assert [scheme for scheme, count in counts.items() if count > 1] == [] diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 43225487179..194bddf2821 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -3120,6 +3120,8 @@ dependencies = [ "grpc_util", "hashing", "protos", + "strum", + "strum_macros", "tokio", ] diff --git a/src/rust/engine/fs/brfs/src/main.rs b/src/rust/engine/fs/brfs/src/main.rs index e14f47c7672..a9a0eca50d6 100644 --- a/src/rust/engine/fs/brfs/src/main.rs +++ b/src/rust/engine/fs/brfs/src/main.rs @@ -19,7 +19,7 @@ use log::{debug, error, warn}; use parking_lot::Mutex; use protos::gen::build::bazel::remote::execution::v2 as remexec; use protos::require_digest; -use store::{RemoteStoreOptions, Store, StoreError}; +use store::{RemoteProvider, RemoteStoreOptions, Store, StoreError}; use tokio::signal::unix::{signal, SignalKind}; use tokio::task; use tokio_stream::wrappers::SignalStream; @@ -770,6 +770,7 @@ async fn main() { let store = match args.value_of("server-address") { Some(address) => local_only_store .into_with_remote(RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: address.to_owned(), instance_name: args.value_of("remote-instance-name").map(str::to_owned), tls_config, diff --git a/src/rust/engine/fs/fs_util/src/main.rs b/src/rust/engine/fs/fs_util/src/main.rs index 2807a0808ee..3ef04c8a127 100644 --- a/src/rust/engine/fs/fs_util/src/main.rs +++ b/src/rust/engine/fs/fs_util/src/main.rs @@ -25,8 +25,8 @@ use protos::require_digest; use serde_derive::Serialize; use std::collections::{BTreeMap, BTreeSet}; use store::{ - RemoteStoreOptions, Snapshot, SnapshotOps, Store, StoreError, StoreFileByDigest, SubsetParams, - UploadSummary, + RemoteProvider, RemoteStoreOptions, Snapshot, SnapshotOps, Store, StoreError, + StoreFileByDigest, SubsetParams, UploadSummary, }; use workunit_store::WorkunitStore; @@ -391,6 +391,7 @@ async fn execute(top_match: &clap::ArgMatches) -> Result<(), ExitError> { ( local_only .into_with_remote(RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: cas_address.to_owned(), instance_name: top_match .value_of("remote-instance-name") diff --git a/src/rust/engine/fs/store/src/lib.rs b/src/rust/engine/fs/store/src/lib.rs index a737a1fe5bc..54f794cc192 100644 --- a/src/rust/engine/fs/store/src/lib.rs +++ b/src/rust/engine/fs/store/src/lib.rs @@ -66,7 +66,7 @@ mod remote_tests; // Consumers of this crate shouldn't need to worry about the exact crate structure that comes // together to make a store. -pub use remote_provider::RemoteStoreOptions; +pub use remote_provider::{RemoteProvider, RemoteStoreOptions}; pub struct LocalOptions { pub files_max_size_bytes: usize, diff --git a/src/rust/engine/fs/store/src/remote_tests.rs b/src/rust/engine/fs/store/src/remote_tests.rs index ff68661116c..16109f7b00b 100644 --- a/src/rust/engine/fs/store/src/remote_tests.rs +++ b/src/rust/engine/fs/store/src/remote_tests.rs @@ -8,7 +8,7 @@ use bytes::Bytes; use grpc_util::tls; use hashing::{Digest, Fingerprint}; use parking_lot::Mutex; -use remote_provider::{ByteStoreProvider, LoadDestination, RemoteStoreOptions}; +use remote_provider::{ByteStoreProvider, LoadDestination, RemoteProvider, RemoteStoreOptions}; use tempfile::TempDir; use testutil::data::TestData; use testutil::file::mk_tempfile; @@ -30,6 +30,7 @@ async fn smoke_test_from_options_reapi_provider() { let cas = new_cas(10); let store = ByteStore::from_options(RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: cas.address(), instance_name: None, tls_config: tls::Config::default(), @@ -78,6 +79,7 @@ async fn smoke_test_from_options_file_provider() { let dir = TempDir::new().unwrap(); let store = ByteStore::from_options(RemoteStoreOptions { + provider: RemoteProvider::ExperimentalFile, store_address: format!("file://{}", dir.path().display()), instance_name: None, tls_config: tls::Config::default(), diff --git a/src/rust/engine/fs/store/src/tests.rs b/src/rust/engine/fs/store/src/tests.rs index b2c81a74ccf..a7c458f992d 100644 --- a/src/rust/engine/fs/store/src/tests.rs +++ b/src/rust/engine/fs/store/src/tests.rs @@ -22,8 +22,8 @@ use workunit_store::WorkunitStore; use crate::local::ByteStore; use crate::{ - EntryType, FileContent, RemoteStoreOptions, Snapshot, Store, StoreError, StoreFileByDigest, - UploadSummary, MEGABYTES, + EntryType, FileContent, RemoteProvider, RemoteStoreOptions, Snapshot, Store, StoreError, + StoreFileByDigest, UploadSummary, MEGABYTES, }; pub(crate) const STORE_BATCH_API_SIZE_LIMIT: usize = 4 * 1024 * 1024; @@ -64,6 +64,7 @@ fn remote_options( headers: BTreeMap, ) -> RemoteStoreOptions { RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address, instance_name, tls_config: tls::Config::default(), diff --git a/src/rust/engine/process_execution/remote/src/remote_cache_tests.rs b/src/rust/engine/process_execution/remote/src/remote_cache_tests.rs index 6b6f369b9b9..57448d0fb86 100644 --- a/src/rust/engine/process_execution/remote/src/remote_cache_tests.rs +++ b/src/rust/engine/process_execution/remote/src/remote_cache_tests.rs @@ -16,7 +16,7 @@ use grpc_util::tls; use hashing::{Digest, EMPTY_DIGEST}; use mock::StubCAS; use protos::gen::build::bazel::remote::execution::v2 as remexec; -use store::{RemoteStoreOptions, Store}; +use store::{RemoteProvider, RemoteStoreOptions, Store}; use testutil::data::{TestData, TestDirectory, TestTree}; use workunit_store::{RunId, RunningWorkunit, WorkunitStore}; @@ -106,6 +106,7 @@ impl StoreSetup { let store = Store::local_only(executor.clone(), store_dir) .unwrap() .into_with_remote(RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: cas.address(), instance_name: None, tls_config: tls::Config::default(), @@ -160,6 +161,7 @@ async fn create_cached_runner( append_only_caches_base_path: None, }, RemoteStoreOptions { + provider: RemoteProvider::Reapi, instance_name: None, store_address: store_setup.cas.address(), tls_config: tls::Config::default(), @@ -763,6 +765,7 @@ async fn make_action_result_basic() { append_only_caches_base_path: None, }, RemoteStoreOptions { + provider: RemoteProvider::Reapi, instance_name: None, store_address: cas.address(), tls_config: tls::Config::default(), diff --git a/src/rust/engine/process_execution/remote/src/remote_tests.rs b/src/rust/engine/process_execution/remote/src/remote_tests.rs index 21171e09994..1f02d4d42a4 100644 --- a/src/rust/engine/process_execution/remote/src/remote_tests.rs +++ b/src/rust/engine/process_execution/remote/src/remote_tests.rs @@ -14,7 +14,7 @@ use prost::Message; use protos::gen::build::bazel::remote::execution::v2 as remexec; use protos::gen::google::longrunning::Operation; use remexec::{execution_stage::Value as ExecutionStageValue, ExecutedActionMetadata}; -use store::{RemoteStoreOptions, SnapshotOps, Store, StoreError}; +use store::{RemoteProvider, RemoteStoreOptions, SnapshotOps, Store, StoreError}; use tempfile::TempDir; use testutil::data::{TestData, TestDirectory, TestTree}; use testutil::{owned_string_vec, relative_paths}; @@ -1270,6 +1270,7 @@ async fn server_sending_triggering_timeout_with_deadline_exceeded() { fn remote_options_for_cas(cas: &mock::StubCAS) -> RemoteStoreOptions { RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: cas.address(), instance_name: None, tls_config: tls::Config::default(), diff --git a/src/rust/engine/process_executor/src/main.rs b/src/rust/engine/process_executor/src/main.rs index 7d6bebb9ea9..24967b0c8ed 100644 --- a/src/rust/engine/process_executor/src/main.rs +++ b/src/rust/engine/process_executor/src/main.rs @@ -22,7 +22,7 @@ use protos::gen::build::bazel::remote::execution::v2::{Action, Command}; use protos::gen::buildbarn::cas::UncachedActionResult; use protos::require_digest; use remote::remote_cache::RemoteCacheRunnerOptions; -use store::{ImmutableInputs, RemoteStoreOptions, Store}; +use store::{ImmutableInputs, RemoteProvider, RemoteStoreOptions, Store}; use workunit_store::{in_workunit, Level, WorkunitStore}; #[derive(Clone, Debug, Default)] @@ -261,7 +261,8 @@ async fn main() { .expect("failed parsing root CA certs"); local_only_store - .into_with_remote(RemoteStoreOptions { + .into_with_remote(RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address: cas_server.to_owned(), instance_name: args.remote_instance_name.clone(), tls_config, @@ -363,6 +364,7 @@ async fn main() { .map(|p| p.to_string_lossy().to_string()), }, RemoteStoreOptions { + provider: RemoteProvider::Reapi, instance_name: process_metadata.instance_name.clone(), store_address: address, tls_config, diff --git a/src/rust/engine/remote_provider/remote_provider_opendal/src/action_cache_tests.rs b/src/rust/engine/remote_provider/remote_provider_opendal/src/action_cache_tests.rs index eda0f60369b..ac6579dd1da 100644 --- a/src/rust/engine/remote_provider/remote_provider_opendal/src/action_cache_tests.rs +++ b/src/rust/engine/remote_provider/remote_provider_opendal/src/action_cache_tests.rs @@ -11,7 +11,7 @@ use hashing::Digest; use opendal::services::Memory; use prost::Message; use protos::gen::build::bazel::remote::execution::v2 as remexec; -use remote_provider_traits::{ActionCacheProvider, RemoteStoreOptions}; +use remote_provider_traits::{ActionCacheProvider, RemoteProvider, RemoteStoreOptions}; use super::Provider; @@ -30,6 +30,7 @@ fn test_path(digest: Digest) -> String { fn remote_options() -> RemoteStoreOptions { RemoteStoreOptions { + provider: RemoteProvider::ExperimentalFile, store_address: "".to_owned(), instance_name: None, tls_config: tls::Config::default(), diff --git a/src/rust/engine/remote_provider/remote_provider_opendal/src/byte_store_tests.rs b/src/rust/engine/remote_provider/remote_provider_opendal/src/byte_store_tests.rs index 3f03f2ff2e3..f5bc1a87096 100644 --- a/src/rust/engine/remote_provider/remote_provider_opendal/src/byte_store_tests.rs +++ b/src/rust/engine/remote_provider/remote_provider_opendal/src/byte_store_tests.rs @@ -6,7 +6,7 @@ use std::time::Duration; use bytes::Bytes; use grpc_util::tls; use opendal::services::Memory; -use remote_provider_traits::{ByteStoreProvider, RemoteStoreOptions}; +use remote_provider_traits::{ByteStoreProvider, RemoteProvider, RemoteStoreOptions}; use testutil::data::TestData; use testutil::file::mk_tempfile; @@ -26,6 +26,7 @@ fn test_path(data: &TestData) -> String { } fn remote_options() -> RemoteStoreOptions { RemoteStoreOptions { + provider: RemoteProvider::ExperimentalFile, store_address: "".to_owned(), instance_name: None, tls_config: tls::Config::default(), diff --git a/src/rust/engine/remote_provider/remote_provider_reapi/src/action_cache_tests.rs b/src/rust/engine/remote_provider/remote_provider_reapi/src/action_cache_tests.rs index 9fbac83789a..2fa0f7c56cc 100644 --- a/src/rust/engine/remote_provider/remote_provider_reapi/src/action_cache_tests.rs +++ b/src/rust/engine/remote_provider/remote_provider_reapi/src/action_cache_tests.rs @@ -5,12 +5,13 @@ use std::{collections::BTreeMap, time::Duration}; use hashing::Digest; use mock::StubCAS; use protos::gen::build::bazel::remote::execution::v2 as remexec; -use remote_provider_traits::{ActionCacheProvider, RemoteStoreOptions}; +use remote_provider_traits::{ActionCacheProvider, RemoteProvider, RemoteStoreOptions}; use super::action_cache::Provider; async fn new_provider(cas: &StubCAS) -> Provider { Provider::new(RemoteStoreOptions { + provider: RemoteProvider::Reapi, instance_name: None, store_address: cas.address(), tls_config: Default::default(), diff --git a/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store_tests.rs b/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store_tests.rs index db12ff51a47..4fb010dbd6f 100644 --- a/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store_tests.rs +++ b/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store_tests.rs @@ -12,7 +12,7 @@ use testutil::file::mk_tempfile; use tokio::fs::File; use workunit_store::WorkunitStore; -use remote_provider_traits::{ByteStoreProvider, RemoteStoreOptions}; +use remote_provider_traits::{ByteStoreProvider, RemoteProvider, RemoteStoreOptions}; use crate::byte_store::Provider; @@ -25,6 +25,7 @@ fn remote_options( batch_api_size_limit: usize, ) -> RemoteStoreOptions { RemoteStoreOptions { + provider: RemoteProvider::Reapi, store_address, instance_name: None, tls_config: tls::Config::default(), diff --git a/src/rust/engine/remote_provider/remote_provider_traits/Cargo.toml b/src/rust/engine/remote_provider/remote_provider_traits/Cargo.toml index bdff0fd5cbf..4a381c3bb7f 100644 --- a/src/rust/engine/remote_provider/remote_provider_traits/Cargo.toml +++ b/src/rust/engine/remote_provider/remote_provider_traits/Cargo.toml @@ -12,6 +12,8 @@ bytes = { workspace = true } grpc_util = { path = "../../grpc_util" } hashing = { path = "../../hashing" } protos = { path = "../../protos" } +strum = { workspace = true } +strum_macros = { workspace = true } tokio = { workspace = true, features = ["fs"] } [lints] diff --git a/src/rust/engine/remote_provider/remote_provider_traits/src/lib.rs b/src/rust/engine/remote_provider/remote_provider_traits/src/lib.rs index d8d44e28c98..69809aa4944 100644 --- a/src/rust/engine/remote_provider/remote_provider_traits/src/lib.rs +++ b/src/rust/engine/remote_provider/remote_provider_traits/src/lib.rs @@ -12,9 +12,20 @@ use remexec::ActionResult; use tokio::fs::File; use tokio::io::{AsyncSeekExt, AsyncWrite}; +// TODO: this is duplicated with global_options.py, it'd be good to have this be the single source +// of truth. +#[derive(Clone, Copy, Debug, strum_macros::EnumString)] +#[strum(serialize_all = "kebab-case")] +pub enum RemoteProvider { + Reapi, + ExperimentalFile, + ExperimentalGithubActionsCache, +} + // TODO: Consider providing `impl Default`, similar to `remote::LocalOptions`. #[derive(Clone)] pub struct RemoteStoreOptions { + pub provider: RemoteProvider, // TODO: this is currently framed for the REAPI provider, with some options used by others, would // be good to generalise pub store_address: String, diff --git a/src/rust/engine/remote_provider/src/lib.rs b/src/rust/engine/remote_provider/src/lib.rs index fe502b7c07f..dba8352bffb 100644 --- a/src/rust/engine/remote_provider/src/lib.rs +++ b/src/rust/engine/remote_provider/src/lib.rs @@ -6,44 +6,41 @@ use std::sync::Arc; // Re-export these so that consumers don't have to know about the exact arrangement of underlying // crates. pub use remote_provider_traits::{ - ActionCacheProvider, ByteStoreProvider, LoadDestination, RemoteStoreOptions, + ActionCacheProvider, ByteStoreProvider, LoadDestination, RemoteProvider, RemoteStoreOptions, }; -const REAPI_ADDRESS_SCHEMAS: [&str; 4] = ["grpc://", "grpcs://", "http://", "https://"]; - // TODO(#19902): a unified view of choosing a provider would be nice pub async fn choose_byte_store_provider( options: RemoteStoreOptions, ) -> Result, String> { let address = options.store_address.clone(); - if REAPI_ADDRESS_SCHEMAS.iter().any(|s| address.starts_with(s)) { - Ok(Arc::new( + match options.provider { + RemoteProvider::Reapi => Ok(Arc::new( remote_provider_reapi::byte_store::Provider::new(options).await?, - )) - } else if let Some(path) = address.strip_prefix("file://") { - // It's a bit weird to support local "file://" for a 'remote' store... but this is handy for - // testing. - Ok(Arc::new(remote_provider_opendal::Provider::fs( - path, - "byte-store".to_owned(), - options, - )?)) - } else if let Some(url) = address.strip_prefix("github-actions-cache+") { - // This is relying on python validating that it was set as `github-actions-cache+https://...` so - // incorrect values could easily slip through here and cause downstream confusion. We're - // intending to change the approach (https://github.com/pantsbuild/pants/issues/19902) so this - // is tolerable for now. - Ok(Arc::new( + )), + RemoteProvider::ExperimentalFile => { + if let Some(path) = address.strip_prefix("file://") { + // It's a bit weird to support local "file://" for a 'remote' store... but this is handy for + // testing. + Ok(Arc::new(remote_provider_opendal::Provider::fs( + path, + "byte-store".to_owned(), + options, + )?)) + } else { + Err(format!( + "file provider requires an address starting with file://, found {}", + options.store_address + )) + } + } + RemoteProvider::ExperimentalGithubActionsCache => Ok(Arc::new( remote_provider_opendal::Provider::github_actions_cache( - url, + &address, "byte-store".to_owned(), options, )?, - )) - } else { - Err(format!( - "Cannot initialise remote byte store provider with address {address}, as the scheme is not supported", - )) + )), } } @@ -52,33 +49,32 @@ pub async fn choose_action_cache_provider( ) -> Result, String> { let address = options.store_address.clone(); - if REAPI_ADDRESS_SCHEMAS.iter().any(|s| address.starts_with(s)) { - Ok(Arc::new( + match options.provider { + RemoteProvider::Reapi => Ok(Arc::new( remote_provider_reapi::action_cache::Provider::new(options).await?, - )) - } else if let Some(path) = address.strip_prefix("file://") { - // It's a bit weird to support local "file://" for a 'remote' store... but this is handy for - // testing. - Ok(Arc::new(remote_provider_opendal::Provider::fs( - path, - "action-cache".to_owned(), - options, - )?)) - } else if let Some(url) = address.strip_prefix("github-actions-cache+") { - // This is relying on python validating that it was set as `github-actions-cache+https://...` so - // incorrect values could easily slip through here and cause downstream confusion. We're - // intending to change the approach (https://github.com/pantsbuild/pants/issues/19902) so this - // is tolerable for now. - Ok(Arc::new( + )), + RemoteProvider::ExperimentalFile => { + if let Some(path) = address.strip_prefix("file://") { + // It's a bit weird to support local "file://" for a 'remote' store... but this is handy for + // testing. + Ok(Arc::new(remote_provider_opendal::Provider::fs( + path, + "action-cache".to_owned(), + options, + )?)) + } else { + Err(format!( + "file provider requires an address starting with file://, found {}", + options.store_address + )) + } + } + RemoteProvider::ExperimentalGithubActionsCache => Ok(Arc::new( remote_provider_opendal::Provider::github_actions_cache( - url, + &address, "action-cache".to_owned(), options, )?, - )) - } else { - Err(format!( - "Cannot initialise remote action cache provider with address {address}, as the scheme is not supported", - )) + )), } } diff --git a/src/rust/engine/src/context.rs b/src/rust/engine/src/context.rs index 886c33e0771..7fbddac08b4 100644 --- a/src/rust/engine/src/context.rs +++ b/src/rust/engine/src/context.rs @@ -35,7 +35,7 @@ use regex::Regex; use remote::remote_cache::{RemoteCacheRunnerOptions, RemoteCacheWarningsBehavior}; use remote::{self, remote_cache}; use rule_graph::RuleGraph; -use store::{self, ImmutableInputs, RemoteStoreOptions, Store}; +use store::{self, ImmutableInputs, RemoteProvider, RemoteStoreOptions, Store}; use task_executor::Executor; use watch::{Invalidatable, InvalidateCaller, InvalidationWatcher}; use workunit_store::{Metric, RunningWorkunit}; @@ -83,6 +83,7 @@ pub struct Core { #[derive(Clone, Debug)] pub struct RemotingOptions { + pub provider: RemoteProvider, pub execution_enable: bool, pub store_address: Option, pub execution_address: Option, @@ -119,6 +120,7 @@ impl RemotingOptions { .clone(); Ok(RemoteStoreOptions { + provider: self.provider, store_address, instance_name: self.instance_name.clone(), tls_config, diff --git a/src/rust/engine/src/externs/interface.rs b/src/rust/engine/src/externs/interface.rs index ac8b0d96049..7c997f1e34a 100644 --- a/src/rust/engine/src/externs/interface.rs +++ b/src/rust/engine/src/externs/interface.rs @@ -40,6 +40,7 @@ use pyo3::{create_exception, IntoPy, PyAny, PyRef}; use regex::Regex; use remote::remote_cache::RemoteCacheWarningsBehavior; use rule_graph::{self, DependencyKey, RuleGraph, RuleId}; +use store::RemoteProvider; use task_executor::Executor; use workunit_store::{ ArtifactOutput, ObservationMetric, UserMetadataItem, Workunit, WorkunitState, WorkunitStore, @@ -300,6 +301,7 @@ struct PyRemotingOptions(RemotingOptions); impl PyRemotingOptions { #[new] fn __new__( + provider: String, execution_enable: bool, store_headers: BTreeMap, store_chunk_bytes: usize, @@ -324,6 +326,7 @@ impl PyRemotingOptions { append_only_caches_base_path: Option, ) -> Self { Self(RemotingOptions { + provider: RemoteProvider::from_str(&provider).unwrap(), execution_enable, store_address, execution_address,