Skip to content

move to union type#2

Merged
ahl merged 3 commits into
masterfrom
edges
Mar 24, 2020
Merged

move to union type#2
ahl merged 3 commits into
masterfrom
edges

Conversation

@ahl

@ahl ahl commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

I went a little far here in terms of exploring the Option type. If this feels wrong or too Java-y that would be extremely useful feedback. It's different. I'm not sure it's better necessarily.

@davepacheco davepacheco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good. The data structure definitely looks cleaner. I found the style that's heavy on get_or_insert and using the return value from that a little hard to follow, but I believe it's correct and it's a good chance for me to learn more of those parts of the API.

When you land this, could you rebase it into one commit with a (slightly) more descriptive message?

Comment thread src/httpapi/router.rs Outdated
Comment thread src/httpapi/router.rs Outdated
Comment thread src/httpapi/router.rs Outdated
@ahl ahl merged commit 70480d1 into master Mar 24, 2020
@ahl ahl deleted the edges branch April 7, 2020 22:21
jmpesp added a commit to jmpesp/omicron that referenced this pull request Mar 7, 2024
Volumes are "checked out" from Nexus for many reasons, some of which
include sending to another service for use in `Volume::construct`. When
that service activates the resulting Volume, this will forcibly take
over any existing downstairs connections based on the Upstairs'
generation number. This is intentional, and was designed so Nexus, in
handing out Volumes with increasing generation numbers, can be sure that
the resulting Volume works no matter what (for example, even if a
previous Upstairs is wedged somehow, even if the service that is running
the previous Upstairs is no longer accepting network connections).

Up until now, Nexus wouldn't allow checking out a Volume if there is any
chance a Propolis could be running that may use that Volume. This meant
restricting certain operations, like creating a snapshot when a disk is
attached to an instance that is stopped: any action Nexus would take to
attempt a snapshot using a Pantry would race with a user's request to
start that instance, and if the Volume checkouts occur in the wrong
order the Pantry would take over connections from Propolis, resulting in
guest OS errors.

Nexus _can_ do this safely though: it has all the information required
to know when a checkout is safe to do, and when it may not be safe.
This commit adds checks to the Volume checkout transaction that are
based on the reason that checkout is occurring, and requires call sites
that are performing a checkout to say why they are. Because these checks
are performed inside a transaction, Nexus can say for sure when it is
safe to allow a Volume to be checked out for a certain reason.

For example, in the scenario of taking a snapshot of a disk attached to
an instance that is stopped, there are two checkout operations that have
the possiblity of racing:

1) the one that Nexus will send to a Pantry during a snapshot create
   saga.

2) the one that Nexus will send to a Propolis during an instance start
   saga.

If #1 occurs before oxidecomputer#2, then Propolis will take over the downstairs
connections that the Pantry has established, and the snapshot create
saga will fail, but the guest OS for that Propolis will not see any
errors. If oxidecomputer#2 occurs before #1, then the #1 checkout will fail due to
one of the conditions added in this commit: the checkout is being
performed for use with a Pantry, and a Propolis _may_ exist, so reject
the checkout attempt.

Fixes oxidecomputer#3289.
labbott added a commit that referenced this pull request Jul 24, 2025
This reverts commit 6570fc6
to bring back the original f897585
and brings in some fixes to actually make it work.

Still uses a basic measurement set with sprockets. Future work will
include rotation of measurements.
labbott added a commit that referenced this pull request Jul 24, 2025
This reverts commit 6570fc6
to bring back the original f897585
and brings in some fixes to actually make it work.

Still uses a basic measurement set with sprockets. Future work will
include rotation of measurements.
labbott added a commit that referenced this pull request Jul 24, 2025
This reverts commit 6570fc6
to bring back the original f897585
and brings in some fixes to actually make it work.

Still uses a basic measurement set with sprockets. Future work will
include rotation of measurements.
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.

2 participants