Stop setup experience on software install failure - #34173
Conversation
iansltx
left a comment
There was a problem hiding this comment.
There are a few test comments that still need to be tweaked I think, plus one or two other items of outstanding feedback, but no further feedback on the changes made earlier today; stuff looks good, thanks!
Will review the integration test changes next.
FYI I won't be able to sign off here until merge conflicts are resolved, but I can get most of the review done barring that work.
iansltx
left a comment
There was a problem hiding this comment.
Three minor tweaks, one question from reviewing the integration test. I've now caught fully up on review.
| const failedSoftware = statuses.filter( | ||
| (s) => s.type === "software" && s.status === "failure" | ||
| (s) => | ||
| (s.type === "software_install" || s.type === "software_script_run") && |
There was a problem hiding this comment.
Supporting no-package software as well, with the understanding that they are queued and processed exactly like regular software. The UI for "cancel setup if software fails" doesn't distinguish between these two.
| // Check if "require all software" is configured for the host's team. | ||
| requireAllSoftware, err := svc.IsAllSetupExperienceSoftwareRequired(ctx, host) | ||
| if err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "checking if all software is required") | ||
| } | ||
|
|
||
| hasFailedSoftwareInstall := false | ||
| for _, r := range res { | ||
| if r.IsForSoftware() && r.Status == fleet.SetupExperienceStatusFailure { | ||
| hasFailedSoftwareInstall = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Moving these checks up since we'll use them both to determine whether to reset previously failed steps, and whether to release the device.
| if (type === "software") { | ||
| if (type === "software_install" || type === "software_script_run") { | ||
| return <SetupSoftwareStatusCell status={status || "pending"} />; | ||
| } | ||
| if (type === "script") { | ||
| if (type === "script_run") { |
There was a problem hiding this comment.
missed in merge conflict resolution; this makes the statuses appear in the table
|
|
||
| var steps []*fleet.SetupExperienceStatusResult | ||
| if len(payload.Software) > 0 { | ||
| steps = payload.Software | ||
| } | ||
|
|
||
| if payload.Script != nil { | ||
| steps = append(steps, payload.Script) | ||
| for _, step := range payload.Software { | ||
| // If any step is not in a terminal state, then we're not done. | ||
| if (step.Status != fleet.SetupExperienceStatusFailure && step.Status != fleet.SetupExperienceStatusSuccess) || | ||
| // If any software failed, and we're requiring all software to succeed, then we'll block completion. | ||
| (step.Status == fleet.SetupExperienceStatusFailure && payload.RequireAllSoftware) { | ||
| allStepsDone = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, step := range steps { | ||
| if step.Status != fleet.SetupExperienceStatusFailure && step.Status != fleet.SetupExperienceStatusSuccess { | ||
| allStepsDone = false | ||
| } | ||
| // If a script is still running, then we're not done. | ||
| if payload.Script != nil && payload.Script.Status != fleet.SetupExperienceStatusFailure && payload.Script.Status != fleet.SetupExperienceStatusSuccess { | ||
| allStepsDone = false | ||
| } |
There was a problem hiding this comment.
Because I'm not 100% sure what data will be sent with no-package software (do they have software title IDs?) I reworked this a bit so that it first checks everything returned in the Software array, and blocks setup experience if anything is failed while "RequireAllSoftware" is true. Then it checks any Script data independently.
There was a problem hiding this comment.
This logic here should apply to no-package scripts because they're inserted via the stmtSoftwareInstallers query in EnqueueSetupExperienceItems() (lines 22-60 in setup_experience.go). They're just regular software installers with .sh or .ps1 extensions, so they have software_title_id set here, as well.
The frontend distinguishes them visually by checking if name.endsWith(".sh") or name.endsWith(".ps1") to display "Ran" instead of "Installed", but from the database/backend perspective, they're standard software installers.
Test file server/datastore/mysql/setup_experience_test.go has the test structure at line 295.
There was a problem hiding this comment.
Thanks @cdcme! It works this way too and is a bit more explicit so I'll leave it as-is.
| AccountConfiguration: acctCfgResult, | ||
| Software: make([]*fleet.SetupExperienceStatusResult, 0), | ||
| OrgLogoURL: appCfg.OrgInfo.OrgLogoURLLightBackground, | ||
| RequireAllSoftware: requireAllSoftware, |
There was a problem hiding this comment.
Added this new field to the response so that Orbit can know whether or not to block the setup experience on failed software.
| if hasFailedSoftwareInstall { | ||
| if resetFailedSetupSteps { |
There was a problem hiding this comment.
| if hasFailedSoftwareInstall { | |
| if resetFailedSetupSteps { | |
| if hasFailedSoftwareInstall && resetFailedSetupSteps { |
and then drop the level of indentation at the bottom too
There was a problem hiding this comment.
Ah yeah -- I didn't notice I'd factored out all of the logic between the two, but not worth dismissing review for.
iansltx
left a comment
There was a problem hiding this comment.
Remaining feedback is non-blocking. If there's nothing else you need to fix anyway based on your testing, clear to merge.
One thing I'm not seeing here is the key unbind mentioned in the story description, though I may have just missed it, so likely worth triple-checking the spec to make sure things are either fully covered here or there's another subtask + PR combo that'll cover the last bits (though I think this is the last PR in for this?).
Ping if you've got other changes; I should have some form of connectivity over the next 18 hours to continue review with the context I have vs. needing to swap to someone else.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #33790 # Details This is just the tooltip update; the code to update the order of software installs is was released as part of #34173 (see https://github.com/fleetdm/fleet/pull/34173/files#diff-c5babdad542a72acf2ec2ecb7cb43967fc53850b6998ac629e253336b87e008b) # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] QA'd all new/changed functionality manually <img width="560" height="321" alt="image" src="https://github.com/user-attachments/assets/1bde02ca-e180-49e1-92a7-e197305dd8ee" />
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #33173 **Related issue:** Resolves #33111 # Details This is the remaining work to implement the "Stop the setup experience when required software fails to install" feature. This didn't turn out to be quite as straightforward as expected so I ended up doing a bit of design-by-code and expect some feedback on the approach. I tried to make it as low-touch as possible. The general design is: 1. In the `maybeUpdateSetupExperienceStatus` function which is called in various places when a setup experience step is marked as completed, call a new `maybeCancelPendingSetupExperienceSteps` function if the setup step was marked as failed. Similarly call `maybeCancelPendingSetupExperienceSteps` if a VPP app install fails to enqueue. 2. In `maybeCancelPendingSetupExperienceSteps`, check whether the specified host is MacOS and whether the "RequireAllSoftwareMacOS" flag is set in the team (or global) config. If so, mark the remaining setup experience items as canceled and cancel any upcoming activities related to those steps. 3. On the front-end, if the `require_all_software_macos` is set and a software step is marked as failed, show a new failure page indicating that setup has failed and showing details of the failed software. 4. On the agent side, when checking setup experience status, send a `reset_after_failure` flag _only the first time_. If this flag is set, then the code in the `/orbit/setup_experience/status` handler will clear and re-queue any failed setup experience steps (but leave successful steps to avoid re-installing already-installed software). This facilitates re-starting the setup experience when the host is rebooted. I also updated the way that software (packages and VPP) is queued up for the setup experience to be ordered alphabetically, to make it easier to test _and_ because this is a desired outcome for a future story. Since the order is not deterministic now, this update shouldn't cause any problems (aside from a couple of test updates), but I'm ok taking it out if desired. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [X] Added/updated automated tests * Added a new integration test for software packages, testing that a failed software package causes the rest of the setup experience to be marked as failed when `require_all_software_macos` is set, and testing that the "reset after failure" code works. * Added a new integration test for VPP packages, testing that a failed VPP enqueue causes the same halting of the setup experience. I _don't_ have test for a failure _during_ a VPP install. It should go through the same code path as the software package failure, so it's not a huge gap. - [ ] QA'd all new/changed functionality manually Working on it ## fleetd/orbit/Fleet Desktop - [X] Verified compatibility with the latest released version of Fleet (see [Must rule](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/workflows/fleetd-development-and-release-strategy.md)) - [X] If the change applies to only one platform, confirmed that `runtime.GOOS` is used as needed to isolate changes - [X] Verified that fleetd runs on macOS, Linux and Windows <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Configurable option to halt macOS device setup if any software install fails. - Device setup page now shows a clear “Device setup failed” state with expandable error details when all software is required on macOS. - Improvements - Setup status now includes per-step error messages for better troubleshooting. - Pending setup steps are automatically canceled after a failure when applicable, with support to reset and retry the setup flow as configured. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Ian Littman <iansltx@gmail.com>
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #33790 # Details This is just the tooltip update; the code to update the order of software installs is was released as part of #34173 (see https://github.com/fleetdm/fleet/pull/34173/files#diff-c5babdad542a72acf2ec2ecb7cb43967fc53850b6998ac629e253336b87e008b) # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] QA'd all new/changed functionality manually <img width="560" height="321" alt="image" src="https://github.com/user-attachments/assets/1bde02ca-e180-49e1-92a7-e197305dd8ee" />
Related issue: Resolves #33173
Related issue: Resolves #33111
Details
This is the remaining work to implement the "Stop the setup experience when required software fails to install" feature. This didn't turn out to be quite as straightforward as expected so I ended up doing a bit of design-by-code and expect some feedback on the approach. I tried to make it as low-touch as possible. The general design is:
maybeUpdateSetupExperienceStatusfunction which is called in various places when a setup experience step is marked as completed, call a newmaybeCancelPendingSetupExperienceStepsfunction if the setup step was marked as failed. Similarly callmaybeCancelPendingSetupExperienceStepsif a VPP app install fails to enqueue.maybeCancelPendingSetupExperienceSteps, check whether the specified host is MacOS and whether the "RequireAllSoftwareMacOS" flag is set in the team (or global) config. If so, mark the remaining setup experience items as canceled and cancel any upcoming activities related to those steps.require_all_software_macosis set and a software step is marked as failed, show a new failure page indicating that setup has failed and showing details of the failed software.reset_after_failureflag only the first time. If this flag is set, then the code in the/orbit/setup_experience/statushandler will clear and re-queue any failed setup experience steps (but leave successful steps to avoid re-installing already-installed software). This facilitates re-starting the setup experience when the host is rebooted.I also updated the way that software (packages and VPP) is queued up for the setup experience to be ordered alphabetically, to make it easier to test and because this is a desired outcome for a future story. Since the order is not deterministic now, this update shouldn't cause any problems (aside from a couple of test updates), but I'm ok taking it out if desired.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
require_all_software_macosis set, and testing that the "reset after failure" code works.I don't have test for a failure during a VPP install. It should go through the same code path as the software package failure, so it's not a huge gap.
Working on it
fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit