Description of problem: When calling the API the LC does not provide a max value, limiting the results to 100 by default. For environments with 100+ DCs, Clusters or hosts this results in incomplete LCs being collected. We can workaround this by using rhevm-config to set a higher SearchResultsLimit limit however the LC should be able to deal with environments of this size without this workaround. Version-Release number of selected component (if applicable): rhevm-log-collector-3.2.2-4.el6ev.noarch How reproducible: Always. Steps to Reproduce: 1. Create an env with 100+ (fake) hosts. 2. Use the LC to list all hosts. 3. Only 100 are displayed by default. Only 100 sosreports would be collected if these were real hosts. Actual results: LC limits collection to 100 hosts from 100 clusters / DCs. Expected results: LC does not limit collection by collecting *all* possible entities from the API. Additional info:
(In reply to Lee Yarwood from comment #0) > Actual results: > LC limits collection to 100 hosts from 100 clusters / DCs. > > Expected results: > LC does not limit collection by collecting *all* possible entities from the > API. Note that collecting more than 100 hosts from 100 clusters / DCs will lead to have a tar.xz of several GB and take several hours to be created. I agree about not limiting the listing of the hosts but I'm not really sure about not limiting the collection.
(In reply to Lee Yarwood from comment #0) > When calling the API the LC does not provide a max value, limiting the > results to 100 by default. For environments with 100+ DCs, Clusters or hosts > this results in incomplete LCs being collected. > > We can workaround this by using rhevm-config to set a higher > SearchResultsLimit limit however the LC should be able to deal with > environments of this size without this workaround. Michael, is there any reason for not having unlimited list by default from the API call and requiring to limit the results size only where it's needed?
Another question for Michael, could we extend list() to allow max=unlimited to avoid setting an arbitrary max value?
(In reply to Sandro Bonazzola from comment #3) > (In reply to Lee Yarwood from comment #0) > > > When calling the API the LC does not provide a max value, limiting the > > results to 100 by default. For environments with 100+ DCs, Clusters or hosts > > this results in incomplete LCs being collected. > > > > We can workaround this by using rhevm-config to set a higher > > SearchResultsLimit limit however the LC should be able to deal with > > environments of this size without this workaround. > > Michael, is there any reason for not having unlimited list by default from > the API call and requiring to limit the results size only where it's needed? in general it was done as optimization in terms of traffic/performance, i guess no need to explain what happens when N entities should be fetched from the DB (possible JOINs on the fly etc.), then they should be converted to the backed entities and in case of api, marshalled to xml, send over the wire, and in case of client, unmarshalled back to the client entity, it would make listing nearly impossible when N aims to infinity, having unlimited by default would make life harder to one that not familiar with api and to others using RHEV in same time, this why we've chosen a restrictive behaviour and introduced a pagination in the engine backuped by the querying mechanism. btw worth mentioning that it's true only for the searchable collections like hosts/vms/etc., non-searchable collections (all sub-collections + few root-collections) does not have this "limitation" by default as they unlikely to grow to big dimensions, they have shaping only.
(In reply to Lee Yarwood from comment #6) > Another question for Michael, could we extend list() to allow max=unlimited > to avoid setting an arbitrary max value? you can disable pagination by sending max=-1 (unlimited), but i'd avoid doing that in large environment (unless you have no choice), finding the right balance between amount & performance is better approach in my view.
la(In reply to Michael Pasternak from comment #8) > (In reply to Lee Yarwood from comment #6) > > Another question for Michael, could we extend list() to allow max=unlimited > > to avoid setting an arbitrary max value? > > you can disable pagination by sending max=-1 (unlimited), but i'd avoid > doing that in large environment (unless you have no choice), finding the > right > balance between amount & performance is better approach in my view. Agreed however given we only make three calls when using the LC that itself isn't launched everyday I think max=-1 is valid for now. In the longer term the LC should really warn/advise users to filter based on DC, cluster etc and then in turn use the SDK/API to filter results within the .list() calls.
(In reply to Lee Yarwood from comment #9) > Agreed however given we only make three calls when using the LC that itself > isn't launched everyday I think max=-1 is valid for now. > > In the longer term the LC should really warn/advise users to filter based on > DC, cluster etc and then in turn use the SDK/API to filter results within > the .list() calls. I agree
afict, the 'max=-1' from api is not respected because of below code path: That said, if it is "-1" , we are taking 'SearchResultsLimit' value which defaults to 100. searchObj.setMaxCount(getParameters().getMaxCount() == -1 ? Config .<Integer> GetValue(ConfigValues.SearchResultsLimit) : getParameters().getMaxCount()); // setting FromSearch value searchObj.setSearchFrom(getParameters().getSearchFrom()); Isnt it ? --Humble
Fast workaround here is using a big number instead of -1. But since -1 was meant to mean unlimited, here it seems indeed we've a bug.
(In reply to Humble Chirammal from comment #13) > afict, the 'max=-1' from api is not respected because of below code path: > That said, if it is "-1" , we are taking 'SearchResultsLimit' value which > defaults to 100. > > > searchObj.setMaxCount(getParameters().getMaxCount() == -1 ? > Config > .<Integer> GetValue(ConfigValues.SearchResultsLimit) > : getParameters().getMaxCount()); > // setting FromSearch value > searchObj.setSearchFrom(getParameters().getSearchFrom()); > > > Isnt it ? > > --Humble eli, i remember us getting rid of this code? am i right?
(In reply to Michael Pasternak from comment #16) > (In reply to Humble Chirammal from comment #13) > > afict, the 'max=-1' from api is not respected because of below code path: > > That said, if it is "-1" , we are taking 'SearchResultsLimit' value which > > defaults to 100. > > > > > > searchObj.setMaxCount(getParameters().getMaxCount() == -1 ? > > Config > > .<Integer> GetValue(ConfigValues.SearchResultsLimit) > > : getParameters().getMaxCount()); > > // setting FromSearch value > > searchObj.setSearchFrom(getParameters().getSearchFrom()); > > > > > > Isnt it ? > > > > --Humble > > eli, > > i remember us getting rid of this code? am i right? Seems like a bug , maybe we have to set to maxint if it is -1 But we shoulkd carfully test the UI paging for this change
Pushed a new patch using 1000 as workaround. Eli please open another bug for tracking the backend issue.
A better solution has been proposed for 3.4 in bug #1015018. I suggest to keep this one for 3.3 and backport to 3.2.z.
(In reply to Sandro Bonazzola from comment #18) > Pushed a new patch using 1000 as workaround. Eli please open another bug for > tracking the backend issue. Done https://bugzilla.redhat.com/show_bug.cgi?id=1025320
(In reply to Eli Mesika from comment #17) > > > searchObj.setMaxCount(getParameters().getMaxCount() == -1 ? > > > Config > > > .<Integer> GetValue(ConfigValues.SearchResultsLimit) > > > : getParameters().getMaxCount()); > > > // setting FromSearch value > > > searchObj.setSearchFrom(getParameters().getSearchFrom()); > > > > > > > > > Isnt it ? > > > > > > --Humble > > > > eli, > > > > i remember us getting rid of this code? am i right? > > Seems like a bug , maybe we have to set to maxint if it is -1 > But we shoulkd carfully test the UI paging for this change Isnt it ( maxint ) a real big value ? I believe this can contribute to performance issues. I feel, we need to test it thoroughly.
Patch merged on upstream master and 3.3 branch
tested on rhevm-log-collector-3.3.1-3.el6ev.noarch I created 200 fake hosts and ran rhevm-log-collector list returns all hosts and not only 100
Added a new gerrit commit to not just limit to 1000 hosts, but use paging instead
Thanks Pablo, patch merged upstream on both master and 3.3 branches.
verified on rhevm-log-collector-3.3.1-4.el6ev.noarch
This bug is currently attached to errata RHBA-2013:15255. If this change is not to be documented in the text for this errata please either remove it from the errata, set the requires_doc_text flag to minus (-), or leave a "Doc Text" value of "--no tech note required" if you do not have permission to alter the flag. Otherwise to aid in the development of relevant and accurate release documentation, please fill out the "Doc Text" field above with these four (4) pieces of information: * Cause: What actions or circumstances cause this bug to present. * Consequence: What happens when the bug presents. * Fix: What was done to fix the bug. * Result: What now happens when the actions or circumstances above occur. (NB: this is not the same as 'the bug doesn't present anymore') Once filled out, please set the "Doc Type" field to the appropriate value for the type of change made and submit your edits to the bug. For further details on the Cause, Consequence, Fix, Result format please refer to: https://bugzilla.redhat.com/page.cgi?id=fields.html#cf_release_notes Thanks in advance.
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. http://rhn.redhat.com/errata/RHBA-2014-0039.html