[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) #1150

ryaeng · 2022-06-03T14:12:19Z

Requires the user to select a size of at least one gibibyte when creating a new disk.
Add a test which attempts to create a disk that is 512 mebibytes in size.

…ranularity size (oxidecomputer#1029) - Requires the user to select a size of at least one gibibyte when creating a new disk. - Add a test which attempts to create a disk that is 512 mebibytes in size.

smklein · 2022-06-07T13:35:58Z

Hey Ryan - this change looks good to me; thanks for contributing (and thank you for adding a test)!

I do have one minor request, both here and on #1157 .

In some of our internal discussions, we mentioned that the choice here, although somewhat arbitrary, might change as we gather more information on use-cases. Could we define this value as a constant?

Perhaps here: https://github.com/oxidecomputer/omicron/blob/main/nexus/src/external_api/params.rs , something like:

const MAX_DISK_SIZE_BYTES: u32 = (1 << 30);  // 1 GiB

Then, in the test, we could just try creating a disk of MAX_DISK_SIZE_BYTES / 2

smklein · 2022-06-07T13:46:23Z

nexus/src/app/disk.rs

@@ -54,6 +54,17 @@ impl super::Nexus {
                        ),
                    });
                }
+
+                // Reject disks where the block size isn't at least


To close out #1029 , could we do a granularity check too? EDIT: of 1 GiB, as well

(I think it's okay to be slightly redundant and have this be a separate check from the block device size)

I'm not familiar with granularity checks. Can you explain this to me or point me in the right direction?

Effectively, it's a constraint on the divisibility of the requested size.

For example, although a 0.9 GiB disk can no longer be requested with this PR, a 1.1 GiB disk could be requested.

Our inclination is to be slightly more conservative - only allowing allocation requests of 1 GiB, 2 GiB, etc.

Got it! Makes sense now. Thanks for the clarification.

nexus/src/app/disk.rs

ryaeng · 2022-06-07T14:18:15Z

Hey Ryan - this change looks good to me; thanks for contributing (and thank you for adding a test)!

I do have one minor request, both here and on #1157 .

In some of our internal discussions, we mentioned that the choice here, although somewhat arbitrary, might change as we gather more information on use-cases. Could we define this value as a constant?

Perhaps here: https://github.com/oxidecomputer/omicron/blob/main/nexus/src/external_api/params.rs , something like:
const MAX_DISK_SIZE_BYTES: u32 = (1 << 30);  // 1 GiB
Then, in the test, we could just try creating a disk of MAX_DISK_SIZE_BYTES / 2

Should this be MIN_DISK_SIZE_BYTES?

smklein · 2022-06-07T14:22:32Z

Hey Ryan - this change looks good to me; thanks for contributing (and thank you for adding a test)!
I do have one minor request, both here and on #1157 .
In some of our internal discussions, we mentioned that the choice here, although somewhat arbitrary, might change as we gather more information on use-cases. Could we define this value as a constant?
Perhaps here: https://github.com/oxidecomputer/omicron/blob/main/nexus/src/external_api/params.rs , something like:
const MAX_DISK_SIZE_BYTES: u32 = (1 << 30);  // 1 GiB
Then, in the test, we could just try creating a disk of MAX_DISK_SIZE_BYTES / 2
Should this be MIN_DISK_SIZE_BYTES?

Whoops, yes!

- Add MIN_DISK_SIZE constant. - Modify tests to create disks with a size of MIN_DISK_SIZE / 2.

ryaeng · 2022-06-07T17:33:10Z

Sean, I've finished. What's the best way to go about amending my changes to this PR?

smklein · 2022-06-07T17:47:22Z

Sean, I've finished. What's the best way to go about amending my changes to this PR?

Adding commits is the way to go - we have a policy of squashing to a single commit when PRs themselves are merged, but we generally have an append-only commit history within the context of a single PR.

The disk size must be divisible by 1 GiB.

ryaeng · 2022-06-07T18:35:18Z

Done

…_disk

zephraph · 2022-06-07T19:20:31Z

nexus/src/app/disk.rs

+                    return Err(Error::InvalidValue {
+                        label: String::from("size"),
+                        message: String::from(
+                            "total size must be at least 1 GiB",


The message is incorrect here, right? MIN_DISK_SIZE_BYTES would need to included in the message instead.

Justin, I was curious about how to handle this one. I'll take care of that now. Thanks for your input.

instead of 1 GiB

smklein · 2022-06-08T14:09:34Z

nexus/src/app/disk.rs

+                    return Err(Error::InvalidValue {
+                        label: String::from("size"),
+                        message: String::from(
+                            "total size must be at least MIN_DISK_SIZE_BYTES",


This error would be okay if this was just an internal error, but since this is user-facing, it might be a little harder to decipher.

I went ahead and made #1171 ; this should make it possible to just:

Convert MIN_DISK_SIZE_BYTES to ByteCount

Display that directly

Once this is committed I'll make sure to implement. Thanks, Sean.

ryaeng · 2022-06-09T14:31:36Z

Sean, I changed the error to utilize ByteCount::from. Works as intended. I'll do the same for memory.

smklein

Thanks for the contribution! If the checks all pass, I'll merge

ryaeng · 2022-06-09T15:01:58Z

I'm also expanding the coverage of the tests that I've written. I'll push that shortly.

Changed disk granularity test to be a multiple of the block_size.

ryaeng · 2022-06-09T15:12:05Z

Done

ryaeng · 2022-06-09T15:12:16Z

Moving on to memory.

ryaeng · 2022-06-09T15:14:36Z

I'm also expanding the coverage of the tests that I've written. I'll push that shortly.

Glad I did this. Found that my disk granularity test wasn't working properly. Needed to change the disk size to be a multiple of block_size in order to hit the right error. Fixed it.

Crucible changes: Remove unused fields in IOop (#1149) New downstairs clone subcommand. (#1129) Simplify the do_work_task loop (#1150) Move `Guest` stuff into a module (#1125) Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite` work to a separate task (#1145) Use fewer borrows in ExtentInner API (#1147) Update Rust crate reedline to 0.28.0 (#1141) Update Rust crate tokio to 1.36 (#1143) Update Rust crate slog-bunyan to 2.5.0 (#1139) Update Rust crate rayon to 1.8.1 (#1138) Update Rust crate itertools to 0.12.1 (#1137) Update Rust crate byte-unit to 5.1.4 (#1136) Update Rust crate base64 to 0.21.7 (#1135) Update Rust crate async-trait to 0.1.77 (#1134) Discard deferred msgs (#1131) Minor Downstairs cleanup (#1127) Update test_fail_live_repair to support pstop (#1128) Ignore client messages after stopping the IO task (#1126) Move client IO task into a struct (#1124) Bump Rust to 1.75 and fix new Clippy lints (#1123) Propolis changes: PHD: convert to async (#633) PHD: assume specialized Windows images (#636) propolis-standalone-config needn't be a crate standalone: Use tar for snapshot/restore phd: use latest "lab-2.0-opte" target, not a specific version (#637) PHD: add tests for migration of running processes (#623) PHD: fix `cargo xtask phd` tidy not doing anything (#630) PHD: add documentation for `cargo xtask phd` (#629) standalone: improve virtual device creation errors (#632) phd: add Windows Server 2019 guest adapter (#627) PHD: add `cargo xtask phd` to make using PHD nicer (#619)

Crucible changes: Remove unused fields in IOop (#1149) New downstairs clone subcommand. (#1129) Simplify the do_work_task loop (#1150) Move `Guest` stuff into a module (#1125) Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite` work to a separate task (#1145) Use fewer borrows in ExtentInner API (#1147) Update Rust crate reedline to 0.28.0 (#1141) Update Rust crate tokio to 1.36 (#1143) Update Rust crate slog-bunyan to 2.5.0 (#1139) Update Rust crate rayon to 1.8.1 (#1138) Update Rust crate itertools to 0.12.1 (#1137) Update Rust crate byte-unit to 5.1.4 (#1136) Update Rust crate base64 to 0.21.7 (#1135) Update Rust crate async-trait to 0.1.77 (#1134) Discard deferred msgs (#1131) Minor Downstairs cleanup (#1127) Update test_fail_live_repair to support pstop (#1128) Ignore client messages after stopping the IO task (#1126) Move client IO task into a struct (#1124) Bump Rust to 1.75 and fix new Clippy lints (#1123) Propolis changes: PHD: convert to async (#633) PHD: assume specialized Windows images (#636) propolis-standalone-config needn't be a crate standalone: Use tar for snapshot/restore phd: use latest "lab-2.0-opte" target, not a specific version (#637) PHD: add tests for migration of running processes (#623) PHD: fix `cargo xtask phd` tidy not doing anything (#630) PHD: add documentation for `cargo xtask phd` (#629) standalone: improve virtual device creation errors (#632) phd: add Windows Server 2019 guest adapter (#627) PHD: add `cargo xtask phd` to make using PHD nicer (#619) Co-authored-by: Alan Hanson <alan@oxide.computer>

[API] Bounds-check that disks are a minimum size, and use a minimum g…

9be2569

…ranularity size (oxidecomputer#1029) - Requires the user to select a size of at least one gibibyte when creating a new disk. - Add a test which attempts to create a disk that is 512 mebibytes in size.

smklein mentioned this pull request Jun 7, 2022

[API] Bounds-check that memory for instance is above a minimum, meets a granularity (#1030) #1157

Merged

smklein self-requested a review June 7, 2022 13:43

smklein reviewed Jun 7, 2022

View reviewed changes

Add MIN_DISK_SIZE parameter

8d2eb60

- Add MIN_DISK_SIZE constant. - Modify tests to create disks with a size of MIN_DISK_SIZE / 2.

Add disk size granularity

7f3b58e

The disk size must be divisible by 1 GiB.

ryaeng force-pushed the disk-size-minimum branch from ab548e8 to 7f3b58e Compare June 7, 2022 18:31

ryaeng added 2 commits June 7, 2022 14:39

Append _BYTES to MIN_DISK_SIZE

5948834

Add disk minimum & granularity to GlobalImage case for project_create…

b4ecb7b

…_disk

zephraph reviewed Jun 7, 2022

View reviewed changes

Change disk size errors to read MIN_DISK_SIZE_BYTES

cbb0070

instead of 1 GiB

smklein reviewed Jun 8, 2022

View reviewed changes

ryaeng added 2 commits June 9, 2022 09:41

Merge branch 'main' into disk-size-minimum

4c26cd5

Change MIN_DISK_SIZE error to utilize ByteCount::from

2e48381

smklein approved these changes Jun 9, 2022

View reviewed changes

Expand min_disk_size test coverage to include error's returned.

10fd3be

Changed disk granularity test to be a multiple of the block_size.

smklein merged commit a05fa49 into oxidecomputer:main Jun 9, 2022

smklein mentioned this pull request Jun 9, 2022

[API] Bounds-check that disks are a minimum size, and use a minimum granularity size #1029

Closed

ryaeng deleted the disk-size-minimum branch June 9, 2022 17:42

leftwo mentioned this pull request Feb 9, 2024

Update Crucible and Propolis #5039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) #1150

[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) #1150

ryaeng commented Jun 3, 2022

smklein commented Jun 7, 2022

smklein Jun 7, 2022 •

edited

Loading

ryaeng Jun 7, 2022

smklein Jun 7, 2022

ryaeng Jun 7, 2022

ryaeng commented Jun 7, 2022

smklein commented Jun 7, 2022

ryaeng commented Jun 7, 2022

smklein commented Jun 7, 2022

ryaeng commented Jun 7, 2022

zephraph Jun 7, 2022

ryaeng Jun 7, 2022

smklein Jun 8, 2022

ryaeng Jun 8, 2022

ryaeng commented Jun 9, 2022

smklein left a comment

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) #1150

[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) #1150

Conversation

ryaeng commented Jun 3, 2022

smklein commented Jun 7, 2022

smklein Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

ryaeng Jun 7, 2022

Choose a reason for hiding this comment

smklein Jun 7, 2022

Choose a reason for hiding this comment

ryaeng Jun 7, 2022

Choose a reason for hiding this comment

ryaeng commented Jun 7, 2022

smklein commented Jun 7, 2022

ryaeng commented Jun 7, 2022

smklein commented Jun 7, 2022

ryaeng commented Jun 7, 2022

zephraph Jun 7, 2022

Choose a reason for hiding this comment

ryaeng Jun 7, 2022

Choose a reason for hiding this comment

smklein Jun 8, 2022

Choose a reason for hiding this comment

ryaeng Jun 8, 2022

Choose a reason for hiding this comment

ryaeng commented Jun 9, 2022

smklein left a comment

Choose a reason for hiding this comment

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

ryaeng commented Jun 9, 2022

smklein Jun 7, 2022 •

edited

Loading