From 04543c2d990c54bb804f551440fa61f08d624b7a Mon Sep 17 00:00:00 2001 From: Bill Minckler Date: Mon, 17 Aug 2026 14:42:41 +0000 Subject: [PATCH 1/3] Fix SKU validation allocation bypass --- crates/api-core/src/cfg/README.md | 2 +- crates/api-core/src/tests/sku.rs | 144 +++++++++++++----- .../src/config/bom_validation.rs | 5 +- crates/machine-controller/src/handler/sku.rs | 41 ++--- docs/provisioning/sku-validation.md | 14 +- 5 files changed, 138 insertions(+), 68 deletions(-) diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index 0584db0967..1a676cd073 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -812,7 +812,7 @@ be propagated there by DPF. | ------- | ------ | --------- | ------------- | | `enabled` | `bool` | `false` | Enable BOM/SKU validation. | | `ignore_unassigned_machines` | `bool` | `false` | Let machines without a SKU bypass validation. | -| `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines allocatable even when validation fails. | +| `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines with assigned SKUs allocatable on validation failure; does not bypass unassigned machines. | | `find_match_interval` | `Duration` | `5m` | Interval between SKU match attempts. | | `auto_generate_missing_sku` | `bool` | `false` | Auto-create missing SKUs from expected machines. | | `auto_generate_missing_sku_interval` | `Duration` | `5m` | Interval between auto-generate attempts. | diff --git a/crates/api-core/src/tests/sku.rs b/crates/api-core/src/tests/sku.rs index 6446a68abd..fe633e143a 100644 --- a/crates/api-core/src/tests/sku.rs +++ b/crates/api-core/src/tests/sku.rs @@ -50,8 +50,8 @@ pub(in crate::tests) mod tests { // A = allow_allocation_on_validation_failure (F=block on failure, T=allow allocation despite failures) // B = auto_generate_missing_sku (F=manual, T=auto-generate) // - // When allow_allocation_on_validation_failure=true, machines in failed states - // (SkuVerificationFailed, SkuMissing, WaitingForSkuAssignment) can transition + // When allow_allocation_on_validation_failure=true, machines with assigned + // SKUs in failed states (SkuVerificationFailed or SkuMissing) can transition // back to Ready/MachineValidation instead of staying blocked. // // ================================================================================ @@ -72,7 +72,7 @@ pub(in crate::tests) mod tests { // │2.1 │ F │ F │ WaitingForSkuAssignment │ Stays in Waiting │ test_stays_in_waiting_state_when_not_assigned │ // │2.2 │ F │ F │ Waiting → Test assigns SKU │ → UpdatingInventory │ test_leave_waiting_when_assigned │ // │2.3 │ F │ F │ No SKU assigned │ Stuck in Waiting │ test_stuck_in_waiting_without_sku │ - // │2.4 │ T │ F │ No SKU (skips Waiting) │ Panic (never enters) │ test_escapes_waiting_with_allow_allocation │ + // │2.4 │ T │ F │ No SKU assigned │ Stays in Waiting │ test_allow_allocation_does_not_bypass_unassigned_sku │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │ 3. SkuMissing State │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ @@ -97,7 +97,7 @@ pub(in crate::tests) mod tests { // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │6.1 │ F │ * │ Second machine, same HW │ Auto-match existing → Ready│ test_auto_match_sku │ // │6.2 │ F │ * │ SKU replacement triggers │ Re-verify (to BomValidating)│ test_replace_triggers_verify │ - // │6.3 │ F │ * │ Unassign + clear status │ Immediate re-match → Ready │ test_auto_match_after_unassign │ + // │6.3 │ F │ * │ Unassign + retained status │ Immediate re-match → Ready │ test_auto_match_after_unassign │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │ 7. SKU Version Compatibility │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ @@ -294,6 +294,17 @@ pub(in crate::tests) mod tests { test_env } + async fn create_test_env_ignoring_unassigned_machines(db_pool: sqlx::PgPool) -> TestEnv { + let mut overrides = TestEnvOverrides::default(); + let mut config = get_config(); + config.bom_validation.enabled = true; + config.bom_validation.ignore_unassigned_machines = true; + config.bom_validation.allow_allocation_on_validation_failure = false; + overrides.config = Some(config); + + create_test_env_with_overrides(db_pool, overrides).await + } + async fn get_machine_state(pool: &sqlx::PgPool, mh: &TestManagedHost) -> ManagedHostState { let mut txn = pool.begin().await.unwrap(); let machine = mh.host().db_machine(&mut txn).await; @@ -337,23 +348,6 @@ pub(in crate::tests) mod tests { .ok_or_else(|| eyre::eyre!("machine not found: {}", machine_id)) } - /// Helper: Clear the SKU status/timestamp on a machine to allow re-matching - /// Used in tests to reset the SKU matching state and test re-match behavior - pub(in crate::tests) async fn clear_sku_status( - txn: &mut PgConnection, - machine_id: &MachineId, - ) -> Result<(), DatabaseError> { - let query = "UPDATE machines SET hw_sku_status=null WHERE id=$1 RETURNING id"; - - let _: () = sqlx::query_as(query) - .bind(machine_id) - .fetch_one(txn) - .await - .map_err(|e| DatabaseError::new("clear sku last match attempt", e))?; - - Ok(()) - } - /// Helper: Test SKU version compatibility and backward compatibility /// Creates a machine with latest SKU version, then generates an older version SKU /// from the same hardware to verify version handling and diff behavior @@ -804,21 +798,18 @@ pub(in crate::tests) mod tests { Ok(()) } - /// Test 2.4: Never enters WaitingForSkuAssignment with allow_allocation_on_validation_failure=true + /// Test 2.4: WaitingForSkuAssignment with allow_allocation_on_validation_failure=true /// Conditions: /// - allow_allocation_on_validation_failure = true /// - auto_generate_missing_sku = false /// - auto_assign_sku_in_fixture = false - /// - expected_state = WaitingForSkuAssignment - /// Expected: fixture panics because machine never enters WaitingForSkuAssignment - /// (machine directly skips to MachineValidation, proving allow_allocation logic works) + /// Expected: machine enters WaitingForSkuAssignment because no SKU validation occurred #[crate::sqlx_test] - #[should_panic(expected = "Expected Machine state condition not hit after")] - async fn test_escapes_waiting_with_allow_allocation(pool: sqlx::PgPool) { + async fn test_allow_allocation_does_not_bypass_unassigned_sku( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { let env = create_test_env_for_bom_validation(pool.clone(), true, None, false).await; - // Expect WaitingForSkuAssignment, but machine will never enter that state - // because allow_allocation=true makes it skip directly to MachineValidation let mut config = ManagedHostConfig::default().with_expected_state(ManagedHostState::BomValidating { bom_validating_state: BomValidating::WaitingForSkuAssignment( @@ -830,9 +821,50 @@ pub(in crate::tests) mod tests { }); config.auto_assign_sku_in_fixture = false; - // This will panic because machine never enters WaitingForSkuAssignment - // (it goes directly from MatchingSku to MachineValidation) - let _mh = create_managed_host_with_config(&env, config).await; + let mh = create_managed_host_with_config(&env, config).await; + let mut txn = pool.begin().await?; + let machine = mh.host().db_machine(&mut txn).await; + + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::WaitingForSkuAssignment(_) + } + )); + assert!(machine.config.hw_sku.is_none()); + + Ok(()) + } + + /// Test 2.5: Machine creation with ignore_unassigned_machines=true + /// Conditions: + /// - ignore_unassigned_machines = true + /// - no SKU is assigned + /// Expected: machine reaches Ready state without an assigned SKU + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_bypasses_unassigned_sku( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + let config = ManagedHostConfig { + auto_assign_sku_in_fixture: false, + ..ManagedHostConfig::default() + }; + + let mh = create_managed_host_with_config(&env, config).await; + let mut txn = pool.begin().await?; + let machine = mh.host().db_machine(&mut txn).await; + + assert_eq!(machine.current_state(), &ManagedHostState::Ready); + assert!(machine.config.hw_sku.is_none()); + assert!( + machine + .status + .hw_sku + .is_some_and(|status| status.last_match_attempt.is_some()) + ); + + Ok(()) } /// Test 3.1: SkuMissing State → stays in SkuMissing @@ -1490,6 +1522,44 @@ pub(in crate::tests) mod tests { Ok(()) } + /// Test 5.1a: Assigned SKU verification failure with ignore_unassigned_machines=true + /// Conditions: + /// - ignore_unassigned_machines = true + /// - allow_allocation_on_validation_failure = false + /// - machine has an assigned SKU that does not match hardware + /// Expected: machine enters SkuVerificationFailed and remains unallocatable + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_does_not_bypass_assigned_sku_verification( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + + let mh = create_managed_host(&env).await; + let machine_id = mh.host().id; + + let mut txn = pool.begin().await?; + let current_sku = db::sku::generate_sku_from_machine(txn.as_mut(), &machine_id).await?; + db::sku::create(&mut txn, ¤t_sku).await?; + assign_mismatched_sku(&mut txn, &machine_id, ¤t_sku.id).await?; + db::machine::update_sku_status_verify_request_time(&mut txn, &machine_id).await?; + txn.commit().await?; + + handle_inventory_update(&pool, &env, &mh).await; + env.run_machine_state_controller_iteration().await; + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::SkuVerificationFailed(_) + } + )); + + Ok(()) + } + /// Test 5.2: SKU replaced but no re-verification triggered → continues to Ready /// Conditions: /// - allow_allocation_on_validation_failure = false (standard mode) @@ -1731,16 +1801,13 @@ pub(in crate::tests) mod tests { Ok(()) } - /// Test 6.3: Auto-matching SKU after unassign and status clear + /// Test 6.3: Auto-matching SKU after unassign with retained SKU status /// Conditions: /// - allow_allocation_on_validation_failure = false (standard mode) /// - auto_generate_missing_sku = false /// - Machine reaches Ready with auto-matched SKU - /// - SKU is unassigned and hw_sku_status is cleared (simulating fresh start) + /// - SKU is unassigned while hw_sku_status retains a verification request /// Expected: Machine re-enters BOM validation flow and successfully re-matches the same SKU - /// Note: Status is cleared to bypass find_match_interval for immediate re-matching. The - /// find_match_interval config throttles retry attempts when NO matching SKU is found, - /// not when a matching SKU exists (which matches immediately). #[crate::sqlx_test] async fn test_auto_match_after_unassign(pool: sqlx::PgPool) -> Result<(), eyre::Error> { let env = create_test_env_for_bom_validation( @@ -1797,8 +1864,7 @@ pub(in crate::tests) mod tests { assert_eq!(machine.config.hw_sku, Some(expected_sku.id.clone())); assert_eq!(machine.current_state(), &ManagedHostState::Ready); - clear_sku_status(&mut txn, &machine_id).await?; - // test that an unassigned can find and assign a machine. + db::machine::update_sku_status_verify_request_time(&mut txn, &machine_id).await?; db::machine::unassign_sku(&mut txn, &machine_id).await?; txn.commit().await?; diff --git a/crates/machine-controller/src/config/bom_validation.rs b/crates/machine-controller/src/config/bom_validation.rs index 46b2cd29a9..4bbb51d712 100644 --- a/crates/machine-controller/src/config/bom_validation.rs +++ b/crates/machine-controller/src/config/bom_validation.rs @@ -32,10 +32,11 @@ pub struct BomValidationConfig { #[serde(default)] pub ignore_unassigned_machines: bool, - /// Allow machines to stay in Ready state and remain allocatable even when SKU validation fails + /// Allow machines with assigned SKUs to stay in Ready state and remain allocatable even when SKU validation fails /// When false (default): Standard mode - validation failures block allocation (machine enters failed state) /// When true: Allow allocation mode - validation still occurs and health reports are recorded, but machines do not transition - /// into failed states (SkuVerificationFailed, SkuMissing, WaitingForSkuAssignment) and can proceed to Ready/MachineValidation + /// into failed states (SkuVerificationFailed, SkuMissing) and can proceed to Ready/MachineValidation + /// This does not bypass machines without an assigned SKU; use ignore_unassigned_machines for that. #[serde(default)] pub allow_allocation_on_validation_failure: bool, diff --git a/crates/machine-controller/src/handler/sku.rs b/crates/machine-controller/src/handler/sku.rs index 70acc75c91..55f36e0627 100644 --- a/crates/machine-controller/src/handler/sku.rs +++ b/crates/machine-controller/src/handler/sku.rs @@ -73,13 +73,11 @@ async fn match_sku_for_machine( mh_snapshot: &ManagedHostStateSnapshot, ) -> Result, StateHandlerError> { let sku_status = mh_snapshot.host_snapshot.status.hw_sku.as_ref(); - if sku_status.is_none() - || sku_status.is_some_and(|ss| { - ss.last_match_attempt.is_some_and(|t| { - t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) - }) + if sku_status.is_none_or(|ss| { + ss.last_match_attempt.is_none_or(|t| { + t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) }) - { + }) { let machine_sku = db::sku::generate_sku_from_machine(&mut *txn, &mh_snapshot.host_snapshot.id).await?; let matching_sku = db::sku::find_matching(txn, &machine_sku).await?; @@ -188,16 +186,18 @@ async fn generate_missing_sku_for_machine( true } -/// Helper function to determine if machine should be allowed to allocate even when validation fails -/// This is useful to avoid blocking allocation when SKU validation issues occur +/// Determine whether a machine with an assigned SKU can allocate despite validation failure. fn should_allow_allocation_on_validation_failure(host_handler_params: &HostHandlerParams) -> bool { - // TODO:tmp solution to consider ignore_unassigned_machines for compatibale with some running sites + host_handler_params + .bom_validation + .allow_allocation_on_validation_failure +} + +/// Determine whether a machine without an assigned SKU can bypass BOM validation. +fn should_ignore_unassigned_machine(host_handler_params: &HostHandlerParams) -> bool { host_handler_params .bom_validation .ignore_unassigned_machines - || host_handler_params - .bom_validation - .allow_allocation_on_validation_failure } pub(crate) async fn handle_bom_validation_requested( @@ -234,12 +234,13 @@ pub(crate) async fn handle_bom_validation_requested( "Cannot find a matching SKU for machine" ); - if should_allow_allocation_on_validation_failure(host_handler_params) { - // Case 1.2.1: Allow allocation despite no SKU match + if should_ignore_unassigned_machine(host_handler_params) { + // Case 1.2.1: Allow allocation without an assigned SKU tracing::info!( machine_id=%mh_snapshot.host_snapshot.id, - "allow_allocation_on_validation_failure is true, staying in Ready state" + "ignore_unassigned_machines is true, staying in Ready state" ); + txn.commit().await?; return Ok(None); } else { // Case 1.2.2: Block allocation, wait for SKU assignment @@ -354,14 +355,14 @@ async fn advance_to_waiting_for_sku_assignment( mh_snapshot: &ManagedHostStateSnapshot, host_handler_params: &HostHandlerParams, ) -> Result, StateHandlerError> { - if should_allow_allocation_on_validation_failure(host_handler_params) + if should_ignore_unassigned_machine(host_handler_params) && mh_snapshot.host_snapshot.config.hw_sku.is_none() { skip_bom_validation_and_advance( txn, host_handler_params, mh_snapshot, - "allow_allocation_when_sku_unassigned", + "ignore_unassigned_machine_before_waiting", ) .await } else { @@ -414,7 +415,7 @@ async fn advance_to_machine_validating( } /// Skip BOM validation and proceed to machine validation (or Ready if machine validation is disabled) -/// Used when BOM validation is disabled or when allow_allocation_on_validation_failure is enabled +/// Used when BOM validation is disabled or a configured policy bypasses BOM validation async fn skip_bom_validation_and_advance( txn: PgTransaction<'static>, host_handler_params: &HostHandlerParams, @@ -645,13 +646,13 @@ pub(crate) async fn handle_bom_validation_state( .is_some() { advance_to_updating_inventory(txn, mh_snapshot).await - } else if should_allow_allocation_on_validation_failure(host_handler_params) { + } else if should_ignore_unassigned_machine(host_handler_params) { // Allow machine to proceed without SKU assignment skip_bom_validation_and_advance( txn, host_handler_params, mh_snapshot, - "allow_allocation_on_validation_failure", + "ignore_unassigned_machine_while_waiting", ) .await } else { diff --git a/docs/provisioning/sku-validation.md b/docs/provisioning/sku-validation.md index 10aafaf5af..1ae905a37e 100644 --- a/docs/provisioning/sku-validation.md +++ b/docs/provisioning/sku-validation.md @@ -66,12 +66,14 @@ auto_generate_missing_sku_interval = "300s" - `enabled` - Enables or disables the entire bom validation process. When disabled, machines will skip bom validation and proceed as if all validation has passed. - - `allow_allocation_on_validation_failure` - When true, machines are allowed to stay in Ready state and remain allocatable - even when SKU validation fails. Validation still occurs but only logs are recorded - health reports are cleared instead - of recording validation failures. Machines do not transition into failed states (SkuVerificationFailed, SkuMissing, - WaitingForSkuAssignment). When false (default), standard mode applies where validation failures are recorded in health - reports and machines enter failed states and become unallocatable until fixed. This is useful for avoiding machine - allocation blockage due to SKU validation issues when you only need logging without health report alerts. + - `allow_allocation_on_validation_failure` - When true, machines with an assigned SKU are allowed to stay in Ready state + and remain allocatable even when SKU validation fails. Validation still occurs but only logs are recorded - health reports + are cleared instead of recording validation failures. Machines do not transition into failed states + (SkuVerificationFailed or SkuMissing). When false (default), standard mode applies where validation failures are recorded + in health reports and machines enter failed states and become unallocatable until fixed. This is useful for avoiding machine + allocation blockage due to SKU validation issues when you only need logging without health report alerts. This option does + not bypass a machine without an assigned SKU; with `ignore_unassigned_machines = false`, that machine waits for SKU + assignment. - `ignore_unassigned_machines` - When true and BOM validation encounters a machine that does not have an associated SKU, it will proceed as if all validation has passed. Only machines with an associated SKU will be validated. This allows existing sites to be upgraded and BOM Validation enabled as SKUs are added to the system without impacting site operation. From d4007bb7b0c56a261a13704a2625652d15198cb9 Mon Sep 17 00:00:00 2001 From: Bill Minckler Date: Wed, 19 Aug 2026 16:09:26 +0000 Subject: [PATCH 2/3] Add SKU bypass regression coverage --- crates/api-core/src/cfg/README.md | 2 +- crates/api-core/src/tests/sku.rs | 293 ++++++++++++++++-- .../src/config/bom_validation.rs | 4 +- docs/provisioning/sku-validation.md | 22 +- 4 files changed, 280 insertions(+), 41 deletions(-) diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index 1a676cd073..50ac3c5d3f 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -811,7 +811,7 @@ be propagated there by DPF. | Field | Type | Default | Description | | ------- | ------ | --------- | ------------- | | `enabled` | `bool` | `false` | Enable BOM/SKU validation. | -| `ignore_unassigned_machines` | `bool` | `false` | Let machines without a SKU bypass validation. | +| `ignore_unassigned_machines` | `bool` | `false` | When enabled, a machine that remains unassigned can skip BOM validation and proceed; machines with an assigned SKU are still validated. | | `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines with assigned SKUs allocatable on validation failure; does not bypass unassigned machines. | | `find_match_interval` | `Duration` | `5m` | Interval between SKU match attempts. | | `auto_generate_missing_sku` | `bool` | `false` | Auto-create missing SKUs from expected machines. | diff --git a/crates/api-core/src/tests/sku.rs b/crates/api-core/src/tests/sku.rs index fe633e143a..8ca8fb23a4 100644 --- a/crates/api-core/src/tests/sku.rs +++ b/crates/api-core/src/tests/sku.rs @@ -46,7 +46,7 @@ pub(in crate::tests) mod tests { // This test suite validates SKU (Stock Keeping Unit) validation behavior across // different configuration combinations and machine states. // - // Key configuration parameters (A×B matrix): + // Key configuration parameters (A×B matrix; policy-specific rows set ignore=T inline): // A = allow_allocation_on_validation_failure (F=block on failure, T=allow allocation despite failures) // B = auto_generate_missing_sku (F=manual, T=auto-generate) // @@ -67,16 +67,21 @@ pub(in crate::tests) mod tests { // │1.1 │ F │ * │ Machine created, no SKU │ Ready (fixture assigns SKU)│ test_machine_creation_succeeds_when_not_assigned │ // │1.2 │ F │ * │ Same as 1.1, fixture off │ Stuck in WaitingForSku │ test_machine_creation_without_auto_sku │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ - // │ 2. WaitingForSkuAssignment State │ + // │ 2. Unassigned SKU Handling │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │2.1 │ F │ F │ WaitingForSkuAssignment │ Stays in Waiting │ test_stays_in_waiting_state_when_not_assigned │ // │2.2 │ F │ F │ Waiting → Test assigns SKU │ → UpdatingInventory │ test_leave_waiting_when_assigned │ // │2.3 │ F │ F │ No SKU assigned │ Stuck in Waiting │ test_stuck_in_waiting_without_sku │ // │2.4 │ T │ F │ No SKU assigned │ Stays in Waiting │ test_allow_allocation_does_not_bypass_unassigned_sku │ + // │2.5 │ F │ F │ ignore=T: No SKU assigned │ Ready │ test_ignore_unassigned_machines_bypasses_unassigned_sku │ + // │2.6 │ F │ F │ ignore=T: Already waiting │ → MachineValidation │ test_ignore_unassigned_machines_releases_waiting_machine │ + // │2.7 │ F │ F │ ignore=T: Matching SKU │ → UpdatingInventory │ test_ignore_unassigned_machines_matches_before_bypassing │ + // │2.8 │ F │ F │ ignore=T: Match status unset│ → UpdatingInventory │ test_ignore_unassigned_matches_without_prior_attempt │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │ 3. SkuMissing State │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │3.1 │ F │ F │ SKU assigned but missing │ Stays in SkuMissing │ test_stays_in_missing_state_when_assigned_sku_is_missing │ + // │3.1a│ F │ F │ ignore=T: SKU missing │ Stays in SkuMissing │ test_ignore_unassigned_machines_does_not_bypass_missing │ // │3.2 │ F │ F │ SkuMissing → Remove SKU ID │ → WaitingForSkuAssignment │ test_proceeds_when_sku_missing │ // │3.3 │ F │ T │ SKU assigned but missing │ Auto-gen → Ready │ test_auto_generates_sku_when_missing │ // │3.4 │ T │ F │ SkuMissing state │ Escapes → MachineValidation│ test_allow_allocation_escapes_from_sku_missing │ @@ -90,6 +95,7 @@ pub(in crate::tests) mod tests { // │ 5. SKU Verification Failure Scenarios │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │5.1 │ F │ * │ Re-verify mismatched SKU │ → VerificationFailed │ test_stays_in_failed_when_verification_fails │ + // │5.1a│ F │ * │ ignore=T: Assigned mismatch │ → VerificationFailed │ test_ignore_unassigned_machines_does_not_bypass_mismatch │ // │5.2 │ F │ * │ Replace SKU, no re-verify │ Stays Ready (no validation)│ test_continues_to_ready_when_verification_fails │ // │5.3 │ T │ * │ SkuVerificationFailed state │ Escapes → Ready │ test_allow_allocation_escapes_from_sku_failed │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ @@ -107,6 +113,7 @@ pub(in crate::tests) mod tests { // Legend: // A = allow_allocation_on_validation_failure (F=false/block, T=true/allow, *=not relevant) // B = auto_generate_missing_sku (F=false/manual, T=true/auto-gen, *=not relevant) + // ignore=T in Scenario = ignore_unassigned_machines=true; its absence means false. // // ================================================================================ @@ -295,11 +302,22 @@ pub(in crate::tests) mod tests { } async fn create_test_env_ignoring_unassigned_machines(db_pool: sqlx::PgPool) -> TestEnv { + create_test_env_ignoring_unassigned_machines_with_match_interval(db_pool, None).await + } + + async fn create_test_env_ignoring_unassigned_machines_with_match_interval( + db_pool: sqlx::PgPool, + find_match_interval: Option, + ) -> TestEnv { let mut overrides = TestEnvOverrides::default(); let mut config = get_config(); config.bom_validation.enabled = true; config.bom_validation.ignore_unassigned_machines = true; config.bom_validation.allow_allocation_on_validation_failure = false; + config.bom_validation.auto_generate_missing_sku = false; + if let Some(find_match_interval) = find_match_interval { + config.bom_validation.find_match_interval = find_match_interval; + } overrides.config = Some(config); create_test_env_with_overrides(db_pool, overrides).await @@ -333,6 +351,41 @@ pub(in crate::tests) mod tests { Ok(wrong_sku.id) } + /// Helper: Create an expected machine with an assigned SKU. + async fn create_expected_machine_with_sku( + txn: &mut PgConnection, + managed_host_config: &ManagedHostConfig, + sku_id: &str, + ) -> Result<(), eyre::Error> { + db::expected_machine::create( + txn, + ExpectedMachine { + id: None, + bmc_mac_address: managed_host_config.bmc_mac_address, + data: ExpectedMachineData { + bmc_username: "admin".to_string(), + bmc_password: "password".to_string(), + serial_number: "1234567890".to_string(), + fallback_dpu_serial_numbers: vec![], + metadata: Metadata::new_with_default_name(), + sku_id: Some(sku_id.to_string()), + default_pause_ingestion_and_poweron: None, + interfaces: vec![], + rack_id: None, + dpf_enabled: Some(true), + bmc_ip_address: None, + bmc_retain_credentials: None, + dpu_policy: Default::default(), + bmc_ip_allocation: Default::default(), + host_lifecycle_profile: Default::default(), + }, + }, + ) + .await?; + + Ok(()) + } + /// Helper: Get machine by ID from database async fn get_machine_by_id( txn: &mut PgConnection, @@ -867,6 +920,160 @@ pub(in crate::tests) mod tests { Ok(()) } + /// Test 2.6: An existing WaitingForSkuAssignment machine proceeds when unassigned machines are ignored. + /// Conditions: + /// - ignore_unassigned_machines = true + /// - machine is already in WaitingForSkuAssignment with no assigned SKU + /// Expected: machine proceeds to machine validation + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_releases_waiting_machine( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + let config = ManagedHostConfig { + auto_assign_sku_in_fixture: false, + ..ManagedHostConfig::default() + }; + let mh = create_managed_host_with_config(&env, config).await; + let machine_id = mh.host().id; + + let waiting_state = ManagedHostState::BomValidating { + bom_validating_state: BomValidating::WaitingForSkuAssignment(BomValidatingContext { + machine_validation_context: Some(MachineValidationContext::Discovery), + ..BomValidatingContext::default() + }), + }; + let mut txn = pool.begin().await?; + db::machine::update_state(&mut txn, &machine_id, &waiting_state).await?; + txn.commit().await?; + + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!(machine.config.hw_sku.is_none()); + assert!(matches!( + machine.current_state(), + ManagedHostState::Validation { + validation_state: ValidationState::MachineValidation { .. } + } + )); + + Ok(()) + } + + /// Test 2.7: A matching SKU takes precedence over the unassigned-machine bypass. + /// Conditions: + /// - ignore_unassigned_machines = true + /// - machine is Ready without an assigned SKU + /// - matching SKU exists + /// Expected: machine proceeds to UpdatingInventory for validation + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_matches_before_bypassing( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines_with_match_interval( + pool.clone(), + Some(Duration::ZERO), + ) + .await; + let config = ManagedHostConfig { + auto_assign_sku_in_fixture: false, + ..ManagedHostConfig::default() + }; + let mh = create_managed_host_with_config(&env, config).await; + let machine_id = mh.host().id; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert_eq!(machine.current_state(), &ManagedHostState::Ready); + assert!(machine.config.hw_sku.is_none()); + let matching_sku = db::sku::generate_sku_from_machine(txn.as_mut(), &machine_id).await?; + db::sku::create(&mut txn, &matching_sku).await?; + txn.commit().await?; + + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::UpdatingInventory(_) + } + )); + + Ok(()) + } + + /// Test 2.8: A matching SKU is attempted when the SKU status has no prior match timestamp. + /// Conditions: + /// - ignore_unassigned_machines = true + /// - machine is Ready without an assigned SKU + /// - SKU status exists but has no last_match_attempt + /// - matching SKU exists + /// Expected: machine proceeds to UpdatingInventory for validation + #[crate::sqlx_test] + async fn test_ignore_unassigned_matches_without_prior_attempt( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines_with_match_interval( + pool.clone(), + Some(Duration::from_secs(300)), + ) + .await; + let managed_host_config = + ManagedHostConfig::default().with_expected_state(ManagedHostState::BomValidating { + bom_validating_state: BomValidating::SkuMissing(BomValidatingContext { + machine_validation_context: Some(MachineValidationContext::Discovery), + ..BomValidatingContext::default() + }), + }); + + let mut txn = pool.begin().await?; + create_expected_machine_with_sku( + &mut txn, + &managed_host_config, + "no-sku-no-prior-match-attempt", + ) + .await?; + txn.commit().await?; + + let mh = create_managed_host_with_config(&env, managed_host_config).await; + let machine_id = mh.host().id; + + let mut txn = pool.begin().await?; + db::machine::unassign_sku(&mut txn, &machine_id).await?; + db::machine::update_sku_status_verify_request_time(&mut txn, &machine_id).await?; + db::machine::update_state(&mut txn, &machine_id, &ManagedHostState::Ready).await?; + let matching_sku = db::sku::generate_sku_from_machine(txn.as_mut(), &machine_id).await?; + db::sku::create(&mut txn, &matching_sku).await?; + txn.commit().await?; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!( + machine + .status + .hw_sku + .is_some_and(|status| status.last_match_attempt.is_none()) + ); + txn.commit().await?; + + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::UpdatingInventory(_) + } + )); + + Ok(()) + } + /// Test 3.1: SkuMissing State → stays in SkuMissing /// Conditions: /// - allow_allocation_on_validation_failure = false (testing standard state machine flow from SkuMissing state) @@ -888,31 +1095,7 @@ pub(in crate::tests) mod tests { }); let mut txn = pool.begin().await?; - db::expected_machine::create( - &mut txn, - ExpectedMachine { - id: None, - bmc_mac_address: managed_host_config.bmc_mac_address, - data: ExpectedMachineData { - bmc_username: "admin".to_string(), - bmc_password: "password".to_string(), - serial_number: "1234567890".to_string(), - fallback_dpu_serial_numbers: vec![], - metadata: Metadata::new_with_default_name(), - sku_id: Some("no-sku".to_string()), - default_pause_ingestion_and_poweron: None, - interfaces: vec![], - rack_id: None, - dpf_enabled: Some(true), - bmc_ip_address: None, - bmc_retain_credentials: None, - dpu_policy: Default::default(), - bmc_ip_allocation: Default::default(), - host_lifecycle_profile: Default::default(), - }, - }, - ) - .await?; + create_expected_machine_with_sku(&mut txn, &managed_host_config, "no-sku").await?; txn.commit().await?; let mh = create_managed_host_with_config(&env, managed_host_config).await; @@ -965,6 +1148,60 @@ pub(in crate::tests) mod tests { Ok(()) } + /// Test 3.1a: Assigned missing SKU remains blocked with ignore_unassigned_machines=true + /// Conditions: + /// - ignore_unassigned_machines = true + /// - allow_allocation_on_validation_failure = false + /// - auto_generate_missing_sku = false + /// - machine has an assigned SKU that does not exist in the database + /// Expected: machine stays in SkuMissing state + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_does_not_bypass_missing( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + let managed_host_config = + ManagedHostConfig::default().with_expected_state(ManagedHostState::BomValidating { + bom_validating_state: BomValidating::SkuMissing(BomValidatingContext { + machine_validation_context: Some(MachineValidationContext::Discovery), + ..BomValidatingContext::default() + }), + }); + + let mut txn = pool.begin().await?; + create_expected_machine_with_sku( + &mut txn, + &managed_host_config, + "no-sku-ignore-unassigned", + ) + .await?; + txn.commit().await?; + + // The fixture starts normal ingestion from Ready and stops after the Ready handler + // transitions the host to SkuMissing. + let mh = create_managed_host_with_config(&env, managed_host_config).await; + let machine_id = mh.host().id; + + env.run_machine_state_controller_iteration().await; + env.run_machine_state_controller_iteration().await; + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert_eq!( + machine.config.hw_sku.as_deref(), + Some("no-sku-ignore-unassigned") + ); + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::SkuMissing(_) + } + )); + + Ok(()) + } + /// Test 3.2: SkuMissing → transitions to WaitingForSkuAssignment when SKU ID is removed /// Conditions: /// - allow_allocation_on_validation_failure = false (standard mode) @@ -1529,7 +1766,7 @@ pub(in crate::tests) mod tests { /// - machine has an assigned SKU that does not match hardware /// Expected: machine enters SkuVerificationFailed and remains unallocatable #[crate::sqlx_test] - async fn test_ignore_unassigned_machines_does_not_bypass_assigned_sku_verification( + async fn test_ignore_unassigned_machines_does_not_bypass_mismatch( pool: sqlx::PgPool, ) -> Result<(), eyre::Error> { let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; diff --git a/crates/machine-controller/src/config/bom_validation.rs b/crates/machine-controller/src/config/bom_validation.rs index 4bbb51d712..5f8546c593 100644 --- a/crates/machine-controller/src/config/bom_validation.rs +++ b/crates/machine-controller/src/config/bom_validation.rs @@ -27,8 +27,8 @@ pub struct BomValidationConfig { #[serde(default)] pub enabled: bool, - /// Allow machines that do not have a SKU assigned to bypass SKU validation - /// When true, machines in WaitingForSkuAssignment state can proceed without a SKU + /// Allow machines without an assigned SKU to bypass BOM validation. + /// When true, a machine that remains unassigned can proceed without a SKU. #[serde(default)] pub ignore_unassigned_machines: bool, diff --git a/docs/provisioning/sku-validation.md b/docs/provisioning/sku-validation.md index 1ae905a37e..a036e2d6bc 100644 --- a/docs/provisioning/sku-validation.md +++ b/docs/provisioning/sku-validation.md @@ -5,7 +5,7 @@ NVIDIA Infra Controller (NICo) supports checking and validating the hardware in ## Summary A SKU is a collection of definitions managed by NICo that define a specific configuration of machine. -Each host managed by NICo must have a SKU associated with it before it can be made available for use by a tenant. +Each host managed by NICo normally has a SKU associated with it before it can be made available for use by a tenant. {/* TODO: did we actually implement this? */} @@ -16,9 +16,9 @@ Machines that are assigned a SKU are automatically validated during ingestion ba Hardware validation occurs during initial ingestion and after an instance is released and new discovery information is received. New machines are automatically checked against existing SKUs and if a match is found, the machine passes -SKU validation and continues with the normal ingestion process. If no match is found the machine waits until -a matching SKU is available or until the machine is made compatible with an existing SKU, if SKU validation is enabled -in the site (`ignore_unassigned_machines` configuration option). +SKU validation and continues with the normal ingestion process. If no match is found, the machine normally waits until +a matching SKU is available or the machine is made compatible with an existing SKU. When +`ignore_unassigned_machines` is enabled, an unassigned machine can instead proceed without waiting for SKU assignment. ## Behavior @@ -74,12 +74,14 @@ auto_generate_missing_sku_interval = "300s" allocation blockage due to SKU validation issues when you only need logging without health report alerts. This option does not bypass a machine without an assigned SKU; with `ignore_unassigned_machines = false`, that machine waits for SKU assignment. - - `ignore_unassigned_machines` - When true and BOM validation encounters a machine that does not have an associated SKU, - it will proceed as if all validation has passed. Only machines with an associated SKU will be validated. This allows - existing sites to be upgraded and BOM Validation enabled as SKUs are added to the system without impacting site operation. - Machines that do not have an assigned SKU will still be usable and assignable. - - `find_match_interval` - determines how often NICo will attempt to find a matching SKU for a machine. NICo will only - attempt to find a SKU when the machine is in the `Ready` state. + - `ignore_unassigned_machines` - When true, BOM validation is skipped for a machine without an assigned SKU, allowing it + to proceed without hardware validation or waiting for SKU assignment. During initial ingestion and while waiting for + assignment, NICo attempts to match eligible machines to existing SKUs; a machine that receives a SKU is validated. + This allows existing sites to be upgraded and BOM Validation enabled as SKUs are added to the system without impacting + site operation. Machines without an assigned SKU can still be usable and assignable. + - `find_match_interval` - determines how often NICo retries finding a matching SKU for an unassigned machine. NICo + attempts a match when an unassigned machine is handled in `Ready` or in the `MatchingSku` or + `WaitingForSkuAssignment` BOM validation states. - `auto_generate_missing_sku` - enable or disable generation of a SKU from a machine. This only applies to a machine with a SKU specified in the expected machine configuration and in the `SkuMissing` state. - `auto_generate_missing_sku_interval` - determines how often NICo will attempt to generate a sku from the machine data. From 0f804a36777095525ff8847ea899bd7e742dc48f Mon Sep 17 00:00:00 2001 From: Bill Minckler Date: Thu, 20 Aug 2026 15:33:08 +0000 Subject: [PATCH 3/3] Clarify SKU validation bypass behavior --- crates/api-core/src/cfg/README.md | 2 +- .../src/config/bom_validation.rs | 13 ++++++++---- docs/provisioning/sku-validation.md | 20 +++++++++---------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index 50ac3c5d3f..f7f4cc8be5 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -812,7 +812,7 @@ be propagated there by DPF. | ------- | ------ | --------- | ------------- | | `enabled` | `bool` | `false` | Enable BOM/SKU validation. | | `ignore_unassigned_machines` | `bool` | `false` | When enabled, a machine that remains unassigned can skip BOM validation and proceed; machines with an assigned SKU are still validated. | -| `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines with assigned SKUs allocatable on validation failure; does not bypass unassigned machines. | +| `allow_allocation_on_validation_failure` | `bool` | `false` | Let machines with assigned SKUs proceed after validation failure; a missing SKU can be transiently reported until the next reconciliation. Does not bypass unassigned machines. | | `find_match_interval` | `Duration` | `5m` | Interval between SKU match attempts. | | `auto_generate_missing_sku` | `bool` | `false` | Auto-create missing SKUs from expected machines. | | `auto_generate_missing_sku_interval` | `Duration` | `5m` | Interval between auto-generate attempts. | diff --git a/crates/machine-controller/src/config/bom_validation.rs b/crates/machine-controller/src/config/bom_validation.rs index 5f8546c593..8923d0feb9 100644 --- a/crates/machine-controller/src/config/bom_validation.rs +++ b/crates/machine-controller/src/config/bom_validation.rs @@ -32,10 +32,15 @@ pub struct BomValidationConfig { #[serde(default)] pub ignore_unassigned_machines: bool, - /// Allow machines with assigned SKUs to stay in Ready state and remain allocatable even when SKU validation fails - /// When false (default): Standard mode - validation failures block allocation (machine enters failed state) - /// When true: Allow allocation mode - validation still occurs and health reports are recorded, but machines do not transition - /// into failed states (SkuVerificationFailed, SkuMissing) and can proceed to Ready/MachineValidation + /// Allow machines with assigned SKUs to proceed when SKU validation fails. + /// A missing SKU found during verification enters SkuMissing and records a health report. + /// + /// When false (default), validation failures block allocation by entering failed states. + /// + /// When true, a Ready machine whose assigned SKU is missing stays Ready. A SKU mismatch is logged, but the + /// machine proceeds without recording a SKU validation health report. On a later reconciliation, a machine with + /// an assigned SKU in SkuMissing or SkuVerificationFailed can leave BOM validation, clearing that report and + /// resuming normal lifecycle processing. /// This does not bypass machines without an assigned SKU; use ignore_unassigned_machines for that. #[serde(default)] pub allow_allocation_on_validation_failure: bool, diff --git a/docs/provisioning/sku-validation.md b/docs/provisioning/sku-validation.md index a036e2d6bc..f78b7da201 100644 --- a/docs/provisioning/sku-validation.md +++ b/docs/provisioning/sku-validation.md @@ -42,9 +42,9 @@ Verifying a SKU against a machine goes through several steps to acquire updated - `MatchingSku` - The state machine will attempt to find an existing SKU that matches the machine inventory. - `UpdatingInventory` - NICo is requesting that scout re-inventory the machine. This ensures that other operations are using a recent version of the machine inventory - `VerifyingSku` - NICo is comparing the machine inventory against the SKU -- `SkuVerificationFailed` - The machine did not match the SKU. Manual intervention is required. The `sku verify` command may be used to retry the verification +- `SkuVerificationFailed` - The machine did not match the SKU. Manual intervention is required unless `allow_allocation_on_validation_failure` is enabled, in which case the machine can proceed on a later reconciliation. The `sku verify` command may be used to retry the verification. - `WaitingForSkuAssignment` - The machine does not have a SKU assigned and the configuration requires one. -- `SkuMissing` - The machine has a SKU assigned, but the SKU does not exist. This happens when a SKU is specified in the expected machines, but was not created. If configured, NICo will attempt to generate a SKU +- `SkuMissing` - The machine has a SKU assigned, but the SKU does not exist. This happens when a SKU is specified in the expected machines, but was not created. If configured, NICo will attempt to generate a SKU. When `allow_allocation_on_validation_failure` is enabled, the machine can proceed on a later reconciliation. ### Versions NICo maintains a version of the SKU schema used when a SKU is created. This ensures that the same comparison is used during the lifetime of a SKU and ensures that the behavior of BOM validation does not change between NICo versions. When new components are added, or new data sources are used during validation, existing SKUs will not be updated with the change and continue to behave as they did in previous NICo versions. In order to use the new version, a new SKU must be created. @@ -66,14 +66,14 @@ auto_generate_missing_sku_interval = "300s" - `enabled` - Enables or disables the entire bom validation process. When disabled, machines will skip bom validation and proceed as if all validation has passed. - - `allow_allocation_on_validation_failure` - When true, machines with an assigned SKU are allowed to stay in Ready state - and remain allocatable even when SKU validation fails. Validation still occurs but only logs are recorded - health reports - are cleared instead of recording validation failures. Machines do not transition into failed states - (SkuVerificationFailed or SkuMissing). When false (default), standard mode applies where validation failures are recorded - in health reports and machines enter failed states and become unallocatable until fixed. This is useful for avoiding machine - allocation blockage due to SKU validation issues when you only need logging without health report alerts. This option does - not bypass a machine without an assigned SKU; with `ignore_unassigned_machines = false`, that machine waits for SKU - assignment. + - `allow_allocation_on_validation_failure` - When true, validation failures for machines with assigned SKUs do not block + allocation. For a SKU mismatch, NICo logs the mismatch and the machine proceeds without a SKU validation health report. + A Ready machine whose assigned SKU is missing stays Ready. A missing SKU found during an in-progress verification can enter + `SkuMissing` and record a health report. On a later reconciliation, a machine with an assigned SKU in `SkuMissing` or + `SkuVerificationFailed` can leave BOM validation, clearing that report and resuming normal lifecycle processing. When false + (default), standard mode applies where validation failures are recorded in health reports and machines enter failed states + and become unallocatable until fixed. This option does not bypass a machine without an assigned SKU; with + `ignore_unassigned_machines = false`, that machine waits for SKU assignment. - `ignore_unassigned_machines` - When true, BOM validation is skipped for a machine without an assigned SKU, allowing it to proceed without hardware validation or waiting for SKU assignment. During initial ingestion and while waiting for assignment, NICo attempts to match eligible machines to existing SKUs; a machine that receives a SKU is validated.