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

Ability to control the number of hosts returned per page #865

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

tchellomello
Copy link
Contributor

Ability to control the number of hosts returned per page which can be definitely useful some users.

image

Result on Foreman:

192.168.169.112 - - [03/Jul/2020:18:00:37 -0400] "GET //api/v2/hosts/3?page=1&per_page=500 HTTP/1.1" 200 2814 "-" "python-requests/2.24.0"
192.168.169.112 - - [03/Jul/2020:18:00:37 -0400] "GET //api/v2/hosts/3?page=1&per_page=500 HTTP/1.1" 200 2814 "-" "python-requests/2.24.0"
192.168.169.112 - - [03/Jul/2020:18:00:38 -0400] "GET //api/v2/hosts/2?page=1&per_page=500 HTTP/1.1" 200 2821 "-" "python-requests/2.24.0"
192.168.169.112 - - [03/Jul/2020:18:00:38 -0400] "GET //api/v2/hosts/2?page=1&per_page=500 HTTP/1.1" 200 2821 "-" "python-requests/2.24.0"
192.168.169.112 - - [03/Jul/2020:18:00:38 -0400] "GET //api/v2/hosts/5?page=1&per_page=500 HTTP/1.1" 200 2796 "-" "python-requests/2.24.0"

plugins/inventory/foreman.py Outdated Show resolved Hide resolved
@mdellweg
Copy link
Member

mdellweg commented Jul 6, 2020

You could introduce a "total limit" parameter here.
But in order to get all the hosts for the inventory, you'd need to cycle through the pagination (the code already seems to do that, so i do not see the what kind of user visible difference your patch introduces).

@evgeni
Copy link
Member

evgeni commented Jul 6, 2020

@mdellweg the code does fetch all hosts:

# get next page
params['page'] += 1

This is just controlling how big the individual batches are.

@tchellomello
Copy link
Contributor Author

tchellomello commented Jul 6, 2020

You could introduce a "total limit" parameter here.
But in order to get all the hosts for the inventory, you'd need to cycle through the pagination (the code already seems to do that, so i do not see the what kind of user visible difference your patch introduces).

The code does get all hosts, but for large environments, increasing the number of hosts minimizes the number of requests sent to the Foreman server. I'm going to send another PR with the max_hosts parameters so the users can limit the number of hosts returned as well.

@mdellweg
Copy link
Member

mdellweg commented Jul 6, 2020

The batch_size @evgeni proposed might be a performance thing. So it would be interesting to get some numbers on that.

Co-authored-by: Matthias Dellweg <2500@gmx.de>
@evgeni evgeni merged commit 199ae14 into theforeman:master Jul 21, 2020
@tchellomello tchellomello deleted the per_page branch July 21, 2020 15:40
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

3 participants