Add cosmo ignition support by labbott · Pull Request #9076 · oxidecomputer/omicron

labbott · 2025-09-24T18:55:52Z

No description provided.

karencfv

Looks good! Just a few nits from my part. I'm not 100% sure on the API versioning format though since this is all new. Could be good to get a review from @iliana here :)

karencfv · 2025-09-25T02:28:18Z

+    pub details: SpIgnition,
+}
+
+/// Ignition command.


Maybe a little more information for this one? 😄

Hah you are finding all the places that were not as well documented

karencfv · 2025-09-25T02:30:26Z

+
+/// State of an ignition target.
+///
+/// TODO: Ignition returns much more information than we're reporting here: do


What could this additional information be used for?

Mostly debugging purposes I think? This is also a pretty old comment at this point.

karencfv · 2025-09-25T02:35:22Z

+        ctrl_detect_0: bool,
+        ctrl_detect_1: bool,
+        flt_a3: bool,
+        flt_a2: bool,
+        flt_rot: bool,
+        flt_sp: bool,


Not sure if this makes sense or not, please ignore this comment if it doesn't. It could be nice to have the fields be more descriptive. For those of us without a background in firmware eng they aren't super obvious. How about something like faults_power_a3, controller_detect_0 etc?

Changing field names is pretty intrusive, I'll see about adding some more comments

karencfv · 2025-09-25T02:41:25Z

+        let handler = async {
+            let out = mgmt_switch
+                .bulk_ignition_state()
+                .await?
+                .map(|(id, state)| ignition::v2::SpIgnitionInfo {
+                    id: id.into(),
+                    details: state.into(),
+                })
+                .collect();
+
+            Ok(HttpResponseOk(out))
+        };


I'm assuming that we'll never have enough items to warrant pagination here?

Interesting question! At least in MGS it does look to possibly allow it https://github.com/oxidecomputer/management-gateway-service/blob/91c9d622027c39f5acdd17ae3dab046330bf6717/gateway-messages/src/sp_impl.rs#L765-L776 but I'm guessing the amount of data returned right now is small enough we just haven't bothered

karencfv

Thanks for addressing my comments! Looks like it's just missing a cargo xtask openapi generate.

If possible, it'd be good to get a review from someone more acquainted with the new API versioning system

jgallagher · 2025-09-26T15:47:48Z

+    }]
+    async fn ignition_command_v2(
+        rqctx: RequestContext<Self::Context>,
+        path: Path<ignition::v2::PathSpIgnitionCommand>,


Why do we need a v2::PathSpIgnitionCommand? I tried comparing v1 and v2, but I couldn't tell what the difference was.

Double checked this, PathSpIgnitionCommand uses IgnitionCommand which is part of the versioned ignition types. Versioning PathSpIgnitionCommand seemed like the least ugly path forward

John explained this to me a little bit more, I'm going to make this not be versioned

jgallagher · 2025-09-26T15:54:25Z

+    #[endpoint {
+        method = GET,
+        path = "/ignition",
+        operation_id = "ignition_list",
+        versions = VERSION_COSMO..
+    }]
+    async fn ignition_list_v2(
+        rqctx: RequestContext<Self::Context>,
+    ) -> Result<HttpResponseOk<Vec<ignition::v2::SpIgnitionInfo>>, HttpError>;


This is a pretty different stylistic choice than @iliana made when revving an API for sled agent on #8983. I think I prefer that style, but maybe it's worth discussing? In that change, the new endpoint and its associated types don't have any versioning in the name or types, which makes it clear (IMO) that they're "the current version". So this would be

Suggested change

#[endpoint {

method = GET,

path = "/ignition",

operation_id = "ignition_list",

versions = VERSION_COSMO..

}]

async fn ignition_list_v2(

rqctx: RequestContext<Self::Context>,

) -> Result<HttpResponseOk<Vec<ignition::v2::SpIgnitionInfo>>, HttpError>;

#[endpoint {

method = GET,

path = "/ignition",

versions = VERSION_COSMO..

}]

async fn ignition_list(

rqctx: RequestContext<Self::Context>,

) -> Result<HttpResponseOk<Vec<SpIgnitionInfo>>, HttpError>;

and only the old version would have v1 in its name and return type.

I'm fine making the style change, I was initially matching https://github.com/oxidecomputer/omicron/pull/8047 which was my initial reference for doing API versioning

jgallagher · 2025-09-29T15:32:21Z

+                    err,
+                })?;
+
+            let info = ignition::v1::SpIgnitionInfo {


I think this impl (and the ignition_list_v1) are fine as-is, but I'm wondering if they could be written in terms of the v2 API? Something like

async fn ignition_get_v1( rqctx: RequestContext<Self::Context>, path: Path<PathSp>, ) -> Result<HttpResponseOk<ignition::v1::SpIgnitionInfo>, HttpError> { let HttpResponseOk(v2_info) = Self::ignition_get(rqctx, path)?; Ok(HttpResponseOk(ignition::v1::SpIgnitionInfo::from(v2_info))) }

to avoid having to duplicate the guts of talking to the ignition controller, mapping errors, etc.

jgallagher · 2025-09-29T15:34:55Z

 #
-gateway-ereport-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "77e316c812aa057b9714d0d99c4a7bdd36d45be2", default-features = false, features = ["debug-impls"] }
-gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "77e316c812aa057b9714d0d99c4a7bdd36d45be2", default-features = false, features = ["std"] }
-gateway-sp-comms = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "77e316c812aa057b9714d0d99c4a7bdd36d45be2" }


We should also bump this hash in package-manifest.toml where pull in package.omicron-faux-mgs. (Always shipping an up-to-date faux-mgs is not critical, but it's nice to have.)

labbott changed the title ~~Add cosmo to ignition~~ Add cosmo ignition support Sep 24, 2025

labbott force-pushed the cosmo_ignition branch from f0808c1 to 41cf877 Compare September 24, 2025 19:02

karencfv reviewed Sep 25, 2025

View reviewed changes

labbott force-pushed the cosmo_ignition branch from 41cf877 to 8d141c2 Compare September 25, 2025 12:28

karencfv approved these changes Sep 25, 2025

View reviewed changes

jgallagher reviewed Sep 26, 2025

View reviewed changes

labbott force-pushed the cosmo_ignition branch from 518df6f to c3b734e Compare September 26, 2025 17:29

jgallagher approved these changes Sep 29, 2025

View reviewed changes

labbott force-pushed the cosmo_ignition branch from f84af39 to 68e9f03 Compare September 29, 2025 17:59

labbott added 3 commits September 29, 2025 23:11

Add cosmo ignition support

4667e49

final fixes/cleanup

68e9f03

fix that

bdfb1b4

labbott merged commit c6628ce into main Sep 30, 2025
19 checks passed

labbott deleted the cosmo_ignition branch September 30, 2025 14:17

jgallagher mentioned this pull request Nov 3, 2025

Guest external IP configuration needs to be dual-stack #9318

Closed

Uh oh!

Conversation

labbott commented Sep 24, 2025

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants