From b32ef1d462366457699f8d75ce766ec82ce2bf6c Mon Sep 17 00:00:00 2001 From: Tom Pollard Date: Thu, 5 Nov 2020 13:55:25 +0000 Subject: [PATCH 1/2] WIP custom platform properties --- .gitlab-ci.yml | 26 +++++++++ .gitlab-ci/buildgrid-remote-execution.yml | 8 ++- .gitlab-ci/buildgrid-server.conf | 69 +++++++++++++++++++++++ doc/source/format_project.rst | 14 +++++ doc/source/using_config.rst | 4 ++ src/buildstream/data/projectconfig.yaml | 3 +- src/buildstream/sandbox/_sandboxreapi.py | 60 ++++++++++++-------- src/buildstream/sandbox/_sandboxremote.py | 26 ++++++++- src/buildstream/testing/runcli.py | 7 +++ tests/conftest.py | 8 +++ tox.ini | 2 + 11 files changed, 197 insertions(+), 30 deletions(-) create mode 100644 .gitlab-ci/buildgrid-server.conf diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2031e2791..cfc97562c 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -193,6 +193,32 @@ tests-remote-execution: REMOTE_EXECUTION_SERVICE: http://docker:50051 SOURCE_CACHE_SERVICE: http://docker:50052 PYTEST_ARGS: "--color=yes --remote-execution" + DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest + +tests-remote-execution-custom-property: + <<: *tests + <<: *remote-test # Spin up server stack + variables: + <<: *docker-variables + COMPOSE_MANIFEST: .gitlab-ci/buildgrid-remote-execution.yml # < *remote-test + ARTIFACT_CACHE_SERVICE: http://docker:50052 + REMOTE_EXECUTION_SERVICE: http://docker:50051 + SOURCE_CACHE_SERVICE: http://docker:50052 + PYTEST_ARGS: "--color=yes --remote-execution" + DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" + CUSTOM_PROPERTIES: "--platform foo=bar" # Custom property, substituted in manifest. This extends defaults + +tests-remote-execution-no-default-properties: + <<: *tests + <<: *remote-test # Spin up server stack + variables: + <<: *docker-variables + COMPOSE_MANIFEST: .gitlab-ci/buildgrid-remote-execution.yml # < *remote-test + ARTIFACT_CACHE_SERVICE: http://docker:50052 + REMOTE_EXECUTION_SERVICE: http://docker:50051 + SOURCE_CACHE_SERVICE: http://docker:50052 + PYTEST_ARGS: "--color=yes --remote-execution" + CUSTOM_PROPERTIES: "--platform OSFamily=linux --platform foo=bar" # No DEFAULT_PROPERTIES set, so these become the *only* properties (default-platform-properties: False) tests-remote-cache: <<: *tests diff --git a/.gitlab-ci/buildgrid-remote-execution.yml b/.gitlab-ci/buildgrid-remote-execution.yml index fd45c45af..2bc76eb6e 100644 --- a/.gitlab-ci/buildgrid-remote-execution.yml +++ b/.gitlab-ci/buildgrid-remote-execution.yml @@ -29,7 +29,11 @@ services: image: registry.gitlab.com/buildgrid/buildgrid.hub.docker.com/buildgrid:nightly command: [ "bgd", "server", "start", "-v", - "/etc/buildgrid/default.conf"] + "/buildgrid-server.conf"] + volumes: + - type: bind + source: ./buildgrid-server.conf + target: /buildgrid-server.conf ports: - 50051:50051 networks: @@ -38,7 +42,7 @@ services: bot: image: registry.gitlab.com/buildgrid/buildgrid.hub.docker.com/buildbox:nightly command: [ - "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas --platform OSFamily=linux --platform ISA=x86-64 --verbose )"] + "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas ${DEFAULT_PROPERTIES} ${CUSTOM_PROPERTIES} --verbose )"] privileged: true volumes: - type: volume diff --git a/.gitlab-ci/buildgrid-server.conf b/.gitlab-ci/buildgrid-server.conf new file mode 100644 index 000000000..ec17d7ada --- /dev/null +++ b/.gitlab-ci/buildgrid-server.conf @@ -0,0 +1,69 @@ +# Taken from https://gitlab.com/BuildGrid/buildgrid/-/blob/master/data/config/default.conf +server: + - !channel + port: 50051 + insecure-mode: true + +description: > + BuildGrid's default configuration: + - Unauthenticated plain HTTP at :50051 + - Single instance: [unnamed] + - In-memory data, max. 2Gio + - DataStore: sqlite:///./example.db + - Hosted services: + - ActionCache + - Execute + - ContentAddressableStorage + - ByteStream + +authorization: + method: none + +monitoring: + enabled: false + +instances: + - name: '' + description: | + The unique '' instance. + + storages: + - !lru-storage &cas-storage + size: 2048M + + schedulers: + - !sql-scheduler &state-database + storage: *cas-storage + connection-string: sqlite:///./example.db + automigrate: yes + connection-timeout: 15 + poll-interval: 0.5 + + caches: + - !lru-action-cache &build-cache + storage: *cas-storage + max-cached-refs: 256 + cache-failed-actions: true + allow-updates: true + + services: + - !action-cache + cache: *build-cache + + - !execution + storage: *cas-storage + action-cache: *build-cache + scheduler: *state-database + max-execution-timeout: 7200 + property-keys: + ## + # BuildGrid will match worker and jobs on foo, if set by job + # and worker platform properties. Used in platform property + # tests + - foo + + - !cas + storage: *cas-storage + + - !bytestream + storage: *cas-storage \ No newline at end of file diff --git a/doc/source/format_project.rst b/doc/source/format_project.rst index 0216e524a..b0b6fc191 100644 --- a/doc/source/format_project.rst +++ b/doc/source/format_project.rst @@ -327,6 +327,10 @@ using the `remote-execution` option: action-cache-service: url: http://bar.action.com:50052 instance-name: development-emea-1 + custom-platform-properties: + docker: docker://marketplace.gcr.io/google/rbe-ubuntu16-04 + default-platform-properties: True + storage-service specifies a remote CAS store and the parameters are the same as those used to specify an :ref:`artifact server `. @@ -347,6 +351,16 @@ name should be given to you by the service provider of each service. Not all remote execution and storage services support instance names. +The custom-platform-properties is optional, properties can be be provided as +key: value pairs and are included with the default properties. Pre-emptive +compatability filtering isn't applied, and default values take precedence +unless explicitly disabled. + +default-platform-properties is an optional bool & specifies if BuildStream +should determine the platform properties & values (which can be set in 'sandbox' config) +to be added to the remote sandbox commands. This behaviour defaults to True if +not specified in the configuration. + The Remote Execution API can be found via https://github.com/bazelbuild/remote-apis. Remote execution configuration can be also provided in the `user diff --git a/doc/source/using_config.rst b/doc/source/using_config.rst index ba38173e3..919b051f9 100644 --- a/doc/source/using_config.rst +++ b/doc/source/using_config.rst @@ -210,6 +210,10 @@ configuration will be used as fallback. action-cache-service: url: http://cache.some_project.example.com:50052 instance-name: main + custom-platform-properties: + docker-image: docker://marketplace.gcr.io/google/rbe-ubuntu16-04 + ISA: arm-a64 + default-platform-properties: False .. _user_config_strict_mode: diff --git a/src/buildstream/data/projectconfig.yaml b/src/buildstream/data/projectconfig.yaml index a2753c312..dce4cc6aa 100644 --- a/src/buildstream/data/projectconfig.yaml +++ b/src/buildstream/data/projectconfig.yaml @@ -71,7 +71,8 @@ environment: environment-nocache: [] # Configuration for the sandbox other than environment variables -# should go in 'sandbox'. +# should go in 'sandbox'. Custom platform properties for Remote +# Execution commands can be set via 'remote-execution' config. sandbox: {} # Defaults for the 'split-rules' public data found on elements diff --git a/src/buildstream/sandbox/_sandboxreapi.py b/src/buildstream/sandbox/_sandboxreapi.py index 5c2851580..fd63ec9cb 100644 --- a/src/buildstream/sandbox/_sandboxreapi.py +++ b/src/buildstream/sandbox/_sandboxreapi.py @@ -119,30 +119,36 @@ def _create_command(self, command, working_directory, environment, read_write_di # Request read-write directories as output output_directories = [os.path.relpath(dir, start=working_directory) for dir in read_write_directories] - config = self._get_config() - - platform_dict = {} - - platform_dict["OSFamily"] = config.build_os - platform_dict["ISA"] = config.build_arch - - if flags & SandboxFlags.INHERIT_UID: - uid = os.geteuid() - gid = os.getegid() - else: - uid = config.build_uid - gid = config.build_gid - if uid is not None: - platform_dict["unixUID"] = str(uid) - if gid is not None: - platform_dict["unixGID"] = str(gid) - - if flags & SandboxFlags.NETWORK_ENABLED: - platform_dict["network"] = "on" - - # Remove unsupported platform properties from the dict - supported_properties = self._supported_platform_properties() - platform_dict = {key: value for (key, value) in platform_dict.items() if key in supported_properties} + # Get the custom platform properties, if specified. These are not filtered + platform_dict = self._get_custom_platform_properties() + + # Unless explicitly disabled, generate default platform properties + if self._use_default_platform_properties(): + config = self._get_config() + default_dict = {} + default_dict["OSFamily"] = config.build_os + default_dict["ISA"] = config.build_arch + + if flags & SandboxFlags.INHERIT_UID: + uid = os.geteuid() + gid = os.getegid() + else: + uid = config.build_uid + gid = config.build_gid + if uid is not None: + default_dict["unixUID"] = str(uid) + if gid is not None: + default_dict["unixGID"] = str(gid) + + if flags & SandboxFlags.NETWORK_ENABLED: + default_dict["network"] = "on" + # Remove unsupported platform properties from the default dict + supported_properties = self._supported_platform_properties() + default_dict = {key: value for (key, value) in default_dict.items() if key in supported_properties} + + # Apply the defaults to the platform_dict. Default values take precedence + # on key collisions + platform_dict = {**platform_dict, **default_dict} # Create Platform message with properties sorted by name in code point order platform = remote_execution_pb2.Platform() @@ -202,6 +208,12 @@ def _execute_action(self, action, flags): def _supported_platform_properties(self): return {"OSFamily", "ISA"} + def _get_custom_platform_properties(self): + return {} + + def _use_default_platform_properties(self): + return True + # _SandboxREAPIBatch() # diff --git a/src/buildstream/sandbox/_sandboxremote.py b/src/buildstream/sandbox/_sandboxremote.py index 6cba7d611..4439dc42c 100644 --- a/src/buildstream/sandbox/_sandboxremote.py +++ b/src/buildstream/sandbox/_sandboxremote.py @@ -40,7 +40,9 @@ from .._remote import RemoteSpec -class RemoteExecutionSpec(namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service")): +class RemoteExecutionSpec( + namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service custom_properties use_defaults") +): pass @@ -96,6 +98,9 @@ def __init__(self, *args, **kwargs): self.action_instance = None self.action_credentials = None + self.custom_properties = config.custom_properties + self.use_defaults = config.use_defaults + self.exec_instance = config.exec_service.get("instance-name", None) self.storage_instance = config.storage_service.get("instance-name", None) @@ -131,11 +136,15 @@ def require_node(config, keyname): service_keys = ["execution-service", "storage-service", "action-cache-service"] - remote_config.validate_keys(["url", *service_keys]) + remote_config.validate_keys( + ["url", "custom-platform-properties", "default-platform-properties", *service_keys] + ) exec_config = require_node(remote_config, "execution-service") storage_config = require_node(remote_config, "storage-service") action_config = remote_config.get_mapping("action-cache-service", default={}) + custom_properties = remote_config.get_mapping("custom-platform-properties", default={}) + use_defaults = remote_config.get_bool("default-platform-properties", default=True) tls_keys = ["client-key", "client-cert", "server-cert"] @@ -181,8 +190,11 @@ def resolve_path(path): if tls_key in config: config[tls_key] = resolve_path(config.get_str(tls_key)) + # Add in the custom platform properties config + service_configs.append(custom_properties) + # TODO: we should probably not be stripping node info and rather load files the safe way - return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs]) + return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs], use_defaults) def run_remote_command(self, channel, action_digest): # Sends an execution request to the remote execution server. @@ -430,6 +442,14 @@ def _check_action_cache(self, action_digest): self.info("Action result found in action cache") return result + def _get_custom_platform_properties(self): + # Dict containing custom platformn properties, if provided in config + return self.custom_properties + + def _use_default_platform_properties(self): + # Bool, defaults to True unless overriden in RE Spec + return self.use_defaults + @staticmethod def _extract_action_result(operation): if operation is None: diff --git a/src/buildstream/testing/runcli.py b/src/buildstream/testing/runcli.py index 7a69191ed..be72f6ae0 100644 --- a/src/buildstream/testing/runcli.py +++ b/src/buildstream/testing/runcli.py @@ -805,7 +805,14 @@ def cli_remote_execution(tmpdir, remote_services): remote_execution["storage-service"] = { "url": remote_services.storage_service, } + if remote_services.custom_properties: + # Expected string with substring pattern "--platform property=value" + remote_execution["custom-platform-properties"] = dict( + s.split("=") for s in remote_services.custom_properties.replace("--platform", "").split() + ) if remote_execution: + if not remote_services.use_defaults: + remote_execution["default-platform-properties"] = False fixture.configure({"remote-execution": remote_execution}) if remote_services.source_service: diff --git a/tests/conftest.py b/tests/conftest.py index d79ad40b0..a0fc1121d 100755 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -103,6 +103,8 @@ def __init__(self, **kwargs): self.storage_service = kwargs.get("storage_service") self.artifact_index_service = kwargs.get("artifact_index_service") self.artifact_storage_service = kwargs.get("artifact_storage_service") + self.use_defaults = kwargs.get("default_properties") + self.custom_properties = kwargs.get("custom_properties") @pytest.fixture(scope="session") @@ -126,6 +128,12 @@ def remote_services(request): if "SOURCE_CACHE_SERVICE" in os.environ: kwargs["source_service"] = os.environ.get("SOURCE_CACHE_SERVICE") + if "DEFAULT_PROPERTIES" in os.environ: + kwargs["default_properties"] = os.environ.get("DEFAULT_PROPERTIES") + + if "CUSTOM_PROPERTIES" in os.environ: + kwargs["custom_properties"] = os.environ.get("CUSTOM_PROPERTIES") + return RemoteServices(**kwargs) diff --git a/tox.ini b/tox.ini index c06d17ba7..31a999db3 100644 --- a/tox.ini +++ b/tox.ini @@ -64,6 +64,8 @@ passenv = SOURCE_CACHE_SERVICE SSL_CERT_FILE BST_PLUGINS_EXPERIMENTAL_VERSION + DEFAULT_PROPERTIES + CUSTOM_PROPERTIES # # These keys are not inherited by any other sections # From 8d4663737c6706a6752ca37c924892c5b41e4444 Mon Sep 17 00:00:00 2001 From: Tom Pollard Date: Thu, 26 Nov 2020 14:51:44 +0000 Subject: [PATCH 2/2] Remove 'default-platform-properties' and reduce scope --- .gitlab-ci.yml | 11 ++- .gitlab-ci/buildgrid-remote-execution.yml | 2 +- doc/source/format_project.rst | 18 ++--- doc/source/using_config.rst | 5 +- src/buildstream/data/projectconfig.yaml | 2 +- .../sandbox/_sandboxbuildboxrun.py | 1 + src/buildstream/sandbox/_sandboxreapi.py | 76 +++++++++++-------- src/buildstream/sandbox/_sandboxremote.py | 28 +++---- src/buildstream/testing/runcli.py | 22 ++++-- tests/conftest.py | 10 +-- tests/remoteexecution/buildfail.py | 28 +++++++ tox.ini | 3 +- 12 files changed, 118 insertions(+), 88 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index cfc97562c..457f6bf42 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -193,9 +193,9 @@ tests-remote-execution: REMOTE_EXECUTION_SERVICE: http://docker:50051 SOURCE_CACHE_SERVICE: http://docker:50052 PYTEST_ARGS: "--color=yes --remote-execution" - DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest + PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest -tests-remote-execution-custom-property: +tests-remote-execution-extra-property: <<: *tests <<: *remote-test # Spin up server stack variables: @@ -205,10 +205,9 @@ tests-remote-execution-custom-property: REMOTE_EXECUTION_SERVICE: http://docker:50051 SOURCE_CACHE_SERVICE: http://docker:50052 PYTEST_ARGS: "--color=yes --remote-execution" - DEFAULT_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" - CUSTOM_PROPERTIES: "--platform foo=bar" # Custom property, substituted in manifest. This extends defaults + PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64 --platform foo=bar" # Custom property 'foo' added -tests-remote-execution-no-default-properties: +tests-remote-execution-no-default-iso: <<: *tests <<: *remote-test # Spin up server stack variables: @@ -218,7 +217,7 @@ tests-remote-execution-no-default-properties: REMOTE_EXECUTION_SERVICE: http://docker:50051 SOURCE_CACHE_SERVICE: http://docker:50052 PYTEST_ARGS: "--color=yes --remote-execution" - CUSTOM_PROPERTIES: "--platform OSFamily=linux --platform foo=bar" # No DEFAULT_PROPERTIES set, so these become the *only* properties (default-platform-properties: False) + PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform foo=bar" # Default property ISA not set, so BST should disable (via ISA: [] set in test suite) tests-remote-cache: <<: *tests diff --git a/.gitlab-ci/buildgrid-remote-execution.yml b/.gitlab-ci/buildgrid-remote-execution.yml index 2bc76eb6e..bef75b57b 100644 --- a/.gitlab-ci/buildgrid-remote-execution.yml +++ b/.gitlab-ci/buildgrid-remote-execution.yml @@ -42,7 +42,7 @@ services: bot: image: registry.gitlab.com/buildgrid/buildgrid.hub.docker.com/buildbox:nightly command: [ - "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas ${DEFAULT_PROPERTIES} ${CUSTOM_PROPERTIES} --verbose )"] + "sh", "-c", "sleep 15 && ( buildbox-casd --cas-remote=http://controller:50051 /var/lib/buildgrid/cache & buildbox-worker --bots-remote=http://controller:50051 --cas-remote=unix:/var/lib/buildgrid/cache/casd.sock --buildbox-run=buildbox-run-bubblewrap --runner-arg=--use-localcas ${PLATFORM_PROPERTIES} --verbose )"] privileged: true volumes: - type: volume diff --git a/doc/source/format_project.rst b/doc/source/format_project.rst index b0b6fc191..e11a396e1 100644 --- a/doc/source/format_project.rst +++ b/doc/source/format_project.rst @@ -327,9 +327,8 @@ using the `remote-execution` option: action-cache-service: url: http://bar.action.com:50052 instance-name: development-emea-1 - custom-platform-properties: + platform-properties: docker: docker://marketplace.gcr.io/google/rbe-ubuntu16-04 - default-platform-properties: True storage-service specifies a remote CAS store and the parameters are the @@ -351,15 +350,12 @@ name should be given to you by the service provider of each service. Not all remote execution and storage services support instance names. -The custom-platform-properties is optional, properties can be be provided as -key: value pairs and are included with the default properties. Pre-emptive -compatability filtering isn't applied, and default values take precedence -unless explicitly disabled. - -default-platform-properties is an optional bool & specifies if BuildStream -should determine the platform properties & values (which can be set in 'sandbox' config) -to be added to the remote sandbox commands. This behaviour defaults to True if -not specified in the configuration. +platform-properties is optional, additional properties specific to the Remote Execution +ennvironment can be be provided as key:value pairs and are included with the default +properties of the sandbox (the values of which are derived from the local sandox enviroment, +unless set in `sandbox` config). Pre-emptive compatability filtering isn't applied and default +property values (such as OSFamily, ISA) cannot be overriden here (configurable in `sandbox` config) +however they can can be explicitly disabled by setting the key value to []. The Remote Execution API can be found via https://github.com/bazelbuild/remote-apis. diff --git a/doc/source/using_config.rst b/doc/source/using_config.rst index 919b051f9..fb6673a8e 100644 --- a/doc/source/using_config.rst +++ b/doc/source/using_config.rst @@ -210,10 +210,9 @@ configuration will be used as fallback. action-cache-service: url: http://cache.some_project.example.com:50052 instance-name: main - custom-platform-properties: + platform-properties: docker-image: docker://marketplace.gcr.io/google/rbe-ubuntu16-04 - ISA: arm-a64 - default-platform-properties: False + ISA: [] .. _user_config_strict_mode: diff --git a/src/buildstream/data/projectconfig.yaml b/src/buildstream/data/projectconfig.yaml index dce4cc6aa..4b129ab75 100644 --- a/src/buildstream/data/projectconfig.yaml +++ b/src/buildstream/data/projectconfig.yaml @@ -72,7 +72,7 @@ environment-nocache: [] # Configuration for the sandbox other than environment variables # should go in 'sandbox'. Custom platform properties for Remote -# Execution commands can be set via 'remote-execution' config. +# Execution services can be set via 'remote-execution' config. sandbox: {} # Defaults for the 'split-rules' public data found on elements diff --git a/src/buildstream/sandbox/_sandboxbuildboxrun.py b/src/buildstream/sandbox/_sandboxbuildboxrun.py index 3d71b7440..594a45fef 100644 --- a/src/buildstream/sandbox/_sandboxbuildboxrun.py +++ b/src/buildstream/sandbox/_sandboxbuildboxrun.py @@ -70,6 +70,7 @@ def check_available(cls): # limit support to native building on the host ISA. cls._isas.add(Platform.get_host_arch()) + # Only called when lauching a local sandbox, as we can't pre-empt the remote environment capabilities @classmethod def check_sandbox_config(cls, platform, config): if config.build_os not in cls._osfamilies: diff --git a/src/buildstream/sandbox/_sandboxreapi.py b/src/buildstream/sandbox/_sandboxreapi.py index fd63ec9cb..1785db662 100644 --- a/src/buildstream/sandbox/_sandboxreapi.py +++ b/src/buildstream/sandbox/_sandboxreapi.py @@ -119,36 +119,49 @@ def _create_command(self, command, working_directory, environment, read_write_di # Request read-write directories as output output_directories = [os.path.relpath(dir, start=working_directory) for dir in read_write_directories] - # Get the custom platform properties, if specified. These are not filtered - platform_dict = self._get_custom_platform_properties() - - # Unless explicitly disabled, generate default platform properties - if self._use_default_platform_properties(): - config = self._get_config() - default_dict = {} - default_dict["OSFamily"] = config.build_os - default_dict["ISA"] = config.build_arch - - if flags & SandboxFlags.INHERIT_UID: - uid = os.geteuid() - gid = os.getegid() + # Get the SandoxConfig + config = self._get_config() + default_dict = {} + default_dict["OSFamily"] = config.build_os + default_dict["ISA"] = config.build_arch + + if flags & SandboxFlags.INHERIT_UID: + uid = os.geteuid() + gid = os.getegid() + else: + uid = config.build_uid + gid = config.build_gid + if uid is not None: + default_dict["unixUID"] = str(uid) + if gid is not None: + default_dict["unixGID"] = str(gid) + + if flags & SandboxFlags.NETWORK_ENABLED: + default_dict["network"] = "on" + # Remove unsupported platform properties from the default dict, this filter is derived from the + # local sandbox capabilities + supported_properties = self._supported_platform_properties() + platform_dict = {key: value for (key, value) in default_dict.items() if key in supported_properties} + + # Get the platform properties dict, if specified. These are not filtered as they are specific + # to the remote server + platform_properties = self._get_platform_properties() + + # Apply the properties to the default_dict. k:v pairs in the default_dict + # can be disabled if given a explicit value of `[]` in platform properties + # with a matching key. + for platform_property, value in platform_properties.items(): + if platform_property in platform_dict: + if value != []: + raise SandboxError( + "Platform Property {}:{} should be configured in sandbox config, not remote-execution.".format( + platform_property, value + ), + reason="invalid-platform-property", + ) + del platform_dict[platform_property] else: - uid = config.build_uid - gid = config.build_gid - if uid is not None: - default_dict["unixUID"] = str(uid) - if gid is not None: - default_dict["unixGID"] = str(gid) - - if flags & SandboxFlags.NETWORK_ENABLED: - default_dict["network"] = "on" - # Remove unsupported platform properties from the default dict - supported_properties = self._supported_platform_properties() - default_dict = {key: value for (key, value) in default_dict.items() if key in supported_properties} - - # Apply the defaults to the platform_dict. Default values take precedence - # on key collisions - platform_dict = {**platform_dict, **default_dict} + platform_dict[platform_property] = value # Create Platform message with properties sorted by name in code point order platform = remote_execution_pb2.Platform() @@ -208,12 +221,9 @@ def _execute_action(self, action, flags): def _supported_platform_properties(self): return {"OSFamily", "ISA"} - def _get_custom_platform_properties(self): + def _get_platform_properties(self): return {} - def _use_default_platform_properties(self): - return True - # _SandboxREAPIBatch() # diff --git a/src/buildstream/sandbox/_sandboxremote.py b/src/buildstream/sandbox/_sandboxremote.py index 4439dc42c..a882a19cd 100644 --- a/src/buildstream/sandbox/_sandboxremote.py +++ b/src/buildstream/sandbox/_sandboxremote.py @@ -41,7 +41,7 @@ class RemoteExecutionSpec( - namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service custom_properties use_defaults") + namedtuple("RemoteExecutionSpec", "exec_service storage_service action_service platform_properties") ): pass @@ -98,8 +98,7 @@ def __init__(self, *args, **kwargs): self.action_instance = None self.action_credentials = None - self.custom_properties = config.custom_properties - self.use_defaults = config.use_defaults + self.platform_properties = config.platform_properties self.exec_instance = config.exec_service.get("instance-name", None) self.storage_instance = config.storage_service.get("instance-name", None) @@ -136,15 +135,12 @@ def require_node(config, keyname): service_keys = ["execution-service", "storage-service", "action-cache-service"] - remote_config.validate_keys( - ["url", "custom-platform-properties", "default-platform-properties", *service_keys] - ) + remote_config.validate_keys(["url", "platform-properties", *service_keys]) exec_config = require_node(remote_config, "execution-service") storage_config = require_node(remote_config, "storage-service") action_config = remote_config.get_mapping("action-cache-service", default={}) - custom_properties = remote_config.get_mapping("custom-platform-properties", default={}) - use_defaults = remote_config.get_bool("default-platform-properties", default=True) + platform_properties = remote_config.get_mapping("platform-properties", default={}) tls_keys = ["client-key", "client-cert", "server-cert"] @@ -190,11 +186,11 @@ def resolve_path(path): if tls_key in config: config[tls_key] = resolve_path(config.get_str(tls_key)) - # Add in the custom platform properties config - service_configs.append(custom_properties) + # Add in the platform properties config + service_configs.append(platform_properties) # TODO: we should probably not be stripping node info and rather load files the safe way - return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs], use_defaults) + return RemoteExecutionSpec(*[conf.strip_node_info() for conf in service_configs]) def run_remote_command(self, channel, action_digest): # Sends an execution request to the remote execution server. @@ -442,13 +438,9 @@ def _check_action_cache(self, action_digest): self.info("Action result found in action cache") return result - def _get_custom_platform_properties(self): - # Dict containing custom platformn properties, if provided in config - return self.custom_properties - - def _use_default_platform_properties(self): - # Bool, defaults to True unless overriden in RE Spec - return self.use_defaults + def _get_platform_properties(self): + # Dict platformn properties, if provided in config + return self.platform_properties @staticmethod def _extract_action_result(operation): diff --git a/src/buildstream/testing/runcli.py b/src/buildstream/testing/runcli.py index be72f6ae0..689a5c69c 100644 --- a/src/buildstream/testing/runcli.py +++ b/src/buildstream/testing/runcli.py @@ -805,14 +805,24 @@ def cli_remote_execution(tmpdir, remote_services): remote_execution["storage-service"] = { "url": remote_services.storage_service, } - if remote_services.custom_properties: - # Expected string with substring pattern "--platform property=value" - remote_execution["custom-platform-properties"] = dict( - s.split("=") for s in remote_services.custom_properties.replace("--platform", "").split() + if remote_services.platform_properties: + default_properties = ["OSFamily", "ISA"] + # Strip cli argument, expected string with substring pattern "--platform property=value" + parsed_properties = dict( + s.split("=") for s in remote_services.platform_properties.replace("--platform", "").split() ) + # If a default property has been set on the server, do not add it to remote-execution config + # as these should configured via sandbox config in bst if required. If a default hasn't been + # set on the server (e.g, no ISO) then this maps to it needing to be explicitly disabled in bst. + for default_property in default_properties: + if default_property in parsed_properties: + del parsed_properties[default_property] + else: + parsed_properties[default_property] = [] + + remote_execution["platform-properties"] = parsed_properties + if remote_execution: - if not remote_services.use_defaults: - remote_execution["default-platform-properties"] = False fixture.configure({"remote-execution": remote_execution}) if remote_services.source_service: diff --git a/tests/conftest.py b/tests/conftest.py index a0fc1121d..759a06005 100755 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -103,8 +103,7 @@ def __init__(self, **kwargs): self.storage_service = kwargs.get("storage_service") self.artifact_index_service = kwargs.get("artifact_index_service") self.artifact_storage_service = kwargs.get("artifact_storage_service") - self.use_defaults = kwargs.get("default_properties") - self.custom_properties = kwargs.get("custom_properties") + self.platform_properties = kwargs.get("platform_properties") @pytest.fixture(scope="session") @@ -128,11 +127,8 @@ def remote_services(request): if "SOURCE_CACHE_SERVICE" in os.environ: kwargs["source_service"] = os.environ.get("SOURCE_CACHE_SERVICE") - if "DEFAULT_PROPERTIES" in os.environ: - kwargs["default_properties"] = os.environ.get("DEFAULT_PROPERTIES") - - if "CUSTOM_PROPERTIES" in os.environ: - kwargs["custom_properties"] = os.environ.get("CUSTOM_PROPERTIES") + if "PLATFORM_PROPERTIES" in os.environ: + kwargs["platform_properties"] = os.environ.get("PLATFORM_PROPERTIES") return RemoteServices(**kwargs) diff --git a/tests/remoteexecution/buildfail.py b/tests/remoteexecution/buildfail.py index 37f4dcafa..3c1dfea24 100644 --- a/tests/remoteexecution/buildfail.py +++ b/tests/remoteexecution/buildfail.py @@ -58,3 +58,31 @@ def test_build_remote_failure(cli, datafiles): # check that the file created before the failure exists filename = os.path.join(checkout_path, "foo") assert os.path.isfile(filename) + + +# Assert that a SandboxError is given if an invalid Remote Execution platform property +# is given which should be configured at a sandbox level, e.g. OSFamily +@pytest.mark.datafiles(DATA_DIR) +def test_default_platform_property_error(cli, datafiles): + project = str(datafiles) + element_path = os.path.join(project, "elements", "element.bst") + + # Write out our test target + element = { + "kind": "script", + "depends": [{"filename": "base.bst", "type": "build",},], + "config": {"commands": ["touch %{install-root}/foo",],}, + } + _yaml.roundtrip_dump(element, element_path) + + services = cli.ensure_services() + assert set(services) == set(["action-cache", "execution", "storage"]) + + # Add invalid platform property to remote execution config, this will override any + # valid [] keys generated for any other testing config. Default properties in relation + # to the local sandbox (e.g, OSFamily & ISO) should only be configured via sandbox config. + cli.config["remote-execution"]["platform-properties"]["OSFamily"] = "macos" + + # Try to build it, this should result in a Sanbox error when contructing the platform dict + result = cli.run(project=project, args=["build", "element.bst"]) + result.assert_task_error(ErrorDomain.SANDBOX, "invalid-platform-property") diff --git a/tox.ini b/tox.ini index 31a999db3..7f94dc8e1 100644 --- a/tox.ini +++ b/tox.ini @@ -64,8 +64,7 @@ passenv = SOURCE_CACHE_SERVICE SSL_CERT_FILE BST_PLUGINS_EXPERIMENTAL_VERSION - DEFAULT_PROPERTIES - CUSTOM_PROPERTIES + PLATFORM_PROPERTIES # # These keys are not inherited by any other sections #