[19/n] report mupdate overrides while fetching target release #8427
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
|
|
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
| /// The minimum generation number required for the system to be back in | ||
| /// charge again. | ||
| pub minimum_generation: i64, |
There was a problem hiding this comment.
I don't see what sense an external API consumer can make of this number. We'd have to explain how we store the target release and what the generation number means, and we'd be way into implementation details. I'd leave it out altogether.
There was a problem hiding this comment.
Ah I had it here beause I wanted to mirror the existing generation number that's already part of views::TargetRelease. There was also the other concern that I didn't just want to return "mupdate_override": true in the JSON, and I also didn't want to just return "mupdate_override": {}. In the future we could also add the time at which the minimum generation was set (though the schema doesn't have that yet).
There was a problem hiding this comment.
I didn't realize there was one there already. I'm not sure that makes sense, though that one could at least tell you if the target release object you're looking at has changed since some previous request was made.
My suggestion would be to make this a boolean for now. Each scheduled release is a major API bump so in principle we can change this easily. I expect that eventually we may want an API that lists "mupdate overrides in place" and gives more details (like: this sled was mupdated, apparently at this time, apparently to this release (or some unknown release)). Short of that, I don't think there's more than a boolean's worth of information to convey here.
Maybe @ahl has some thoughts here.
There was a problem hiding this comment.
I just have a bias against untyped booleans I guess 😅
There was a problem hiding this comment.
Turned this into a bool for now.
| /// Whether the system has been updated to a new release through the | ||
| /// recovery (MUPdate) path since the target release was last set. | ||
| /// | ||
| /// In this case, the target release is considered to be no longer active, | ||
| /// and a new release must be set. |
There was a problem hiding this comment.
| /// Whether the system has been updated to a new release through the | |
| /// recovery (MUPdate) path since the target release was last set. | |
| /// | |
| /// In this case, the target release is considered to be no longer active, | |
| /// and a new release must be set. | |
| /// If true, indicates that at least one sled in the system has been updated through the recovery (MUPdate) path since the last time the target release was set. | |
| /// | |
| /// In this case, the system will ignore the currently-set target release, on the assumption that continuing an update may re-introduce or exacerbate whatever problem caused the recovery path to be used. You must set the target release again in order to resume automated upgrades. |
Feel free to edit or go back to yours but what I was trying to edit here was:
- it wasn't necessarily a new release that we mupdated to
- elaborating on why the system ignores this value and what the operator needs to do to fix it
nexus/db-model/src/deployment.rs
Outdated
| /// Abridged form of [`Blueprint`] to get just the | ||
| /// `target_release_minimum_generation` column. |
There was a problem hiding this comment.
nit: I can sort of see why you'd want to do this over reading a whole nexus_types::Blueprint, but this doesn't seem worth it to me over using nexus_db_model::Blueprint. (Just selecting one column instead of 8 from the same row, right?) No big deal either way.
There was a problem hiding this comment.
Don't have a strong opinion here, so used nexus_db_model::Blueprint.
sunshowers
changed the base branch from
sunshowers/spr/main.19n-report-mupdate-overrides-while-fetching-target-release
to
main
Created using spr 1.3.6-beta.1
Use the blueprint to report whether a mupdate override is in place.