These are chat archives for ManageIQ/manageiq/performance

26th
Apr 2016
Keenan Brock
@kbrock
Apr 26 2016 15:41

@Fryguy I'm facing a daunting task:
before: our belongs_to filters traverse objects to find all the children, then all their children, following the filter (parent_id type problem).
after: belongs_to filter generates a sql "query" that describes the relationship. this will get included into another query for filtering.

Does this seem doable? (this is the only area left in rbac that does this id lookup / merge gook)

aside: and I use the phrase "query" as air quotes -aka "laser"
Joe Rafaniello
@jrafanie
Apr 26 2016 15:43
@kbrock where is it a problem? I know it's very old, just not sure what brought you to them as opposed to any of the other old crusty areas?
Keenan Brock
@kbrock
Apr 26 2016 15:43
we have a bz open with a customer that this is causing hundreds of queries
and bringing back TONS of rows to only filter it in ruby
they have filters on all 3 tabs (tags, and both belongsto)
um. group filters
Joe Rafaniello
@jrafanie
Apr 26 2016 15:44
is hundreds of queries responsible for seconds of response time in the UI?
Keenan Brock
@kbrock
Apr 26 2016 15:44
minutes?
Jason Frey
@Fryguy
Apr 26 2016 15:44
@jrafanie Probably :)
Keenan Brock
@kbrock
Apr 26 2016 15:45
also, all other areas have been converted to use a single query
Jason Frey
@Fryguy
Apr 26 2016 15:45
belongs_to are basically a regular association disguised in the form of a tag
Keenan Brock
@kbrock
Apr 26 2016 15:45
this is the only place left that is insisting on doing a bunch of queries bring back ids
Jason Frey
@Fryguy
Apr 26 2016 15:45
I don't understand the parent_id bit
Joe Rafaniello
@jrafanie
Apr 26 2016 15:45
relevant, belongsto filters are moving on master to entitlements: ManageIQ/manageiq#8102
still largely unchanged though
Jason Frey
@Fryguy
Apr 26 2016 15:46
but that aside, I think converting that taglike thing to a query would be fine
Keenan Brock
@kbrock
Apr 26 2016 15:46
they will be unchanged
but it is using sends
and also using with_relationship_type('ems_metadata')
Oleg Barenboim
@chessbyte
Apr 26 2016 15:47
basic question - why don't we have a standard ActiveRecord association for the 2 types of filters?
Keenan Brock
@kbrock
Apr 26 2016 15:47
what is that?
@chessbyte yes - that is the end plan
heck, rbac will just be a standard "association / scope"
"next year" ;)
Oleg Barenboim
@chessbyte
Apr 26 2016 15:48
but I am saying move each of the filters to a real has_many or belongs_to association
Jason Frey
@Fryguy
Apr 26 2016 15:48
but is this rewrite of belongsto for a tactical fix? seems like there might be simpler wins there that aren't as potentially error-prone
Keenan Brock
@kbrock
Apr 26 2016 15:48
great question
Joe Rafaniello
@jrafanie
Apr 26 2016 15:48
@chrisarcand :point_up: April 26, 2016 11:47 AM
Keenan Brock
@kbrock
Apr 26 2016 15:49
this is the only piece left that forces us to query every id from the tables rather than just adding an additional where clause
yes, tactical
Oleg Barenboim
@chessbyte
Apr 26 2016 15:49
@Fryguy I think that getting RBAC to use scopes properly will speed up the whole product across the board
Jason Frey
@Fryguy
Apr 26 2016 15:49
@kbrock with_relationship_type is a context switch for the relationships table....basically an object can in live in multiple trees in the relationships table
@chessbyte I understand the long term goal but @kbrock said "I have a BZ for a customer..."
Keenan Brock
@kbrock
Apr 26 2016 15:49
lucy gave a quick fix for them
Jason Frey
@Fryguy
Apr 26 2016 15:50
ah ok
Keenan Brock
@kbrock
Apr 26 2016 15:50
but this is showing the performance issues
sorry - let me clarify
this is tactical
but the customer issue showed me that this is performance too
Jason Frey
@Fryguy
Apr 26 2016 15:50
oh the BZ is not performance related in itself, but something else that exposed a performance issue?
Keenan Brock
@kbrock
Apr 26 2016 15:50
didn't mean to imply that this work is for 5.5 customer
Oleg Barenboim
@chessbyte
Apr 26 2016 15:50
@kbrock - the question is do we need to give customer a fix in the next few days for the performance issue?
Keenan Brock
@kbrock
Apr 26 2016 15:50
no, bz is performance
no
Jason Frey
@Fryguy
Apr 26 2016 15:50
I'm so lost
Oleg Barenboim
@chessbyte
Apr 26 2016 15:51
so, it is NOT tactical then
Keenan Brock
@kbrock
Apr 26 2016 15:51
@chessbyte lucy found this problem
lucy fixed with another fix
punted on this
customer will be happy
Oleg Barenboim
@chessbyte
Apr 26 2016 15:51
@kbrock - so, is it tactical or not?
Jason Frey
@Fryguy
Apr 26 2016 15:51
let me phrase another way...are you planning to backport the belongsto rewrite you are proposing in order to fix a customer?
Keenan Brock
@kbrock
Apr 26 2016 15:51
tactical. a) performance boost b) scope not queryies
no backport
Jason Frey
@Fryguy
Apr 26 2016 15:52
then it is not tactical
Oleg Barenboim
@chessbyte
Apr 26 2016 15:52
when @Fryguy and I hear tactical and a BZ, then we are trying to give a fix to a customer asap
sounds like this is NOT the case
Jason Frey
@Fryguy
Apr 26 2016 15:52
tactical means you do a suboptimal fix that introduces little risk and takes little time to implement
Keenan Brock
@kbrock
Apr 26 2016 15:52
thanks
Oleg Barenboim
@chessbyte
Apr 26 2016 15:52
@Fryguy agree
Keenan Brock
@kbrock
Apr 26 2016 15:52
not tactical
It is longer term
Jason Frey
@Fryguy
Apr 26 2016 15:52
you know the kind of fixes where you "jam it in a column" haha
Keenan Brock
@kbrock
Apr 26 2016 15:52
but it "gives us an advantage in the future"
yup
and they are good and all... just produce debt
Oleg Barenboim
@chessbyte
Apr 26 2016 15:53
@kbrock good - so now, back to your question. what was it again?
Keenan Brock
@kbrock
Apr 26 2016 15:53
lol
did I have a snowball's chance in hell of changing belongs to filter into a pure query instead of pulling back all these ids from the db
Chris Arcand
@chrisarcand
Apr 26 2016 15:53
Trying…to…catch…up...
Jason Frey
@Fryguy
Apr 26 2016 15:54
FYI, when you hear me say "strategic" fix, it means "long-term fix"
Keenan Brock
@kbrock
Apr 26 2016 15:54
thanks
stuff like with_relationship_type('ems_metadata') is a little confusing
Chris Arcand
@chrisarcand
Apr 26 2016 15:54
https://gitter.im/ManageIQ/manageiq/performance?at=571f8d8ae8a4670f2b5ca9d4 @chessbyte Because it wasn’t that way originally and hasn’t been done yet :D Tons and tons of work, filters are manipulated all over the place and thrown into that column :sob:
Jason Frey
@Fryguy
Apr 26 2016 15:54
@kbrock re: with_relationship_type; so, for example a VM is in a tree named ems_metadata and also in a tree named "genealogy"
when you say .children on a VM, it needs to know which tree
Keenan Brock
@kbrock
Apr 26 2016 15:55
I'm basically in here
Oleg Barenboim
@chessbyte
Apr 26 2016 15:55
@chrisarcand I know the history. My question is should they be moved into standard associations or do they make sense the way they are and just need to be optimized?
Keenan Brock
@kbrock
Apr 26 2016 15:55
@Fryguy what is 'ems_metadata'?
random name?
Jason Frey
@Fryguy
Apr 26 2016 15:56
it's the name of the tree that represents the folder/datacenter/cluster/host/etc
@chessbyte hahahah
Keenan Brock
@kbrock
Apr 26 2016 15:56
yes!
Chris Arcand
@chrisarcand
Apr 26 2016 15:56
@chessbyte I believe they’ll need to be changed for future tenancy work as they are rather difficult to deal with as it is and soon need to be ‘mergable’ :(
Keenan Brock
@kbrock
Apr 26 2016 15:56
but oleg, this is the one right before the boss battle
Chris Arcand
@chrisarcand
Apr 26 2016 15:56
lol :point_up: April 26, 2016 11:56 AM
Keenan Brock
@kbrock
Apr 26 2016 15:57
(this isn't even the hardest one :( )
Oleg Barenboim
@chessbyte
Apr 26 2016 15:57
@chrisarcand so, it sounds like we agree that they should be re-designed into standard AR associations
@kbrock always looking for a challenge
Keenan Brock
@kbrock
Apr 26 2016 15:58
you have to get this into sql before associations can work - right?
Chris Arcand
@chrisarcand
Apr 26 2016 15:58
I don’t know about associated tables necessarily, but that their format needs to be essentially rewritten, yeah.
Keenan Brock
@kbrock
Apr 26 2016 15:58
eh
their format is not so bad
Chris Arcand
@chrisarcand
Apr 26 2016 15:59
s/rewritten/stored quite a bit differently than they are, likely changing how they are used and manipulated across the app drastically.
Oleg Barenboim
@chessbyte
Apr 26 2016 15:59
but looking at the comment: # /belongsto/ExtManagementSystem|<name>/EmsCluster|<name>/EmsFolder|<name>, I would rather have has_many belongs_to_filters and each one point at something in a table so it does not need parsing
Keenan Brock
@kbrock
Apr 26 2016 15:59
chris - are you saying the filter, or the query?
um, the description of the query (the filter) or the actual query?
because while the filter could be written nicer, it is the query that is of concern to me
Oleg Barenboim
@chessbyte
Apr 26 2016 16:00
the fact that we are parsing a string to figure out associations sounds sub-optimal as @matthewd would say
Keenan Brock
@kbrock
Apr 26 2016 16:00
lol
anytime you see "constantize"...
Chris Arcand
@chrisarcand
Apr 26 2016 16:01
We’re talking about two different things related intimately, I’m just answering a previous ping :D
Keenan Brock
@kbrock
Apr 26 2016 16:01
:)
Chris Arcand
@chrisarcand
Apr 26 2016 16:01
@chessbyte Heh, indeed. MiqFilter is my favorite part of the app.
Oleg Barenboim
@chessbyte
Apr 26 2016 16:01
glad you found a favorite already!
Jason Frey
@Fryguy
Apr 26 2016 16:03
what I don't understand about belongto filters is that I think only the tail is necessary
the rest is the entire metadata tree, but I don't know why this is needed
Keenan Brock
@kbrock
Apr 26 2016 16:03
yea
I was there
Chris Arcand
@chrisarcand
Apr 26 2016 16:03
@Fryguy My money is on building the UI tree.
Oleg Barenboim
@chessbyte
Apr 26 2016 16:03
because belongsto filters were written before the relationships table?
Keenan Brock
@kbrock
Apr 26 2016 16:03
but if you look, the object ids are brought back along the way
Jason Frey
@Fryguy
Apr 26 2016 16:04
@chessbyte Maybe, but the relationships themselves have always been there
you could always have done... cluster.parent_folder.parent_folder
Keenan Brock
@kbrock
Apr 26 2016 16:05
in many places, you only care about the end object
Jason Frey
@Fryguy
Apr 26 2016 16:05
@chrisarcand What do belongstos usually point to at the end? A Vm? Or like a Cluster?
Keenan Brock
@kbrock
Apr 26 2016 16:05
as we see with reports and our virtual columns
Jason Frey
@Fryguy
Apr 26 2016 16:05
@kbrock do you see the other parts being used?
Oleg Barenboim
@chessbyte
Apr 26 2016 16:05
yes, maybe @gtanzillo can lend a bit of perspective on this
Keenan Brock
@kbrock
Apr 26 2016 16:05
but in this case, you not only want joins to to through the relationships, but you also want particular values in the middle
you have where clauses in the middle relationships
Jason Frey
@Fryguy
Apr 26 2016 16:06
that do what?
rbac?
Keenan Brock
@kbrock
Apr 26 2016 16:06
the filter itself
no?
Jason Frey
@Fryguy
Apr 26 2016 16:06
are you telling me or guessing?
Keenan Brock
@kbrock
Apr 26 2016 16:06
looking
Jason Frey
@Fryguy
Apr 26 2016 16:06
haha
Oleg Barenboim
@chessbyte
Apr 26 2016 16:06
and the filter is part of RBAC, I think?
Keenan Brock
@kbrock
Apr 26 2016 16:06
yes
Oleg Barenboim
@chessbyte
Apr 26 2016 16:06
because, why not?
Keenan Brock
@kbrock
Apr 26 2016 16:06
part of rbac
@Fryguy the traversing code
is picking out each of the nodes
and it also has values in there
this works for cluster 5 and 6
and for hosts 2 and 3 in cluster 5
and for hosts 6 and 7 in cluster 6
seems that it cares about the whole chain, not just the end (vms)
Chris Arcand
@chrisarcand
Apr 26 2016 16:08
:pointup: April 26, 2016 12:05 PM @Fryguy I think either. Agreed though that you _should only need the tail, the rest can/should be deduced rather than shoving a UI detail in that (if that’s really all that it’s used for)
Jason Frey
@Fryguy
Apr 26 2016 16:08
right, that's what I'm think @chrisarcand
Chris Arcand
@chrisarcand
Apr 26 2016 16:08
Damn you Gitter.
Jason Frey
@Fryguy
Apr 26 2016 16:09
if it points to, say, a host, then you take that host and do host.ancestors and you are done...you have the parent chain
Keenan Brock
@kbrock
Apr 26 2016 16:09
looking at the code, I also see it using constantize on every level, which suggests that the end game is all that matters
Jason Frey
@Fryguy
Apr 26 2016 16:09
no need to parse a string and do ids manipulation
or host.ems_cluster or whatever if you need a specific level
really need @gtanzillo to understand the motivation behind the format, if he can remember
Keenan Brock
@kbrock
Apr 26 2016 16:10
but I don't want to lookup the host and all those ids to call that code
I want vm.where(ems_cluster => host => $filter_hosts)
Jason Frey
@Fryguy
Apr 26 2016 16:11
yeah, that's going to be really tough to do with the relationships table
well, it will be fine specifically for cluster/host/vm because those use normal rails relationships
but as soon as you introduce folders or resource_pools it gets complicated
which is why I asked what the tails of belongsto are
Keenan Brock
@kbrock
Apr 26 2016 16:13
yea
aah
so maybe I just need to solve for 1 belongsto case
and the answer is anything that can be passed into rbac as a "belongsto"
Keenan Brock
@kbrock
Apr 26 2016 16:25
ok, answer is: hosts & clusters and Vms & templates - so it has folders
Gregg Tanzillo
@gtanzillo
Apr 26 2016 16:26
Trying to read back… What’s the question directed to me?
Keenan Brock
@kbrock
Apr 26 2016 16:27
lol
run away
I'm rewriting Rbac.get_belongsto_matches to not hit the db but rather produce a (sub?)query
so that links to MiqFilter.belongsto2object_list which currently fetches ids at every level and then pulls back more ids at the next level.
the question
would it be possible to only care about the base level (@fryguy did I get that right?)
Jason Frey
@Fryguy
Apr 26 2016 16:31
more or less :)
Gregg Tanzillo
@gtanzillo
Apr 26 2016 16:32
Got it… Yeah, from my recollection, that functionality was pretty difficult to write.
Jason Frey
@Fryguy
Apr 26 2016 16:32
basically a belongs_to filter appears like a tree path "/belongsto/ExtManagementSystem|<name>/EmsFolder|<name>/EmsCluster|<name>/"
or something like that
but is the entire tree needed or just the tail?
@chrisarcand or @kbrock Do you have an example of a "real" belongsto filter so we have a concrete example?
Gregg Tanzillo
@gtanzillo
Apr 26 2016 16:34
I think you have to consider the full hierarchy because You can have duplicate named things under multiple patchs. I think that the full hierarchy is needed to qualify it.
Jason Frey
@Fryguy
Apr 26 2016 16:34
(so we can be on the same page when talking)
but why is name important?
that is, could one just have a normal rails belongs_to relationship to the tail and be done with it?
Gregg Tanzillo
@gtanzillo
Apr 26 2016 16:35
To prevent showing objects that live under a parth that user is not entitled to see
Yes, I think that would be fine
Jason Frey
@Fryguy
Apr 26 2016 16:35
but then normal RBAC could post filter that
right
Gregg Tanzillo
@gtanzillo
Apr 26 2016 16:36
I think the full path is necessary for assigning the belongs_to filter, tho
Keenan Brock
@kbrock
Apr 26 2016 16:58
@Fryguy the customer's filter (names changed to protect the innocent):
["/belongsto/ExtManagementSystem|VC01/EmsFolder|Datacenters/EmsFolder|Ctr1/EmsFolder|host/EmsCluster|CP-ETE", "/belongsto/ExtManagementSystem|VC01/EmsFolder|Datacenters/EmsFolder|Ctr1/EmsFolder|vm/EmsFolder|QA", "/belongsto/ExtManagementSystem|VC01/EmsFolder|Datacenters/EmsFolder|Ctr1/EmsFolder|vm/EmsFolder|Development", "/belongsto/ExtManagementSystem|VC01/EmsFolder|Datacenters/EmsFolder|Ctr1/EmsFolder|vm/EmsFolder|Templates"]
Daniel Berger
@djberg96
Apr 26 2016 16:59
@chrisarcand I see some broken image links
Chris Arcand
@chrisarcand
Apr 26 2016 17:01
I blame @lpichler :stuck_out_tongue_winking_eye:
Daniel Berger
@djberg96
Apr 26 2016 17:01
Is nil padding out of the question?
Generally, I'd lean towards a JSON solution, though.
Chris Arcand
@chrisarcand
Apr 26 2016 17:10
^ That’s what I said ;)
Jason Frey
@Fryguy
Apr 26 2016 17:30
Yeah, I think link to the tail and derive the rest, but I don't understand why the rest is needed
It all feels UI related, as @chrisarcand said
Daniel Berger
@djberg96
Apr 26 2016 17:33
@chrisarcand sorry, meant "your" json solution there
(for some reason it's not letting me edit)
Chris Arcand
@chrisarcand
Apr 26 2016 17:46
oh hah not trying to take credit, just thought you came to the same conclusion. This idea was before I understood the craziness of the intention behind the nesting format of managed filters; I’m not entirely sure how it’d be done with that in mind.
Daniel Berger
@djberg96
Apr 26 2016 17:57
Store it in json format, and create/override a model method that reformats as needed?
I guess I don't know enough of your problem domaine to give better suggestions.
Chris Arcand
@chrisarcand
Apr 26 2016 18:15
That’s my initial thought as well.
Keenan Brock
@kbrock
Apr 26 2016 18:17
sorry had to get :fries: