-
Notifications
You must be signed in to change notification settings - Fork 183
feat(dpf): gate the outdated-DPU scan on DPFOperatorConfig readiness #5299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
da7f633
a041ab5
808392e
678b3b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2248,7 +2248,55 @@ impl<R: DpuRepository, L> DpfSdk<R, L> { | |
| } | ||
| } | ||
|
|
||
| impl<R: DpuDeploymentRepository + DpuRepository, L> DpfSdk<R, L> { | ||
| /// Name of the singleton DPFOperatorConfig, as created by helm-prereqs and by the | ||
| /// manual install in `docs/manuals/dpf.md`. | ||
| const DPF_OPERATOR_CONFIG_NAME: &str = "dpfoperatorconfig"; | ||
|
|
||
| impl<R: DpuDeploymentRepository + DpuRepository + DpfOperatorConfigRepository, L> DpfSdk<R, L> { | ||
| /// Whether the DPF operator reports `Ready=True` at its current generation. | ||
| /// | ||
| /// Fails closed: absent, unreconciled, or condition-less all read as not | ||
| /// ready, so callers that gate disruptive work skip rather than guess. | ||
| async fn dpf_operator_config_is_ready(&self) -> Result<bool, DpfError> { | ||
| let config = DpfOperatorConfigRepository::get( | ||
| &*self.repo, | ||
| DPF_OPERATOR_CONFIG_NAME, | ||
| &self.namespace, | ||
| ) | ||
| .await?; | ||
|
|
||
| let Some(config) = config else { | ||
| tracing::info!( | ||
| name = DPF_OPERATOR_CONFIG_NAME, | ||
| namespace = %self.namespace, | ||
| "DPFOperatorConfig not found; treating DPF as not ready" | ||
| ); | ||
| return Ok(false); | ||
| }; | ||
|
|
||
| let ready = config | ||
| .status | ||
| .as_ref() | ||
| .and_then(|status| status.conditions.as_ref()) | ||
| .and_then(|conditions| conditions.iter().find(|c| c.type_ == "Ready")) | ||
| .is_some_and(|condition| { | ||
| condition.status == "True" | ||
| && observed_generation_is_current( | ||
| condition.observed_generation, | ||
| config.metadata.generation, | ||
| ) | ||
| }); | ||
|
|
||
| if !ready { | ||
| tracing::info!( | ||
| name = DPF_OPERATOR_CONFIG_NAME, | ||
| namespace = %self.namespace, | ||
| "DPFOperatorConfig is not Ready; treating DPF as not ready" | ||
| ); | ||
| } | ||
| Ok(ready) | ||
| } | ||
|
Comment on lines
+2260
to
+2298
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Add table-driven tests for the readiness gate. This gate controls whether the scan can report DPU mismatches. Add cases for absent configuration, missing status or conditions, As per coding guidelines: “Prefer table-driven tests for any function that maps inputs to outputs, errors, or other observable results.” Also applies to: 2337-2343, 2510-2518 🤖 Prompt for AI AgentsSource: Coding guidelines
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid — the readiness gate has no test coverage. It is a known gap, already called out under Additional Notes. To be explicit about what the existing suite does and does not reach: every test in Table-driven cases for the readiness matrix — absent config, no status, no conditions,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The required table-driven readiness matrix and closed-gate scan test remain necessary. I will keep this finding open until the tests are present. ✏️ Learnings added
You are interacting with an AI system. |
||
|
|
||
| /// Find DPUs whose installed BFB, BlueFieldSoftware, or `spec.dpuFlavor` no | ||
| /// longer matches the values declared on the DPUDeployment that owns them. | ||
| /// | ||
|
|
@@ -2286,6 +2334,14 @@ impl<R: DpuDeploymentRepository + DpuRepository, L> DpfSdk<R, L> { | |
| &self, | ||
| dpu_label_selector: Option<&str>, | ||
| ) -> Result<Vec<DpuMismatch>, DpfError> { | ||
| // A DPF upgrade republishes the CRs this scan reads, so mid-upgrade a DPU | ||
| // can look outdated against a deployment that is still settling. Report | ||
| // nothing until the operator says it is Ready, so an upgrade never | ||
| // triggers reprovisioning on its own. | ||
| if !self.dpf_operator_config_is_ready().await? { | ||
| return Ok(vec![]); | ||
| } | ||
|
|
||
| let deployments = DpuDeploymentRepository::list(&*self.repo, &self.namespace).await?; | ||
| let ready_deployments: HashMap<String, &DPUDeployment> = deployments | ||
| .iter() | ||
|
|
@@ -2451,6 +2507,16 @@ fn dpu_deployment_is_ready(d: &DPUDeployment) -> bool { | |
| cond.status == "True" && cond.observed_generation == Some(generation) | ||
| } | ||
|
|
||
| /// True when a condition's `observedGeneration` matches the object's, or when | ||
| /// either is absent. Stamping it is optional, and demanding it would leave the | ||
| /// object permanently unready against an operator that omits it. | ||
| fn observed_generation_is_current(observed: Option<i64>, generation: Option<i64>) -> bool { | ||
| match (observed, generation) { | ||
| (Some(observed), Some(generation)) => observed == generation, | ||
| _ => true, | ||
|
abvarshney-nv marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| impl<R: DpuNodeMaintenanceRepository, L> DpfSdk<R, L> { | ||
| /// Release the hold on a DPU node maintenance. | ||
| /// If the DpuNodeMaintenance CR doesn't exist, this is a no-op | ||
|
|
@@ -3815,6 +3881,15 @@ mod tests { | |
|
|
||
| #[async_trait] | ||
| impl crate::repository::DpfOperatorConfigRepository for SdkMock { | ||
| async fn get( | ||
| &self, | ||
| _name: &str, | ||
| _ns: &str, | ||
| ) -> Result<Option<crate::crds::dpfoperatorconfigs_generated::DPFOperatorConfig>, DpfError> | ||
| { | ||
| Ok(None) | ||
| } | ||
|
|
||
| async fn patch(&self, _: &str, _: &str, _: serde_json::Value) -> Result<(), DpfError> { | ||
| Ok(()) | ||
| } | ||
|
|
@@ -4514,6 +4589,15 @@ mod tests { | |
|
|
||
| #[async_trait] | ||
| impl crate::repository::DpfOperatorConfigRepository for SecretTrackingMock { | ||
| async fn get( | ||
| &self, | ||
| _name: &str, | ||
| _ns: &str, | ||
| ) -> Result<Option<crate::crds::dpfoperatorconfigs_generated::DPFOperatorConfig>, DpfError> | ||
| { | ||
| Ok(None) | ||
| } | ||
|
|
||
| async fn patch(&self, _: &str, _: &str, _: serde_json::Value) -> Result<(), DpfError> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,12 +29,14 @@ use async_trait::async_trait; | |
| use dashmap::DashMap; | ||
| use kube::core::ObjectMeta; | ||
|
|
||
| use crate::crds::dpfoperatorconfigs_generated::DPFOperatorConfig; | ||
| use crate::crds::dpudeployments_generated::DPUDeployment; | ||
| use crate::crds::dpus_generated::DPU; | ||
| use crate::crds::dpuservicetemplates_generated::DPUServiceTemplate; | ||
| use crate::error::DpfError; | ||
| use crate::repository::{ | ||
| DpuDeploymentRepository, DpuRepository, DpuServiceTemplateRepository, K8sConfigRepository, | ||
| DpfOperatorConfigRepository, DpuDeploymentRepository, DpuRepository, | ||
| DpuServiceTemplateRepository, K8sConfigRepository, | ||
| }; | ||
| use crate::sdk::DpfSdkBuilder; | ||
|
|
||
|
|
@@ -48,6 +50,7 @@ const OWNED_BY_LABEL: &str = "svc.dpu.nvidia.com/owned-by-dpudeployment"; | |
| struct OutdatedDpuMock { | ||
| dpus: Arc<DashMap<String, DPU>>, | ||
| deployments: Arc<DashMap<String, DPUDeployment>>, | ||
| operator_config: Arc<DashMap<String, DPFOperatorConfig>>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win Make
The lookup also indexes only by Also applies to: 98-102 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Splitting the two points. Namespace keying — no change needed. This file is single-namespace by construction: Seeding a ready fixture — agreed, and needed. Nothing is broken today, since the tests here all go through
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The ready ✏️ Learnings added
You are interacting with an AI system. |
||
| } | ||
|
|
||
| impl OutdatedDpuMock { | ||
|
|
@@ -92,6 +95,16 @@ impl DpuRepository for OutdatedDpuMock { | |
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl DpfOperatorConfigRepository for OutdatedDpuMock { | ||
| async fn get(&self, name: &str, _ns: &str) -> Result<Option<DPFOperatorConfig>, DpfError> { | ||
| Ok(self.operator_config.get(name).map(|c| c.clone())) | ||
| } | ||
| async fn patch(&self, _: &str, _: &str, _: serde_json::Value) -> Result<(), DpfError> { | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl DpuDeploymentRepository for OutdatedDpuMock { | ||
| async fn get(&self, name: &str, _ns: &str) -> Result<Option<DPUDeployment>, DpfError> { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.