-
Notifications
You must be signed in to change notification settings - Fork 25
Split "check reconciliation state" from "start reconciliation" #1732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leftwo
approved these changes
Jun 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence about the name of start_reconciliation()
as we are just checking to see if we need to do reconciliation. But, really, that is part of the reconciliation process, to first determine if additional work is what we need to do, so I think I'm fine with the name.
jmpesp
approved these changes
Jun 27, 2025
21a03f8
to
1b91f6c
Compare
jmpesp
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Aug 11, 2025
Pick up the following propolis PRs: - Bump crucible rev to latest (oxidecomputer/propolis#922) - Added block_size for file backends in propolis_server (workers is optional) (oxidecomputer/propolis#917) Pick up the following crucible PRs: - Snapshots existing already are ok! (oxidecomputer/crucible#1759) - Less verbose logging (oxidecomputer/crucible#1756) - Remove unused `Vec<JoinHandle>` (oxidecomputer/crucible#1754) - Split "check reconciliation state" from "start reconciliation" (oxidecomputer/crucible#1732) - Improve `ClientIoTask` start logic (oxidecomputer/crucible#1731) - Use data-bearing enum variants pattern in negotiation (oxidecomputer/crucible#1727) - Make Downstairs stoppable (oxidecomputer/crucible#1730) - Don't log every region's metadata (oxidecomputer/crucible#1729) - Compute reconciliation from `ClientMap` instead of three clients (oxidecomputer/crucible#1726) - Make Offline -> Faulted transition happen without reconnecting (oxidecomputer/crucible#1725) - Remove `WaitActive` state during negotiation (oxidecomputer/crucible#1722) - Add explicit `UpstairsState::Disabled` (oxidecomputer/crucible#1721) - Print version on startup for pantry and agent (oxidecomputer/crucible#1723) - Update test mem to also show physical space used by regions. (oxidecomputer/crucible#1724) - Add AllStopped command and endpoint for downstairs status (oxidecomputer/crucible#1718) - Update tests to honor REGION_SETS env if provided. (oxidecomputer/crucible#1720) - Fix panic in `set_active_request` when client is in Stopping(Replacing) state (oxidecomputer/crucible#1717) - DTrace updates (oxidecomputer/crucible#1715)
jmpesp
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Aug 11, 2025
Pick up the following propolis PRs: - oxidecomputer/propolis#922 - oxidecomputer/propolis#917 Pick up the following crucible PRs: - oxidecomputer/crucible#1759 - oxidecomputer/crucible#1756 - oxidecomputer/crucible#1754 - oxidecomputer/crucible#1732 - oxidecomputer/crucible#1731 - oxidecomputer/crucible#1727 - oxidecomputer/crucible#1730 - oxidecomputer/crucible#1729 - oxidecomputer/crucible#1726 - oxidecomputer/crucible#1725 - oxidecomputer/crucible#1722 - oxidecomputer/crucible#1721 - oxidecomputer/crucible#1723 - oxidecomputer/crucible#1724 - oxidecomputer/crucible#1718 - oxidecomputer/crucible#1720 - oxidecomputer/crucible#1717 - oxidecomputer/crucible#1715
jmpesp
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Aug 11, 2025
Pick up the following propolis PRs: - Bump crucible rev to latest (oxidecomputer/propolis#922) - Added block_size for file backends in propolis_server (workers is optional) (oxidecomputer/propolis#917) Pick up the following crucible PRs: - Snapshots existing already are ok! (oxidecomputer/crucible#1759) - Less verbose logging (oxidecomputer/crucible#1756) - Remove unused `Vec<JoinHandle>` (oxidecomputer/crucible#1754) - Split "check reconciliation state" from "start reconciliation" (oxidecomputer/crucible#1732) - Improve `ClientIoTask` start logic (oxidecomputer/crucible#1731) - Use data-bearing enum variants pattern in negotiation (oxidecomputer/crucible#1727) - Make Downstairs stoppable (oxidecomputer/crucible#1730) - Don't log every region's metadata (oxidecomputer/crucible#1729) - Compute reconciliation from `ClientMap` instead of three clients (oxidecomputer/crucible#1726) - Make Offline -> Faulted transition happen without reconnecting (oxidecomputer/crucible#1725) - Remove `WaitActive` state during negotiation (oxidecomputer/crucible#1722) - Add explicit `UpstairsState::Disabled` (oxidecomputer/crucible#1721) - Print version on startup for pantry and agent (oxidecomputer/crucible#1723) - Update test mem to also show physical space used by regions. (oxidecomputer/crucible#1724) - Add AllStopped command and endpoint for downstairs status (oxidecomputer/crucible#1718) - Update tests to honor REGION_SETS env if provided. (oxidecomputer/crucible#1720) - Fix panic in `set_active_request` when client is in Stopping(Replacing) state (oxidecomputer/crucible#1717) - DTrace updates (oxidecomputer/crucible#1715)
askfongjojo
pushed a commit
to oxidecomputer/omicron
that referenced
this pull request
Aug 11, 2025
Bump crucible and propolis revs to latest Pick up the following propolis PRs: - oxidecomputer/propolis#922 - oxidecomputer/propolis#917 Pick up the following crucible PRs: - oxidecomputer/crucible#1759 - oxidecomputer/crucible#1756 - oxidecomputer/crucible#1754 - oxidecomputer/crucible#1732 - oxidecomputer/crucible#1731 - oxidecomputer/crucible#1727 - oxidecomputer/crucible#1730 - oxidecomputer/crucible#1729 - oxidecomputer/crucible#1726 - oxidecomputer/crucible#1725 - oxidecomputer/crucible#1722 - oxidecomputer/crucible#1721 - oxidecomputer/crucible#1723 - oxidecomputer/crucible#1724 - oxidecomputer/crucible#1718 - oxidecomputer/crucible#1720 - oxidecomputer/crucible#1717 - oxidecomputer/crucible#1715 --------- Co-authored-by: Artemis Everfree <artemis@oxidecomputer.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(staged on top of #1731)
This is a small cleanup: previously,
collate
calledcollate_inner
, then asserted that nothing changed ifcollate_inner
returned an error.However, it turns out that
collate_inner
can be split into two independent pieces:&self
which checks whether the state is valid and can fail (check_region_metadata
)start_reconciliation
)This division makes "assert nothing changed" unnecessary: the checking function can't modify
&self
, and starting reconciliation can't fail.