[nexus] improve abandoned_vmm_reaper OMDB status
#6541
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While I was working on #6503, I became quite taken with the way @jmpesp had done the OMDB status for his region-replacement background tasks --- the approach of defining a struct in `nexus-types` representing the status JSON that both the background task implementation and OMDB could depend on seems much nicer than constructing an untyped JSON message and then deserializing it to a type that only exists in OMDB, because this way, OMDB and the background task implementation are kept in sync automatically. So, I thought it would be nice to also rework the `abandoned_vmm_reaper` task to use this style. This commit does that. While I was making this change, I also changed the status JSON to include a list of all errors that occurred during the task's activation, instead of just the last one, which will probably make it easier to debug any issues that come up. I also added some code for aligning the numeric columns in the OMDB output.
hawkw
force-pushed
the
eliza/omdb-nice
branch
from
0ed4b6f to
b7c1585
Compare
hawkw
added a commit
that referenced
this pull request
Sep 7, 2024
Depends on #6541. Similarly to #6541, this commit refactors the `instance_updater` background task's OMDB status implementation to use a shared Rust struct in `nexus-types` to represent the status JSON object. I also changed the status message to include a full list of errors that occurred, instead of just the most recent one and a count, and I made some tweaks to the output alignment in OMDB.
|
Huh, this is weird, it looks like something was unexpectedly cancelled whilst running the "OMDB success cases" test: https://buildomat.eng.oxide.computer/wg/0/details/01J76SDJMRSAKXDSCDB937HA7W/HHiBDL096VXGcptoEmeZYVs0fOREFldEtGAm1fVPnfzCKYF8/01J76SDWEK961ZM8FDZSAAQK5T#S5356 That feels sketchy, because...I did change the OMDB output, but that test failure doesn't look related. Gonna rerun it and see what happens... |
jmpesp
approved these changes
Sep 9, 2024
hawkw
added a commit
that referenced
this pull request
Sep 11, 2024
Depends on #6541. Similarly to #6541, this commit refactors the `instance_updater` background task's OMDB status implementation to use a shared Rust struct in `nexus-types` to represent the status JSON object. I also changed the status message to include a full list of errors that occurred, instead of just the most recent one and a count, and I made some tweaks to the output alignment in OMDB.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While I was working on #6503, I became quite taken with the way @jmpesp
had done the OMDB status for his region-replacement background tasks ---
the approach of defining a struct in
nexus-typesrepresenting thestatus JSON that both the background task implementation and OMDB could
depend on seems much nicer than constructing an untyped JSON message and
then deserializing it to a type that only exists in OMDB, because this
way, OMDB and the background task implementation are kept in sync
automatically. So, I thought it would be nice to also rework the
abandoned_vmm_reapertask to use this style.This commit does that. While I was making this change, I also changed
the status JSON to include a list of all errors that occurred during the
task's activation, instead of just the last one, which will probably
make it easier to debug any issues that come up. I also added some code
for aligning the numeric columns in the OMDB output.
I also changed the pagination of the database query to just use
the existing
SQL_BATCH_SIZEconstant.