From b6454c43c852c39f9fc9af6e303c7624e0895d50 Mon Sep 17 00:00:00 2001 From: Aman Kumar Date: Fri, 14 Aug 2026 21:37:08 +0530 Subject: [PATCH] Match undeclared platform properties dynamically instead of rejecting them --- nativelink-config/src/schedulers.rs | 5 + nativelink-scheduler/BUILD.bazel | 1 + .../src/platform_property_manager.rs | 9 +- .../src/worker_capability_index.rs | 16 ++- .../tests/platform_property_manager_test.rs | 114 ++++++++++++++++++ .../tests/worker_capability_index_test.rs | 46 +++++++ nativelink-util/src/platform_properties.rs | 12 +- .../tests/platform_properties_tests.rs | 37 ++++++ 8 files changed, 235 insertions(+), 5 deletions(-) create mode 100644 nativelink-scheduler/tests/platform_property_manager_test.rs diff --git a/nativelink-config/src/schedulers.rs b/nativelink-config/src/schedulers.rs index 3b840e545..e0e66b04c 100644 --- a/nativelink-config/src/schedulers.rs +++ b/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>, /// The amount of time to retain completed actions for in case diff --git a/nativelink-scheduler/BUILD.bazel b/nativelink-scheduler/BUILD.bazel index 06a67f680..450ab3a5f 100644 --- a/nativelink-scheduler/BUILD.bazel +++ b/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", diff --git a/nativelink-scheduler/src/platform_property_manager.rs b/nativelink-scheduler/src/platform_property_manager.rs index 81201c0ff..cb68ce8bd 100644 --- a/nativelink-scheduler/src/platform_property_manager.rs +++ b/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 { 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())) } } diff --git a/nativelink-scheduler/src/worker_capability_index.rs b/nativelink-scheduler/src/worker_capability_index.rs index 1453a1217..2d7cb7253 100644 --- a/nativelink-scheduler/src/worker_capability_index.rs +++ b/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(), diff --git a/nativelink-scheduler/tests/platform_property_manager_test.rs b/nativelink-scheduler/tests/platform_property_manager_test.rs new file mode 100644 index 000000000..6e33a0c4a --- /dev/null +++ b/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()) + ); +} diff --git a/nativelink-scheduler/tests/worker_capability_index_test.rs b/nativelink-scheduler/tests/worker_capability_index_test.rs index dea773c5a..5f023857b 100644 --- a/nativelink-scheduler/tests/worker_capability_index_test.rs +++ b/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)); +} diff --git a/nativelink-util/src/platform_properties.rs b/nativelink-util/src/platform_properties.rs index e234ae138..be9c040b0 100644 --- a/nativelink-util/src/platform_properties.rs +++ b/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, } } diff --git a/nativelink-util/tests/platform_properties_tests.rs b/nativelink-util/tests/platform_properties_tests.rs index 134e9c58a..cef7be83d 100644 --- a/nativelink-util/tests/platform_properties_tests.rs +++ b/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);