Add system-version to omdb db #10481
Conversation
…his version update was requested
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
This is great! Thanks for working on this command.
| /// Print the current target system release and the update date (print all of the releases when `--all` is present) | ||
| SystemVersion(system_version::SystemVersionArgs), |
There was a problem hiding this comment.
Not all components may be at the target release version at any given time. Having a "system-version" command could be confusing. It's probably more accurate to call it "target-release" :)
There was a problem hiding this comment.
Thanks so much for the review, appreciate it :) I've changed it to target-release.
| Sleds(SledsArgs), | ||
| /// Show instances grouped by the sled they are running on | ||
| SledInstances(SledInstancesArgs), | ||
| /// Print the current target system release and the update date (print all of the releases when `--all` is present) |
There was a problem hiding this comment.
We typically don't mention command flags in the doc string.
|
Tested the generated binary on dogfood: omdb db target-releaseomdb db target-release --all |
…ga and omdb nexus blueprints list
|
Tested latest commit (dd347a4) on dogfood. Newest record at the bottom. omdb db target-release --allomdb db target-release |
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Thanks for addressing my comment! I took a closer look and have some questions. Thanks again for adding this command!
| //! Shows the current target release and date when update was requested. | ||
| //! When `--all` option is used, lists all target release records. |
There was a problem hiding this comment.
Taking a closer look, we may want to change the functionality of the command a bit. Right now we only have two options right? All or one. What if the user wants to see just the latest 5?
In this scenario, it's probably a simpler user experience to have a command that always shows a list of target releases with a limit of records shown. Something like omdb target-release list or omdb target-releases with an optional fetch-limit or --limit flag. This way a user can say I want to see the latest one, or the last 10, or 100 or whatever.
We already have this implemented for several endpoints (DbFetchOptions), see cmd_db_blueprints, cmd_db_physical_disks etc. If DbFetchOptions doesn't make sense for this command, you can also look at how cmd_reconfigurator_config_history implements a --limit flag.
There was a problem hiding this comment.
We already have a --fetch-limit flag on omdb db that applies to any subcommand that respects it (which this one does AFAICT, but it would be good to test / confirm).
There was a problem hiding this comment.
I re-added the DbFetchOptions to respect this.
root@oxz_switch0:/tmp# ./omdb-em db --fetch-limit 8 target-release --all
note: database URL not specified. Will search DNS.
note: (override with --db-url or OMDB_DB_URL)
note: using DNS from system config (typically /etc/resolv.conf)
note: (if this is not right, use --dns-server to specify an alternate DNS server)
note: using database URL postgresql://root@[fd00:1122:3344:109::3]:32221,[fd00:1122:3344:105::3]:32221,[fd00:1122:3344:10b::3]:32221,[fd00:1122:3344:107::3]:32221,[fd00:1122:3344:108::3]:32221/omicron?sslmode=disable
WARN: found schema version 257.0.0, expected 261.0.0
It's possible the database is running a version that's different from what this
tool understands. This may result in errors or incorrect output.
WARN: listing target releases: found 8 items (the limit). There may be more items that were ignored. Consider overriding with --fetch-limit.
TARGET_RELEASE TIME_REQUESTED
19.0.0-0.ci+git42726ecbb1b 2026-04-08T00:25:34.431Z
19.1.0-0.ci+git073b1306678 2026-04-11T03:25:49.438Z
19.1.0-0.ci+gitbc696216ef5 2026-04-13T23:24:19.034Z
19.1.0-0.ci+gitc7312331b08 2026-04-14T21:02:48.258Z
19.2.0-0.ci+gite4b75dde134 2026-04-21T23:11:22.248Z
19.3.0-0.ci+gitd4a126e8cc3 2026-05-05T23:45:46.284Z
19.4.0-0.ci+gitbae719a3bdb 2026-05-13T15:11:13.005Z
20.0.0-0.ci+git8ca64d81e70 2026-05-15T19:02:14.696Z
| .release_source() | ||
| .context("interpreting target release source")? | ||
| { | ||
| TargetReleaseSource::Unspecified => "<unspecified>".to_string(), |
There was a problem hiding this comment.
Unspecified means no target release has been set yet, is it useful to have this as part of the list or target releases?
There was a problem hiding this comment.
This is printing rows from the db - what would you do instead of printing a sentinel like this? Neither skipping nor failing seems great to me.
There was a problem hiding this comment.
I was thinking of skipping, but yeah, it would be useful to know when there have been unspecified target releases
|
|
||
| #[derive(Debug, Args, Clone)] | ||
| pub(super) struct TargetReleaseArgs { | ||
| /// List of target releases sorted from oldest to newest |
There was a problem hiding this comment.
Looking at the sample, isn't it the other way around?
|
|
||
| let mut rows = Vec::with_capacity(releases.len()); | ||
| for release in releases { | ||
| let system_version = match release |
There was a problem hiding this comment.
nit: we are adding unspecified here too, so the variable isn't really system_version, but rather target_release.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
+1 to Karen's comments about stale comments and switching everything over to "target release". Everything else LGTM - thanks!
…n header to TARGET_RELEASE
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Thanks for the changes! There are a couple of things we may want to think about. With the current implementation if we run omdb db target-release --help which should only return a single row we get the following information
$ ./target/debug/omdb db target-release --help
Print the current target release and the update date
Usage: omdb db target-release [OPTIONS]
Options:
--all List the most recent target releases, sorted from oldest to newest
--log-level <LOG_LEVEL> log level filter [env: LOG_LEVEL=] [default: warn]
--color <COLOR> Color output [default: auto] [possible values: auto, always, never]
-h, --help Print help
Connection Options:
--db-url <DB_URL> URL of the database SQL interface [env: OMDB_DB_URL=]
--dns-server <DNS_SERVER> [env: OMDB_DNS_SERVER=]
Database Options:
--fetch-limit <FETCH_LIMIT> limit to apply to queries that fetch rows [env: OMDB_FETCH_LIMIT=] [default: 500]
--include-deleted whether to include soft-deleted records when enumerating objects that can be soft-deleted
...As you can see this includes --fetch-limit which feels a little strange since you're only expecting a single entry. We only really want the --fetch-limit when we're returning many rows with the --all flag.
Another thing to think about, is that you can set the fetch limit via an environment variable OMDB_FETCH_LIMIT. omdb sets this to 500 automatically, so technically we're never really returning "all" which might catch some people off-guard if they're using a flag called "--all".
To not make this command extremely complex we could simplify it by just having a single omdb db target-releases or omdb db target-release list command that lists all the target releases up to the maximum amount set by --fetch-limit.
This way, if you only want the latest target release you can do
$ omdb db target-releases --fetch-limit 1If you want all of them you can set a bogus amount
$ omdb db target-releases --fetch-limit 100000000000000If you want a subset
$ omdb db target-releases --fetch-limit 5If you want as many rows as the default fetch limit has
$ omdb db target-releases What do you think?
|
|
||
| #[derive(Debug, Args, Clone)] | ||
| pub(super) struct TargetReleaseArgs { | ||
| /// List the most recent target releases, sorted from oldest to newest |
There was a problem hiding this comment.
I think it's newest to oldest?
| args: &TargetReleaseArgs, | ||
| ) -> Result<(), anyhow::Error> { | ||
| let releases = if args.all { | ||
| let limit = fetch_opts.fetch_limit; |
There was a problem hiding this comment.
&DbFetchOptions also has an --include-deleted flag. We should probably handle that no? I don't think we soft delete target releases. We can either just print out a warning about that if someone uses that flag, or return an error saying that flag is invalid here.
I would probably lean towards returning an error in case someone wishes to parse the output of this command.
| .release_source() | ||
| .context("interpreting target release source")? | ||
| { | ||
| TargetReleaseSource::Unspecified => "<unspecified>".to_string(), |
There was a problem hiding this comment.
I was thinking of skipping, but yeah, it would be useful to know when there have been unspecified target releases
4 participants
Add
system-versionsubcommand toomdb dbto print the current system version and when this version update was requested.Also add
--all, to print all system versions and their requested dates.