Skip to content

client: re-export instance spec types for use in sled-agent #899

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
merged 5 commits into from
Apr 28, 2025

Conversation

gjcolombo
Copy link
Contributor

Follow-up to #896. Using generated instance spec types in sled-agent's API has some substantial downsides:

  1. Some component types (e.g. storage backends) have fields that we'd like to redact when logging their contents using their Debug impls. The native versions of these types manually impl Debug for these types, but their generated counterparts always derive Debug (and AFAIK there's no way to ask Progenitor not to add this derive).
  2. The Dropshot-generated OpenAPI doc for sled-agent's API winds up having very gnarly type descriptions that include fully-escaped versions of the Progenitor-generated doc comments from propolis-client.1

To get around these problems:

  • Add a propolis-client submodule that re-exports all instance spec-related types from propolis_api_types.
  • Add replace directives to the generated client that replace the generated versions of InstanceSpecV0, VersionedInstanceSpec, and ReplacementComponent with their re-exported equivalents.

This way, sled-agent can use the re-exported spec types internally and in its API. This reuses their manual impls when they exist (solving problem 1) and avoids presenting generated JSON schema documentation when generating the sled-agent API document (solving problem 2).

Remove the JsonSchema implementations for generated spec types, since they're no longer needed.

Finally, fix a one-line clippy nit that started showing up alongside these changes.

Tests: cargo test and PHD; picked up this change into oxidecomputer/omicron#8002 and verified that (a) that change still works (and that it produces a much cleaner sled-agent API document), and (b) log messages from sled-agent redact spec components that we want to be redacted.

Footnotes

  1. Concretely: suppose you have type foo::Widget with a doc comment, and you have a foo_server crate with a Dropshot API that makes use of this type. If you generate an OpenAPI document from this API, and then use Progenitor to generate a client from that document, foo_client::types::Widget will have a doc comment that includes both the original foo::Widget doc comment and a Progenitor-generated description of the type's JSON schema. This is all well and good and what we expect. Now suppose you then create crate bar_server and include foo_client::types::Widget in its Dropshot API. The description of Widget in bar_server's API document will contain both the original foo::Widget description and the generated JSON schema text from foo_client::types::Widget in the type's "description" field. And if you then generate bar_client::types::Widget, the schema will be generated again and re-included in the generated type's doc comment. Needless to say: yuck.

@gjcolombo
Copy link
Contributor Author

An explanation of ffeb813: libc defines the F_WRLCK flag as an i16 on solarish OSes but defines it as an i32 for Linux. For some reason, starting with this change, my local rust-analyzer-initiated Clippy runs started complaining about this seemingly-unnecessary cast. The mystery here (from my perspective) is not so much why the cast is there (you definitely need it on Linux!) as why my local dev environment started screeching about it today.

In any case, ffeb813 fixes the Linux build; I'll figure out what's going on with my dev environment separately. (I also see this on the latest master, fwiw.)

@iximeow iximeow self-requested a review April 24, 2025 20:21
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This approach makes sense to me, no major concerns!

}
}

impl From<&str> for SpecKey {
Copy link
Member

Choose a reason for hiding this comment

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

nice :)

Comment on lines +91 to +93
.into_backend_name()
.into_string()
.into();
Copy link
Member

Choose a reason for hiding this comment

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

wow... 😵‍💫

Co-authored-by: Eliza Weisman <[email protected]>
@gjcolombo gjcolombo merged commit 8a414e9 into master Apr 28, 2025
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/re-export-instance-spec-types branch April 28, 2025 17:09
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.

2 participants