FYI @iliana

Just FYI, I had some bad experience panicking during a saga. The recovery mechanisms will basically just retry this node indefinitely, not unwind the whole thing. I'd consider returning an error instead.

Each step in the saga must be idempotent - namely, repeatedly calling ensure_instance_disk_attach_state, either as part of the action or undo-action must:

• Result in the same effective state
• Return the same result

Is this currently the case?

(I think it is - especially since we're operating at exclusively a DB layer here - but wanted to check that assumption)

I'm pretty sure this is true for the same reason but I'm not sure. At the very least, repeated calls to instance_attach_disk and instance_detach_disk is tested by some of the disk related integration tests.

What isn't tested is doing that through the sled agent for a running instance but that code path isn't hit here (?).

(Not explicitly a request, more just out-loud thinking)

To clarify the path, from a high-level:

• sic_attach_disks_to_instance happens right before actually sending the "instance ensure" request
• It calls Nexus::instance_attach_disk, which calls Nexus::disk_set_runtime
• If the instance state is InstanceState::Creating and the requested disk state is DiskStateRequested::Attached, we write the requested disk state ("attached") to the database, but don't send any requests to the sled agent (yet).

I wonder if we want to bifurcate disk_set_runtime into two different functions, within Nexus. The fact that it "may or may not make a request to sled agent" feels a little unintuitive to me - I 100% understand why it's acting that way, and I think deferring the request until actually calling "instance ensure" makes sense, but understanding when it would / would not make a call to sled agent feels a little non-obvious.

WDYT? I think this is subjective, but just wanna make sure the flows are as straightforward as they can be.

This is true (though the actual path traversed through the code has changed as part of 581bdee), with the idea that attaching disks to the instance only in the DB is preparing for the logic in instance_set_runtime that queries the disks attached to the instance.

We may want to remove the disk_put path and do everything through instance_put. That way we can funnel changes through instance_set_runtime and let propolis handle the disk attach or remove as it needs to.

Is this remarking that the API to disk_set_runtime is insufficient? I know most of this hot-plug path isn't implemented (which is 100% fine) but I just wanna make sure we're very explicit about what we expect to work / not work.

propolis needs to be handed the volume construction request, so sled agent's disk_put is currently insufficient, yeah, but what do you think about

We may want to remove the disk_put path and do everything through instance_put. That way we can funnel changes through instance_set_runtime and let propolis handle the disk attach or remove as it needs to.

?

That change sounds good to me!

WDYT about calling append_parallel here, and running these concurrently? Same for attach?

I can't, unfortunately, unless I store the strings somewhere persistent. append_parallel wants tuples that look like (&str, &str, Arc<dyn steno::Action<SagaInstanceCreate>>).

Would that not be possible through:

template_builder.append_parallel(
  (0..MAX_DISKS).map(|i| {
    (
      &format!("create_disks{}", i),
      "CreateDisksForInstance",
      AsyncFunc::new_action(....),
    )
  }).collect::<Vec<_>>()
);

(same for "AttachDisks" below)

If Rust gives you issues with lifetimes, then maybe:

let names = (0...MAX_DISKS).map(|i| format!("create_disks{}", i)).collect::<Vec<String>>();
template_builder.append_parallel(
  (0..MAX_DISKS).map(|i| {
    (
      names[i],
      "CreateDisksForInstance",
      AsyncFunc::new_action(....),
    )
  }).collect::<Vec<_>>()
);

Rust does give you issues with lifetimes for the first one, and for the second I'm pretty sure names has to live longer than the template builder function.

I'm pretty sure names doesn't need to last longer than the function - the signature doesn't imply that, and the implementation actually copies whatever string is provided by calling .to_string() : https://github.com/oxidecomputer/steno/blob/578498bbb43f9061c636b938dff98e7ece0b0efc/src/saga_template.rs#L315-L359