Skip to content

Commit

Permalink
Add a boot device field to Instance, forward along as appropriate (#6585
Browse files Browse the repository at this point in the history
)

the Omicron side of adding explicit boot order selection to instances
(counterpart to
[propolis#756](oxidecomputer/propolis#756)).

first, this extends `params::InstanceCreate` to take a new `boot_disk:
Option<params::InstanceDiskAttachment>`.

additionally, this adds a `PUT /v1/instances/{instance}` to update
instances. the only property that can be updated at the moment is
`boot_disk`, pick a new boot disk or unset it entirely. this also
partially subsumes #6321.

finally, this updates Omicron to reference a recent enough Propolis that
#756 is included.

a surprising discovery along the way: you can specify a disk to be
attached multiple times in `disks` today, when creating an instance, and
we're fine with it! this carries through with the new `boot_disk` field:
if you specify the disk as `boot_disk` and in `disks`, it is technically
listing the disk for attachment twice but this will succeed.
  • Loading branch information
iximeow authored Oct 1, 2024
1 parent 90b8499 commit 144e91a
Show file tree
Hide file tree
Showing 46 changed files with 1,463 additions and 47 deletions.
37 changes: 29 additions & 8 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -511,9 +511,9 @@ prettyplease = { version = "0.2.22", features = ["verbatim"] }
proc-macro2 = "1.0"
progenitor = "0.8.0"
progenitor-client = "0.8.0"
bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "fae5334bcad5e864794332c6fed5e6bb9ec88831" }
propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "fae5334bcad5e864794332c6fed5e6bb9ec88831" }
propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "fae5334bcad5e864794332c6fed5e6bb9ec88831" }
bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "11371b0f3743f8df5b047dc0edc2699f4bdf3927" }
propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "11371b0f3743f8df5b047dc0edc2699f4bdf3927" }
propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "11371b0f3743f8df5b047dc0edc2699f4bdf3927" }
proptest = "1.5.0"
qorb = { git = "https://github.com/oxidecomputer/qorb", branch = "master" }
quote = "1.0"
Expand Down
13 changes: 13 additions & 0 deletions common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,16 @@ impl TryFrom<String> for NameOrId {
}
}

impl FromStr for NameOrId {
// TODO: We should have better error types here.
// See https://github.com/oxidecomputer/omicron/issues/347
type Err = String;

fn from_str(value: &str) -> Result<Self, Self::Err> {
NameOrId::try_from(String::from(value))
}
}

impl From<Name> for NameOrId {
fn from(name: Name) -> Self {
NameOrId::Name(name)
Expand Down Expand Up @@ -1183,6 +1193,9 @@ pub struct Instance {
/// RFC1035-compliant hostname for the Instance.
pub hostname: String,

/// the ID of the disk used to boot this Instance, if a specific one is assigned.
pub boot_disk_id: Option<Uuid>,

#[serde(flatten)]
pub runtime: InstanceRuntimeState,

Expand Down
3 changes: 3 additions & 0 deletions dev-tools/omdb/src/bin/omdb/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2941,6 +2941,7 @@ async fn cmd_db_instance_info(
const VCPUS: &'static str = "vCPUs";
const MEMORY: &'static str = "memory";
const HOSTNAME: &'static str = "hostname";
const BOOT_DISK: &'static str = "boot disk";
const AUTO_RESTART: &'static str = "auto-restart";
const STATE: &'static str = "nexus state";
const LAST_MODIFIED: &'static str = "last modified at";
Expand All @@ -2963,6 +2964,7 @@ async fn cmd_db_instance_info(
DELETED,
VCPUS,
MEMORY,
BOOT_DISK,
HOSTNAME,
AUTO_RESTART,
STATE,
Expand Down Expand Up @@ -3006,6 +3008,7 @@ async fn cmd_db_instance_info(
println!(" {VCPUS:>WIDTH$}: {}", instance.ncpus.0 .0);
println!(" {MEMORY:>WIDTH$}: {}", instance.memory.0);
println!(" {HOSTNAME:>WIDTH$}: {}", instance.hostname);
println!(" {BOOT_DISK:>WIDTH$}: {:?}", instance.boot_disk_id);
print_multiline_debug(AUTO_RESTART, &instance.auto_restart);
println!("\n{:=<80}", "== RUNTIME STATE ");
let InstanceRuntimeState {
Expand Down
5 changes: 3 additions & 2 deletions end-to-end-tests/src/instance_launch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,10 @@ async fn instance_launch() -> Result<()> {
hostname: "localshark".parse().unwrap(), // 🦈
memory: ByteCount(1024 * 1024 * 1024),
ncpus: InstanceCpuCount(2),
disks: vec![InstanceDiskAttachment::Attach {
boot_disk: Some(InstanceDiskAttachment::Attach {
name: disk_name.clone(),
}],
}),
disks: Vec::new(),
network_interfaces: InstanceNetworkInterfaceAttachment::Default,
external_ips: vec![ExternalIpCreate::Ephemeral { pool: None }],
user_data: String::new(),
Expand Down
15 changes: 15 additions & 0 deletions nexus/db-model/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ pub struct Instance {
#[diesel(embed)]
pub auto_restart: InstanceAutoRestart,

/// The primary boot disk for this instance.
#[diesel(column_name = boot_disk_id)]
pub boot_disk_id: Option<Uuid>,

#[diesel(embed)]
pub runtime_state: InstanceRuntimeState,

Expand Down Expand Up @@ -115,6 +119,9 @@ impl Instance {
memory: params.memory.into(),
hostname: params.hostname.to_string(),
auto_restart,
// Intentionally ignore `params.boot_disk_id` here: we can't set
// `boot_disk_id` until the referenced disk is attached.
boot_disk_id: None,

runtime_state,

Expand Down Expand Up @@ -520,3 +527,11 @@ mod optional_time_delta {
.serialize(serializer)
}
}

/// The parts of an Instance that can be directly updated after creation.
#[derive(Clone, Debug, AsChangeset, Serialize, Deserialize)]
#[diesel(table_name = instance, treat_none_as_null = true)]
pub struct InstanceUpdate {
#[diesel(column_name = boot_disk_id)]
pub boot_disk_id: Option<Uuid>,
}
1 change: 1 addition & 0 deletions nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ table! {
hostname -> Text,
auto_restart_policy -> Nullable<crate::InstanceAutoRestartPolicyEnum>,
auto_restart_cooldown -> Nullable<Interval>,
boot_disk_id -> Nullable<Uuid>,
time_state_updated -> Timestamptz,
state_generation -> Int8,
active_propolis_id -> Nullable<Uuid>,
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::collections::BTreeMap;
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(106, 0, 0);
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(107, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(107, "add-instance-boot-disk"),
KnownVersion::new(106, "dataset-kinds-update"),
KnownVersion::new(105, "inventory-nvme-firmware"),
KnownVersion::new(104, "lookup-bgp-config-indexes"),
Expand Down
12 changes: 11 additions & 1 deletion nexus/db-queries/src/db/datastore/disk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,11 @@ impl DataStore {
.into_boxed()
.filter(instance::dsl::state
.eq_any(ok_to_detach_instance_states)
.and(instance::dsl::active_propolis_id.is_null())),
.and(instance::dsl::active_propolis_id.is_null())
.and(
instance::dsl::boot_disk_id.ne(authz_disk.id())
.or(instance::dsl::boot_disk_id.is_null())
)),
disk::table
.into_boxed()
.filter(disk::dsl::disk_state.eq_any(ok_to_detach_disk_state_labels)),
Expand Down Expand Up @@ -388,6 +392,12 @@ impl DataStore {
// Ok-to-be-detached instance states:
api::external::InstanceState::Creating |
api::external::InstanceState::Stopped => {
if collection.boot_disk_id == Some(authz_disk.id()) {
return Err(Error::conflict(
"boot disk cannot be detached"
));
}

// We can't detach, but the error hasn't
// helped us infer why.
return Err(Error::internal_error(
Expand Down
Loading

0 comments on commit 144e91a

Please sign in to comment.