Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #39827 +/- ##
==========================================
+ Coverage 66.26% 66.32% +0.06%
==========================================
Files 2438 2447 +9
Lines 195266 196546 +1280
Branches 8643 8643
==========================================
+ Hits 129386 130366 +980
- Misses 54167 54362 +195
- Partials 11713 11818 +105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Arrrrrrghhhh I had a somewhat completed review and lost all comments when I switched back to conversation and back again to files changed... |
|
Oh no wait, I can go back to it, it just started a new review but the pending one is still available... New UX I think? |
mna
left a comment
There was a problem hiding this comment.
I took a first look without reviewing the tests, left some comments. My main concern is with the unified queue, to ensure we don't introduce scenarios where the queue gets stuck.
I'll take a full look (including tests) on my next review.
|
@mna I think I've addressed your feedback either with changes or by adding comments. Please lmk if anything else jumps out at you, and ty! |
|
@cdcme ty for the ping, I'll take another look this PM! |
mna
left a comment
There was a problem hiding this comment.
LGTM, just a minor thing and the reuse of the cancel activity logic. Let me know if you'd prefer to address this in a subsequent PR, if so I'll approve this one.
|
|
||
| _, err := svc.ds.InsertSoftwareInstallRequest(ctx, host.ID, installer.InstallerID, fleet.HostSoftwareInstallOptions{ | ||
| SelfService: false, | ||
| WithRetries: true, |
There was a problem hiding this comment.
Looking at other usages here, it looks like we manage retries three different ways:
- This (manually initiated installs)
- Setup experience software retries (already existed prior)
- Policy failure software install retries (already existed prior)
The latter two don't use the retry mechanism implemented here because they perform retries at a higher level of abstraction, right?
I'm pretty sure I saw the setup experience retries ticket come across a bit ago but don't recall when policy failure based retries happened. Can you link me the ticket for that one?
There was a problem hiding this comment.
Good question! Yes, broadly speaking, I think that's right. Are you thinking of PR #38018?
There was a problem hiding this comment.
Yep, that's what I'm thinking of for policies. Setup experience was done earlier.
|
Deferring back to @mna for the last review pass since he's starting at 90% context here rather than 20%, so this'll get merged quicker (on Monday) that way. |
Fixes #34068
Summary
Adds automatic retries (up to 3 attempts) for failed software installs from host details, self-service, and setup experience across all installer types (except VPP apps).
Details
Non-policy software installs previously had no retry mechanism. A single failure required manual re-triggering. This extends the existing policy-automation retry pattern to all install paths.
Key changes:
WithRetriesflag onHostSoftwareInstallOptionsenables attempt tracking at activation timeMaxSoftwareInstallAttempts(3) timesExpected Behavior