Repository navigation
api: Count only the update's target in rollout summaries - #311
Conversation
vkhoroz
left a comment
vkhoroz
left a comment
There was a problem hiding this comment.
LGTM
I only have one mental teaser to think of.
| // A device reports other targets too, like the one it rolls back to after this update fails | ||
| if targets != nil && len(status.TargetName) > 0 && !targets[status.TargetName] { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This seems to be correct in a static environment.
I also believe that it is correct in a dynamic environment too:
- A rollout log only contains logs for a specific update.
- But, all logs from a device which is assigned to this update are coming into it; even if the device was effectively updated to some different update (e.g. from a previous rollout).
- So, what this thing does - it filters out non-update related events; only keeping update related events.
Is this how it works?
I am inclined to think this is what we want.
But, it might hide problems, or seem confusing, as we may show an event which wasn't the last as the last one.
Possibly, we need to somehow show both events (for each device participating in an update):
- the last event related to a given update;
- the absolute last event.
There was a problem hiding this comment.
@doanac what do you think about this.
I'm going to approve this PR, just wondering your opinion on the topic.
There was a problem hiding this comment.
as we may show an event which wasn't the last as the last one.
Are you saying that in the case of if targets != nil && len(status.TargetName) > 0 && !targets[status.TargetName] { we should do something like delete lastStates[status.Uuid]
There was a problem hiding this comment.
What I'm saying is this TL;DR.
In the previous version we've shown the last event related to the update. It could be a successful update, failed update, or successful rollback - whatever was the last one.
Now, a successful rollback is removed from equation; as we shown a failed update for such devices.
For one, it is good as we tell the user that an update failed.
But, a device could successfully roll back afterwards, leaving a user under a false impression as if the device is still in a failed state.
So, my idea is: maybe extend our API/UI to show both events.
I have no opinion what is better; just contemplating.
There was a problem hiding this comment.
The typical failed situation will lead to a successful rollback. Looking at the logs it is possible to identify if the device is still in a Failed state. To make things more clear to the user, perhaps we can introduce a new RolledBackState, and keep the failed state only for the devices that did not recover properly. It fits in lastKnownStates with the same API shape.
There was a problem hiding this comment.
Let's merge this one, and leave changes for a new PR.
ddcc328 to
5745c39
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Overlapping hardware targets still allow rollback successes to mask failures, and accepted metadata without hardware IDs or versions now causes reporting errors.
2 open issues
What changed in this PR
This PR updates rollout reporting to exclude rollback-target events, aiming to prevent failed updates from appearing successful.
Changes:
- Selects the latest tagged target per hardware ID and filters summary and state-for events.
- Preserves unfiltered counting when target metadata is absent or no target matches.
- Adds tests for both rollback patterns, invalid versions, and missing metadata.
Verified through source inspection; tests were not run during this review.
| File | Description |
|---|---|
storage/api/api_storage_update.go |
Adds target selection and rollout-status filtering. |
storage/api/api_storage_update_test.go |
Adds regression and metadata-handling tests. |
Review effort: Balanced
| names := make(map[string]bool, len(byHwId)) | ||
| for _, l := range byHwId { | ||
| names[l.name] = true |
5745c39 to
d7da485
Compare
d7da485 to
a68cb8c
Compare
A device also reports the target it goes back to when an update fails: the previous one, reinstalled after an app fails to start or synced after the boot loader falls back. That success landed last in the update's rollout log and the summary counted the failed update as succeeded. Signed-off-by: Andre Detsch <andre.detsch@foundries.io> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a68cb8c to
2a086e6
Compare
When an update fails, the device also reports the target it goes back to, under the same update. That happens in two cases:
Either way, the previous target's
Installation completed; succeededis the device's last line in the update's rollout log. The summary keeps only each device's last status, so the failed update counted as a success. Lab runs caught this on the rollout summary of a failed app update.The summary and state-for queries now count only events for the update's own target: the latest version with the update's tag, per hardware id, from its
targets.json. Events without a target name still count. So do all events of an update without targets metadata or without a matching target, as before.Tested with
go test ./...(all packages pass) and golangci-lint onstorage/. The newTestUpdateSummaryCountsOnlyTheUpdateTargetcovers both rollback patterns, and it fails without the change (3 succeeded instead of 2 failed and 1 succeeded).🤖 Generated with Claude Code