Add software installer upload/download progress to GitOps runs - #50250
Draft
jkatz01 wants to merge 9 commits into
Draft
Add software installer upload/download progress to GitOps runs#50250jkatz01 wants to merge 9 commits into
jkatz01 wants to merge 9 commits into
Conversation
Software batch upload runs asynchronously on the server, so fleetctl gitops had nothing to print between starting a batch and its completion. Long uploads made a run look stuck. The batch now records each package in Redis as it starts and finishes downloading, the batch status endpoint returns that progress while the batch is still processing, and the gitops client prints a line per package as it goes. Dry run output is unchanged.
Report progress from the branch that fetches from a URL, so a package whose bytes are already in storage, a cached Fleet-maintained app and a script package all stay silent instead of claiming a download that never happened. Mark a package failed when its download errors, so the client prints a line for it rather than leaving a dangling "downloading" line, and return progress alongside a failed batch status so packages that finish in the last poll window aren't dropped from the output. Dry runs print the same lines, since they download packages too. The downloaded line no longer names the fleet, which the surrounding applying and applied lines already establish.
A package whose download fails now gets its own line, so a run doesn't leave a "downloading" line dangling while the remaining packages keep downloading for another minute. Replace the finished and failed flags with a single status, since a package is downloading, downloaded or failed and never two of those. Drop the prefix from the download error messages, which read as two stacked apologies once the batch wraps them, and drop the name fallbacks that only the paths that no longer report progress could reach.
Reading a batch result reports package names, which a fleet's observers can already read from its library, so authorize it as a software inventory read instead of an installer write. That replaces the pair of checks the method used to run, one of which only covered the completed branch. Report the applied count from the payload rather than the fleet's installer rows, which count cached Fleet-maintained app versions separately and would disagree with the applying count. Cover the progress output: one line per package however often the client polls, silence for packages the batch never downloads, a failed download reporting itself and never reporting as downloaded, dry runs, unassigned hosts, multiple fleets, and a progress write failure leaving the batch alone.
Marshal the progress under the lock but write it outside, so a slow write never holds up a download once the goroutine limit is raised. Give the key an hour instead of ten minutes: its siblings are only read once the batch is done, while this one has to outlast a single slow download. Stop the result endpoint dereferencing a nil result, which would panic instead of reporting the missing status.
Cut what the code already says and keep the reasons that aren't visible from reading it: why the progress lives in its own key, why the write happens outside the lock, why that key outlives its siblings, and why reading a batch result is scoped like reading a fleet's software.
The keepalive refreshed only the status key, so a batch running longer than ten minutes lost its progress and its categories. The categories were the worst of it: the client read an empty list and then deleted self-service categories that were still in use. Refresh both alongside the status, and drop the hour-long expiration the progress key was given to work around this. Mark a package's download failed where the download fails rather than from any error the goroutine returns, which no longer needs a named return or a guard against errors raised outside a download. Read a batch result with the same read as the fleet's installers rather than as software inventory. A result reports the packages the batch is about to delete, which observers can't see anywhere else, so it isn't the read that backs the software library. Report the software section's count the same way in both modes, for a fleet and for unassigned hosts, so the progress lines are bracketed by an applying and an applied line either way. Print the progress lines where they're logged instead of naming them alongside formats that aren't errors, and let the result endpoint dereference its result like every other endpoint does. Cover the output end to end in the gitops integration tests, which exercise a real download through a real server. Keep the client test for what they can't reach, a package reported across several polls, and the service test for a progress write that fails.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50250 +/- ##
==========================================
+ Coverage 67.99% 68.06% +0.07%
==========================================
Files 3928 3932 +4
Lines 250128 250880 +752
Branches 13170 13170
==========================================
+ Hits 170076 170766 +690
- Misses 64752 64781 +29
- Partials 15300 15333 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The dry run and real run assertions in the gitops integration tests hold every line of output to a fixed set of verbs. Software packages report each download as it starts and finishes, which the real run set didn't cover, and a dry run downloads them in full, so those lines are the one thing it says it did rather than would do. Stop reporting a fleet's software as "applying" on a dry run, which claimed work the run isn't doing and is what the dry run assertion exists to catch. Serve the package the setup experience test downloads. Its url pointed at a file the test server doesn't have, so the batch failed on the download and the case passed on that error rather than on the setup experience conflict it was written for, after spending a minute retrying the download. Assert the conflict itself so it can't pass that way again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #45278
Changes:
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), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually