Enable instance create saga to create a new disk #1873

just-be-dev · 2022-10-25T02:02:53Z

Fixes #812

Wires up the instance disk create flow by directly appending the disk create saga as a sub saga. I also corrected the behavior of the attach action by ensuring it doesn't spawn a saga in its action but rather just updates the relevant data in the datastore. There's a TODO to make that functionality more robust, but I covered everything that's handled currently by instance attach as defined in nexus... which admittedly isn't much.

While I was in here I also removed some of the unnecessary behavior around looping through the max number of disks and passing the index down as apart of the saga params. Now the disk create or attach params themselves are passed down and we only loop as many times as is needed. This was an artifact of having static sagas previously.

There's still more cleanup to do for NICs/external IPs which I haven't tackled and of course more robust tests are required. I'm going to leave the latter up to @smklein as he's been working on that for disk create already.

nexus/src/app/sagas/instance_create.rs

just-be-dev · 2022-10-27T03:08:25Z

nexus/src/app/sagas/instance_create.rs

-            subsaga_builder: steno::DagBuilder,
+            subsaga_dag: steno::Dag,


On @andrewjstone's recommendation I updated this function to take a dag instead of a builder. It's easy enough to call the builder at the append callsite and this lets us reuse the disk create saga's make_saga_dag function.

nexus/src/app/sagas/instance_create.rs

just-be-dev · 2022-10-27T17:52:08Z

nexus/src/app/sagas/instance_create.rs

+    let (.., authz_disk, _db_disk) = LookupPath::new(&opctx, &datastore)
+        .project_id(project_id)
+        .disk_name(&disk_name)
+        .fetch()
+        .await
+        .map_err(ActionError::action_failed)?;


We really shouldn't be looking up disks by name here, but that's all we have. Technically I could do a special lookup for disks that were previously created and grab their IDs, but for disks that are only being attached all we have is what the API provides.

This is a bit of an aside to this issue, but I believe that all names should be resolved inside of http_entrypoints. Once calls flow further down in the system the should be operating solely on IDs. Whenever we begin to implement RFD-322 we should probably consider that.

