Address PR comments

Signed-off-by: Felicity Xu <hanyux@nvidia.com>

Author: hanyux-nv

crates/api-core/src/cfg/README.md

@@ -20,6 +20,7 @@ applicable.
 | `ib_config` | `Option<IBFabricConfig>` | — | InfiniBand fabric configuration (see [IBFabricConfig](#ibfabricconfig)). |
 | `asn` | `u32` | **required** | Autonomous System Number, fixed per environment. Used by nico-dpu-agent for `frr.conf` BGP routing. |
 | `dhcp_servers` | `Vec<Ipv4Addr>` | `[]` | DHCP server addresses announced to DPUs during network provisioning. |
+| `ntp_servers` | `Vec<Ipv4Addr>` | `[]` | Site-level NTP server IPs used for BMC time configuration and DHCP NTP Server configuration. |
 | `route_servers` | `Vec<String>` | `[]` | Route server IPs for L2VPN Ethernet Virtual network support. |
 | `enable_route_servers` | `bool` | `false` | Enables route server injection into DPU FRR configs for L2VPN. |
 | `deny_prefixes` | `Vec<Ipv4Network>` | `[]` | IPv4 CIDR prefixes that tenant instances are blocked from reaching. Generates iptables DROP rules and nvue ACL policies. |

crates/api-core/src/tests/host_bmc_firmware_test.rs

@@ -92,11 +92,14 @@ async fn test_preingestion_bmc_upgrade(
         .await?
         .into_inner();
 
-    // First, a host where it's already up to date; it should immediately go to complete.
+    // First, a host where it's already up to date; it should go to complete
+    // after passing through the explicit NTP setup state.
     let addr = response.address.as_str();
     insert_endpoint_version(&mut txn, addr, "6.00.30.00", "1.13.2", false).await?;
     txn.commit().await?;
 
+    mgr.run_single_iteration().await?;
+
     mgr.run_single_iteration().await?;
     let mut txn = pool.begin().await.unwrap();
     assert!(
@@ -117,6 +120,7 @@ async fn test_preingestion_bmc_upgrade(
     txn.commit().await?;
     let mut txn = pool.begin().await.unwrap();
 
+    mgr.run_single_iteration().await?;
     mgr.run_single_iteration().await?;
 
     assert!(
@@ -136,6 +140,7 @@ async fn test_preingestion_bmc_upgrade(
     insert_endpoint_version(&mut txn, addr, "4.9", "1.13.2", false).await?;
     txn.commit().await?;
 
+    mgr.run_single_iteration().await?;
     mgr.run_single_iteration().await?;
     // The "upload" is synchronous now and will be complete at this point.
 
@@ -287,6 +292,7 @@ async fn test_preingestion_upgrade_script(
     insert_endpoint_version(&mut txn, addr, "0", "0", false).await?;
     txn.commit().await?;
 
+    mgr.run_single_iteration().await?;
     mgr.run_single_iteration().await?;
 
     let mut txn = pool.begin().await.unwrap();
@@ -999,6 +1005,7 @@ async fn test_preingestion_preupdate_powercycling(
     insert_endpoint_version(&mut txn, addr, "4.9", "1.1", true).await?;
     txn.commit().await?;
 
+    mgr.run_single_iteration().await?;
     mgr.run_single_iteration().await?;
     // The "upload" is synchronous now and will be complete at this point.
 
@@ -2299,7 +2306,13 @@ async fn test_explicit_update(pool: sqlx::PgPool) -> CarbideResult<()> {
 async fn test_preingestion_time_sync_ok(
     pool: sqlx::PgPool,
 ) -> Result<(), Box<dyn std::error::Error>> {
-    let env = common::api_fixtures::create_test_env(pool.clone()).await;
+    let mut config = get_config();
+    config.ntp_servers = vec![
+        "198.51.100.10".parse().unwrap(),
+        "198.51.100.11".parse().unwrap(),
+    ];
+    let env =
+        create_test_env_with_overrides(pool.clone(), TestEnvOverrides::with_config(config)).await;
 
     let mgr = PreingestionManager::new(
         pool.clone(),
@@ -2330,7 +2343,23 @@ async fn test_preingestion_time_sync_ok(
     insert_endpoint_version(&mut txn, addr, "6.00.30.00", "1.13.2", false).await?;
     txn.commit().await?;
 
-    // Run preingestion manager - should check time sync, pass, then check firmware, and complete
+    let timepoint = env.redfish_sim.timepoint();
+
+    // Run preingestion manager - should apply site NTP servers, check time sync,
+    // then check firmware and complete.
+    mgr.run_single_iteration().await?;
+
+    mgr.run_single_iteration().await?;
+
+    let actions = env.redfish_sim.actions_since(&timepoint);
+    assert!(
+        actions
+            .all_hosts()
+            .iter()
+            .any(|a| matches!(a, RedfishSimAction::SetNtpServers(_))),
+        "Expected SetNtpServers when site NTP is configured"
+    );
+
     mgr.run_single_iteration().await?;
 
     let mut txn = pool.begin().await.unwrap();
@@ -2346,6 +2375,141 @@ async fn test_preingestion_time_sync_ok(
     Ok(())
 }
 
+/// Test that an empty NTP server config skips Redfish NTP setup and proceeds
+/// directly to initial checks from the SetNtpServers state.
+#[crate::sqlx_test]
+async fn test_preingestion_set_ntp_servers_empty(
+    pool: sqlx::PgPool,
+) -> Result<(), Box<dyn std::error::Error>> {
+    let mut config = get_config();
+    config.ntp_servers.clear(); // Use empty NTP servers.
+
+    let env =
+        create_test_env_with_overrides(pool.clone(), TestEnvOverrides::with_config(config)).await;
+
+    let mgr = PreingestionManager::new(
+        pool.clone(),
+        env.config.preingestion_manager(),
+        env.redfish_sim.clone(),
+        env.test_meter.meter(),
+        None,
+        None,
+        None,
+        env.api.work_lock_manager_handle.clone(),
+        env.config.ntp_servers.clone(),
+    );
+
+    let response = env
+        .api
+        .discover_dhcp(
+            DhcpDiscovery::builder("b8:3f:d2:90:97:a6", "192.0.2.1")
+                .vendor_string("iDRac")
+                .tonic_request(),
+        )
+        .await?
+        .into_inner();
+
+    let addr = response.address.as_str();
+    let ip_addr = IpAddr::from_str(addr).unwrap();
+    let mut txn = pool.begin().await.unwrap();
+    insert_endpoint_version(&mut txn, addr, "6.00.30.00", "1.13.2", false).await?;
+
+    // Transition to start of SetNtpServers state with no attempts made so far.
+    db::explored_endpoints::set_preingestion_set_ntp_servers(ip_addr, None, 0, &mut txn).await?;
+    txn.commit().await?;
+
+    let timepoint = env.redfish_sim.timepoint();
+    mgr.run_single_iteration().await?;
+
+    // Expect no SetNtpServers actions since NTP server config is empty.
+    let actions = env.redfish_sim.actions_since(&timepoint);
+    assert!(
+        !actions
+            .all_hosts()
+            .iter()
+            .any(|a| matches!(a, RedfishSimAction::SetNtpServers(_))),
+        "Did not expect SetNtpServers when NTP server config is empty"
+    );
+
+    // Expect to go to complete since no need to set NTP server config.
+    let mut txn = pool.begin().await.unwrap();
+    assert_eq!(
+        db::explored_endpoints::find_all_preingestion_complete(&mut txn)
+            .await?
+            .len(),
+        1
+    );
+    txn.commit().await?;
+
+    Ok(())
+}
+
+/// Test that exhausting NTP setup attempts proceeds to initial checks without
+/// failing preingestion or trying to set NTP again.
+#[crate::sqlx_test]
+async fn test_preingestion_set_ntp_servers_max_attempts(
+    pool: sqlx::PgPool,
+) -> Result<(), Box<dyn std::error::Error>> {
+    let mut config = get_config();
+    config.ntp_servers = vec!["198.51.100.10".parse().unwrap()];
+    let env =
+        create_test_env_with_overrides(pool.clone(), TestEnvOverrides::with_config(config)).await;
+
+    let mgr = PreingestionManager::new(
+        pool.clone(),
+        env.config.preingestion_manager(),
+        env.redfish_sim.clone(),
+        env.test_meter.meter(),
+        None,
+        None,
+        None,
+        env.api.work_lock_manager_handle.clone(),
+        env.config.ntp_servers.clone(),
+    );
+
+    let response = env
+        .api
+        .discover_dhcp(
+            DhcpDiscovery::builder("b8:3f:d2:90:97:a6", "192.0.2.1")
+                .vendor_string("iDRac")
+                .tonic_request(),
+        )
+        .await?
+        .into_inner();
+
+    let addr = response.address.as_str();
+    let ip_addr = IpAddr::from_str(addr).unwrap();
+    let mut txn = pool.begin().await.unwrap();
+    insert_endpoint_version(&mut txn, addr, "6.00.30.00", "1.13.2", false).await?;
+
+    db::explored_endpoints::set_preingestion_set_ntp_servers(ip_addr, None, 3, &mut txn).await?;
+    txn.commit().await?;
+
+    let timepoint = env.redfish_sim.timepoint();
+    mgr.run_single_iteration().await?;
+
+    let actions = env.redfish_sim.actions_since(&timepoint);
+    assert!(
+        !actions
+            .all_hosts()
+            .iter()
+            .any(|a| matches!(a, RedfishSimAction::SetNtpServers(_))),
+        "Did not expect SetNtpServers after max attempts are exhausted"
+    );
+
+    // The next iteration should go to complete since NTP setup is given up.
+    let mut txn = pool.begin().await.unwrap();
+    assert_eq!(
+        db::explored_endpoints::find_all_preingestion_complete(&mut txn)
+            .await?
+            .len(),
+        1
+    );
+    txn.commit().await?;
+
+    Ok(())
+}
+
 /// Test that preingestion handles the TimeSyncReset state machine correctly
 #[crate::sqlx_test]
 async fn test_preingestion_time_sync_reset_flow(
@@ -2405,12 +2569,6 @@ async fn test_preingestion_time_sync_reset_flow(
         all_actions.contains(&RedfishSimAction::SetUtcTimezone),
         "Expected SetUtcTimezone action to be called during TimeSyncReset Start phase"
     );
-    assert!(
-        all_actions
-            .iter()
-            .any(|a| matches!(a, RedfishSimAction::SetNtpServers(_))),
-        "Expected SetNtpServers action to be called during TimeSyncReset Start phase"
-    );
 
     let mut txn = pool.begin().await.unwrap();
     let endpoints =

crates/api-db/src/explored_endpoints.rs

@@ -16,7 +16,7 @@
  */
 use std::net::IpAddr;
 
-use chrono::Utc;
+use chrono::{DateTime, Utc};
 use config_version::ConfigVersion;
 use mac_address::MacAddress;
 use model::firmware::FirmwareComponentType;
@@ -429,6 +429,16 @@ pub async fn set_preingestion_initial_bmc_reset(
     set_preingestion(address, state, txn).await
 }
 
+pub async fn set_preingestion_set_ntp_servers(
+    address: IpAddr,
+    set_at: Option<DateTime<Utc>>,
+    attempts: u32,
+    txn: &mut PgConnection,
+) -> Result<(), DatabaseError> {
+    let state = PreingestionState::SetNtpServers { set_at, attempts };
+    set_preingestion(address, state, txn).await
+}
+
 pub async fn set_preingestion_time_sync_reset(
     address: IpAddr,
     phase: TimeSyncResetPhase,

crates/api-model/src/site_explorer/mod.rs

@@ -407,6 +407,14 @@ pub enum PreingestionState {
     InitialBMCReset {
         phase: InitialBmcResetPhase,
     },
+    /// Configure site NTP servers on the BMC before checking whether its clock
+    /// is synchronized. `set_at` records a successful Redfish update so the
+    /// state machine can wait for the setting to take effect before checking.
+    SetNtpServers {
+        set_at: Option<DateTime<Utc>>,
+        #[serde(default)]
+        attempts: u32,
+    },
     TimeSyncReset {
         phase: TimeSyncResetPhase,
         last_time: DateTime<Utc>,