Implement global images #915

jmpesp · 2022-04-14T18:39:20Z

Fill in image creation code, adding a few fields to the image from RFD
4 (version and digest specifically). Also add block_size due to the
constraint that block_size must be present during any volume creation.

Store volume construction requests created from the parameters in
global_image_create. This commit does not implement global_image_delete
because that involves much work to figure out how to safely do this (are
any disks using it? are other volumes using this as a sub volume?).

Both "Image" and "GlobalImage" are required to separate authz policy, so
this commit separates the two. Project images will be implemented in
another PR.

Implement authz for global images. GlobalImagesList is a dummy authz
resource that was added to have a place to check list_children
permissions for.

Fill in image creation code, adding a few fields to the image from RFD 4 (version and digest specifically). Also add block_size due to the constraint that block_size must be present during any volume creation. Store volume construction requests created from the parameters in global_image_create. This commit does not implement global_image_delete because that involves much work to figure out how to safely do this (are any disks using it? are other volumes using this as a sub volume?). Both "Image" and "GlobalImage" are required to separate authz policy, so this commit separates the two. Project images will be implemented in another PR. Implement authz for global images. GlobalImagesList is a dummy authz resource that was added to have a place to check list_children permissions for.

jmpesp · 2022-04-14T18:40:47Z

@davepacheco one unresolved part is that GlobalImage and GlobalImageList are both children of Fleet: https://github.com/oxidecomputer/omicron/pull/915/files#diff-f5f8df6872a0c2cc4cbc022eb186649c66c8aee85c51329b639736a82424d305R2776

I couldn't make Fleet -> GlobalImageList -> GlobalImage work, would appreciate a look

nexus/src/authz/api_resources.rs

common/src/api/external/mod.rs

nexus/src/db/datastore.rs

common/src/sql/dbinit.sql

nexus/src/authz/api_resources.rs

davepacheco · 2022-04-15T16:13:05Z

nexus/src/nexus.rs

