Use production propolis-server flags in PHD CI builds

We use the dev (e.g. non-release) build of propolis-server in PHD to get
debug assertions, which we want, but we should use the `omicron-build`
feature to be in line with production binaries.

`omicron-build` had also acquired code related to failure injection,
which we *also* want in CI so we can test migration failure codepaths.
That is moved to a new `failure-injection` feature flag with a bit of
ceremony to stop us early if we're ever inadvertently packaging a
propolis-server with `omicron-build` and `failure-injection`.

Fixes #412.

Author: iximeow

.github/buildomat/jobs/phd-build.sh

@@ -30,7 +30,10 @@ rustc --version
 # Build the Propolis server binary with 'dev' profile to enable assertions that
 # should fire during tests.
 banner build-propolis
-ptime -m cargo build --verbose -p propolis-server
+
+export PHD_BUILD="true"
+ptime -m cargo build --verbose -p propolis-server \
+	--features omicron-build,failure-injection
 
 # The PHD runner requires unwind-on-panic to catch certain test failures, so
 # build it with the 'dev' profile which is so configured.

bin/propolis-server/Cargo.toml

@@ -83,3 +83,9 @@ omicron-build = ["propolis/omicron-build"]
 
 # Falcon builds require corresponding bits turned on in the dependency libs
 falcon = ["propolis/falcon"]
+
+# Testing necessitates injecting failures which should hopefully be rare or even
+# never occur on real otherwise-unperturbed systems. We conditionally compile
+# code supporting failure injection to avoid the risk of somehow injecting
+# failures into a real system not under test.
+failure-injection = ["propolis/failure-injection"]

bin/propolis-server/src/lib/initializer.rs

