WIP: custom platform properties

.gitlab-ci.yml

@@ -193,6 +193,31 @@ tests-remote-execution:
     REMOTE_EXECUTION_SERVICE: http://docker:50051
     SOURCE_CACHE_SERVICE: http://docker:50052
     PYTEST_ARGS: "--color=yes --remote-execution"
+    PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64" # Default properties, substitued in manifest
+
+tests-remote-execution-extra-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"
+    PLATFORM_PROPERTIES: "--platform OSFamily=linux --platform ISA=x86-64 --platform foo=bar" # Custom property 'foo' added
+
+tests-remote-execution-no-default-iso:
+  <<: *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"
+    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

.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 ${PLATFORM_PROPERTIES} --verbose )"]
     privileged: true
     volumes:
       - type: volume

.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

doc/source/format_project.rst

@@ -327,6 +327,9 @@ using the `remote-execution` option:
     action-cache-service:
       url: http://bar.action.com:50052
       instance-name: development-emea-1
+    platform-properties:
+      docker: docker://marketplace.gcr.io/google/rbe-ubuntu16-04
+
 
 storage-service specifies a remote CAS store and the parameters are the
 same as those used to specify an :ref:`artifact server <cache_servers>`.
@@ -347,6 +350,13 @@ name should be given to you by the service provider of each
 service. Not all remote execution and storage services support
 instance names.
 
+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.
 
 Remote execution configuration can be also provided in the `user

doc/source/using_config.rst

@@ -210,6 +210,9 @@ configuration will be used as fallback.
         action-cache-service:
           url: http://cache.some_project.example.com:50052
           instance-name: main
+        platform-properties:
+          docker-image: docker://marketplace.gcr.io/google/rbe-ubuntu16-04
+          ISA: []
 
 
 .. _user_config_strict_mode:

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 services can be set via 'remote-execution' config.
 sandbox: {}
 
 # Defaults for the 'split-rules' public data found on elements

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:

src/buildstream/sandbox/_sandboxreapi.py

@@ -119,12 +119,11 @@ 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 SandoxConfig
         config = self._get_config()
-
-        platform_dict = {}
-
-        platform_dict["OSFamily"] = config.build_os
-        platform_dict["ISA"] = config.build_arch
+        default_dict = {}
+        default_dict["OSFamily"] = config.build_os
+        default_dict["ISA"] = config.build_arch
 
         if flags & SandboxFlags.INHERIT_UID:
             uid = os.geteuid()
@@ -133,16 +132,36 @@ def _create_command(self, command, working_directory, environment, read_write_di
             uid = config.build_uid
             gid = config.build_gid
         if uid is not None:
-            platform_dict["unixUID"] = str(uid)
+            default_dict["unixUID"] = str(uid)
         if gid is not None:
-            platform_dict["unixGID"] = str(gid)
+            default_dict["unixGID"] = str(gid)
 
         if flags & SandboxFlags.NETWORK_ENABLED:
-            platform_dict["network"] = "on"
-
-        # Remove unsupported platform properties from the dict
+            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 platform_dict.items() if key in supported_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:
+                platform_dict[platform_property] = value
 
         # Create Platform message with properties sorted by name in code point order
         platform = remote_execution_pb2.Platform()
@@ -202,6 +221,9 @@ def _execute_action(self, action, flags):
     def _supported_platform_properties(self):
         return {"OSFamily", "ISA"}
 
+    def _get_platform_properties(self):
+        return {}
+
 
 # _SandboxREAPIBatch()
 #

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 platform_properties")
+):
     pass
 
 
@@ -96,6 +98,8 @@ def __init__(self, *args, **kwargs):
             self.action_instance = None
             self.action_credentials = None
 
+        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)
 
@@ -131,11 +135,12 @@ 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", "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={})
+        platform_properties = remote_config.get_mapping("platform-properties", default={})
 
         tls_keys = ["client-key", "client-cert", "server-cert"]
 
@@ -181,6 +186,9 @@ def resolve_path(path):
                 if tls_key in config:
                     config[tls_key] = resolve_path(config.get_str(tls_key))
 
+        # 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])
 
@@ -430,6 +438,10 @@ def _check_action_cache(self, action_digest):
                 self.info("Action result found in action cache")
                 return result
 
+    def _get_platform_properties(self):
+        # Dict platformn properties, if provided in config
+        return self.platform_properties
+
     @staticmethod
     def _extract_action_result(operation):
         if operation is None:

src/buildstream/testing/runcli.py

@@ -805,6 +805,23 @@ def cli_remote_execution(tmpdir, remote_services):
         remote_execution["storage-service"] = {
             "url": remote_services.storage_service,
         }
+    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:
         fixture.configure({"remote-execution": remote_execution})
 

tests/conftest.py

@@ -103,6 +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.platform_properties = kwargs.get("platform_properties")
 
 
 @pytest.fixture(scope="session")
@@ -126,6 +127,9 @@ def remote_services(request):
     if "SOURCE_CACHE_SERVICE" in os.environ:
         kwargs["source_service"] = os.environ.get("SOURCE_CACHE_SERVICE")
 
+    if "PLATFORM_PROPERTIES" in os.environ:
+        kwargs["platform_properties"] = os.environ.get("PLATFORM_PROPERTIES")
+
     return RemoteServices(**kwargs)
 
 

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")

tox.ini

@@ -64,6 +64,7 @@ passenv =
     SOURCE_CACHE_SERVICE
     SSL_CERT_FILE
     BST_PLUGINS_EXPERIMENTAL_VERSION
+    PLATFORM_PROPERTIES
 #
 # These keys are not inherited by any other sections
 #