+            }
+
+            params::ImageSource::Snapshot(_id) => {
+                unimplemented!()


How about returning a 500 here instead of panicking? (You could even use unimplemented_todo, but it's not necessary.)

opted for 503 in 3b88391

I think using 503 for this case makes it harder for us to identify operational issues (usually 503s) vs. bugs (usually 500s) in the field...but hopefully this will never ship so it won't matter!

nexus/src/sagas.rs

davepacheco · 2022-04-15T16:14:53Z

nexus/src/sagas.rs

+            .global_image_fetch(&opctx, image_id)
+            .await
+            .map_err(|e| {
+                ActionError::action_failed(Error::internal_error(


Why is this and the later one an internal error instead of directly using e?

I get

the trait `From<omicron_common::api::external::Error>` is not implemented for `ActionError`

Sorry, I meant .map_err(ActionError::action_failed)

done in ef02727

Thanks. There's another one further down I think.

nexus/src/sagas.rs

davepacheco · 2022-04-15T16:18:55Z

@davepacheco one unresolved part is that GlobalImage and GlobalImageList are both children of Fleet: https://github.com/oxidecomputer/omicron/pull/915/files#diff-f5f8df6872a0c2cc4cbc022eb186649c66c8aee85c51329b639736a82424d305R2776

I couldn't make Fleet -> GlobalImageList -> GlobalImage work, would appreciate a look

I did the same thing with ConsoleSession. I don't think I thought too hard about it. I can help dig in next week if you'd like to resolve it but I don't think it's a big deal either way.

davepacheco · 2022-04-15T16:19:46Z

Very nice! I had several suggestions above but none are critical.

smklein

Looks awesome, only a couple minor comments.

common/src/sql/dbinit.sql

nexus/src/external_api/views.rs

nexus/src/sagas.rs

nexus/tests/integration_tests/images.rs

smklein · 2022-04-18T13:36:42Z

nexus/tests/integration_tests/unauthorized.rs

@@ -54,6 +55,24 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) {
    let client = &cptestctx.external_client;
    let log = &cptestctx.logctx.log;

+    // Run a httptest server


To make this is more explicit, we do this for:

Clickhouse (bind to port zero):

omicron/nexus/test-utils/src/lib.rs

Line 99 in 3817f7e

let clickhouse = dev::clickhouse::ClickHouseInstance::new(0).await.unwrap();

Nexus (use ConfigDropshot's default, which is 127.0.0.1:0):

omicron/nexus/test-utils/src/lib.rs

Lines 105 to 107 in 3817f7e

let server = omicron_nexus::Server::start(&config, rack_id, &logctx.log)

.await

.unwrap();

omicron/nexus/src/config.rs

Lines 57 to 60 in 3817f7e

/// Dropshot configuration for external API server

pub dropshot_external: ConfigDropshot,

/// Dropshot configuration for internal API server

pub dropshot_internal: ConfigDropshot,

https://docs.rs/dropshot/0.6.0/src/dropshot/config.rs.html#54-61

Etc.

Dropshot's local_addr() method makes it easy to access the "real" port that was allocated after the fact:

omicron/nexus/test-utils/src/lib.rs

Line 131 in 3817f7e

server.http_server_internal.local_addr(),

davepacheco

Did you decide not to do something similar with this mock that we do with CockroachDB and others?

davepacheco · 2022-04-20T22:10:05Z

common/src/sql/dbinit.sql

@@ -442,11 +442,8 @@ CREATE TABLE omicron.public.image (
    project_id UUID NOT NULL,
    volume_id UUID NOT NULL,

-    /* Optional URL: Images may be backed by either a URL or a volume */
    url STRING(8192),


I see you removed the "optional" comments here but the fields are still optional. Is that the intent or are they supposed to be NOT NULL?

They are supposed to be optional

davepacheco · 2022-04-20T22:11:23Z

nexus/src/authz/api_resources.rs

-    fleet: Fleet,
+/// ConsoleSessionList is a synthetic resource used for modeling who has access
+/// to create sessions.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]


It seems like something about the way this was merged caused this stuff to show up as part of your change. That's weird.

davepacheco · 2022-04-20T22:18:40Z

nexus/src/sagas.rs

+            Some(Box::new(serde_json::from_str(volume.data()).map_err(
+                |e| {
+                    ActionError::action_failed(Error::internal_error(
+                        &e.to_string(),


maybe want a little more context in this message? ("failed to deserialize volume data"?)

👍 added in 4752359

davepacheco · 2022-04-20T22:20:05Z

nexus/src/sagas.rs

+        params::DiskSource::Blank { block_size } => {
+            db::model::BlockSize::try_from(*block_size).map_err(|e| {
+                ActionError::action_failed(Error::internal_error(
+                    &e.to_string(),


maybe want a little more context in this message? I'm not sure what should go here -- I'm just imagining we see this error in the log. We'll be able to tell it came from this saga action, at least.

we'll at least see invalid block size {} from the db::model::BlockSize try_from, though I'm not sure how we would ever hit this error because params::BlockSize try_from also validates that it's one of the (currently) three supported values?

Crucible: update rust crate base64 to 0.21.3 (#913) update rust crate slog-async to 2.8 (#915) update rust crate async-recursion to 1.0.5 (#912) Move active jobs into a separate data structure and optimize `ackable_work` (#908) Check repair IDs correctly (#910) update actions/checkout action to v4 (#903) update rust crate tokio to 1.32 (#890) Remove single-item Vecs (#898) Move "extent under repair" into a helper function (#907) offset_mod shouldn't be randomized (#905) Only rehash if a write may have failed (#899) Make negotiation state an enum (#901) Test update for fast write ack and gather errors on test failure (#897) Propolis: Update cpuid-gen util for better coverage Make storage backend config more flexible and consistent Use correct register sizes for PIIX3 PM device Update bitflags dependency fix softnpu port order (#517) Use hex formatting for unhandled MMIO/PIO/MSRs Update deps for new crucible and oximeter Update standalone-with-crucible docs (#514)

Crucible: update rust crate base64 to 0.21.3 (#913) update rust crate slog-async to 2.8 (#915) update rust crate async-recursion to 1.0.5 (#912) Move active jobs into a separate data structure and optimize `ackable_work` (#908) Check repair IDs correctly (#910) update actions/checkout action to v4 (#903) update rust crate tokio to 1.32 (#890) Remove single-item Vecs (#898) Move "extent under repair" into a helper function (#907) offset_mod shouldn't be randomized (#905) Only rehash if a write may have failed (#899) Make negotiation state an enum (#901) Test update for fast write ack and gather errors on test failure (#897) Propolis: Update cpuid-gen util for better coverage Make storage backend config more flexible and consistent Use correct register sizes for PIIX3 PM device Update bitflags dependency fix softnpu port order (#517) Use hex formatting for unhandled MMIO/PIO/MSRs Update deps for new crucible and oximeter Update standalone-with-crucible docs (#514) Co-authored-by: Alan Hanson <[email protected]>

jmpesp requested review from davepacheco and smklein April 14, 2022 18:39

different port for github actions

c9900f8

david-crespo mentioned this pull request Apr 15, 2022

Global images page oxidecomputer/console#794

Closed

davepacheco reviewed Apr 15, 2022

View reviewed changes

jmpesp added 11 commits April 15, 2022 15:34

Merge remote-tracking branch 'upstream/main' into create_some_images

0098275

remove GlobalImageList from ResourceType

6aa953e

don't store fleet, it's a singleton

8341194

remove global_image_fetch

17042f4

GlobalImage not custom, but FleetChild

a52de65

separate project and global image views

bd825de

return 503 instead of unimplemented

3b88391

warn to debug

5acc551

change Into into From

5b35c58

Merge remote-tracking branch 'upstream/main' into create_some_images

6cb46a6

use only ActionError::action_failed

ef02727

smklein reviewed Apr 18, 2022

View reviewed changes

jmpesp added 7 commits April 20, 2022 10:40

remove non-useful comments

68879fa

remove project image comment reference to global image

e4007d4

create DiskSource enum for DiskCreate, make creation type safe

0477683

fmt

7d62b0b

Merge remote-tracking branch 'upstream/main' into create_some_images

639eba1

clippy

3b516c4

tag = "type" is the best

6b0a10f

smklein approved these changes Apr 20, 2022

View reviewed changes

davepacheco approved these changes Apr 20, 2022

View reviewed changes

more error context

4752359

jmpesp merged commit f9bc90e into oxidecomputer:main Apr 21, 2022

jmpesp deleted the create_some_images branch April 21, 2022 12:54

smklein mentioned this pull request Apr 24, 2022

Implement "Images" API #734

Closed

5 tasks

leftwo mentioned this pull request Sep 11, 2023

Update Crucible and Propolis to latest #4079

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement global images #915

Implement global images #915

jmpesp commented Apr 14, 2022

jmpesp commented Apr 14, 2022

davepacheco Apr 15, 2022

jmpesp Apr 15, 2022

davepacheco Apr 20, 2022

davepacheco Apr 15, 2022

jmpesp Apr 15, 2022

davepacheco Apr 15, 2022

jmpesp Apr 15, 2022

davepacheco Apr 15, 2022

davepacheco commented Apr 15, 2022

davepacheco commented Apr 15, 2022

smklein left a comment

smklein Apr 18, 2022

davepacheco left a comment

davepacheco Apr 20, 2022

jmpesp Apr 21, 2022

davepacheco Apr 20, 2022

davepacheco Apr 20, 2022

jmpesp Apr 21, 2022

davepacheco Apr 20, 2022

jmpesp Apr 21, 2022

	let server = omicron_nexus::Server::start(&config, rack_id, &logctx.log)
	.await
	.unwrap();

	/// Dropshot configuration for external API server
	pub dropshot_external: ConfigDropshot,
	/// Dropshot configuration for internal API server
	pub dropshot_internal: ConfigDropshot,

Implement global images #915

Implement global images #915

Conversation

jmpesp commented Apr 14, 2022

jmpesp commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Apr 15, 2022

davepacheco commented Apr 15, 2022

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment