Implement disk creation during instance creation

[jmpesp]

#771 adds support for attaching an already created disk during the instance creation saga but leaves out support for creating a disk during instance creation. This issue tracks that work.

[david-crespo]

Would like to prioritize this so we can stop doing the workaround in console of making a separate request to create the disk before calling instance create.

[smklein]

The reason we struggle to do this cleanly in Omicron right now is a lack of "sub-sagas". Basically:

• instance creation is a saga
• disk creation is a saga
• It would be nice to have a way to create one or more "disk creation" sagas from the "instance creation" saga

Potential solutions:

1. Doing so right now would require basically copy-pasting the entire disk-creation saga into the instance-creation saga, to flatten it into one. We could refactor functions a bit, but we will need to enumerate all the disk creation saga steps within the instance creation saga.
2. Adding sub-sagas to steno - some way of saying "please run this other saga as a step within our own saga graph" - would be a solution that requires less boilerplate, and which leaves the instance creation saga mostly isolated from the details of "how do we actually create a disk".

@andrewjstone - you mentioned you were eyeing steno recently. Any interest in checking-in on the status of "sub-sagas"?

[davepacheco]

I don't want to discourage the subsaga work because that'll be really valuable.

We could refactor functions a bit, but we will need to enumerate all the disk creation saga steps within the instance creation saga.

I wonder if we could abstract this further so that there's a common function that accepts a SagaTemplateBuilder and adds a sequence of actions to it. Then there's very little that would be duplicated. The hard part seems like the fact that the sagas probably have different SagaTypes.

[andrewjstone]

@smklein @davepacheco I am definitely interested in this problem. Steno could use some love and I'm fascinated by it. I didn't look too deeply into sub-sagas though, and was wondering about how dynamic they are. I was also wondering if there was a way to do what Dave suggests or #2 that Sean suggests where we can do something like #include saga type into other sagas for possible use. It would be great if we could chat about this so I can get my bearings. I'll ping you both.

[david-crespo]

Posted in chat to clarify the priority:

What we have right now works in the console in the basic sense that we can create an instance that works. We do that by calling the disk create endpoint with the specified image and the passing that info to the instance create as an existing disk. on the other hand the API advertised by the OpenAPI spec is basically false, since creating a disk as part of the create instance is part of the API but always fails.

[andrewjstone]

I don't want to discourage the subsaga work because that'll be really valuable.

We could refactor functions a bit, but we will need to enumerate all the disk creation saga steps within the instance creation saga.

I wonder if we could abstract this further so that there's a common function that accepts a SagaTemplateBuilder and adds a sequence of actions to it. Then there's very little that would be duplicated. The hard part seems like the fact that the sagas probably have different SagaTypes.

I looked into this and different SagaTypes is indeed the problem with the sharing of actions. However, all sagas in nexus share a SagaContext, and so the only difference is the SagaParamsType associated type in each of SagaInstanceCreate and SagaDiskCreate. However, it looks like the saga params for SagaInstanceCreate include the DiskCreate params. So the instance disk create should have all the data it needs to do the creation, but the functions in SagaDiskCreate template actions refer directly to the fields on params.

I think If instead of accessing the params fields directly in the actions functions for SagaDiskCreate, we called a method on the params type that returned the proper params::DiskCreate despite which SagaType the builder was called with and made the disk create action functions be generic over SagaType, and added a constraint for the method that would return params::DiskCreate on the correct SagaType we could do what Dave suggests.

If I'm reading things correctly, this doesn't seem too hard to do, but it does start coupling SagaTypes which may set a bad precedent. I believe sub-sagas are the cleaner abstraction, and after reading all this code I now understand why. However, this code base hasn't used them yet, and I wouldn't mind cleaning up steno and adding automated tests before it did, so I can understand the rough edges first. I'm still just digging into this part of omicron and steno for the first time really and am not looking to break things without understanding :)

In summary, I believe I can probably get this working very short term by playing games with SagaTypes. And then I can go and retrofit it with subsagas later. I guess it depends how urgently we want this.

[andrewjstone]

I realized I never updated this. I'm currently working on making steno more dynamic and fully supporting subsagas.

[andrewjstone]

The Steno PR that enables subsagas is out for review: oxidecomputer/steno#29

[davepacheco]

The Steno PR is nearly ready to land! I'm updating Omicron to use it in #1532.

[zephraph]

To close this out we need to utilized a sub-saga in the sic_create_disks_for_instances function.