@@ -814,7 +814,7 @@ impl MachineInitializer<'_> {
         Ok(())
     }
 
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "failure-injection")]
     pub fn initialize_test_devices(&mut self) {
         use propolis::hw::testdev::{
             MigrationFailureDevice, MigrationFailures,

bin/propolis-server/src/lib/migrate/preamble.rs

@@ -52,7 +52,7 @@ impl Preamble {
             };
 
             match comp {
-                #[cfg(feature = "omicron-build")]
+                #[cfg(not(feature = "failure-injection"))]
                 ReplacementComponent::MigrationFailureInjector(_) => {
                     return Err(MigrateError::InstanceSpecsIncompatible(
                         format!(
@@ -62,7 +62,7 @@ impl Preamble {
                     ));
                 }
 
-                #[cfg(not(feature = "omicron-build"))]
+                #[cfg(feature = "failure-injection")]
                 ReplacementComponent::MigrationFailureInjector(comp) => {
                     let ComponentV0::MigrationFailureInjector(src) = to_amend
                     else {

bin/propolis-server/src/lib/spec/api_spec_v0.rs

@@ -27,7 +27,7 @@ use super::{
     StorageDevice,
 };
 
-#[cfg(not(feature = "omicron-build"))]
+#[cfg(feature = "failure-injection")]
 use super::MigrationFailure;
 
 #[cfg(feature = "falcon")]
@@ -65,7 +65,7 @@ impl From<Spec> for InstanceSpecV0 {
             serial,
             pci_pci_bridges,
             pvpanic,
-            #[cfg(not(feature = "omicron-build"))]
+            #[cfg(feature = "failure-injection")]
             migration_failure,
             #[cfg(feature = "falcon")]
             softnpu,
@@ -157,7 +157,7 @@ impl From<Spec> for InstanceSpecV0 {
             );
         }
 
-        #[cfg(not(feature = "omicron-build"))]
+        #[cfg(feature = "failure-injection")]
         if let Some(mig) = migration_failure {
             insert_component(
                 &mut spec,
@@ -310,14 +310,14 @@ impl TryFrom<InstanceSpecV0> for Spec {
                     // apply it to the builder later.
                     boot_settings = Some((device_id, settings));
                 }
-                #[cfg(feature = "omicron-build")]
+                #[cfg(not(feature = "failure-injection"))]
                 ComponentV0::MigrationFailureInjector(_) => {
                     return Err(ApiSpecError::FeatureCompiledOut {
                         component: device_id,
-                        feature: "omicron-build",
+                        feature: "failure-injection",
                     });
                 }
-                #[cfg(not(feature = "omicron-build"))]
+                #[cfg(feature = "failure-injection")]
                 ComponentV0::MigrationFailureInjector(mig) => {
                     builder.add_migration_failure_device(MigrationFailure {
                         id: device_id,

bin/propolis-server/src/lib/spec/builder.rs

@@ -26,7 +26,7 @@ use super::{
     Board, BootOrderEntry, BootSettings, Disk, Nic, QemuPvpanic, SerialPort,
 };
 
-#[cfg(not(feature = "omicron-build"))]
+#[cfg(feature = "failure-injection")]
 use super::MigrationFailure;
 
 #[cfg(feature = "falcon")]
@@ -50,7 +50,7 @@ pub(crate) enum SpecBuilderError {
     #[error("pvpanic device already specified")]
     PvpanicInUse,
 
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "failure-injection")]
     #[error("migration failure injection already enabled")]
     MigrationFailureInjectionInUse,
 
@@ -269,7 +269,7 @@ impl SpecBuilder {
         Ok(self)
     }
 
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "failure-injection")]
     pub fn add_migration_failure_device(
         &mut self,
         mig: MigrationFailure,

bin/propolis-server/src/lib/spec/mod.rs

@@ -34,7 +34,7 @@ use propolis_api_types::instance_spec::{
 };
 use thiserror::Error;
 
-#[cfg(not(feature = "omicron-build"))]
+#[cfg(feature = "failure-injection")]
 use propolis_api_types::instance_spec::components::devices::MigrationFailureInjector;
 
 #[cfg(feature = "falcon")]
@@ -73,7 +73,7 @@ pub(crate) struct Spec {
     pub pci_pci_bridges: BTreeMap<SpecKey, PciPciBridge>,
     pub pvpanic: Option<QemuPvpanic>,
 
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "failure-injection")]
     pub migration_failure: Option<MigrationFailure>,
 
     #[cfg(feature = "falcon")]
@@ -285,7 +285,7 @@ pub struct QemuPvpanic {
     pub spec: QemuPvpanicDesc,
 }
 
-#[cfg(not(feature = "omicron-build"))]
+#[cfg(feature = "failure-injection")]
 #[derive(Clone, Debug)]
 pub struct MigrationFailure {
     pub id: SpecKey,

bin/propolis-server/src/lib/vm/ensure.rs

@@ -564,10 +564,8 @@ async fn initialize_vm_objects(
     ))?;
     init.initialize_network_devices(&chipset).await?;
 
-    #[cfg(not(feature = "omicron-build"))]
+    #[cfg(feature = "failure-injection")]
     init.initialize_test_devices();
-    #[cfg(feature = "omicron-build")]
-    info!(log, "`omicron-build` feature enabled, ignoring any test devices");
 
     #[cfg(feature = "falcon")]
     {

bin/propolis-server/src/main.rs

@@ -278,6 +278,19 @@ fn main() -> anyhow::Result<()> {
     // Ensure proper setup of USDT probes
     register_probes().unwrap();
 
+    #[cfg(all(
+        feature = "omicron-build",
+        any(feature = "failure-injection", feature = "falcon")
+    ))]
+    if option_env!("PHD_BUILD") != Some("true") {
+        panic!(
+            "`omicron-build` is enabled alongside development features, \
+            this build is NOT SUITABLE for production. Set PHD_BUILD=true in \
+            the environment and rebuild propolis-server if you really need \
+            this to work."
+        );
+    }
+
     // Command line arguments.
     let args = Args::parse();
 

crates/propolis-api-types/src/instance_spec/components/devices.rs

@@ -193,8 +193,8 @@ pub struct P9fs {
 /// Describes a synthetic device that registers for VM lifecycle notifications
 /// and returns errors during attempts to migrate.
 ///
-/// This is only supported by Propolis servers compiled without the
-/// `omicron-build` feature.
+/// This is only supported by Propolis servers compiled with the
+/// `failure-injection` feature.
 #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
 #[serde(deny_unknown_fields)]
 pub struct MigrationFailureInjector {

lib/propolis/Cargo.toml

@@ -61,3 +61,5 @@ falcon = ["libloading", "p9ds", "dlpi", "ispf", "rand", "softnpu", "viona_api/fa
 # TODO until crucible#1280 is addressed, enabling Nexus notifications is done
 # through a feature flag.
 omicron-build = ["crucible/notify-nexus"]
+
+failure-injection = []

xtask/src/task_phd.rs

@@ -4,7 +4,7 @@
 
 use anyhow::Context;
 use camino::{Utf8Path, Utf8PathBuf};
-use std::{fs, process::Command, time};
+use std::{collections::HashMap, fs, process::Command, time};
 
 macro_rules! cargo_log {
     ($tag:literal, $($arg:tt)*) => {
@@ -270,15 +270,15 @@ impl Cmd {
                 return Ok(());
             }
             Self::List { phd_args } => {
-                let phd_runner = build_bin("phd-runner", false)?;
+                let phd_runner = build_bin("phd-runner", false, None)?;
                 let status = run_exit_code(
                     phd_runner.command().arg("list").args(phd_args),
                 )?;
                 std::process::exit(status);
             }
 
             Self::RunnerHelp { phd_args } => {
-                let phd_runner = build_bin("phd-runner", false)?;
+                let phd_runner = build_bin("phd-runner", false, None)?;
                 let status = run_exit_code(
                     phd_runner.command().arg("help").args(phd_args),
                 )?;
@@ -292,7 +292,14 @@ impl Cmd {
                 cmd
             }
             None => {
-                let bin = build_bin("propolis-server", propolis_args.release)?;
+                let mut server_build_env = HashMap::new();
+                server_build_env
+                    .insert("PHD_BUILD".to_string(), "true".to_string());
+                let bin = build_bin(
+                    "propolis-server",
+                    propolis_args.release,
+                    Some(server_build_env),
+                )?;
                 let path = bin
                     .path()
                     .try_into()
@@ -345,7 +352,7 @@ impl Cmd {
             anyhow::bail!("Missing artifacts config `{artifacts_toml}`!");
         }
 
-        let phd_runner = build_bin("phd-runner", false)?;
+        let phd_runner = build_bin("phd-runner", false, None)?;
         let mut cmd = if cfg!(target_os = "illumos") {
             let mut cmd = Command::new("pfexec");
             cmd.arg(phd_runner.path());
@@ -421,12 +428,18 @@ impl BasePropolisArgs {
 fn build_bin(
     name: impl AsRef<str>,
     release: bool,
+    build_env: Option<HashMap<String, String>>,
 ) -> anyhow::Result<escargot::CargoRun> {
     let name = name.as_ref();
     cargo_log!("Compiling", "{name}");
 
     let mut cmd =
         escargot::CargoBuild::new().package(name).bin(name).current_target();
+    if let Some(env) = build_env {
+        for (k, v) in env {
+            cmd = cmd.env(k, v);
+        }
+    }
     let profile = if release {
         cmd = cmd.release();
         "release [optimized]"