Update NewLabel method to use more efficient update mechanism #25777
Conversation
sgress454
changed the title
|
@jacobshandling some failing tests, i'll ping you for review once they're resolved. |
|
@jacobshandling all set, I just had to do the thing I said I would do 🙄 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25777 +/- ##
==========================================
- Coverage 63.61% 63.60% -0.02%
==========================================
Files 1623 1623
Lines 155465 155456 -9
Branches 4077 4077
==========================================
- Hits 98905 98883 -22
- Misses 48759 48769 +10
- Partials 7801 7804 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Code looks good.
Can we also update this comment to keep it accurate to the new matching functionality?
| @@ -165,8 +165,11 @@ VALUES ` + strings.Join(placeholders, ", ") | |||
| } | |||
| return nil | |||
| }) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Is this necessary? Seems like every possible err is being handled already
There was a problem hiding this comment.
This is the error returned from the retry function above. Errors are handled inside the function e.g. here but we still need to check whether the retry function failed so we can return out of UpdateLabelMembershipByHostIDs early. We were previously doing this implicitly with
return ctxerr.Wrap(ctx, err, "UpdateLabelMembershipByHostIDs transaction")
where err would usually be nil.
|
|
||
| return ctxerr.Wrap(ctx, err, "UpdateLabelMembershipByHostIDs transaction") | ||
| return ds.labelDB(ctx, labelID, teamFilter, ds.writer(ctx)) |
There was a problem hiding this comment.
What about ds.reader(ctx), or even using
this function?
There was a problem hiding this comment.
The problem with both of those is that they use the read replica (ds.reader()), which doesn't have the updates yet.
For #25555
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)This PR updates the
NewLabelservice to use theUpdateLabelMembershipByHostIDsmethod previously added by @jacobshandling rather than usingApplyLabels. The latter method has performance issues when adding large numbers of hosts at once to a manual label (see #25555) because it does an expensive lookup of host names before transforming those into Fleet host IDs. The new code skips the middleman and transforms host identifiers directly to Fleet host IDs, and does so using a batching strategy to ensure the queries don't get too large.This PR does update
UpdateLabelMembershipByHostIDsslightly to return an updated Label object and host IDs array, as this is the expected return value forNewLabel. I update the method's tests accordingly. I don't think any new tests forNewLabelare needed as it should have the same functionality and return values.Manual Testing
On the main branch, I launched my local MySQL with the thread stack size set to the minimal allowed, and used the API to try and create a new label with 5,000 hosts attached, and received a 422 response from the server. Server logs showed:
On this branch, I kept the same MySQL settings and tried my API request again and it was successful:
QA
The script I used to create a new manual label with lots of hosts is at: https://gist.github.com/sgress454/84f12064c437da456c456e25c26d9069
To run it, first grab a bearer token from any API request by opening the network tab, clicking a Fleet API request, and in the headers tab scrolling down to Authorization:
(only take the part after "Bearer")
Then download the script from that gist and in its folder run:
e.g.
This will invoke the API on https://localhost:8080 and try to add 5000 hosts a new label "some test label".
If you need to change the # of hosts or the url of the server, there are additional arguments:
e.g.