-
Notifications
You must be signed in to change notification settings - Fork 407
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
Add new low_disk_space
URL parameter to the GET /hosts endpoint
#7592
Comments
low_disk_space
query parameter to the GET /hosts
endpointlow_disk_space
URL parameter to the GET /hosts endpoint
@mna Another API update. Would you please review the specs and let me know if you have any questions or concerns? |
@lukeheath the concern here is performance - this information is in the |
We need to have a bigger conversation before we keep adding more and more options to this. If, for some reason, we cannot have that conversation before this ticket, the best I can think of is: let's move that column out of the host table, and index that. Which is going to be an annoying migration to run. |
@mna can you help me understand the costs of doing this? Will this increase the time it takes for migrations to run when users upgrade? Will this force us the change the response of the That being said, having a conversation about filtering and performance would be valuable. Goal being Noah understands the costs for adding more and more filters. |
Potentially, but what we could do is a "lazy migration" in that we create the new table (so the So the migration is very fast, at the cost of not back-filling it with already existing information - instead we wait for new information to come in to fill it. So it would take ~1h with default settings for all hosts to report back the information in the new table, and until then we would have incomplete information when querying about disk space (even though that information exists in the old table, because we now ignore that old table's field). And then in a future version we remove that column, which is slow-ish but not as slow as moving all the data for all hosts to the new table and then removing that column. Note that removing could be done right now, but I'd wait for the new table to fill up before doing this just in case e.g. users have to rollback the upgrade or something and end up with no data for this column.
I think not, it should be an implementation detail that doesn't leak to the payload. |
+1 to this.
12hrs in some customer's setups, fwiw. And that is assuming all devices are online. |
@noahtalerman Can you confirm that the "lazy" migration process mentioned in the above 2 comments is acceptable from your point of view? Basically, it would be as if the |
Given the requirements of the disk encryption ticket (#3906 (comment)), I'll go ahead and create a new |
@mna this sounds good to me 👍 |
A bit surprised that it's not already there, but |
Tested with data (1000 hosts) that it does use the new index on
(updated with the actual query executed when listing hosts by low_disk_space) |
@noahtalerman @lukeheath Two things I'd like to clarify:
|
One item worth considering is that in the future it's likely there will be additional ranges, for example "show all hosts with less than 64 GB". Is it better for us to have separate boolean query params for this, or should we consider setting a number now? For example, |
@lukeheath This is an interesting point - I think a number would better convey that it only works for filtering hosts with low disk space (otherwise it could be expected for API users that Of course we'd have to validate that number so we don't accept extreme values that could cause full scans, but otherwise it might be best to allow that flexibility? What do you think of simply using that query parameter as-is, but as a number instead of boolean, e.g. For performance this does not matter as long as we don't blindly accept any number - internally the boolean is converted to a number in the query anyway, i.e. we don't pre-compute the low-disk hosts or something like that. |
@mna Got it. Good idea on the query param. Yes, let's go ahead with setting the query param to |
Problem
I'm an engineer managing thousands of hosts and I'm overwhelmed with tracking the following goal:
This makes it hard to know which hosts I need to take action on.
Goal
Requirements
Related
low_disk_space_count
toGET /host_summary
response #7591low_disk_space
query param when listing all hosts #7593 (blocked)Figma
https://www.figma.com/file/hdALBDsrti77QuDNSzLdkx/%F0%9F%9A%A7-Fleet-EE-(dev-ready%2C-scratchpad)?node-id=9223%3A304723
API Specs
Example response
GET /hosts?low_disk_space=32
Tasks
1
low_disk_space
integer query parameter to theGET /hosts
endpoint.team_id
.GET /hosts
, it also affectsGET /hosts/count
, which uses the same filters to get the matching counts.The text was updated successfully, but these errors were encountered: