From 01640fcac64fa6fe1f03cabd0a7bb90d0d736321 Mon Sep 17 00:00:00 2001 From: aadvani Date: Sun, 16 Aug 2026 06:02:09 +0000 Subject: [PATCH] Add cleanup for dpf scoped service interfaces Signed-off-by: aadvani --- crates/api-core/src/cfg/README.md | 2 +- crates/api-core/src/cfg/file.rs | 6 +- crates/dpf/src/repository/kube.rs | 10 ++ crates/dpf/src/repository/traits.rs | 1 + crates/dpf/src/sdk.rs | 123 +++++++++++++- crates/dpf/src/test/sdk_initialization.rs | 193 +++++++++++++++------- crates/dpf/src/types.rs | 7 +- 7 files changed, 268 insertions(+), 74 deletions(-) diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index b615039083..fd5f61c19a 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -718,7 +718,7 @@ events, so consumers handle them identically. | Field | Type | Default | Description | | ------- | ------ | --------- | ------------- | | `enabled` | `bool` | `false` | Enable DPF Kubernetes deployment. | -| `deployment_scoped_service_interfaces` | `bool` | `false` | Opt the complete DPF namespace into deployment-scoped `-bf3`, `-bf4`, and `-astra` DPUServiceInterfaces. Each resource selects Nodes in the remote DPU cluster through DPF's propagated `svc.dpu.nvidia.com/owned-by-dpudeployment=_` ownership label; management-cluster DPUNode deployment labels are not used for this selector. Enabling or disabling is a planned migration: stop NICo, remove old-mode NICo ServiceInterfaces in both transition directions, perform DPU re-ingestion, and restart. NICo neither detects nor deletes old-mode resources; skipping cleanup can leave competing interface generations active. Astra requires this setting. | +| `deployment_scoped_service_interfaces` | `bool` | `false` | Legacy migration knob for deployment-scoped DPUServiceInterfaces. BF3-only and BF4-generic-only sites remain unscoped by default for backward compatibility. BF4 Astra requires this setting so the whole DPF namespace uses scoped ServiceInterfaces and legacy match-all ServiceInterfaces cannot also bind Astra nodes. NICo removes ServiceInterfaces from the opposite mode during a transition. A failed transition fails NICo DPF initialization and is reported in its startup/reconciliation logs. It is not rolled back: resolve the reported deletion or apply error and retry initialization; either generation may be partially present. | | `pf_total_sf_reserved` | `u32` | `30` | SF capacity reserved beyond the NICo-managed HBN, DHCP, and FMDS endpoints when an intercept-bridging inventory is configured. NICo sets `PF_TOTAL_SF` to the effective inventory's endpoint count plus this value for BF3 and generic BF4. Without configured intercept bridging, this value is the complete `PF_TOTAL_SF`, preserving the legacy default of `30`; BF4 Astra retains its fixed flavor and ignores this setting. Changing this value changes the BF3/generic-BF4 flavor. Every intercept-inventory change requires controlled ServiceInterface cleanup and DPU re-ingestion, even when the serialized flavor and its hash remain unchanged. Operators must select a value compatible with their platform's SF and BAR capacity. With configured intercept bridging, startup rejects configurations whose managed endpoint count plus reserve exceeds `u32::MAX`. | | `dpu_service_sync_enabled` | `bool` | `true` | Whether NICo rolls a changed DPUService out on its own, by releasing the DPF maintenance hold on hosts whose DPUs already match their DPUDeployment. Selects *who* opens the gate, never whether one exists: DPF is always configured to park a changed DPUService behind a hold, so no service update reaches a DPU unchecked. Setting `false` does not resume unchecked rollout — the held DPUs wait for an operator to release them deliberately. Hosts still awaiting reprovisioning, and hosts carrying a live tenant instance, keep their hold either way. | | `dpu_agent_bootstrap_ca` | `DpfDpuAgentBootstrapCa` | `legacy_download` | Bootstrap trust for the containerized DPU agent. Supports `legacy_download` and `mounted`, as described in the following examples. | diff --git a/crates/api-core/src/cfg/file.rs b/crates/api-core/src/cfg/file.rs index 43eeb9df60..7e75c61d55 100644 --- a/crates/api-core/src/cfg/file.rs +++ b/crates/api-core/src/cfg/file.rs @@ -1569,8 +1569,10 @@ pub struct DpfConfig { #[serde(default)] pub enabled: bool, /// Opts the DPF namespace into deployment-scoped DPUServiceInterfaces. - /// Changing modes requires operators to remove old-mode NICo resources and - /// re-ingest DPUs; NICo neither detects nor deletes those resources. + /// BF4 Astra requires this to be enabled for the whole namespace so legacy + /// match-all ServiceInterfaces do not bind Astra nodes. Initialization + /// removes ServiceInterfaces from the opposite mode during a transition; + /// failures are not rolled back and require a retry after remediation. #[serde(default)] pub deployment_scoped_service_interfaces: bool, /// SF capacity reserved beyond configured NICo-managed service endpoints. diff --git a/crates/dpf/src/repository/kube.rs b/crates/dpf/src/repository/kube.rs index 591703f911..d3c5b2fea5 100644 --- a/crates/dpf/src/repository/kube.rs +++ b/crates/dpf/src/repository/kube.rs @@ -556,6 +556,16 @@ impl DpuServiceInterfaceRepository for KubeRepository { ) .await?) } + + async fn delete(&self, name: &str, namespace: &str) -> Result<(), DpfError> { + let api: Api = self.api(namespace); + match api.delete(name, &Default::default()).await { + Ok(_) => {} + Err(kube::Error::Api(error)) if error.code == 404 => {} + Err(error) => return Err(error.into()), + } + Ok(()) + } } #[async_trait] diff --git a/crates/dpf/src/repository/traits.rs b/crates/dpf/src/repository/traits.rs index f09a80a8db..1b4a450811 100644 --- a/crates/dpf/src/repository/traits.rs +++ b/crates/dpf/src/repository/traits.rs @@ -235,6 +235,7 @@ pub trait DpuServiceInterfaceRepository: Send + Sync { ) -> Result, DpfError>; async fn list(&self, namespace: &str) -> Result, DpfError>; async fn apply(&self, iface: &DPUServiceInterface) -> Result; + async fn delete(&self, name: &str, namespace: &str) -> Result<(), DpfError>; } /// Repository for Kubernetes ConfigMaps and Secrets. diff --git a/crates/dpf/src/sdk.rs b/crates/dpf/src/sdk.rs index c340e70567..32fa0c50c3 100644 --- a/crates/dpf/src/sdk.rs +++ b/crates/dpf/src/sdk.rs @@ -107,6 +107,8 @@ const MAX_HBN_SERVICE_INTERFACES: usize = 32; /// Label set by DPF on deployment-owned resources and propagated to the corresponding /// DPU-cluster Node. Value format: `_`. const DPU_OWNED_BY_DEPLOYMENT_LABEL: &str = "svc.dpu.nvidia.com/owned-by-dpudeployment"; +const SERVICE_INTERFACE_DELETE_TIMEOUT: Duration = Duration::from_secs(60); +const SERVICE_INTERFACE_DELETE_POLL_INTERVAL: Duration = Duration::from_millis(250); /// Returns DPF's canonical ownership-label value for one DPUDeployment. fn dpu_deployment_owner_label_value(namespace: &str, deployment_name: &str) -> String { @@ -664,8 +666,8 @@ pub fn deployment_cr_suffix(deployment_type: DpuDeploymentType) -> &'static str /// Suffix appended to deployment-scoped DPUServiceInterface CR names. /// /// Unlike the existing service CR compatibility scheme, every deployment type -/// is suffixed. The migration is opt-in; operators must remove the old -/// generation manually because NICo neither detects nor deletes it. +/// is suffixed. Scoped initialization prunes the old unscoped generation so +/// match-all legacy resources do not bind the same DPU nodes. fn service_interface_cr_suffix(deployment_type: DpuDeploymentType) -> &'static str { match deployment_type { DpuDeploymentType::Bf3 => "bf3", @@ -1566,6 +1568,96 @@ pub async fn apply_service_interface_templates< apply_service_interface_templates_with_scope(repo, namespace, interfaces, "", None).await } +async fn wait_for_service_interface_deletion< + R: crate::repository::DpuServiceInterfaceRepository, +>( + repo: &R, + name: &str, + namespace: &str, +) -> Result<(), DpfError> { + tokio::time::timeout(SERVICE_INTERFACE_DELETE_TIMEOUT, async { + loop { + if crate::repository::DpuServiceInterfaceRepository::get(repo, name, namespace) + .await? + .is_none() + { + return Ok(()); + } + tokio::time::sleep(SERVICE_INTERFACE_DELETE_POLL_INTERVAL).await; + } + }) + .await + .map_err(|_| { + DpfError::timeout( + "DPUServiceInterface deletion", + format!("{namespace}/{name} still exists after {SERVICE_INTERFACE_DELETE_TIMEOUT:?}"), + ) + })? +} + +async fn delete_stale_legacy_service_interfaces< + R: crate::repository::DpuServiceInterfaceRepository, +>( + repo: &R, + namespace: &str, +) -> Result<(), crate::error::DpfError> { + let mut live_interfaces = + crate::repository::DpuServiceInterfaceRepository::list(repo, namespace).await?; + live_interfaces.sort_by(|left, right| left.metadata.name.cmp(&right.metadata.name)); + for live in live_interfaces { + let Some(name) = live.metadata.name.clone() else { + continue; + }; + if live.spec.template.spec.node_selector.is_some() { + continue; + } + + tracing::info!( + namespace, + service_interface = name, + "Deleting stale legacy unscoped DPUServiceInterface during scoped migration" + ); + crate::repository::DpuServiceInterfaceRepository::delete(repo, &name, namespace).await?; + wait_for_service_interface_deletion(repo, &name, namespace).await?; + } + + Ok(()) +} + +async fn delete_stale_scoped_service_interfaces< + R: crate::repository::DpuServiceInterfaceRepository, +>( + repo: &R, + namespace: &str, +) -> Result<(), crate::error::DpfError> { + let mut live_interfaces = + crate::repository::DpuServiceInterfaceRepository::list(repo, namespace).await?; + live_interfaces.sort_by(|left, right| left.metadata.name.cmp(&right.metadata.name)); + for live in live_interfaces { + let Some(name) = live.metadata.name.clone() else { + continue; + }; + if !["bf3", "bf4", "astra"] + .iter() + .any(|suffix| name.ends_with(&format!("-{suffix}"))) + { + continue; + } + if live.spec.template.spec.node_selector.is_none() { + continue; + } + tracing::info!( + namespace, + service_interface = name, + "Deleting stale scoped DPUServiceInterface during legacy unscoped migration" + ); + crate::repository::DpuServiceInterfaceRepository::delete(repo, &name, namespace).await?; + wait_for_service_interface_deletion(repo, &name, namespace).await?; + } + + Ok(()) +} + async fn create_flavor_services_and_deployment< R: DpuServiceTemplateRepository + DpuServiceConfigurationRepository @@ -1616,9 +1708,24 @@ async fn create_flavor_services_and_deployment< // management-plane DPUNode and DPUDeployment selection. Reusing them here matches zero remote // Nodes and prevents DPF from instantiating concrete ServiceInterfaces. // - // Changing either names or selectors triggers DPF reconciliation and must remain an explicit - // operator-controlled migration. + // Changing either names or selectors triggers DPF reconciliation. Initialization deletes + // resources from the opposite mode so stale templates do not remain active beside the + // current generation. // + // A legacy interface matches every remote DPU-cluster Node. Wait for resources from the + // opposite mode to be deleted before creating their replacements, so generations do not + // overlap during a namespace-wide transition. + let cleanup_result = if !config.deployment_scoped_service_interfaces { + delete_stale_scoped_service_interfaces(repo, namespace).await + } else { + delete_stale_legacy_service_interfaces(repo, namespace).await + }; + cleanup_result.map_err(|error| { + DpfError::InvalidState(format!( + "failed to remove the previous DPUServiceInterface generation ({error}); resolve the deletion failure, then retry initialization. The previous generation may be only partially removed" + )) + })?; + // Patch CRs require their peer bridge, so preserve flavor creation before interface templates. apply_service_interface_templates_with_scope( repo, @@ -1627,8 +1734,12 @@ async fn create_flavor_services_and_deployment< interface_suffix, dpu_cluster_node_labels.as_ref(), ) - .await?; - + .await + .map_err(|error| { + DpfError::InvalidState(format!( + "failed to create replacement DPUServiceInterfaces ({error}); resolve the apply failure, then retry initialization. The previous generation has been removed and replacements may be only partially created" + )) + })?; // Each deployment gets its own service/NAD CRs (suffixed by deployment type) // so BF3 and BF4 do not overwrite each other's Helm values/versions in the // shared namespace. `nad_rename` maps each deployment-local NAD name to its diff --git a/crates/dpf/src/test/sdk_initialization.rs b/crates/dpf/src/test/sdk_initialization.rs index 866eea9b2b..0e3fad8096 100644 --- a/crates/dpf/src/test/sdk_initialization.rs +++ b/crates/dpf/src/test/sdk_initialization.rs @@ -395,6 +395,11 @@ impl DpuServiceInterfaceRepository for InitializationMock { .insert(resource_key(iface), iface.clone()); Ok(iface.clone()) } + + async fn delete(&self, name: &str, ns: &str) -> Result<(), DpfError> { + self.service_interfaces.remove(&ns_key(ns, name)); + Ok(()) + } } #[async_trait] @@ -493,6 +498,38 @@ async fn test_create_initialization_objects() { drop(sdk); } +#[tokio::test] +async fn bf4_generic_initialization_preserves_legacy_unscoped_interfaces() { + let mock = InitializationMock::default(); + let config = InitDpfResourcesConfig { + bluefield_software: Some(BlueFieldSoftwareParams { + os_iso: "http://example.com/bf4.iso".to_string(), + pldm_fw_bundle: Some("http://example.com/bf4.pldm".to_string()), + }), + deployment_type: DpuDeploymentType::Bf4Generic, + ..Default::default() + }; + + let sdk = crate::sdk::DpfSdkBuilder::new(mock.clone(), TEST_NS, "test-password".to_string()) + .initialize(&config) + .await + .unwrap(); + + let p0 = DpuServiceInterfaceRepository::get(&mock, "p0", TEST_NS) + .await + .unwrap() + .expect("BF4 generic legacy p0 ServiceInterface must exist"); + assert!(p0.spec.template.spec.node_selector.is_none()); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p0-bf4", TEST_NS) + .await + .unwrap() + .is_none() + ); + + drop(sdk); +} + /// Verifies SF overflow fails before the builder writes its BMC Secret or any DPF CR because /// invalid capacity must not leave a partially initialized DPF namespace. #[tokio::test] @@ -979,75 +1016,107 @@ async fn scoped_bf3_bf4_and_astra_initialization_coexists() { drop(sdk); } -/// Verifies unrelated ServiceInterfaces with NICo-like names or labels cannot block either -/// initialization mode and remain untouched because shape alone is not ownership evidence. #[tokio::test] -async fn existing_service_interfaces_do_not_block_or_get_deleted() { - let cases = [ - // A matching name and logical label must not be treated as NICo-owned legacy state. - ("vendor-uplink", "vendor-uplink", true), - // A deployment-like suffix must not be treated as NICo-owned scoped state. - ("vendor-uplink-bf3", "vendor-uplink", false), - ]; - - for (existing_name, logical_name, desired_scoped) in cases { - // Seed a foreign-managed resource whose name and inner label resemble one NICo mode. - let mock = InitializationMock::default(); - let mut existing = crate::sdk::build_service_interface( - &crate::sdk::build_dpu_interfaces_vec()[0], - TEST_NS, - ); - existing.metadata.name = Some(existing_name.to_string()); - existing.metadata.labels = Some(BTreeMap::from([( - "app.kubernetes.io/managed-by".to_string(), - "vendor-dpf-operator".to_string(), - )])); - existing - .spec - .template - .spec - .template - .metadata - .as_mut() - .unwrap() - .labels - .as_mut() - .unwrap() - .insert("interface".to_string(), logical_name.to_string()); - let existing_snapshot = serde_json::to_value(&existing).unwrap(); - mock.service_interfaces - .insert(resource_key(&existing), existing); +async fn service_interface_migration_prunes_previous_generation() { + let definitions = crate::sdk::build_dpu_interfaces_vec(); - // Initialize the opposite shape in both directions through the complete SDK path. - let config = InitDpfResourcesConfig { + // Legacy-to-scoped: legacy interfaces are pruned, including stale interfaces from a + // previously configured intercept inventory. + let mock = InitializationMock::default(); + let stale_p1 = crate::sdk::build_service_interface( + definitions + .iter() + .find(|definition| definition.name == "p1") + .expect("static test inventory must contain p1"), + TEST_NS, + ); + mock.service_interfaces + .insert(resource_key(&stale_p1), stale_p1); + let old_intercept_interfaces = crate::sdk::build_effective_dpu_interfaces( + DEFAULT_DPU_NUM_OF_VFS, + Some(&configured_intercept_bridging()), + ); + let stale_c2pf3 = crate::sdk::build_service_interface( + old_intercept_interfaces + .iter() + .find(|definition| definition.name == "c2pf3") + .expect("configured test inventory must contain c2pf3"), + TEST_NS, + ); + mock.service_interfaces + .insert(resource_key(&stale_c2pf3), stale_c2pf3); + let sdk = crate::sdk::DpfSdkBuilder::new(mock.clone(), TEST_NS, "test-password".to_string()) + .with_labeler(InitializationLabeler) + .initialize(&InitDpfResourcesConfig { bfb_url: "http://example.com/test.bfb".to_string(), - deployment_scoped_service_interfaces: desired_scoped, + deployment_scoped_service_interfaces: true, ..Default::default() - }; - // `InitializationMock` clones share Arc-backed stores, so assertions through `mock` - // observe the exact writes performed through the repository clone held by the SDK. - let sdk = - crate::sdk::DpfSdkBuilder::new(mock.clone(), TEST_NS, "test-password".to_string()) - .with_labeler(InitializationLabeler) - .initialize(&config) - .await - .unwrap(); - - // Initialization must apply its own resources without mutating or pruning the existing one. - assert!(!mock.bfbs.is_empty()); - assert!(!mock.flavors.is_empty()); - assert!(!mock.deployments.is_empty()); - let existing_after = DpuServiceInterfaceRepository::get(&mock, existing_name, TEST_NS) + }) + .await + .unwrap(); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p1", TEST_NS) .await .unwrap() - .expect("pre-existing ServiceInterface must remain"); - assert_eq!( - serde_json::to_value(existing_after).unwrap(), - existing_snapshot - ); + .is_none() + ); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p1-bf3", TEST_NS) + .await + .unwrap() + .is_some() + ); + assert!( + DpuServiceInterfaceRepository::get(&mock, "c2pf3", TEST_NS) + .await + .unwrap() + .is_none() + ); + drop(sdk); - drop(sdk); - } + // Scoped-to-legacy: a bad global-scoped BF3 generation is pruned even if the deployment was + // renamed before the site returns to the default unscoped model. + let mock = InitializationMock::default(); + let sdk = crate::sdk::DpfSdkBuilder::new(mock.clone(), TEST_NS, "test-password".to_string()) + .with_labeler(InitializationLabeler) + .build_without_resources() + .await + .unwrap(); + sdk.create_initialization_objects(&InitDpfResourcesConfig { + bfb_url: "http://example.com/test.bfb".to_string(), + deployment_name: "old-deployment".to_string(), + deployment_scoped_service_interfaces: true, + ..Default::default() + }) + .await + .unwrap(); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p1-bf3", TEST_NS) + .await + .unwrap() + .is_some() + ); + sdk.create_initialization_objects(&InitDpfResourcesConfig { + bfb_url: "http://example.com/test.bfb".to_string(), + deployment_name: "new-deployment".to_string(), + deployment_scoped_service_interfaces: false, + ..Default::default() + }) + .await + .unwrap(); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p1", TEST_NS) + .await + .unwrap() + .is_some() + ); + assert!( + DpuServiceInterfaceRepository::get(&mock, "p1-bf3", TEST_NS) + .await + .unwrap() + .is_none() + ); + drop(sdk); } /// Verifies a missing referenced template fails the complete inventory lookup diff --git a/crates/dpf/src/types.rs b/crates/dpf/src/types.rs index fee32bd1bc..7ca73cf8ee 100644 --- a/crates/dpf/src/types.rs +++ b/crates/dpf/src/types.rs @@ -90,9 +90,10 @@ pub struct InitDpfResourcesConfig { pub pf_total_sf_reserved: u32, /// Enables deployment-scoped DPUServiceInterface names and node selectors. /// False preserves the legacy global resource naming and selector mode for - /// BF3 and generic BF4; BF4 Astra requires this to be true. - /// Mode transitions require manual old-resource cleanup and DPU re-ingestion; - /// the SDK neither detects nor deletes the previous generation. + /// BF3 and generic BF4. BF4 Astra requires this to be true for the whole namespace so + /// legacy match-all resources do not bind Astra nodes. Initialization removes + /// ServiceInterfaces from the opposite mode during a transition; failures are not rolled + /// back and require a retry after remediation. pub deployment_scoped_service_interfaces: bool, /// Optional intercept-bridging topology for BF3 and generic BF4. `Some` replaces the /// ordinary static PF/VF inventory and contains exactly one configured PF.