Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

send an APNs push to hosts with pending commands every minute #19941

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

roperzh
Copy link
Member

@roperzh roperzh commented Jun 21, 2024

Initial naive implementation that sends push notifications to hosts with pending enqueued commands every 1 minute.

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/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Added/updated tests
  • Manual QA for all new/changed functionality

mna
mna previously approved these changes Jun 25, 2024
Copy link
Member

@mna mna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A small concern with a query but that's not a blocker, it's likely fine just something to keep in mind in a very large deployment (we might want to load test this eventually for that scenario).

Also as a non-blocker comment, should we make the cron interval configurable?

SELECT DISTINCT neq.id
FROM nano_enrollment_queue neq
LEFT JOIN nano_command_results ncr ON ncr.command_uuid = neq.command_uuid
WHERE neq.active = 1 AND ncr.status IS NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that query with a DISTINCT could become a performance issue, though it's on the reader so it might not be as bad...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm good point, it does take considerable time in dogfood:

mysql> SELECT DISTINCT neq.id FROM nano_enrollment_queue neq LEFT JOIN nano_command_results ncr ON ncr.command_uuid = neq.command_uuid WHERE neq.active = 1 AND ncr.status IS NULL;

+--------------------------------------+
| id                                   |
+--------------------------------------+
REDACTED
+--------------------------------------+
19 rows in set (1 min 10.89 sec)

mysql> EXPLAIN SELECT DISTINCT neq.id FROM nano_enrollment_queue neq LEFT JOIN nano_command_results ncr ON ncr.command_uuid = neq.command_uuid WHERE neq.active = 1 AND ncr.status IS NULL;
+----+-------------+-------+------------+------+-------------------------------+--------------+---------+------------------------+---------+----------+-----------------------------------+
| id | select_type | table | partitions | type | possible_keys                 | key          | key_len | ref                    | rows    | filtered | Extra                             |
+----+-------------+-------+------------+------+-------------------------------+--------------+---------+------------------------+---------+----------+-----------------------------------+
|  1 | SIMPLE      | neq   | NULL       | ALL  | PRIMARY,command_uuid,priority | NULL         | NULL    | NULL                   | 1178015 |    10.00 | Using where; Using temporary      |
|  1 | SIMPLE      | ncr   | NULL       | ref  | command_uuid                  | command_uuid | 510     | fleet.neq.command_uuid |       8 |    10.00 | Using where; Not exists; Distinct |
+----+-------------+-------+------------+------+-------------------------------+--------------+---------+------------------------+---------+----------+-----------------------------------+
2 rows in set, 1 warning (0.20 sec)

A tiny improvement, but it's still at the filter stage would be to scope down and grab rows with updated_at in the last week or something like that.

Did you have another query in mind? I'm going to experiment with NOT EXISTS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it significantly faster if you remove the DISTINCT? It could be deduplicated in memory, and we could add a LIMIT so that it processes a maximum of rows in any given cron run (with the expectation that those rows would get a result eventually, so subsequent runs would get the rows that were missed on the previous run). Though the LIMIT would be a number of rows, not a number of host uuids...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DISTINCT didn't made a difference, seems like the query is just bad with the indexes we have :(

adding a neq.created_at >= NOW() - INTERVAL 7 DAY made a huge impact (query took 1 second), so maybe combining that with a LIMIT? I'm thinking on doing both so hosts that are stuck don't clog the queue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 makes sense to me! Yeah some hosts could be offline and you'd just keep try to send a push on every cron.

mna
mna previously approved these changes Jun 25, 2024
mna
mna previously approved these changes Jun 25, 2024
@roperzh roperzh marked this pull request as ready for review July 12, 2024 14:39
@roperzh roperzh requested review from gillespi314 and a team as code owners July 12, 2024 14:39
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 26.31579% with 56 lines in your changes missing coverage. Please review.

Project coverage is 37.15%. Comparing base (e4bbf9a) to head (0e6f7e4).
Report is 7 commits behind head on main.

Files Patch % Lines
cmd/fleet/cron.go 0.00% 25 Missing ⚠️
cmd/fleet/serve.go 0.00% 11 Missing ⚠️
server/service/apple_mdm.go 37.50% 8 Missing and 2 partials ⚠️
server/mock/datastore_mock.go 0.00% 5 Missing ⚠️
server/datastore/mysql/apple_mdm.go 80.00% 2 Missing and 1 partial ⚠️
server/mdm/apple/commander.go 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #19941       +/-   ##
===========================================
- Coverage   55.26%   37.15%   -18.12%     
===========================================
  Files        1416     1423        +7     
  Lines      132980   133997     +1017     
  Branches     3199     3199               
===========================================
- Hits        73496    49791    -23705     
- Misses      53498    79708    +26210     
+ Partials     5986     4498     -1488     
Flag Coverage Δ
backend 35.73% <26.31%> (-19.67%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@roperzh roperzh merged commit f20e403 into main Jul 12, 2024
24 checks passed
@roperzh roperzh deleted the pushes-every-minute branch July 12, 2024 18:40
roperzh added a commit that referenced this pull request Jul 12, 2024
Initial naive implementation that sends push notifications to hosts with
pending enqueued commands every 1 minute.

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [ ] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] Manual QA for all new/changed functionality
roperzh added a commit that referenced this pull request Jul 12, 2024
#20419)

> [!NOTE]
> This PR already merged in `main`, see
#19941. This is against the release
branch so it can be included in 4.54.0

Initial naive implementation that sends push notifications to hosts with
pending enqueued commands every 1 minute.

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [ ] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes

files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] Manual QA for all new/changed functionality
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants