Skip to content

Internal cleanup for boot disk/instance update #6738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Internal cleanup for boot disk/instance update #6738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41d2e2f
Internal cleanup for boot disk/instance update
iximeow Oct 1, 2024
66f8afb
even trying to set a boot disk is optional, really
iximeow Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion nexus/db-model/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,9 +529,18 @@ mod optional_time_delta {
}

/// The parts of an Instance that can be directly updated after creation.
///
/// The model's name `InstanceReconfigure` is in contrast to the external
/// [`params::InstanceUpdate`]: `reconfigure` is the internal name of changing
/// an instance's configuration in places it might conflict with the phrase
/// "instance update". If we rename `instance update` (see
/// <https://github.com/oxidecomputer/omicron/issues/6631>), then this could
/// reasonably be renamed `InstanceUpdate` like other update models, with its
/// user `instance_reconfigure` also renamed to `instance_update` like other
/// update queries.
#[derive(Clone, Debug, AsChangeset, Serialize, Deserialize)]
#[diesel(table_name = instance, treat_none_as_null = true)]
pub struct InstanceUpdate {
pub struct InstanceReconfigure {
#[diesel(column_name = boot_disk_id)]
pub boot_disk_id: Option<Uuid>,
}
4 changes: 2 additions & 2 deletions nexus/db-queries/src/db/datastore/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ use crate::db::model::Generation;
use crate::db::model::Instance;
use crate::db::model::InstanceAutoRestart;
use crate::db::model::InstanceAutoRestartPolicy;
use crate::db::model::InstanceReconfigure;
use crate::db::model::InstanceRuntimeState;
use crate::db::model::InstanceState;
use crate::db::model::InstanceUpdate;
use crate::db::model::Migration;
use crate::db::model::MigrationState;
use crate::db::model::Name;
Expand Down Expand Up @@ -1009,7 +1009,7 @@ impl DataStore {
&self,
opctx: &OpContext,
authz_instance: &authz::Instance,
update: InstanceUpdate,
update: InstanceReconfigure,
) -> Result<InstanceAndActiveVmm, Error> {
opctx.authorize(authz::Action::Modify, authz_instance).await?;

Expand Down
4 changes: 2 additions & 2 deletions nexus/src/app/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::external_api::params;
use cancel_safe_futures::prelude::*;
use futures::future::Fuse;
use futures::{FutureExt, SinkExt, StreamExt};
use nexus_db_model::InstanceUpdate;
use nexus_db_model::InstanceReconfigure;
use nexus_db_model::IpAttachState;
use nexus_db_model::IpKind;
use nexus_db_model::Vmm as DbVmm;
Expand Down Expand Up @@ -330,7 +330,7 @@ impl super::Nexus {
None => None,
};

let update = InstanceUpdate { boot_disk_id };
let update = InstanceReconfigure { boot_disk_id };
self.datastore()
.instance_reconfigure(opctx, &authz_instance, update)
.await
Expand Down
17 changes: 13 additions & 4 deletions nexus/src/app/sagas/instance_create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,11 @@ impl NexusSaga for SagaInstanceCreate {
)?;
}

builder.append(set_boot_disk_action());
// We only need to set the boot disk if there is one to set.
if params.create_params.boot_disk.is_some() {
builder.append(set_boot_disk_action());
}

builder.append(move_to_stopped_action());
Ok(builder.build()?)
}
Expand Down Expand Up @@ -1077,8 +1081,9 @@ async fn sic_set_boot_disk(
.await
.map_err(ActionError::action_failed)?;

let initial_configuration =
nexus_db_model::InstanceUpdate { boot_disk_id: Some(authz_disk.id()) };
let initial_configuration = nexus_db_model::InstanceReconfigure {
boot_disk_id: Some(authz_disk.id()),
};

datastore
.instance_reconfigure(&opctx, &authz_instance, initial_configuration)
Expand Down Expand Up @@ -1109,8 +1114,12 @@ async fn sic_set_boot_disk_undo(

// If there was a boot disk, clear it. If there was not a boot disk,
// this is a no-op.
if params.create_params.boot_disk.is_none() {
return Ok(());
}
Comment on lines +1117 to +1119
Copy link
Member

@hawkw hawkw Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...it occurs to me that it might be nicer to just change the make_saga_dag function to just not append a sic_set_boot_disk node if there is no boot disk param. Like, change this:

omicron/nexus/src/app/sagas/instance_create.rs

Line 309 in 41d2e2f

builder.append(set_boot_disk_action());

to:

if params.create_params.boot_disk.is_some() {
    builder.append(set_boot_disk_action());
}

That way, we would never do the forward or reverse action if there's no boot disk.

Copy link
Member Author

@iximeow iximeow Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... huh, i suppose that makes sense. i'd read something in one of the comments here about Steno not handling dynamically-shaped sagas, but the comment earlier about MAX_NICS_PER_INSTANCE makes me think i'd read something old without realizing it.

Copy link
Member

@hawkw hawkw Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm...a saga's DAG can definitely depend on its params, so it's possible that the comment either meant something else by "dynamically-shaped", or was just out of date...


let undo_configuration =
nexus_db_model::InstanceUpdate { boot_disk_id: None };
nexus_db_model::InstanceReconfigure { boot_disk_id: None };

datastore
.instance_reconfigure(&opctx, &authz_instance, undo_configuration)
Expand Down
Loading