Insert VNIs via a next-item query (#1107)

* Better selection of Geneve VNI for a new VPC

- Adds a `NextItem` query for the Geneve Virtual Network Identifier
  (VNI) for a new VPC. Previously, this was selected randomly and
  conflicts resulted in a 500. The new query starts from a random VNI,
  and selects the first available.
- Makes the generic `NextItem` queries "wrapping". The previous
  implementation had a subtle problem, where the size of the search
  space was depdendent on the base / starting value, since we just
  searched from the base to the provided maximum value. This new
  implementation accepts a maximum _leftward_ or negative shift from the
  base value as well, and does a wrapping search from base, to the
  maximum, to the minimum, and back to the base.
- Adds a call to `usdt::register_probes()` inside the test utility that
  sets up a test CRDB instance. This ensures that the `diesel-dtrace`
  probes are registered for tests.

* Add test for the wrapping next-item query

* Add initial slice of reserved VNIs

* Maximum VNI value off by one

Author: bnaecker

Cargo.lock

@@ -1745,12 +1745,16 @@ impl JsonSchema for MacAddr {
 pub struct Vni(u32);
 
 impl Vni {
-    const MAX_VNI: u32 = 1 << 24;
+    /// Virtual Network Identifiers are constrained to be 24-bit values.
+    pub const MAX_VNI: u32 = 0xFF_FFFF;
+
+    /// Oxide reserves a slice of initial VNIs for its own use.
+    pub const MIN_GUEST_VNI: u32 = 1024;
 
     /// Create a new random VNI.
     pub fn random() -> Self {
         use rand::Rng;
-        Self(rand::thread_rng().gen_range(0..=Self::MAX_VNI))
+        Self(rand::thread_rng().gen_range(Self::MIN_GUEST_VNI..=Self::MAX_VNI))
     }
 }
 

common/src/api/external/mod.rs

@@ -54,7 +54,7 @@ impl super::Nexus {
         // which may not even happen here. Creating the vpc, its system router,
         // and that routers default route should all be a part of the same
         // transaction.
-        let vpc = db::model::Vpc::new(
+        let vpc = db::model::IncompleteVpc::new(
             vpc_id,
             authz_project.id(),
             system_router_id,

nexus/src/app/vpc.rs

@@ -38,8 +38,11 @@ use crate::db::fixed_data::role_builtin::BUILTIN_ROLES;
 use crate::db::fixed_data::silo::DEFAULT_SILO;
 use crate::db::lookup::LookupPath;
 use crate::db::model::DatabaseString;
+use crate::db::model::IncompleteVpc;
+use crate::db::model::Vpc;
 use crate::db::queries::network_interface::InsertNetworkInterfaceQuery;
 use crate::db::queries::network_interface::NetworkInterfaceError;
+use crate::db::queries::vpc::InsertVpcQuery;
 use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery;
 use crate::db::queries::vpc_subnet::SubnetError;
 use crate::db::{
@@ -52,7 +55,7 @@ use crate::db::{
         OrganizationUpdate, OximeterInfo, ProducerEndpoint, Project,
         ProjectUpdate, Region, RoleAssignment, RoleBuiltin, RouterRoute,
         RouterRouteUpdate, Silo, SiloUser, Sled, SshKey,
-        UpdateAvailableArtifact, UserBuiltin, Volume, Vpc, VpcFirewallRule,
+        UpdateAvailableArtifact, UserBuiltin, Volume, VpcFirewallRule,
         VpcRouter, VpcRouterUpdate, VpcSubnet, VpcSubnetUpdate, VpcUpdate,
         Zpool,
     },
@@ -2019,17 +2022,21 @@ impl DataStore {
         &self,
         opctx: &OpContext,
         authz_project: &authz::Project,
-        vpc: Vpc,
+        vpc: IncompleteVpc,
     ) -> Result<(authz::Vpc, Vpc), Error> {
         use db::schema::vpc::dsl;
 
         assert_eq!(authz_project.id(), vpc.project_id);
         opctx.authorize(authz::Action::CreateChild, authz_project).await?;
 
         // TODO-correctness Shouldn't this use "insert_resource"?
-        let name = vpc.name().clone();
+        //
+        // Note that to do so requires adding an `rcgen` column to the project
+        // table.
+        let name = vpc.identity.name.clone();
+        let query = InsertVpcQuery::new(vpc);
         let vpc = diesel::insert_into(dsl::vpc)
-            .values(vpc)
+            .values(query)
             .returning(Vpc::as_returning())
             .get_result_async(self.pool())
             .await

nexus/src/db/datastore.rs

@@ -10,6 +10,7 @@ use crate::defaults;
 use crate::external_api::params;
 use chrono::{DateTime, Utc};
 use db_macros::Resource;
+use ipnetwork::IpNetwork;
 use omicron_common::api::external;
 use uuid::Uuid;
 
@@ -30,28 +31,45 @@ pub struct Vpc {
     pub firewall_gen: Generation,
 }
 
-impl Vpc {
+/// An `IncompleteVpc` is a candidate VPC, where some of the values may be
+/// modified and returned as part of the query inserting it into the database.
+/// In particular, the requested VNI may not actually be available, in which
+/// case the database will select an available one (if it exists).
+#[derive(Clone, Debug)]
+pub struct IncompleteVpc {
+    pub identity: VpcIdentity,
+    pub project_id: Uuid,
+    pub system_router_id: Uuid,
+    pub vni: Vni,
+    pub ipv6_prefix: IpNetwork,
+    pub dns_name: Name,
+    pub firewall_gen: Generation,
+}
+
+impl IncompleteVpc {
     pub fn new(
         vpc_id: Uuid,
         project_id: Uuid,
         system_router_id: Uuid,
         params: params::VpcCreate,
     ) -> Result<Self, external::Error> {
         let identity = VpcIdentity::new(vpc_id, params.identity);
-        let ipv6_prefix = match params.ipv6_prefix {
-            None => defaults::random_vpc_ipv6_prefix(),
-            Some(prefix) => {
-                if prefix.is_vpc_prefix() {
-                    Ok(prefix)
-                } else {
-                    Err(external::Error::invalid_request(
-                        "VPC IPv6 address prefixes must be in the 
+        let ipv6_prefix = IpNetwork::from(
+            match params.ipv6_prefix {
+                None => defaults::random_vpc_ipv6_prefix(),
+                Some(prefix) => {
+                    if prefix.is_vpc_prefix() {
+                        Ok(prefix)
+                    } else {
+                        Err(external::Error::invalid_request(
+                            "VPC IPv6 address prefixes must be in the \
                             Unique Local Address range `fd00::/48` (RFD 4193)",
-                    ))
+                        ))
+                    }
                 }
-            }
-        }?
-        .into();
+            }?
+            .0,
+        );
         Ok(Self {
             identity,
             project_id,

nexus/src/db/model/vpc.rs

@@ -8,5 +8,5 @@
 #[macro_use]
 mod next_item;
 pub mod network_interface;
-pub mod vni;
+pub mod vpc;
 pub mod vpc_subnet;

nexus/src/db/queries/mod.rs

@@ -7,6 +7,7 @@
 use crate::app::MAX_NICS_PER_INSTANCE;
 use crate::db;
 use crate::db::model::IncompleteNetworkInterface;
+use crate::db::queries::next_item::DefaultShiftGenerator;
 use crate::db::queries::next_item::NextItem;
 use crate::defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES;
 use chrono::DateTime;
@@ -331,8 +332,10 @@ impl NextGuestIpv4Address {
     pub fn new(subnet: Ipv4Network, subnet_id: Uuid) -> Self {
         let subnet = IpNetwork::from(subnet);
         let net = IpNetwork::from(first_available_address(&subnet));
-        let max_offset = last_address_offset(&subnet);
-        Self { inner: NextItem::new_scoped(net, subnet_id, max_offset) }
+        let max_shift = i64::from(last_address_offset(&subnet));
+        let generator =
+            DefaultShiftGenerator { base: net, max_shift, min_shift: 0 };
+        Self { inner: NextItem::new_scoped(generator, subnet_id) }
     }
 }
 
@@ -387,9 +390,12 @@ pub struct NextNicSlot {
 
 impl NextNicSlot {
     pub fn new(instance_id: Uuid) -> Self {
-        Self {
-            inner: NextItem::new_scoped(0, instance_id, MAX_NICS_PER_INSTANCE),
-        }
+        let generator = DefaultShiftGenerator {
+            base: 0,
+            max_shift: i64::from(MAX_NICS_PER_INSTANCE),
+            min_shift: 0,
+        };
+        Self { inner: NextItem::new_scoped(generator, instance_id) }
     }
 }