Match undeclared platform properties dynamically instead of rejecting them

nativelink-config/src/schedulers.rs

@@ -116,6 +116,11 @@ pub struct SimpleSpec {
     /// have the `"cpu_arch"` label. We have no special treatment of any platform
     /// property labels other and entirely driven by worker configs and this
     /// config.
+    ///
+    /// Properties that are not listed here are matched dynamically: workers
+    /// that declare the key must match the value exactly, and workers that do
+    /// not declare the key are not restricted by it. List a property here to
+    /// enforce stricter matching.
     pub supported_platform_properties: Option<HashMap<String, PropertyType>>,
 
     /// The amount of time to retain completed actions for in case

nativelink-scheduler/BUILD.bazel

@@ -70,6 +70,7 @@ rust_test_suite(
         "tests/action_messages_test.rs",
         "tests/cache_lookup_scheduler_test.rs",
         "tests/historical_resource_scheduler_test.rs",
+        "tests/platform_property_manager_test.rs",
         "tests/property_modifier_scheduler_test.rs",
         "tests/redis_store_awaited_action_db_test.rs",
         "tests/simple_scheduler_state_manager_test.rs",

nativelink-scheduler/src/platform_property_manager.rs

@@ -15,7 +15,7 @@
 use std::collections::HashMap;
 
 use nativelink_config::schedulers::PropertyType;
-use nativelink_error::{Code, Error, ResultExt, make_input_err};
+use nativelink_error::{Code, Error, ResultExt};
 use nativelink_metric::{
     MetricFieldData, MetricKind, MetricPublishKnownKindData, MetricsComponent, group,
 };
@@ -75,6 +75,11 @@ impl PlatformPropertyManager {
     /// Given a specific key and value, returns the translated `PlatformPropertyValue`. This will
     /// automatically convert any strings to the type-value pairs of `PlatformPropertyValue` based
     /// on the configuration passed into the `PlatformPropertyManager` constructor.
+    ///
+    /// Keys that are not declared in the configuration become
+    /// `PlatformPropertyValue::Unknown` and are matched dynamically: workers
+    /// that declare the key must match the value exactly, workers that do not
+    /// declare the key are not restricted by it.
     pub fn make_prop_value(&self, key: &str, value: &str) -> Result<PlatformPropertyValue, Error> {
         if let Some(prop_type) = self.known_properties.get(key) {
             return match prop_type {
@@ -91,6 +96,6 @@ impl PlatformPropertyManager {
                 PropertyType::Ignore => Ok(PlatformPropertyValue::Ignore(value.to_string())),
             };
         }
-        Err(make_input_err!("Unknown platform property '{}'", key))
+        Ok(PlatformPropertyValue::Unknown(value.to_string()))
     }
 }

nativelink-scheduler/src/worker_capability_index.rs

@@ -123,7 +123,8 @@ impl WorkerCapabilityIndex {
     /// The caller should apply additional filtering (e.g., worker availability).
     ///
     /// IMPORTANT: This method returns candidates based on STATIC properties only.
-    /// - Exact and Unknown properties are fully matched
+    /// - Exact and Unknown properties are fully matched; Unknown properties
+    ///   additionally match workers that do not declare the key
     /// - Priority properties just require the key to exist
     /// - Minimum properties return workers that HAVE the property (presence check only)
     ///
@@ -157,7 +158,18 @@ impl WorkerCapabilityIndex {
                         value: value.clone(),
                     };
 
-                    let matching = self.exact_index.get(&key).cloned().unwrap_or_default();
+                    let mut matching = self.exact_index.get(&key).cloned().unwrap_or_default();
+
+                    // Unknown properties only restrict workers that declare the
+                    // key, so workers without the key remain candidates.
+                    if matches!(value, PlatformPropertyValue::Unknown(_)) {
+                        match self.property_presence.get(name) {
+                            Some(with_key) => {
+                                matching.extend(self.all_workers.difference(with_key).cloned());
+                            }
+                            None => matching.extend(self.all_workers.iter().cloned()),
+                        }
+                    }
 
                     let internal_candidates = match candidates {
                         Some(existing) => existing.intersection(&matching).cloned().collect(),

nativelink-scheduler/tests/platform_property_manager_test.rs

@@ -0,0 +1,114 @@
+// Copyright 2024 The NativeLink Authors. All rights reserved.
+//
+// Licensed under the Functional Source License, Version 1.1, Apache 2.0 Future License (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    See LICENSE file for details
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Tests for the platform property manager.
+
+use std::collections::HashMap;
+
+use nativelink_config::schedulers::PropertyType;
+use nativelink_scheduler::platform_property_manager::PlatformPropertyManager;
+use nativelink_util::platform_properties::PlatformPropertyValue;
+
+fn make_manager(props: &[(&str, PropertyType)]) -> PlatformPropertyManager {
+    PlatformPropertyManager::new(
+        props
+            .iter()
+            .map(|(name, prop_type)| ((*name).to_string(), *prop_type))
+            .collect(),
+    )
+}
+
+#[test]
+fn known_properties_are_typed_from_config() {
+    let manager = make_manager(&[
+        ("cpu_count", PropertyType::Minimum),
+        ("cpu_arch", PropertyType::Exact),
+        ("priority", PropertyType::Priority),
+        ("ignored", PropertyType::Ignore),
+    ]);
+
+    assert_eq!(
+        manager.make_prop_value("cpu_count", "8").unwrap(),
+        PlatformPropertyValue::Minimum(8)
+    );
+    assert_eq!(
+        manager.make_prop_value("cpu_arch", "aarch64").unwrap(),
+        PlatformPropertyValue::Exact("aarch64".to_string())
+    );
+    assert_eq!(
+        manager.make_prop_value("priority", "high").unwrap(),
+        PlatformPropertyValue::Priority("high".to_string())
+    );
+    assert_eq!(
+        manager.make_prop_value("ignored", "foo").unwrap(),
+        PlatformPropertyValue::Ignore("foo".to_string())
+    );
+}
+
+#[test]
+fn minimum_property_requires_u64_value() {
+    let manager = make_manager(&[("cpu_count", PropertyType::Minimum)]);
+    assert!(
+        manager
+            .make_prop_value("cpu_count", "not-a-number")
+            .is_err()
+    );
+}
+
+#[test]
+fn undeclared_property_becomes_unknown() {
+    let manager = make_manager(&[("cpu_arch", PropertyType::Exact)]);
+
+    assert_eq!(
+        manager
+            .make_prop_value("InputRootAbsolutePath", "/some/path")
+            .unwrap(),
+        PlatformPropertyValue::Unknown("/some/path".to_string())
+    );
+}
+
+#[test]
+fn undeclared_property_does_not_fail_platform_properties() {
+    let manager = make_manager(&[("cpu_count", PropertyType::Minimum)]);
+
+    let mut request = HashMap::new();
+    request.insert("cpu_count".to_string(), "4".to_string());
+    request.insert("gpu_model".to_string(), "a100".to_string());
+
+    let platform_properties = manager.make_platform_properties(request).unwrap();
+    assert_eq!(
+        platform_properties.properties.get("cpu_count").unwrap(),
+        &PlatformPropertyValue::Minimum(4)
+    );
+    assert_eq!(
+        platform_properties.properties.get("gpu_model").unwrap(),
+        &PlatformPropertyValue::Unknown("a100".to_string())
+    );
+}
+
+#[test]
+fn empty_config_accepts_any_property() {
+    let manager = make_manager(&[]);
+
+    let mut request = HashMap::new();
+    request.insert("OSFamily".to_string(), "linux".to_string());
+    request.insert("container-image".to_string(), "docker://foo".to_string());
+
+    let platform_properties = manager.make_platform_properties(request).unwrap();
+    assert_eq!(platform_properties.properties.len(), 2);
+    assert_eq!(
+        platform_properties.properties.get("OSFamily").unwrap(),
+        &PlatformPropertyValue::Unknown("linux".to_string())
+    );
+}

nativelink-scheduler/tests/worker_capability_index_test.rs

@@ -274,3 +274,49 @@ fn test_no_priority_property_match() {
         "No candidate workers due to a lack of key 'os'. Job asked for Priority(\"linux\")"
     ));
 }
+
+#[test]
+fn test_unknown_property_matches_declared_value_or_absent_key() {
+    let mut index = WorkerCapabilityIndex::new();
+
+    let worker1 = make_worker_id("worker1");
+    let worker2 = make_worker_id("worker2");
+    let worker3 = make_worker_id("worker3");
+
+    index.add_worker(
+        &worker1,
+        &make_properties(&[("gpu", PlatformPropertyValue::Unknown("a100".to_string()))]),
+    );
+    index.add_worker(
+        &worker2,
+        &make_properties(&[("gpu", PlatformPropertyValue::Unknown("v100".to_string()))]),
+    );
+    index.add_worker(&worker3, &make_properties(&[]));
+
+    // Workers that declare the key must match the value, workers that do not
+    // declare the key remain candidates.
+    let props = make_properties(&[("gpu", PlatformPropertyValue::Unknown("a100".to_string()))]);
+    let result = index.find_matching_workers(&props, true);
+    assert_eq!(result.len(), 2);
+    assert!(result.contains(&worker1));
+    assert!(result.contains(&worker3));
+}
+
+#[test]
+fn test_unknown_property_no_worker_declares_key() {
+    let mut index = WorkerCapabilityIndex::new();
+
+    let worker1 = make_worker_id("worker1");
+    index.add_worker(
+        &worker1,
+        &make_properties(&[("os", PlatformPropertyValue::Exact("linux".to_string()))]),
+    );
+
+    let props = make_properties(&[(
+        "InputRootAbsolutePath",
+        PlatformPropertyValue::Unknown("/some/path".to_string()),
+    )]);
+    let result = index.find_matching_workers(&props, true);
+    assert_eq!(result.len(), 1);
+    assert!(result.contains(&worker1));
+}

nativelink-util/src/platform_properties.rs

@@ -68,6 +68,10 @@ impl PlatformProperties {
                     return false;
                 }
             } else {
+                // Unknown properties only restrict workers that declare the key.
+                if let PlatformPropertyValue::Unknown(_) = check_value {
+                    continue;
+                }
                 if full_worker_logging {
                     info!("Property missing on worker property {property}");
                 }
@@ -121,6 +125,9 @@ impl From<&PlatformProperties> for ProtoPlatform {
 /// Ignore   - Jobs can request this key, but workers do not have to have it. This allows
 ///            for example the `InputRootAbsolutePath` case for chromium builds, where we can safely
 ///            ignore it without having to change the worker configs.
+/// Unknown  - The key was not declared in the scheduler's configuration. Workers
+///            that declare the key must match the value exactly, workers that do
+///            not declare the key are not restricted by it.
 #[derive(Eq, PartialEq, Hash, Clone, Ord, PartialOrd, Debug, Serialize, Deserialize)]
 pub enum PlatformPropertyValue {
     Exact(String),
@@ -148,8 +155,11 @@ impl PlatformPropertyValue {
             // workers can be selected, but might be used to prefer certain workers
             // over others.
             Self::Priority(_) | Self::Ignore(_) => true,
+            // Unknown properties are not typed by the scheduler config, so
+            // compare by value regardless of the worker's variant.
+            Self::Unknown(value) => worker_value.as_str() == value.as_str(),
             // Success exact case is handled above.
-            Self::Exact(_) | Self::Unknown(_) => false,
+            Self::Exact(_) => false,
         }
     }
 

nativelink-util/tests/platform_properties_tests.rs

@@ -21,6 +21,43 @@ fn ignore_property_match_all() {
     assert!(ignore_properties.is_satisfied_by(&PlatformProperties::new(HashMap::new()), true));
 }
 
+#[test]
+fn unknown_property_matches_worker_with_same_value() {
+    let unknown_property = PlatformPropertyValue::Unknown("foo".to_string());
+    assert!(unknown_property.is_satisfied_by(&PlatformPropertyValue::Unknown("foo".to_string())));
+    assert!(unknown_property.is_satisfied_by(&PlatformPropertyValue::Exact("foo".to_string())));
+    assert!(!unknown_property.is_satisfied_by(&PlatformPropertyValue::Unknown("bar".to_string())));
+    assert!(!unknown_property.is_satisfied_by(&PlatformPropertyValue::Exact("bar".to_string())));
+}
+
+#[test]
+fn unknown_property_does_not_restrict_worker_without_key() {
+    let mut property_map = HashMap::new();
+    property_map.insert(
+        "foo".into(),
+        PlatformPropertyValue::Unknown("bar".to_string()),
+    );
+    let unknown_properties = PlatformProperties::new(property_map);
+
+    // A worker that does not declare the key is not restricted by it.
+    assert!(unknown_properties.is_satisfied_by(&PlatformProperties::new(HashMap::new()), true));
+
+    // A worker that declares the key must match the value.
+    let mut mismatched_map = HashMap::new();
+    mismatched_map.insert(
+        "foo".into(),
+        PlatformPropertyValue::Unknown("baz".to_string()),
+    );
+    assert!(!unknown_properties.is_satisfied_by(&PlatformProperties::new(mismatched_map), true));
+
+    let mut matched_map = HashMap::new();
+    matched_map.insert(
+        "foo".into(),
+        PlatformPropertyValue::Unknown("bar".to_string()),
+    );
+    assert!(unknown_properties.is_satisfied_by(&PlatformProperties::new(matched_map), true));
+}
+
 #[nativelink_test]
 fn minimum_property_logs_error() {
     let minimum_property = PlatformPropertyValue::Minimum(1);