Skip to content
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

Regions can now be read-only #6150

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

jmpesp
Copy link
Contributor

@jmpesp jmpesp commented Jul 24, 2024

Up until this point, regions were only ever read-write, and region snapshots were read-only. In order to support snapshot replacement, Crucible recently gained support for read-only downstairs that performs a "clone" operation to copy blocks from another read-only downstairs.

This commit adds a "read-only" flag to Region, and adds support for Nexus initializing a downstairs with this new clone option.

@jmpesp jmpesp requested review from smklein and leftwo July 24, 2024 16:02
Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

As usual, I have questions because I don't know how anything works

nexus/db-queries/src/db/datastore/region.rs Show resolved Hide resolved
nexus/src/app/crucible.rs Show resolved Hide resolved
schema/crdb/region-read-only/up02.sql Show resolved Hide resolved
Up until this point, regions were only ever read-write, and region
snapshots were read-only. In order to support snapshot replacement,
Crucible recently gained support for read-only downstairs that performs
a "clone" operation to copy blocks from another read-only downstairs.

This commit adds a "read-only" flag to Region, and adds support for
Nexus initializing a downstairs with this new clone option.
Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

Just to make sure I grok this correctly:

  • Nothing actually tells Crucible about this flag yet, right?
  • The case of read_only = false is currently treated identically to read_only = true, with the only exception being the error thrown in region_replacement_start.rs, correct?

Did you expect for this flag to have a meaning before merging, or is this acting as more of a placeholder?

@jmpesp
Copy link
Contributor Author

jmpesp commented Jul 29, 2024

Nothing actually tells Crucible about this flag yet, right?

Correct, this PR was carved out of the snapshot replacement work to be reviewed separately.

The case of read_only = false is currently treated identically to read_only = true, with the only exception being the error thrown in region_replacement_start.rs, correct?

Yeah - allocation wise there isn't much difference between a read-only region and a read-write region, they both require the same amount of space. The difference is in how they're initialized.

Did you expect for this flag to have a meaning before merging, or is this acting as more of a placeholder?

Right now this is only a placeholder - the snapshot replacement PR number 3 are is the one that will make use of it.

@jmpesp jmpesp merged commit 1bb75f2 into oxidecomputer:main Jul 29, 2024
16 checks passed
@jmpesp jmpesp deleted the region_can_be_read_only branch July 29, 2024 17:32
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.

3 participants