Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions crates/admin-cli/src/instance/allocate/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
*/

use carbide_uuid::machine::MachineId;
use carbide_uuid::vpc::VpcPrefixId;
use carbide_uuid::vpc::{VpcId, VpcPrefixId};
use clap::{ArgGroup, Parser};
use rpc::forge::{InstanceOperatingSystemConfig, InstanceSpxConfig};

#[derive(Parser, Debug)]
#[clap(group(ArgGroup::new("selector").required(true).args(&["subnet", "vpc_prefix_id"])))]
#[clap(group(ArgGroup::new("selector").required(true).multiple(true).args(&["subnet", "vpc_prefix_id", "flat_vpc_id"])))]
#[command(after_long_help = "\
EXAMPLES:

Expand Down Expand Up @@ -101,8 +101,11 @@ pub struct Args {
#[clap(short, long, help = "The VPC prefix to assign to a PF")]
pub vpc_prefix_id: Vec<VpcPrefixId>,

#[clap(long, help = "Allocate a zero-dpu host")]
pub zero_dpu: bool,
#[clap(
long,
help = "Create an instance in the given \"flat\" VPC, for machines without DPUs"
)]
pub flat_vpc_id: Option<VpcId>,

#[clap(long, help = "The VPC prefix to assign to a VF")]
pub vf_vpc_prefix_id: Vec<VpcPrefixId>,
Expand Down
4 changes: 2 additions & 2 deletions crates/admin-cli/src/instance/allocate/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ pub async fn allocate(
api_client,
&mut machine_ids,
min_interface_count,
allocate_request.zero_dpu,
allocate_request.flat_vpc_id,
)
.await
else {
Expand Down Expand Up @@ -109,7 +109,7 @@ pub async fn allocate(
api_client,
&mut machine_ids,
min_interface_count,
allocate_request.zero_dpu,
allocate_request.flat_vpc_id,
)
.await
else {
Expand Down
201 changes: 119 additions & 82 deletions crates/admin-cli/src/instance/show/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,99 +178,125 @@ async fn convert_instance_to_nice_format(

let width = 25;
writeln!(&mut lines, "INTERFACES:")?;
let if_configs = instance
let network_config = instance
.config
.as_ref()
.and_then(|config| config.network.as_ref())
.and_then(|config| config.network.as_ref());
let if_configs = network_config
.map(|config| config.interfaces.as_slice())
.unwrap_or_default();
let auto_network = network_config.is_some_and(|config| config.auto_config.is_some());
let if_status = instance
.status
.as_ref()
.and_then(|status| status.network.as_ref())
.map(|status| status.interfaces.as_slice())
.unwrap_or_default();

if if_configs.is_empty() || if_status.is_empty() {
if if_status.is_empty() {
writeln!(&mut lines, "\tEMPTY")?;
} else if if_configs.len() != if_status.len() {
} else if !auto_network && if_configs.len() != if_status.len() {
writeln!(&mut lines, "\tLENGTH MISMATCH")?;
} else {
for (i, interface) in if_configs.iter().enumerate() {
let status = &if_status[i];

let vpc = if let Some(network_segment_id) = interface.network_segment_id {
get_vpc_for_interface_network_segment(api_client, network_segment_id).await?
} else {
None
};

let data: &[(&str, Cow<str>)] = &[
(
"FUNCTION_TYPE",
forgerpc::InterfaceFunctionType::try_from(interface.function_type)
.ok()
.map(|ty| format!("{ty:?}").into())
.unwrap_or_else(|| "INVALID".into()),
),
(
"VF ID",
status
.virtual_function_id
.map(|id| id.to_string().into())
.unwrap_or_default(),
),
(
"SEGMENT ID",
interface
.network_segment_id
.unwrap_or_default()
.to_string()
let vpcs: Vec<Vpc> = if auto_network {
futures::future::join_all(
if_status
.iter()
.filter_map(|s| s.vpc_id)
.map(|vpc_id| get_vpc_by_id(api_client, vpc_id)),
)
.await
.into_iter()
.collect::<Result<Vec<_>, _>>()?
.into_iter()
} else {
futures::future::join_all(if_configs.iter().filter_map(|c| c.network_segment_id).map(
|segment_id| async move {
get_vpc_for_interface_network_segment(api_client, segment_id).await
},
))
.await
.into_iter()
.collect::<Result<Vec<_>, _>>()?
.into_iter()
}
.flatten()
.collect();
if !auto_network && if_configs.len() != if_status.len() {
writeln!(&mut lines, "\tLENGTH MISMATCH")?;
} else {
for (idx, status) in if_status.iter().enumerate() {
let vpc = vpcs.get(idx);
let if_config = if_configs.get(idx);
Comment on lines +201 to +230

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve interface-to-VPC alignment when lookups are missing.

filter_map(...).flatten() shrinks the lookup result list, so a missing vpc_id, missing segment ID, or not-found VPC shifts later VPCs onto the wrong interface rows. Collect one Option<Vpc> per rendered interface.

Proposed fix
-        let vpcs: Vec<Vpc> = if auto_network {
+        let vpcs: Vec<Option<Vpc>> = if auto_network {
             futures::future::join_all(
                 if_status
                     .iter()
-                    .filter_map(|s| s.vpc_id)
-                    .map(|vpc_id| get_vpc_by_id(api_client, vpc_id)),
+                    .map(|s| async move {
+                        if let Some(vpc_id) = s.vpc_id {
+                            get_vpc_by_id(api_client, vpc_id).await
+                        } else {
+                            Ok(None)
+                        }
+                    }),
             )
             .await
             .into_iter()
             .collect::<Result<Vec<_>, _>>()?
-            .into_iter()
         } else {
-            futures::future::join_all(if_configs.iter().filter_map(|c| c.network_segment_id).map(
-                |segment_id| async move {
+            futures::future::join_all(if_configs.iter().map(|c| async move {
+                if let Some(segment_id) = c.network_segment_id {
                     get_vpc_for_interface_network_segment(api_client, segment_id).await
-                },
-            ))
+                } else {
+                    Ok(None)
+                }
+            }))
             .await
             .into_iter()
             .collect::<Result<Vec<_>, _>>()?
-            .into_iter()
         }
-        .flatten()
-        .collect();
         if !auto_network && if_configs.len() != if_status.len() {
             writeln!(&mut lines, "\tLENGTH MISMATCH")?;
         } else {
             for (idx, status) in if_status.iter().enumerate() {
-                let vpc = vpcs.get(idx);
+                let vpc = vpcs.get(idx).and_then(Option::as_ref);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/admin-cli/src/instance/show/cmd.rs` around lines 201 - 230, The
current implementation uses filter_map and flatten operations that remove
missing VPC lookup results, causing the remaining VPCs to shift to earlier
indices and misalign with their corresponding interfaces. Replace the
filter_map(s.vpc_id).map(...) and filter_map(c.network_segment_id).map(...)
patterns with approaches that preserve an Option<Vpc> for each interface instead
of discarding missing results. Specifically, for the auto_network branch, map
each interface status to an Option containing either the looked-up VPC or None
if vpc_id is missing; for the else branch, map each interface config to an
Option containing either the looked-up VPC or None if segment_id is missing or
the lookup fails. Remove the flatten() call since you now have Vec<Option<Vpc>>
with proper 1:1 alignment, and adjust the vpcs.get(idx) lookups to handle the
Option wrapper appropriately when displaying results.

let data: &[(&str, Cow<str>)] = &[
(
"FUNCTION_TYPE",
format!(
"{:?}",
match status.virtual_function_id {
Some(_) => forgerpc::InterfaceFunctionType::Virtual,
None => forgerpc::InterfaceFunctionType::Physical,
}
)
.into(),
),
(
"VPC PREFIX ID",
match &interface.network_details {
Some(forgerpc::instance_interface_config::NetworkDetails::SegmentId(_)) => {
"Segment Based Allocation".into()
}
Some(forgerpc::instance_interface_config::NetworkDetails::VpcPrefixId(
x,
)) => x.to_string().into(),
None => "NA".into(),
},
),
(
"MAC ADDR",
status
.mac_address
.as_ref()
.map(|s| s.as_str().into())
.unwrap_or_default(),
),
("ADDRESSES", status.addresses.as_slice().join(", ").into()),
(
"VPC ID",
vpc.as_ref()
.map(|v| v.id.unwrap_or_default().to_string().into())
.unwrap_or("<not found>".into()),
),
(
"VPC NAME",
vpc.as_ref()
.and_then(|v| v.metadata.as_ref())
.map(|v| Cow::Borrowed(v.name.as_str()))
.unwrap_or("<not found>".into()),
),
];

for (key, value) in data {
writeln!(&mut lines, "\t{key:<width$}: {value}")?;
),
(
"VF ID",
status
.virtual_function_id
.map(|id| id.to_string().into())
.unwrap_or_default(),
),
(
"SEGMENT ID",
if_config
.and_then(|c| c.network_segment_id)
.unwrap_or_default()
.to_string()
.into(),
),
(
"VPC PREFIX ID",
match if_config.and_then(|c| c.network_details.as_ref()) {
Some(
forgerpc::instance_interface_config::NetworkDetails::SegmentId(_),
) => "Segment Based Allocation".into(),
Some(
forgerpc::instance_interface_config::NetworkDetails::VpcPrefixId(x),
) => x.to_string().into(),
None => "NA".into(),
},
),
(
"MAC ADDR",
status
.mac_address
.as_ref()
.map(|s| s.as_str().into())
.unwrap_or_default(),
),
("ADDRESSES", status.addresses.as_slice().join(", ").into()),
(
"VPC ID",
vpc.map(|v| v.id.unwrap_or_default().to_string().into())
.unwrap_or("<not found>".into()),
),
(
"VPC NAME",
vpc.and_then(|v| v.metadata.as_ref())
.map(|v| Cow::Borrowed(v.name.as_str()))
.unwrap_or("<not found>".into()),
),
];

for (key, value) in data {
writeln!(&mut lines, "\t{key:<width$}: {value}")?;
}
writeln!(
&mut lines,
"\t--------------------------------------------------"
)?;
}
writeln!(
&mut lines,
"\t--------------------------------------------------"
)?;
}
}

Expand Down Expand Up @@ -555,7 +581,6 @@ pub async fn handle_show(args: Args, ctx: &mut RuntimeContext) -> CarbideCliResu
Ok(())
}

#[allow(deprecated)]
async fn get_vpc_for_interface_network_segment(
api_client: &ApiClient,
network_segment_id: NetworkSegmentId,
Expand All @@ -565,10 +590,10 @@ async fn get_vpc_for_interface_network_segment(
.await?;

if !network_segments.network_segments.is_empty()
&& let Some(vpc_id) = network_segments
.network_segments
.first()
.and_then(|s| s.config.as_ref().and_then(|c| c.vpc_id).or(s.vpc_id))
&& let Some(vpc_id) = network_segments.network_segments.first().and_then(|s| {
#[allow(deprecated)]
s.config.as_ref().and_then(|c| c.vpc_id).or(s.vpc_id)
})
{
let vpc_ids: Vec<VpcId> = vec![vpc_id];
Ok(api_client
Expand All @@ -582,3 +607,15 @@ async fn get_vpc_for_interface_network_segment(
Ok(None)
}
}

async fn get_vpc_by_id(api_client: &ApiClient, vpc_id: VpcId) -> CarbideCliResult<Option<Vpc>> {
Ok(api_client
.0
.find_vpcs_by_ids(VpcsByIdsRequest {
vpc_ids: vec![vpc_id],
})
.await?
.vpcs
.into_iter()
.next())
}
5 changes: 3 additions & 2 deletions crates/admin-cli/src/machine/show/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use std::fmt::Write;
use ::rpc::admin_cli::OutputFormat;
use ::rpc::forge as forgerpc;
use carbide_uuid::machine::MachineId;
use carbide_uuid::vpc::VpcId;
use prettytable::{Table, row};
use rpc::Machine;

Expand Down Expand Up @@ -428,7 +429,7 @@ pub async fn get_next_free_machine(
api_client: &ApiClient,
machine_ids: &mut VecDeque<MachineId>,
min_interface_count: usize,
zero_dpu: bool,
flat_vpc_id: Option<VpcId>,
) -> Option<Machine> {
while let Some(id) = machine_ids.pop_front() {
tracing::debug!("Checking {}", id);
Expand All @@ -437,7 +438,7 @@ pub async fn get_next_free_machine(
tracing::debug!("Machine is not ready");
continue;
}
if zero_dpu {
if flat_vpc_id.is_some() {
return Some(machine);
}
Comment on lines +441 to 443

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Filter flat-VPC allocation to static-membership machines.

This returns the first Ready machine without checking whether it is eligible for zero-DPU flat-VPC auto allocation. Since the server rejects auto_config on DPU-backed machines, batch allocation can fail before trying a later valid host.

Proposed fix
-            if flat_vpc_id.is_some() {
-                return Some(machine);
-            }
+            if flat_vpc_id.is_some() {
+                let supports_static_membership = machine
+                    .instance_network_restrictions
+                    .as_ref()
+                    .is_some_and(|restrictions| {
+                        restrictions.network_segment_membership_type
+                            == forgerpc::InstanceNetworkSegmentMembershipType::Static as i32
+                    });
+
+                if supports_static_membership {
+                    return Some(machine);
+                }
+
+                tracing::debug!(%id, "machine does not support flat VPC auto allocation");
+                continue;
+            }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/admin-cli/src/machine/show/cmd.rs` around lines 441 - 443, The current
condition checking if flat_vpc_id.is_some() returns the first eligible machine
without verifying it is suitable for zero-DPU flat-VPC auto allocation. Add an
additional eligibility check in the if statement to ensure the machine is a
static-membership machine before returning Some(machine), since the server
rejects auto_config on DPU-backed machines and this prevents batch allocation
from trying valid hosts.

if let Some(discovery_info) = &machine.discovery_info {
Expand Down
Loading
Loading