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

Add a producer kind to oximeter metric producers #4497

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

bnaecker
Copy link
Collaborator

@bnaecker bnaecker commented Nov 14, 2023

  • Adds the kind enum to metric producer information, including DB schema, model, and various client parameter types. This records the supported types of metric producers, and is intended to aid debugging and future work around updates and instance lifecycle management.
  • Add schema update files which create the DB enum type and add it as a column to the metric_producer table. This currently drops the existing table and recreates it with the new column, rather than adding the column using ALTER TABLE. That is intended to remove old entries in bulk, since nothing previously removed the records for Propolis servers when their instance was stopped.

This is the initial PR in a sequence that will eventually make this field required in both the database and API requests. As there are consumers of this API outside of the Omicron repository, this field needs to start as optional, to avoid introducing a commit with incompatible clients. Here are the planned steps:

  • This PR introduces an optional kind field into the metric_producer table and ProducerEndpoint registration request body
  • A PR in Crucible which includes the optional field in its API requests
  • A PR in Dendrite which includes the optional field in its API requests
  • A PR in Propolis which includes the optional field in its API requests
  • A PR in Omicron which makes the database and API request fields required, and points to the new versions of the three previous dependencies
  • A PR in Dendrite, Crucible, and Propolis which adapts to the new required field from the previous step
  • A final PR in Omicron which points to the new versions of the three previous dependencies

@bnaecker bnaecker marked this pull request as draft November 14, 2023 22:47
@bnaecker bnaecker force-pushed the add-oximeter-producer-kind branch from 57dc5b4 to 10ce1f2 Compare November 14, 2023 23:51
@bnaecker bnaecker marked this pull request as ready for review November 14, 2023 23:52
@bnaecker bnaecker requested a review from davepacheco November 14, 2023 23:52
common/src/api/internal/nexus.rs Show resolved Hide resolved
common/src/api/internal/nexus.rs Outdated Show resolved Hide resolved
schema/crdb/11.0.0/up01.sql Outdated Show resolved Hide resolved
schema/crdb/11.0.0/up01.sql Outdated Show resolved Hide resolved
schema/crdb/dbinit.sql Show resolved Hide resolved
@bnaecker bnaecker force-pushed the add-oximeter-producer-kind branch from 0fc13f8 to e7c9283 Compare November 15, 2023 21:10
@bnaecker
Copy link
Collaborator Author

This whole dance requires a sequence of updates, laid out in the PR description. There are currently a few other revisions that need to be orchestrated between Crucible, Propolis, and Omicron, so I'm going to wait until those are through the pipeline before starting on this sequence.

@bnaecker
Copy link
Collaborator Author

Friendly ping on this @davepacheco, I'd like to tie this off before the holiday, during a quiet few days, if possible.

- Adds the `kind` enum to metric producer information, including DB
  schema, model, and various client parameter types. This records the
  supported types of metric producers, and is intended to aid debugging
  and future work around updates and instance lifecycle management. Note
  that this is currently a nullable / optional value, to aid schema
  upgrades with other clients outside the repo. A follow-up commit will
  make this required in the API call and `NOT NULL` in the database.
- Add schema update files which create the DB enum type and add it as a
  column to the `metric_producer` table. This currently _drops_ the
  existing table and recreates it with the new column, rather than
  adding the column using `ALTER TABLE`. That is intended to remove old
  entries in bulk, since nothing previously removed the records for
  Propolis servers when their instance was stopped.

Review feedback

Fixup OpenAPI specs

Rebase fixup

Move schema update files to next version
@bnaecker bnaecker force-pushed the add-oximeter-producer-kind branch from af5ec52 to f73e352 Compare November 20, 2023 17:56
@davepacheco
Copy link
Collaborator

I think the change is fine and I'm just trying to work through the sequence of PRs to make sure we aren't going to accidentally break something. Sorry to be tedious about it but every time I don't spell it out like this I get it wrong and break stuff.

  1. This PR to omicron repo: add an optional field to the Nexus internal API and update any consumers in this repo. Other repos with consumers of this API (Crucible, Propolis, and Dendrite) continue to build using the older API version and client. So builds all work. If you deploy the TUF repo built from this version, you'll get a Nexus that accepts the optional parameter and the out-of-repo components (Crucible, Propolis, Dendrite) will not be providing it.
  2. PR to propolis that updates the Nexus client to the newer version and provides the new parameter as Some(some_kind). Nothing changes in the build or deployment of any other repo. If you rebuild the full TUF repo at this point, there's no change from the previous state since omicron is still pointing at the previous Propolis.
  3. PR to dendrite that does the same, with similar results. Again, at this point, if you rebuilt and deployed the TUF repo, you'd be getting a Nexus that accepts the optional parameter and components that do not provide it.
  4. PR to crucible that does the same. etc.
  5. PR to omicron repo to make the field required, both in the API and the database schema. At this point, all repo builds continue to work. Crucible, Propolis, and Dendrite are still using a Nexus client where the parameter is optional. If you deploy at this point, you're deploying a Nexus that requires the parameter and a bunch of components that provide it thinking it's optional. (Which is fine! This works because providing an optional parameter and a required parameter looks the same on the wire.)
  6. PR to propolis that updates the Nexus client to the newest version and provides the new parameter as some_kind (compare with Some(some_kind) above). Nothing changes in the build or deployment of any other repo. If you rebiuld the full TUF repo at this point, there's no change from the previous state since omicron is still pointing at the previous Propolis.
  7. PR to dendrite that does the same, with similar results. Again, at this point, if you rebuilt and deployed the TUF repo, you'd be getting a Nexus that requires the parameter and components that do provide it, albeit believing that it's optional.
  8. PR to crucible that does the same. etc.
  9. PR to omicron once again that updates the versions of propolis, dendrite, and crucible that it pulls down. At this point if you build and deploy the TUF repo, you'll get components that are all aligned on the fact that this parameter is required.

By contrast, I asked offline what would happen if we skipped the "optional" phase altogether. If we did, then in step 1, if you built and deployed the TUF repo, you'd get a Nexus that requires the parameter and a Propolis that doesn't provide it and that would not work.

@bnaecker bnaecker enabled auto-merge (squash) November 20, 2023 22:14
@bnaecker bnaecker disabled auto-merge November 20, 2023 22:14
@bnaecker bnaecker enabled auto-merge (squash) November 20, 2023 22:14
@bnaecker bnaecker merged commit 08041d6 into main Nov 20, 2023
20 of 21 checks passed
@bnaecker bnaecker deleted the add-oximeter-producer-kind branch November 20, 2023 23:27
@bnaecker
Copy link
Collaborator Author

Update: Steps 1-4 have been completed. Omicron has support for the optional parameter, as well as the 3 out-of-tree consumers. I haven't yet pointed Omicron at those updated commits.

@bnaecker
Copy link
Collaborator Author

bnaecker commented Nov 29, 2023

#4571 is up for review now, which takes this through step 5.

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