💯 is that not worth doing here? If not, I wonder if we should keep the comment that was here before (the TODO-security TODO-correctness one that references omicron#1536).

I can re-add the comment. I still generally believe that we shouldn't just handle this in a one-off way, but more as a holistic solution. I know that's a non-trivial amount of work, but I still believe it to be important.

andrewjstone

@zephraph This looks great! It's really nice to see you creating just enough saga nodes necessary to create and attach disks, and not the MAX. I only had one nit and one question.

This is probably safe to merge, although, I'd like a few others to review. Presumably you tested this all locally and you can now create disks during instance creation :)

nexus/src/app/sagas/instance_create.rs

andrewjstone · 2022-10-28T00:10:34Z

nexus/src/app/sagas/instance_create.rs

-            }
+    if attached {
+        datastore
+            .instance_attach_disk(


Just for my understanding, as I'm not super familiar with this code: Why change the call from using osagactx.nexus().instance_XXX to datastore.instance_XXX?

It seems you basically duplicated the methods from nexus/src/app/instance.rs

Yeah, this was a misunderstanding on my part. I'd mistakenly thought that functions off of osagactx.nexus() spawned sagas when that's clearly just an implementation detail. ~~I'll change it back to what it was.~~

Actually a correction to that: nexus().instance_attach_disk isn't the best form. It takes names and does lookups which are resolved before calling the datastore method. You're right that we should reuse instance_attach_disk in /nexus/src/app/instance but ideally we'd have a different form that takes pre-resolved arguments like the datastore method.

Ideally we want to resolve as many names as possible at the very start of a saga (or better, before the saga is invoked).

Do folks think I should change it back or is this form acceptable until we're able to follow up with a better interface for attaching? cc @smklein, @davepacheco.

This sort of plays into my previous comment.

At this point I'd personally err on the side of "doing less name lookups" - the boundary between nexus and datastore is really thin from the point-of-view of sagas. The datastore itself should provide necessary encapsulation for safe DB access, but forcing sagas to go through nexus isn't adding much IMO.

A few things:

Yes, I think at some point we discussed having the Nexus-level functions accept LookupPath objects rather than "names" so that we could use them from places that had ids or names.

Yes, I think sagas should use the datastore freely when that's the right level of abstraction.

At the same time, it might also be appropriate to use the Nexus-level functions when there's real application-level logic that has to happen. For example, I think it'd be better for sagas that need to update instance state to use the app-level instance_set_runtime(). In this specific case, it seems like the app-level wrapper doesn't do anything but the name resolution and then call the datastore function. So it seems fine.

smklein

Have you looked at the CI failures? I'm not sure if they're flakes, or related to this PR

nexus/src/app/sagas/instance_create.rs

smklein · 2022-10-28T15:58:59Z

nexus/src/app/sagas/instance_create.rs

-            }
+    if attached {
+        datastore
+            .instance_attach_disk(


At this point I'd personally err on the side of "doing less name lookups" - the boundary between nexus and datastore is really thin from the point-of-view of sagas. The datastore itself should provide necessary encapsulation for safe DB access, but forcing sagas to go through nexus isn't adding much IMO.

davepacheco

This looks good! Such a huge improvement, not just to what's here but the API and (I expect) the console, too.

I know @smklein is still working on examples for doing unit tests of sagas. But I think we could still have end-to-end tests that exercise a few key cases here:

an instance create that creates a disk, attaches another disk, and succeeds
an instance create that creates one disk, attaches another disk, and fails...we should at least verify after this that the created disk was destroyed and the other disk is detached. It'd be nice if we can make sure it fails in a way that did create the first disk and/or attach the other disk so that we're exercising the undo actions.

Even if we also had unit tests already I think we'd still want these basic end-to-end tests.

davepacheco · 2022-10-28T18:31:32Z

nexus/src/app/sagas/instance_create.rs

+    let (.., authz_disk, _db_disk) = LookupPath::new(&opctx, &datastore)
+        .project_id(project_id)
+        .disk_name(&disk_name)
+        .fetch()
+        .await
+        .map_err(ActionError::action_failed)?;


💯 is that not worth doing here? If not, I wonder if we should keep the comment that was here before (the TODO-security TODO-correctness one that references omicron#1536).

davepacheco · 2022-10-28T18:35:43Z

nexus/src/app/sagas/instance_create.rs

-            }
+    if attached {
+        datastore
+            .instance_attach_disk(


A few things:

Yes, I think at some point we discussed having the Nexus-level functions accept LookupPath objects rather than "names" so that we could use them from places that had ids or names.

Yes, I think sagas should use the datastore freely when that's the right level of abstraction.

At the same time, it might also be appropriate to use the Nexus-level functions when there's real application-level logic that has to happen. For example, I think it'd be better for sagas that need to update instance state to use the app-level instance_set_runtime(). In this specific case, it seems like the app-level wrapper doesn't do anything but the name resolution and then call the datastore function. So it seems fine.

just-be-dev · 2022-10-28T21:20:37Z

Have you looked at the CI failures? I'm not sure if they're flakes, or related to this PR

I've looked at them, yeah. A snapshot integration test is timing out. I'm trying to dig into what's going on there.

nexus/src/app/sagas/instance_create.rs

just-be-dev · 2022-10-31T18:48:15Z

an instance create that creates one disk, attaches another disk, and fails...we should at least verify after this that the created disk was destroyed and the other disk is detached. It'd be nice if we can make sure it fails in a way that did create the first disk and/or attach the other disk so that we're exercising the undo actions.

@davepacheco I think the last test I added tests this case sufficiently. It pre-creates two disks, one of which is faulted. It creates the boot disk, attaches the first good disk, and then tries to attach the faulted disk which results in the saga failing. The test tries to ensure that the same number of disks exist in the expected state post cleanup.

smklein

Looks good - only a couple minor suggestions for the tests!

nexus/tests/integration_tests/instances.rs

davepacheco

It'd be nice to have a comment on the tests saying what they're testing. Either way, this looks good!

davepacheco · 2022-11-01T22:56:23Z

nexus/tests/integration_tests/instances.rs

+    let builder =
+        RequestBuilder::new(client, http::Method::POST, &url_instances)
+            .body(Some(&instance_params))
+            .expect_status(Some(http::StatusCode::CREATED));
+
+    let response = NexusRequest::new(builder)


I think you can replace most of this with let response = NexusRequest::objects_post(client, &url_instances, &instance_params)) (it's equivalent -- not better or worse aside from less code)

just-be-dev added 2 commits October 24, 2022 20:58

Start wiring up disk-create in instance-create

a3dd93c

Attach disk after its created

09e6d87

just-be-dev commented Oct 25, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Outdated Show resolved Hide resolved

just-be-dev added 2 commits October 25, 2022 14:04

Flatten attach

0b0466d

Maybe fix create enum

0085424

just-be-dev marked this pull request as ready for review October 25, 2022 20:24

just-be-dev requested a review from smklein October 25, 2022 20:24

just-be-dev added 2 commits October 25, 2022 15:42

Merge branch 'main' into instance-disk-create

4bbefcb

Implement undo

59a7dc6

just-be-dev commented Oct 25, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Outdated Show resolved Hide resolved

just-be-dev mentioned this pull request Oct 26, 2022

Remove instance create hack oxidecomputer/console#1249

Merged

smklein reviewed Oct 26, 2022

View reviewed changes

just-be-dev added 3 commits October 26, 2022 19:51

Merge branch 'main' into instance-disk-create

db23c77

Update subsaga_append to take a dag instead of builder

f8c0a5a

Wire up disk create subsaga

09447c2

just-be-dev commented Oct 27, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Show resolved Hide resolved

just-be-dev commented Oct 27, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Show resolved Hide resolved

just-be-dev marked this pull request as draft October 27, 2022 03:15

just-be-dev added 5 commits October 26, 2022 22:19

Fix lint error

cf1fc5b

Update fn name to be singular to clarify it attaches 1 disk

c1bf2da

Improve attach/detaching to not create a saga as a node

9126223

Clean up attach iteration logic

d66965c

Pass direct references down instead of disk array

5da7014

just-be-dev commented Oct 27, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Show resolved Hide resolved

just-be-dev added 2 commits October 27, 2022 12:38

Fix disk create subsaga params; clean up comments

6dcaa27

Appease ye almighty clippy

8d0c3ce

just-be-dev marked this pull request as ready for review October 27, 2022 17:46

just-be-dev commented Oct 27, 2022

View reviewed changes

just-be-dev requested a review from smklein October 27, 2022 17:52

just-be-dev requested review from andrewjstone and davepacheco October 27, 2022 17:52

Update attach params shape to include serialized_athn; project_id

3927efe

andrewjstone reviewed Oct 28, 2022

View reviewed changes

Address nit

033f583

smklein reviewed Oct 28, 2022

View reviewed changes

davepacheco reviewed Oct 28, 2022

View reviewed changes

Add TODO on correctness of disk lookup by name

be534b9

just-be-dev commented Oct 31, 2022

View reviewed changes

nexus/src/app/sagas/instance_create.rs Outdated Show resolved Hide resolved

just-be-dev added 4 commits October 31, 2022 11:49

instance_id isn't in the subsaga context, pass it down instead

d21ce3e

Fix clippy failures

d68e99b

Add integration test to create and attach disks via instance create

1a673a4

Add test for undos of creates/attaches for failed instance create saga

d3bbae0

just-be-dev requested review from davepacheco and smklein October 31, 2022 18:52

Reduce disk sizes to hopefully resolve zpool failures

445cda7

just-be-dev force-pushed the instance-disk-create branch from 5ddb1ff to 445cda7 Compare October 31, 2022 21:29

just-be-dev requested a review from andrewjstone October 31, 2022 22:56

smklein approved these changes Nov 1, 2022

View reviewed changes

nexus/tests/integration_tests/instances.rs Outdated Show resolved Hide resolved

nexus/tests/integration_tests/instances.rs Outdated Show resolved Hide resolved

nexus/tests/integration_tests/instances.rs Outdated Show resolved Hide resolved

Address PR feedback on tests

5f618ef

just-be-dev force-pushed the instance-disk-create branch from 1e67336 to 5f618ef Compare November 1, 2022 19:51

davepacheco approved these changes Nov 1, 2022

View reviewed changes

Comment what test is validating

3636fd1

just-be-dev enabled auto-merge (squash) November 2, 2022 05:07

just-be-dev merged commit 2b3a643 into main Nov 2, 2022

just-be-dev deleted the instance-disk-create branch November 2, 2022 06:11

Enable instance create saga to create a new disk #1873

Enable instance create saga to create a new disk #1873

Uh oh!

Conversation

just-be-dev commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewjstone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev commented Oct 28, 2022

Uh oh!

Uh oh!

just-be-dev commented Oct 31, 2022

Uh oh!

smklein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

just-be-dev commented Oct 25, 2022 •

edited

Loading

just-be-dev Oct 28, 2022 •

edited

Loading