Skip to content

Conversation

@leftwo
Copy link
Contributor

@leftwo leftwo commented Oct 7, 2022

Remove the Read Only Parent

This introduces an internal API endpoint and a saga to remove a volume's read only parent.

The new internal API endpoint is at: /volume/{volume_id}/remove-read-only-parent

The basic flow for the saga to remove a read only parent is as follows:

  • A temporary "empty" volume is created
  • In a single transaction:
    1. The original volume is retrieved from the database.
    2. The original volume's read only parent is set to None
    3. The original volume is updated in the database.
    4. The temp volume has is retreived from the database.
    5. The temp volume has the read only parent set to the value from the original volume.
    6. The temp volume is updated in the database
  • The temporary volume is deleted (via the volume delete saga).

Several tests were added that cover various valid and invalid conditions.

There is nothing yet that calls this endpoint, but in a future commit, propolis (via. crucible) will have performed a volume scrub and will call this when it is time to remove a read only parent.

@leftwo leftwo marked this pull request as ready for review October 12, 2022 06:27
@leftwo leftwo requested review from davepacheco and jmpesp October 12, 2022 06:27
Alan Hanson added 7 commits October 14, 2022 01:27
Update volume remove read only parent API endpoint path.
Add more descriptive error message.
@leftwo
Copy link
Contributor Author

leftwo commented Oct 19, 2022

Out of band comments recorded here:

sean (he/him)
Silly question - why does svr_create_temp_volume not have an undo action?
Like, if we create the temporary volume, then try to delete the read-only parent (but can't, for some reason), we'd leave the saga with an extra temporary volume lying around, right?

Alan (he/him)
Yeah, an actual failure in deleting the ROP would do this, but won't a failure cause the saga to be retried? (my knowledge of sagas's here is new, so sorry if I don't know what I'm talking about)
The saga should "succeed" even if the volume is gone, or has no read only parent, or is the wrong kind of volume, etc. The only time removal returns ROP is on an error reading or updating the database, or with decoding the VCR string.

sean (he/him)
Depends on how you define "failure" here
if Nexus crashes, it'll retry the action

Alan (he/him)
Yeah, I want a retry in that case.

sean (he/him)
however, if an action ever returns an error, it'll start unwinding

Alan (he/him)
Yeah, I want a retry in that case.
Right, the saga executor should handle that for you automatically
An example where unwind would occur: if the call to osagactx.datastore().volume_remove_rop(...) returns an error, for whatever reason
the saga executor would see that, and try to "unwind" everything by calling undo actions. It would call the undo action for svr_create_temp_volume, but there isn't one, so it wouldn't clean up the temporary volume
Does that make sense?

Alan (he/him)
Yes.
I don't expect an error from volume_remove_rop(), but it could hit one, and having an unwind is not going to hurt.
And, as you point out, without unwinding we have this leftover volume.
So, I'll go add an unwind.
Thanks!

sean (he/him)
np - other than that small comment, the PR looks good!

Alan (he/him)
Hmm.. Actually...
I think what I want if svr_remove_read_only_parent fails is not an undo, but to continue with the rest of the saga? Can we even do that?
Otherwise, what I need is a subsaga for the undo step.

sean (he/him)

Alan (he/him)
I think what I want if svr_remove_read_only_parent fails is not an undo, but to continue with the rest of the saga? Can we even do that?

Isn't this the last node though? What would continuing mean?

Alan (he/him)
Otherwise, what I need is a subsaga for the undo step.

I figured the undo would just be to delete the temporary volume
Then it's as if the remove didn't happen

Alan (he/him)
There is a "step 3" to the saga, but it's built inside the make_saga_dag
For the "what happens when we undo", it actually depends on where exactly the failure happens inside svr_remove_read_only_parent.. Though, maybe because it's a transaction, it can't fail in a way that would result in partial completion..

sean (he/him)
I think this is supposed to be the problem sagas help solve - we should either move forward and complete the operation, or roll back and completely remove any "partial" steps

Alan (he/him)
Yeah, yeah. okay.

sean (he/him)
Having each node take idempotent, atomic actions is definitely a part of that, so yeah, the DB transaction should help here
But, for example, a very real error we've seen with DB transactions is an error thrown due to contention on a row

Alan (he/him)
So then yeah, I can just delete my temp volume without any special work, as it won't be a real volume yet.

@leftwo leftwo merged commit 9b2b583 into main Oct 19, 2022
@leftwo leftwo deleted the subsagavol branch October 19, 2022 19:46
leftwo pushed a commit that referenced this pull request Nov 11, 2025
Crucible changes are:
Print file name for extents (#1811)
Add threads argument to `crucible-downstairs verify` (#1807)
Add `--verbose` option to `crucible-verify-raw` (#1806)
Restore `--gen` argument for binaries (#1805)
Bump to 2024 edition (#1799)
Perform reconciliation if all three downstairs are in live-repair (#1784)
Rename crucible-dtrace -> crucible-utils (#1803)
Add `crucible-verify-raw` and `crucible-raw-extent packages` (#1800)
Added extent-info to dump out region/extent/block specific offsets (#1797)

Propolis changes are:
Rework resource accessors to alleviate lock contention
Implement NVMe Doorbell Buffer feature
Overhaul block attachment and request dispatch
propolis-cli should be able to send TOML-defined CPU profiles (#943)
nvme: CQEs with command-specific error 0 are acceptable (#965)
leftwo added a commit that referenced this pull request Nov 13, 2025
Update Propolis and Crucible
    
Crucible changes are:
Print file name for extents (#1811)
Add threads argument to `crucible-downstairs verify` (#1807)
Add `--verbose` option to `crucible-verify-raw` (#1806)
Restore `--gen` argument for binaries (#1805)
Bump to 2024 edition (#1799)
Perform reconciliation if all three downstairs are in live-repair
(#1784)
Rename crucible-dtrace -> crucible-utils (#1803)
Add `crucible-verify-raw` and `crucible-raw-extent packages` (#1800)
Added extent-info to dump out region/extent/block specific offsets
(#1797)
    
Propolis changes are:
Rework resource accessors to alleviate lock contention
Implement NVMe Doorbell Buffer feature
Overhaul block attachment and request dispatch
propolis-cli should be able to send TOML-defined CPU profiles (#943)
nvme: CQEs with command-specific error 0 are acceptable (#965)

I also changed a bunch of `gen` -> `generation` as that is now what
Crucible has.

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants