Restrict hardcoded PROPOLIS_PORT to simply being the default in sagas

Adds a database column for Nexus to track `propolis_port` in addition
to the `propolis_ip`, such that tests using sled-agent-sim don't collide.

common/src/address.rs

@@ -44,8 +44,6 @@ pub const DNS_PORT: u16 = 53;
 pub const DNS_HTTP_PORT: u16 = 5353;
 pub const SLED_AGENT_PORT: u16 = 12345;
 
-/// The port propolis-server listens on inside the propolis zone.
-pub const PROPOLIS_PORT: u16 = 12400;
 pub const COCKROACH_PORT: u16 = 32221;
 pub const CRUCIBLE_PORT: u16 = 32345;
 pub const CLICKHOUSE_PORT: u16 = 8123;

nexus/db-model/src/schema.rs

@@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion;
 ///
 /// This should be updated whenever the schema is changed. For more details,
 /// refer to: schema/crdb/README.adoc
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(37, 0, 1);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(38, 0, 0);
 
 table! {
     disk (id) {
@@ -380,6 +380,7 @@ table! {
         instance_id -> Uuid,
         sled_id -> Uuid,
         propolis_ip -> Inet,
+        propolis_port -> Int4,
         state -> crate::InstanceStateEnum,
         time_state_updated -> Timestamptz,
         state_generation -> Int8,

nexus/db-model/src/vmm.rs

@@ -43,6 +43,9 @@ pub struct Vmm {
     /// The IP address at which this VMM is serving the Propolis server API.
     pub propolis_ip: ipnetwork::IpNetwork,
 
+    /// The socket port on which this VMM is serving the Propolis server API.
+    pub propolis_port: i32,
+
     /// Runtime state for the VMM.
     #[diesel(embed)]
     pub runtime: VmmRuntimeState,
@@ -61,6 +64,7 @@ impl Vmm {
         instance_id: Uuid,
         sled_id: Uuid,
         propolis_ip: ipnetwork::IpNetwork,
+        propolis_port: u16,
         initial_state: VmmInitialState,
     ) -> Self {
         use omicron_common::api::external::InstanceState as ApiInstanceState;
@@ -78,6 +82,7 @@ impl Vmm {
             instance_id,
             sled_id,
             propolis_ip,
+            propolis_port: propolis_port as i32,
             runtime: VmmRuntimeState {
                 state: InstanceState::new(api_state),
                 time_state_updated: now,

nexus/db-queries/src/db/datastore/vmm.rs

@@ -23,6 +23,7 @@ use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::LookupType;
 use omicron_common::api::external::ResourceType;
 use omicron_common::api::external::UpdateResult;
+use std::net::SocketAddr;
 use uuid::Uuid;
 
 impl DataStore {
@@ -134,23 +135,28 @@ impl DataStore {
         Ok(updated)
     }
 
-    /// Forcibly overwrites the Propolis IP in the supplied VMM's record with
+    /// Forcibly overwrites the Propolis IP/Port in the supplied VMM's record with
     /// the supplied Propolis IP.
     ///
     /// This is used in tests to overwrite the IP for a VMM that is backed by a
     /// mock Propolis server that serves on localhost but has its Propolis IP
     /// allocated by the instance start procedure. (Unfortunately, this can't be
     /// marked #[cfg(test)] because the integration tests require this
     /// functionality.)
-    pub async fn vmm_overwrite_ip_for_test(
+    pub async fn vmm_overwrite_addr_for_test(
         &self,
         opctx: &OpContext,
         vmm_id: &Uuid,
-        new_ip: ipnetwork::IpNetwork,
+        new_addr: SocketAddr,
     ) -> UpdateResult<Vmm> {
+        let new_ip = ipnetwork::IpNetwork::from(new_addr.ip());
+        let new_port = new_addr.port();
         let vmm = diesel::update(dsl::vmm)
             .filter(dsl::id.eq(*vmm_id))
-            .set(dsl::propolis_ip.eq(new_ip))
+            .set((
+                dsl::propolis_ip.eq(new_ip),
+                dsl::propolis_port.eq(new_port as i32),
+            ))
             .returning(Vmm::as_returning())
             .get_result_async(&*self.pool_connection_authorized(opctx).await?)
             .await

nexus/src/app/instance.rs

@@ -29,7 +29,6 @@ use nexus_db_queries::db::identity::Resource;
 use nexus_db_queries::db::lookup;
 use nexus_db_queries::db::lookup::LookupPath;
 use nexus_types::external_api::views;
-use omicron_common::address::PROPOLIS_PORT;
 use omicron_common::api::external::http_pagination::PaginatedBy;
 use omicron_common::api::external::ByteCount;
 use omicron_common::api::external::CreateResult;
@@ -1223,7 +1222,7 @@ impl super::Nexus {
                     propolis_id: *propolis_id,
                     propolis_addr: SocketAddr::new(
                         initial_vmm.propolis_ip.ip(),
-                        PROPOLIS_PORT,
+                        initial_vmm.propolis_port as u16,
                     )
                     .to_string(),
                 },
@@ -1762,7 +1761,7 @@ impl super::Nexus {
                 | InstanceState::Rebooting
                 | InstanceState::Migrating
                 | InstanceState::Repairing => {
-                    Ok(SocketAddr::new(vmm.propolis_ip.ip(), PROPOLIS_PORT))
+                    Ok(SocketAddr::new(vmm.propolis_ip.ip(), vmm.propolis_port as u16))
                 }
                 InstanceState::Creating
                 | InstanceState::Starting

nexus/src/app/sagas/instance_common.rs

@@ -24,6 +24,9 @@ use uuid::Uuid;
 
 use super::NexusActionContext;
 
+/// The port propolis-server listens on inside the propolis zone.
+const DEFAULT_PROPOLIS_PORT: u16 = 12400;
+
 /// Reserves resources for a new VMM whose instance has `ncpus` guest logical
 /// processors and `guest_memory` bytes of guest RAM. The selected sled is
 /// random within the set of sleds allowed by the supplied `constraints`.
@@ -98,6 +101,7 @@ pub async fn create_and_insert_vmm_record(
         instance_id,
         sled_id,
         IpAddr::V6(propolis_ip).into(),
+        DEFAULT_PROPOLIS_PORT,
         initial_state,
     );
 

nexus/src/app/sagas/instance_migrate.rs

@@ -12,7 +12,6 @@ use crate::app::sagas::{
 use crate::external_api::params;
 use nexus_db_queries::db::{identity::Resource, lookup::LookupPath};
 use nexus_db_queries::{authn, authz, db};
-use omicron_common::address::PROPOLIS_PORT;
 use serde::Deserialize;
 use serde::Serialize;
 use sled_agent_client::types::{
@@ -424,8 +423,10 @@ async fn sim_instance_migrate(
     let db_instance =
         sagactx.lookup::<db::model::Instance>("set_migration_ids")?;
 
-    let src_vmm_addr =
-        SocketAddr::new(params.src_vmm.propolis_ip.ip(), PROPOLIS_PORT);
+    let src_vmm_addr = SocketAddr::new(
+        params.src_vmm.propolis_ip.ip(),
+        params.src_vmm.propolis_port as u16,
+    );
 
     let src_propolis_id = db_instance.runtime().propolis_id.unwrap();
     let dst_vmm = sagactx.lookup::<db::model::Vmm>("dst_vmm_record")?;

nexus/tests/integration_tests/instances.rs

@@ -3762,7 +3762,7 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) {
     let nexus = &apictx.nexus;
     let instance_name = "kris-picks";
 
-    cptestctx
+    let propolis_addr = cptestctx
         .sled_agent
         .sled_agent
         .start_local_mock_propolis_server(&cptestctx.logctx.log)
@@ -3819,12 +3819,12 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) {
         .runtime()
         .propolis_id
         .expect("running instance should have vmm");
-    let localhost = std::net::IpAddr::V6(std::net::Ipv6Addr::LOCALHOST);
     let updated_vmm = datastore
-        .vmm_overwrite_ip_for_test(&opctx, &propolis_id, localhost.into())
+        .vmm_overwrite_addr_for_test(&opctx, &propolis_id, propolis_addr)
         .await
         .unwrap();
-    assert_eq!(updated_vmm.propolis_ip.ip(), localhost);
+    assert_eq!(updated_vmm.propolis_ip.ip(), propolis_addr.ip());
+    assert_eq!(updated_vmm.propolis_port as u16, propolis_addr.port());
 
     // Query serial output history endpoint
     // This is the first line of output generated by the mock propolis-server.

schema/crdb/38.0.0/up.sql

@@ -0,0 +1,11 @@
+-- 12400 was the hardcoded PROPOLIS_PORT prior to this addition; default to
+-- that for all existing instances when creating the column.
+-- in production, this value will always end up still being 12400.
+-- however, nexus' testability in scenarios where each "propolis-server" API
+-- isn't necessarily served from an illumos zone with its own IP address, but
+-- rather in a sled-agent-sim thread during a cargo nextest run? useful to
+-- allow some dynamic range there to avoid flakes caused by port collisions.
+ALTER TABLE omicron.public.vmm
+ADD COLUMN IF NOT EXISTS propolis_port INT4 NOT NULL
+    CHECK (propolis_port BETWEEN 0 AND 65535)
+    DEFAULT 12400;

schema/crdb/dbinit.sql

@@ -3310,7 +3310,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.vmm (
     time_state_updated TIMESTAMPTZ NOT NULL,
     state_generation INT NOT NULL,
     sled_id UUID NOT NULL,
-    propolis_ip INET NOT NULL
+    propolis_ip INET NOT NULL,
+    propolis_port INT4 NOT NULL CHECK (propolis_port BETWEEN 0 AND 65535) DEFAULT 12400
 );
 
 /*
@@ -3551,7 +3552,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    ( TRUE, NOW(), NOW(), '37.0.1', NULL)
+    ( TRUE, NOW(), NOW(), '38.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;

sled-agent/src/sim/sled_agent.rs

@@ -27,7 +27,6 @@ use illumos_utils::opte::params::{
     DeleteVirtualNetworkInterfaceHost, SetVirtualNetworkInterfaceHost,
 };
 use nexus_client::types::PhysicalDiskKind;
-use omicron_common::address::PROPOLIS_PORT;
 use omicron_common::api::external::{
     ByteCount, DiskState, Error, Generation, ResourceType,
 };
@@ -684,14 +683,15 @@ impl SledAgent {
     }
 
     /// Used for integration tests that require a component to talk to a
-    /// mocked propolis-server API.
-    // TODO: fix schemas so propolis-server's port isn't hardcoded in nexus
-    // such that we can run more than one of these.
-    // (this is only needed by test_instance_serial at present)
+    /// mocked propolis-server API. Returns the socket on which the dropshot
+    /// service is listening, which *must* be patched into Nexus with
+    /// `nexus_db_queries::db::datastore::vmm_overwrite_addr_for_test` after
+    /// the instance creation saga if functionality touching propolis-server
+    /// is to be tested (e.g. serial console connection).
     pub async fn start_local_mock_propolis_server(
         &self,
         log: &Logger,
-    ) -> Result<(), Error> {
+    ) -> Result<SocketAddr, Error> {
         let mut mock_lock = self.mock_propolis.lock().await;
         if mock_lock.is_some() {
             return Err(Error::ObjectAlreadyExists {
@@ -700,7 +700,7 @@ impl SledAgent {
             });
         }
         let propolis_bind_address =
-            SocketAddr::new(Ipv6Addr::LOCALHOST.into(), PROPOLIS_PORT);
+            SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 0);
         let dropshot_config = dropshot::ConfigDropshot {
             bind_address: propolis_bind_address,
             ..Default::default()
@@ -721,12 +721,10 @@ impl SledAgent {
             Error::unavail(&format!("initializing propolis-server: {}", error))
         })?
         .start();
-        let client = propolis_client::Client::new(&format!(
-            "http://{}",
-            srv.local_addr()
-        ));
+        let addr = srv.local_addr();
+        let client = propolis_client::Client::new(&format!("http://{}", addr));
         *mock_lock = Some((srv, client));
-        Ok(())
+        Ok(addr)
     }
 
     pub fn inventory(&self, addr: SocketAddr) -> anyhow::Result<Inventory> {