Skip to content

Commit f0c692d

Browse files
gjcolombogjcolombo
committed
PR feedback: SocketAddrV6, misc cleanup
1 parent 72103b1 commit f0c692d

File tree

6 files changed

+66
-86
lines changed

6 files changed

+66
-86
lines changed

‎nexus/db-queries/src/db/datastore/volume.rs

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2578,11 +2578,15 @@ fn region_in_vcr(
25782578

25792579
VolumeConstructionRequest::Region { opts, .. } => {
25802580
for target in &opts.target {
2581-
if let SocketAddr::V6(target) = target {
2582-
if *target == *region {
2581+
match target {
2582+
SocketAddr::V6(t) if *t == *region => {
25832583
region_found = true;
25842584
break;
25852585
}
2586+
SocketAddr::V6(_) => {}
2587+
SocketAddr::V4(_) => {
2588+
bail!("region target contains an IPv4 address");
2589+
}
25862590
}
25872591
}
25882592
}
@@ -2644,10 +2648,16 @@ fn read_only_target_in_vcr(
26442648
}
26452649

26462650
for target in &opts.target {
2647-
if let SocketAddr::V6(target) = target {
2648-
if *target == *read_only_target && opts.read_only {
2651+
match target {
2652+
SocketAddr::V6(t)
2653+
if *t == *read_only_target && opts.read_only =>
2654+
{
26492655
return Ok(true);
26502656
}
2657+
SocketAddr::V6(_) => {}
2658+
SocketAddr::V4(_) => {
2659+
bail!("region target contains an IPv4 address");
2660+
}
26512661
}
26522662
}
26532663
}
@@ -4986,10 +4996,7 @@ mod tests {
49864996
.unwrap();
49874997

49884998
let volumes = datastore
4989-
.find_volumes_referencing_socket_addr(
4990-
&opctx,
4991-
address_1.into(),
4992-
)
4999+
.find_volumes_referencing_socket_addr(&opctx, address_1.into())
49935000
.await
49945001
.unwrap();
49955002

‎nexus/src/app/background/tasks/region_snapshot_replacement_step.rs

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -568,14 +568,15 @@ mod test {
568568
use omicron_uuid_kinds::DatasetUuid;
569569
use sled_agent_client::CrucibleOpts;
570570
use sled_agent_client::VolumeConstructionRequest;
571+
use std::net::SocketAddrV6;
571572
use uuid::Uuid;
572573

573574
type ControlPlaneTestContext =
574575
nexus_test_utils::ControlPlaneTestContext<crate::Server>;
575576

576577
async fn add_fake_volume_for_snapshot_addr(
577578
datastore: &DataStore,
578-
snapshot_addr: String,
579+
snapshot_addr: SocketAddrV6,
579580
) -> Uuid {
580581
let new_volume_id = Uuid::new_v4();
581582

@@ -587,7 +588,7 @@ mod test {
587588
DatasetUuid::new_v4(),
588589
Uuid::new_v4(),
589590
Uuid::new_v4(),
590-
snapshot_addr.clone(),
591+
snapshot_addr.to_string(),
591592
))
592593
.await
593594
.unwrap();
@@ -604,7 +605,7 @@ mod test {
604605
gen: 0,
605606
opts: CrucibleOpts {
606607
id: Uuid::new_v4(),
607-
target: vec![snapshot_addr.parse().unwrap()],
608+
target: vec![snapshot_addr.into()],
608609
lossy: false,
609610
flush_timeout: None,
610611
key: None,
@@ -656,13 +657,14 @@ mod test {
656657
let dataset_id = DatasetUuid::new_v4();
657658
let region_id = Uuid::new_v4();
658659
let snapshot_id = Uuid::new_v4();
659-
let snapshot_addr = String::from("[fd00:1122:3344::101]:9876");
660+
let snapshot_addr: SocketAddrV6 =
661+
"[fd00:1122:3344::101]:9876".parse().unwrap();
660662

661663
let fake_region_snapshot = RegionSnapshot::new(
662664
dataset_id,
663665
region_id,
664666
snapshot_id,
665-
snapshot_addr.clone(),
667+
snapshot_addr.to_string(),
666668
);
667669

668670
datastore.region_snapshot_create(fake_region_snapshot).await.unwrap();
@@ -746,28 +748,22 @@ mod test {
746748
// Add some fake volumes that reference the region snapshot being
747749
// replaced
748750

749-
let new_volume_1_id = add_fake_volume_for_snapshot_addr(
750-
&datastore,
751-
snapshot_addr.clone(),
752-
)
753-
.await;
754-
let new_volume_2_id = add_fake_volume_for_snapshot_addr(
755-
&datastore,
756-
snapshot_addr.clone(),
757-
)
758-
.await;
751+
let new_volume_1_id =
752+
add_fake_volume_for_snapshot_addr(&datastore, snapshot_addr).await;
753+
let new_volume_2_id =
754+
add_fake_volume_for_snapshot_addr(&datastore, snapshot_addr).await;
759755

760756
// Add some fake volumes that do not
761757

762758
let other_volume_1_id = add_fake_volume_for_snapshot_addr(
763759
&datastore,
764-
String::from("[fd00:1122:3344::101]:1000"),
760+
"[fd00:1122:3344::101]:1000".parse().unwrap(),
765761
)
766762
.await;
767763

768764
let other_volume_2_id = add_fake_volume_for_snapshot_addr(
769765
&datastore,
770-
String::from("[fd12:5544:3344::912]:3901"),
766+
"[fd12:5544:3344::912]:3901".parse().unwrap(),
771767
)
772768
.await;
773769

‎nexus/src/app/instance.rs

Lines changed: 7 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,7 +1052,6 @@ impl super::Nexus {
10521052
)
10531053
.await?;
10541054

1055-
let mut found_boot_disk = None;
10561055
let mut disk_reqs = vec![];
10571056
for disk in &disks {
10581057
// Disks that are attached to an instance should always have a slot
@@ -1087,12 +1086,6 @@ impl super::Nexus {
10871086
)
10881087
.await?;
10891088

1090-
if let Some(wanted_id) = &db_instance.boot_disk_id {
1091-
if disk.id() == *wanted_id {
1092-
found_boot_disk = Some(*wanted_id);
1093-
}
1094-
}
1095-
10961089
disk_reqs.push(sled_agent_client::types::InstanceDisk {
10971090
disk_id: disk.id(),
10981091
name: disk.name().to_string(),
@@ -1102,38 +1095,13 @@ impl super::Nexus {
11021095
});
11031096
}
11041097

1105-
let boot_settings = if let Some(boot_disk_id) = found_boot_disk {
1106-
Some(InstanceBootSettings { order: vec![boot_disk_id] })
1107-
} else {
1108-
if let Some(instance_boot_disk_id) =
1109-
db_instance.boot_disk_id.as_ref()
1110-
{
1111-
// This should never occur: when setting the boot disk we ensure it is
1112-
// attached, and when detaching a disk we ensure it is not the boot
1113-
// disk. If this error is seen, the instance somehow had a boot disk
1114-
// that was not attached anyway.
1115-
//
1116-
// When Propolis accepts an ID rather than name, and we don't need to
1117-
// look up a name when assembling the Propolis request, we might as well
1118-
// remove this check; we can just pass the ID and rely on Propolis' own
1119-
// check that the boot disk is attached.
1120-
if found_boot_disk.is_none() {
1121-
error!(self.log, "instance boot disk is not attached";
1122-
"boot_disk_id" => ?instance_boot_disk_id,
1123-
"instance id" => %db_instance.id());
1124-
1125-
return Err(InstanceStateChangeError::Other(
1126-
Error::internal_error(&format!(
1127-
"instance {} has boot disk {:?} but it is not attached",
1128-
db_instance.id(),
1129-
db_instance.boot_disk_id.as_ref(),
1130-
)),
1131-
));
1132-
}
1133-
}
1134-
1135-
None
1136-
};
1098+
// The routines that maintain an instance's boot options are supposed to
1099+
// guarantee that the boot disk ID, if present, is the ID of an attached
1100+
// disk. If this invariant isn't upheld, Propolis will catch the failure
1101+
// when it processes its received VM configuration.
1102+
let boot_settings = db_instance
1103+
.boot_disk_id
1104+
.map(|id| InstanceBootSettings { order: vec![id] });
11371105

11381106
let nics = self
11391107
.db_datastore

‎nexus/src/app/sagas/snapshot_create.rs

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,8 @@ use steno::ActionError;
125125
use steno::Node;
126126
use uuid::Uuid;
127127

128+
type ReplaceSocketsMap = BTreeMap<SocketAddrV6, SocketAddrV6>;
129+
128130
// snapshot create saga: input parameters
129131

130132
#[derive(Debug, Deserialize, Serialize)]
@@ -1384,7 +1386,7 @@ async fn ssc_detach_disk_from_pantry(
13841386

13851387
async fn ssc_start_running_snapshot(
13861388
sagactx: NexusActionContext,
1387-
) -> Result<BTreeMap<SocketAddr, SocketAddr>, ActionError> {
1389+
) -> Result<ReplaceSocketsMap, ActionError> {
13881390
let log = sagactx.user_data().log();
13891391
let osagactx = sagactx.user_data();
13901392
let params = sagactx.saga_params::<Params>()?;
@@ -1410,7 +1412,7 @@ async fn ssc_start_running_snapshot(
14101412
.await
14111413
.map_err(ActionError::action_failed)?;
14121414

1413-
let mut map: BTreeMap<SocketAddr, SocketAddr> = BTreeMap::new();
1415+
let mut map: ReplaceSocketsMap = BTreeMap::new();
14141416

14151417
for (dataset, region) in datasets_and_regions {
14161418
let Some(dataset_addr) = dataset.address() else {
@@ -1454,7 +1456,7 @@ async fn ssc_start_running_snapshot(
14541456
);
14551457

14561458
info!(log, "map {} to {}", region_addr, snapshot_addr);
1457-
map.insert(region_addr.into(), snapshot_addr.into());
1459+
map.insert(region_addr, snapshot_addr);
14581460

14591461
// Once snapshot has been validated, and running snapshot has been
14601462
// started, add an entry in the region_snapshot table to correspond to
@@ -1564,8 +1566,8 @@ async fn ssc_create_volume_record(
15641566
// The volume construction request must then be modified to point to the
15651567
// read-only crucible agent downstairs (corresponding to this snapshot)
15661568
// launched through this saga.
1567-
let replace_sockets_map = sagactx
1568-
.lookup::<BTreeMap<SocketAddr, SocketAddr>>("replace_sockets_map")?;
1569+
let replace_sockets_map =
1570+
sagactx.lookup::<ReplaceSocketsMap>("replace_sockets_map")?;
15691571
let snapshot_volume_construction_request: VolumeConstructionRequest =
15701572
create_snapshot_from_disk(
15711573
&disk_volume_construction_request,
@@ -1689,7 +1691,7 @@ async fn ssc_release_volume_lock(
16891691
/// VolumeConstructionRequest and modifying it accordingly.
16901692
fn create_snapshot_from_disk(
16911693
disk: &VolumeConstructionRequest,
1692-
socket_map: &BTreeMap<SocketAddr, SocketAddr>,
1694+
socket_map: &ReplaceSocketsMap,
16931695
) -> anyhow::Result<VolumeConstructionRequest> {
16941696
// When copying a disk's VolumeConstructionRequest to turn it into a
16951697
// snapshot:
@@ -1751,6 +1753,16 @@ fn create_snapshot_from_disk(
17511753

17521754
if work.socket_modification_required {
17531755
for target in &mut opts.target {
1756+
let target = match target {
1757+
SocketAddr::V6(v6) => v6,
1758+
SocketAddr::V4(_) => {
1759+
anyhow::bail!(
1760+
"unexpected IPv4 address in VCR: {:?}",
1761+
work.vcr_part
1762+
)
1763+
}
1764+
};
1765+
17541766
*target = *socket_map.get(target).ok_or_else(|| {
17551767
anyhow!("target {} not found in map!", target)
17561768
})?;
@@ -1869,8 +1881,7 @@ mod test {
18691881
],
18701882
};
18711883

1872-
let mut replace_sockets: BTreeMap<SocketAddr, SocketAddr> =
1873-
BTreeMap::new();
1884+
let mut replace_sockets = ReplaceSocketsMap::new();
18741885

18751886
// Replacements for top level Region only
18761887
replace_sockets.insert(

‎nexus/tests/integration_tests/volume_management.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3567,7 +3567,7 @@ struct TestReadOnlyRegionReferenceUsage {
35673567
datastore: Arc<DataStore>,
35683568

35693569
region: db::model::Region,
3570-
region_address: SocketAddr,
3570+
region_address: SocketAddrV6,
35713571

35723572
first_volume_id: Uuid,
35733573
second_volume_id: Uuid,
@@ -3628,9 +3628,8 @@ impl TestReadOnlyRegionReferenceUsage {
36283628
// so fill in a random port here.
36293629
datastore.region_set_port(region.id(), 12345).await.unwrap();
36303630

3631-
let region_address = SocketAddr::V6(
3632-
datastore.region_addr(region.id()).await.unwrap().unwrap(),
3633-
);
3631+
let region_address =
3632+
datastore.region_addr(region.id()).await.unwrap().unwrap();
36343633

36353634
let region = datastore.get_region(region.id()).await.unwrap();
36363635

@@ -3661,7 +3660,7 @@ impl TestReadOnlyRegionReferenceUsage {
36613660
gen: 1,
36623661
opts: CrucibleOpts {
36633662
id: Uuid::new_v4(),
3664-
target: vec![self.region_address],
3663+
target: vec![self.region_address.into()],
36653664
lossy: false,
36663665
flush_timeout: None,
36673666
key: None,
@@ -3791,7 +3790,7 @@ impl TestReadOnlyRegionReferenceUsage {
37913790
gen: 1,
37923791
opts: CrucibleOpts {
37933792
id: Uuid::new_v4(),
3794-
target: vec![self.region_address],
3793+
target: vec![self.region_address.into()],
37953794
lossy: false,
37963795
flush_timeout: None,
37973796
key: None,
@@ -3824,7 +3823,7 @@ impl TestReadOnlyRegionReferenceUsage {
38243823
gen: 1,
38253824
opts: CrucibleOpts {
38263825
id: Uuid::new_v4(),
3827-
target: vec![self.region_address],
3826+
target: vec![self.region_address.into()],
38283827
lossy: false,
38293828
flush_timeout: None,
38303829
key: None,
@@ -3859,7 +3858,7 @@ impl TestReadOnlyRegionReferenceUsage {
38593858
gen: 1,
38603859
opts: CrucibleOpts {
38613860
id: Uuid::new_v4(),
3862-
target: vec![self.region_address],
3861+
target: vec![self.region_address.into()],
38633862
lossy: false,
38643863
flush_timeout: None,
38653864
key: None,

‎sled-agent/src/instance.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -869,8 +869,8 @@ impl InstanceRunner {
869869
// components (e.g., converting device slot numbers into concrete PCI
870870
// bus/device/function numbers).
871871
//
872-
// Today's Propolis VM creation API (the one used below) differs from
873-
// this scheme in two important ways:
872+
// The Propolis VM creation API used below differs from this scheme in
873+
// two important ways:
874874
//
875875
// 1. Callers are responsible for filling in all the details of a VM's
876876
// configuration (e.g., choosing PCI BDFs for PCI devices).
@@ -974,20 +974,19 @@ impl InstanceRunner {
974974
let nics: Vec<NicComponents> = running_zone
975975
.opte_ports()
976976
.map(|port| -> Result<NicComponents, Error> {
977-
let pos = self
977+
let nic = self
978978
.requested_nics
979979
.iter()
980980
// We expect to match NIC slots to OPTE port slots.
981981
// Error out if we can't find a NIC for a port.
982-
.position(|nic| nic.slot == port.slot())
982+
.find(|nic| nic.slot == port.slot())
983983
.ok_or(Error::Opte(
984984
illumos_utils::opte::Error::NoNicforPort(
985985
port.name().into(),
986986
port.slot().into(),
987987
),
988988
))?;
989989

990-
let nic = &self.requested_nics[pos];
991990
Ok(NicComponents {
992991
id: nic.id,
993992
device: VirtioNic {
@@ -1012,7 +1011,7 @@ impl InstanceRunner {
10121011
// - Crucible disks: the target needs to connect to its downstairs
10131012
// instances with new generation numbers supplied from Nexus
10141013
// - OPTE ports: the target needs to bind its VNICs to the correct
1015-
// devices for its new host, which devices may have different names
1014+
// devices for its new host; those devices may have different names
10161015
// than their counterparts on the source host
10171016
//
10181017
// If this is a request to migrate, construct a list of source component

0 commit comments

Comments
 (0)