WIP custom platform properties

11 files changed, 197 insertions, 30 deletions

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 <cache_servers>`.
@@ -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 @@ class SandboxREAPI(Sandbox):
         # 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 @@ class SandboxREAPI(Sandbox):
     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 .._cas import CASRemote
 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 @@ class SandboxRemote(SandboxREAPI):
             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 @@ class SandboxRemote(SandboxREAPI):
 
         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 @@ class SandboxRemote(SandboxREAPI):
                 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 @@ class SandboxRemote(SandboxREAPI):
                 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 @@ class RemoteServices:
         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
 #