Bug 1014379 - When calling the API the LC does not provide a max value, limiting the returned results to 100 by default.
When calling the API the LC does not provide a max value, limiting the return...
Status: CLOSED ERRATA
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine-log-collector (Show other bugs)
3.2.0
x86_64 Linux
urgent Severity high
: ---
: 3.3.0
Assigned To: Sandro Bonazzola
Tareq Alayan
integration
: Triaged, ZStream
Depends On:
Blocks: 1025409
  Show dependency treegraph
 
Reported: 2013-10-01 16:51 EDT by Lee Yarwood
Modified: 2014-01-21 12:00 EST (History)
17 users (show)

See Also:
Fixed In Version: rhevm-log-collector-3.3.1-4.el6ev
Doc Type: Bug Fix
Doc Text:
The log collector was limited to 1000 hypervisors, now that limit has been removed.
Story Points: ---
Clone Of:
: 1015018 1025409 (view as bug list)
Environment:
Last Closed: 2014-01-21 12:00:31 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
oVirt gerrit 19759 None None None Never
oVirt gerrit 20764 None None None Never
oVirt gerrit 21218 None None None Never
oVirt gerrit 21247 None None None Never

  None (edit)
Description Lee Yarwood 2013-10-01 16:51:04 EDT
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:
Comment 2 Sandro Bonazzola 2013-10-02 02:54:26 EDT
(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.
Comment 3 Sandro Bonazzola 2013-10-02 02:59:10 EDT
(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?
Comment 6 Lee Yarwood 2013-10-02 07:19:39 EDT
Another question for Michael, could we extend list() to allow max=unlimited to avoid setting an arbitrary max value?
Comment 7 Michael Pasternak 2013-10-03 04:58:14 EDT
(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.
Comment 8 Michael Pasternak 2013-10-03 05:04:07 EDT
(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.
Comment 9 Lee Yarwood 2013-10-03 05:49:42 EDT
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.
Comment 10 Sandro Bonazzola 2013-10-04 02:03:38 EDT
(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
Comment 13 Humble Chirammal 2013-10-30 03:33:15 EDT
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
Comment 15 Sandro Bonazzola 2013-10-31 07:57:40 EDT
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.
Comment 16 Michael Pasternak 2013-10-31 08:04:41 EDT
(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?
Comment 17 Eli Mesika 2013-10-31 08:48:39 EDT
(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
Comment 18 Sandro Bonazzola 2013-10-31 09:11:34 EDT
Pushed a new patch using 1000 as workaround. Eli please open another bug for tracking the backend issue.
Comment 19 Sandro Bonazzola 2013-10-31 09:15:49 EDT
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.
Comment 20 Eli Mesika 2013-10-31 09:35:26 EDT
(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
Comment 21 Humble Chirammal 2013-10-31 10:02:13 EDT
(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.
Comment 22 Sandro Bonazzola 2013-10-31 10:41:24 EDT
Patch merged on upstream master and 3.3 branch
Comment 25 Tareq Alayan 2013-11-05 09:53:57 EST
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
Comment 26 Pablo Iranzo Gómez 2013-11-13 10:48:21 EST
Added a new gerrit commit to not just limit to 1000 hosts, but use paging instead
Comment 27 Sandro Bonazzola 2013-11-14 03:07:46 EST
Thanks Pablo, patch merged upstream on both master and 3.3 branches.
Comment 28 Tareq Alayan 2013-11-21 07:23:34 EST
verified on rhevm-log-collector-3.3.1-4.el6ev.noarch
Comment 29 Charlie 2013-11-27 20:16:17 EST
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.
Comment 30 errata-xmlrpc 2014-01-21 12:00:31 EST
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

Note You need to log in before you can comment on or make changes to this bug.