Adds concept of a primary network interface

common/src/api/external/mod.rs

@@ -1811,6 +1811,9 @@ pub struct NetworkInterface {
     pub ip: IpAddr,
     // TODO-correctness: We need to split this into an optional V4 and optional
     // V6 address, at least one of which must be specified.
+    /// True if this interface is the primary for the instance to which it's
+    /// attached.
+    pub primary: bool,
 }
 
 #[derive(

common/src/sql/dbinit.sql

@@ -674,7 +674,14 @@ CREATE TABLE omicron.public.network_interface (
      * Limited to 8 NICs per instance. This value must be kept in sync with
      * `crate::nexus::MAX_NICS_PER_INSTANCE`.
      */
-    slot INT2 NOT NULL CHECK (slot >= 0 AND slot < 8)
+    slot INT2 NOT NULL CHECK (slot >= 0 AND slot < 8),
+
+    /* True if this interface is the primary interface for the instance.
+     *
+     * The primary interface appears in DNS and its address is used for external
+     * connectivity for the instance.
+     */
+    is_primary BOOL NOT NULL
 );
 
 /* TODO-completeness

nexus/src/app/instance.rs

@@ -13,7 +13,8 @@ use crate::db;
 use crate::db::identity::Resource;
 use crate::db::lookup::LookupPath;
 use crate::db::model::Name;
-use crate::db::queries::network_interface::NetworkInterfaceError;
+use crate::db::queries::network_interface::DeleteNetworkInterfaceError;
+use crate::db::queries::network_interface::InsertNetworkInterfaceError;
 use crate::external_api::params;
 use omicron_common::api::external;
 use omicron_common::api::external::CreateResult;
@@ -642,7 +643,8 @@ impl super::Nexus {
         // instance between this check and when we actually create the NIC
         // record. One solution is to place the state verification in the query
         // to create the NIC. Unfortunately, that query is already very
-        // complicated.
+        // complicated. See
+        // https://github.com/oxidecomputer/omicron/issues/1134.
         let stopped =
             db::model::InstanceState::new(external::InstanceState::Stopped);
         if db_instance.runtime_state.state != stopped {
@@ -680,7 +682,7 @@ impl super::Nexus {
                 interface,
             )
             .await
-            .map_err(NetworkInterfaceError::into_external)?;
+            .map_err(InsertNetworkInterfaceError::into_external)?;
         Ok(interface)
     }
 
@@ -724,6 +726,9 @@ impl super::Nexus {
     }
 
     /// Delete a network interface from the provided instance.
+    ///
+    /// Note that the primary interface for an instance cannot be deleted if
+    /// there are any secondary interfaces.
     pub async fn instance_delete_network_interface(
         &self,
         opctx: &OpContext,
@@ -746,6 +751,8 @@ impl super::Nexus {
             .await?;
 
         // TODO-completeness: We'd like to relax this once hot-plug is supported
+        // TODO-correctness: There's a race condition here. Someone may start
+        // the instance after this check but before we actually delete the NIC.
         let stopped =
             db::model::InstanceState::new(external::InstanceState::Stopped);
         if db_instance.runtime_state.state != stopped {
@@ -754,8 +761,13 @@ impl super::Nexus {
             ));
         }
         self.db_datastore
-            .instance_delete_network_interface(opctx, &authz_interface)
+            .instance_delete_network_interface(
+                opctx,
+                &authz_instance,
+                &authz_interface,
+            )
             .await
+            .map_err(DeleteNetworkInterfaceError::into_external)
     }
 
     /// Invoked by a sled agent to publish an updated runtime state for an

nexus/src/app/sagas/instance_create.rs

@@ -9,7 +9,8 @@ use crate::app::{MAX_DISKS_PER_INSTANCE, MAX_NICS_PER_INSTANCE};
 use crate::context::OpContext;
 use crate::db::identity::Resource;
 use crate::db::lookup::LookupPath;
-use crate::db::queries::network_interface::NetworkInterfaceError;
+use crate::db::queries::network_interface::InsertNetworkInterfaceError;
+use crate::defaults::DEFAULT_PRIMARY_NIC_NAME;
 use crate::external_api::params;
 use crate::saga_interface::SagaContext;
 use crate::{authn, authz, db};
@@ -242,7 +243,7 @@ async fn sic_create_network_interfaces(
     match sagactx.saga_params().create_params.network_interfaces {
         params::InstanceNetworkInterfaceAttachment::None => Ok(()),
         params::InstanceNetworkInterfaceAttachment::Default => {
-            sic_create_default_network_interface(&sagactx).await
+            sic_create_default_primary_network_interface(&sagactx).await
         }
         params::InstanceNetworkInterfaceAttachment::Create(
             ref create_params,
@@ -347,7 +348,7 @@ async fn sic_create_custom_network_interfaces(
             // crashed. The saga recovery machinery will replay just this node,
             // without first unwinding it, so any previously-inserted interfaces
             // will still exist. This is expected.
-            Err(NetworkInterfaceError::DuplicatePrimaryKey(_)) => {
+            Err(InsertNetworkInterfaceError::DuplicatePrimaryKey(_)) => {
                 // TODO-observability: We should bump a counter here.
                 let log = osagactx.log();
                 warn!(
@@ -369,8 +370,9 @@ async fn sic_create_custom_network_interfaces(
     Ok(())
 }
 
-/// Create the default network interface for an instance during the create saga
-async fn sic_create_default_network_interface(
+/// Create a default primary network interface for an instance during the create
+/// saga.
+async fn sic_create_default_primary_network_interface(
     sagactx: &ActionContext<SagaInstanceCreate>,
 ) -> Result<(), ActionError> {
     let osagactx = sagactx.user_data();
@@ -379,13 +381,23 @@ async fn sic_create_default_network_interface(
     let opctx =
         OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn);
     let instance_id = sagactx.lookup::<Uuid>("instance_id")?;
+
+    // The literal name "default" is currently used for the VPC and VPC Subnet,
+    // when not specified in the client request.
+    // TODO-completeness: We'd like to select these from Project-level defaults.
+    // See https://github.com/oxidecomputer/omicron/issues/1015.
     let default_name = Name::try_from("default".to_string()).unwrap();
     let internal_default_name = db::model::Name::from(default_name.clone());
+
+    // The name of the default primary interface.
+    let iface_name =
+        Name::try_from(DEFAULT_PRIMARY_NIC_NAME.to_string()).unwrap();
+
     let interface_params = params::NetworkInterfaceCreate {
         identity: IdentityMetadataCreateParams {
-            name: default_name.clone(),
+            name: iface_name.clone(),
             description: format!(
-                "default interface for {}",
+                "default primary interface for {}",
                 saga_params.create_params.identity.name,
             ),
         },
@@ -427,7 +439,7 @@ async fn sic_create_default_network_interface(
             interface,
         )
         .await
-        .map_err(NetworkInterfaceError::into_external)
+        .map_err(InsertNetworkInterfaceError::into_external)
         .map_err(ActionError::action_failed)?;
     Ok(())
 }

nexus/src/db/datastore.rs

@@ -40,8 +40,10 @@ 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::DeleteNetworkInterfaceError;
+use crate::db::queries::network_interface::DeleteNetworkInterfaceQuery;
+use crate::db::queries::network_interface::InsertNetworkInterfaceError;
 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;
@@ -1575,23 +1577,23 @@ impl DataStore {
         authz_subnet: &authz::VpcSubnet,
         authz_instance: &authz::Instance,
         interface: IncompleteNetworkInterface,
-    ) -> Result<NetworkInterface, NetworkInterfaceError> {
+    ) -> Result<NetworkInterface, InsertNetworkInterfaceError> {
         opctx
             .authorize(authz::Action::CreateChild, authz_instance)
             .await
-            .map_err(NetworkInterfaceError::External)?;
+            .map_err(InsertNetworkInterfaceError::External)?;
         opctx
             .authorize(authz::Action::CreateChild, authz_subnet)
             .await
-            .map_err(NetworkInterfaceError::External)?;
+            .map_err(InsertNetworkInterfaceError::External)?;
         self.instance_create_network_interface_raw(&opctx, interface).await
     }
 
     pub(super) async fn instance_create_network_interface_raw(
         &self,
         opctx: &OpContext,
         interface: IncompleteNetworkInterface,
-    ) -> Result<NetworkInterface, NetworkInterfaceError> {
+    ) -> Result<NetworkInterface, InsertNetworkInterfaceError> {
         use db::schema::network_interface::dsl;
         let query = InsertNetworkInterfaceQuery::new(interface.clone());
         diesel::insert_into(dsl::network_interface)
@@ -1600,10 +1602,10 @@ impl DataStore {
             .get_result_async(
                 self.pool_authorized(opctx)
                     .await
-                    .map_err(NetworkInterfaceError::External)?,
+                    .map_err(InsertNetworkInterfaceError::External)?,
             )
             .await
-            .map_err(|e| NetworkInterfaceError::from_pool(e, &interface))
+            .map_err(|e| InsertNetworkInterfaceError::from_pool(e, &interface))
     }
 
     /// Delete all network interfaces attached to the given instance.
@@ -1634,28 +1636,32 @@ impl DataStore {
     }
 
     /// Delete a `NetworkInterface` attached to a provided instance.
+    ///
+    /// Note that the primary interface for an instance cannot be deleted if
+    /// there are any secondary interfaces.
     pub async fn instance_delete_network_interface(
         &self,
         opctx: &OpContext,
+        authz_instance: &authz::Instance,
         authz_interface: &authz::NetworkInterface,
-    ) -> DeleteResult {
-        opctx.authorize(authz::Action::Delete, authz_interface).await?;
-
-        use db::schema::network_interface::dsl;
-        let now = Utc::now();
-        let interface_id = authz_interface.id();
-        diesel::update(dsl::network_interface)
-            .filter(dsl::id.eq(interface_id))
-            .filter(dsl::time_deleted.is_null())
-            .set((dsl::time_deleted.eq(now),))
-            .execute_async(self.pool_authorized(opctx).await?)
+    ) -> Result<(), DeleteNetworkInterfaceError> {
+        opctx
+            .authorize(authz::Action::Delete, authz_interface)
             .await
-            .map_err(|e| {
-                public_error_from_diesel_pool(
-                    e,
-                    ErrorHandler::NotFoundByResource(authz_interface),
-                )
-            })?;
+            .map_err(DeleteNetworkInterfaceError::External)?;
+        let query = DeleteNetworkInterfaceQuery::new(
+            authz_instance.id(),
+            authz_interface.id(),
+        );
+        query
+            .clone()
+            .execute_async(
+                self.pool_authorized(opctx)
+                    .await
+                    .map_err(DeleteNetworkInterfaceError::External)?,
+            )
+            .await
+            .map_err(|e| DeleteNetworkInterfaceError::from_pool(e, &query))?;
         Ok(())
     }
 

nexus/src/db/model/network_interface.rs

@@ -26,6 +26,8 @@ pub struct NetworkInterface {
     // If neither is specified, auto-assign one of each?
     pub ip: ipnetwork::IpNetwork,
     pub slot: i16,
+    #[diesel(column_name = is_primary)]
+    pub primary: bool,
 }
 
 impl From<NetworkInterface> for external::NetworkInterface {
@@ -37,6 +39,7 @@ impl From<NetworkInterface> for external::NetworkInterface {
             subnet_id: iface.subnet_id,
             ip: iface.ip.ip(),
             mac: *iface.mac,
+            primary: iface.primary,
         }
     }
 }