gistify247437.txt

From: Dave Tucker <datucker@redhat.com>
Subject: Re: interesting talk to Ed on irc about createMissingParents
Date: August 16, 2014 at 1:19:38 PM EDT
To: Flavio Fernandes <ffernand@redhat.com>
Cc: "rh-sdn-dev@redhat.com" <rh-sdn-dev@redhat.com>

Thanks Flavio,

I think the solution is therefore:

1) Change OF13 provider to createParents=true

2) Change the plugin-mdsal-adapter to create aNode that is both and OVSDB managed node and a flow capable node.

These could both be bug fixes as far as Helium is concerned...

Sent from my iPhone

On 15 Aug 2014, at 20:50, Flavio Fernandes <ffernand@redhat.com> wrote:


in case you miss this in your favorite irc channel. :)

Madhu, Dave: please correct me on anything I may have said that is wrong!

— flavio

[15:09:03] <flaviof>	edwarnicke: on the topic of creating parents / etc....
[15:09:14] <edwarnicke>	 Yed
[15:09:17] <edwarnicke>	 Yes
[15:09:23] <edwarnicke>	 Happy to answer as best I can
[15:09:26] <edwarnicke>	 What can I do to help?
[15:09:54] <flaviof>	edwarnicke: is it the case that OF expects createParents == true from apps like ovsdb?
[15:10:10] <edwarnicke>	 flaviof: That's a little bit of an oversimplification
[15:10:36] <edwarnicke>	 flaviof: Basically... it expects you to manage the lifecycle of things you care about... but gives you a backdoor with createParents==true to say 'fuck if I know'
[15:10:40] <edwarnicke>	 Let me give an examlpe
[15:10:44] <edwarnicke>	 StatsManager
[15:10:53] <edwarnicke>	 StatsManager writes stats for flows for a node
[15:11:04] <flaviof>	dave_tucker Madhu_offline: ^^^^
[15:11:11] <edwarnicke>	 But because we are in a networked world
[15:11:22] <edwarnicke>	 It can happen that we get stats back *after* the switch has disconnected
[15:11:31] <edwarnicke>	 When the switch disconnects
[15:11:39] <edwarnicke>	 Its node is removed from the operational store
[15:11:48] <edwarnicke>	 Because we literally have no operational data for it anymore
[15:11:53] <edwarnicke>	 So... for StatsManager
[15:11:59] <edwarnicke>	 It shouldn't write stats if a node doesn't exist
[15:12:09] <edwarnicke>	 For StatsManager, createParent==true would be the wrong thing
[15:12:20] <edwarnicke>	 Because of the particularlities of what it is doing
[15:12:23] <edwarnicke>	 flaviof: With me so far?
[15:12:30] <flaviof>	edwarnicke: yes!
[15:12:44] <edwarnicke>	 Cool :)
[15:13:06] <edwarnicke>	 So... 'the right thing' is going to vary a bit from app to app
[15:13:22] <edwarnicke>	 I honestly don't know what the right thing is for you guys (though happy to dig deeper to offer advice)
[15:13:33] <edwarnicke>	 So... tell me of your use case :)
[15:13:36] <edwarnicke>	 What are you guys doing
[15:13:49] <edwarnicke>	 And we can try to figure out together what makes the most sense to you :)
[15:13:56] <flaviof>	 in the case of ovsdb's implementation, folks like Madhu_offline was under the assumption the it is not proper to create the node part of the tree; only the flowId portion. this is for the config tree
[15:14:42] <flaviof>	 so, what I came up with was to add a check like: i1 node portion of the tree, go ahead and create the missing part ovsdb is 'responsible' for in the config
[15:14:53] <flaviof>	 s/1i/if
[15:15:17] <edwarnicke>	 Is this for config data or operational data?
[15:15:28] <flaviof>	 conversely, if the node part was not there, we 'bail' and try again 'later'.
[15:15:46] <flaviof>	 this is for config; the only tree ovsdb touched -- at least in that codepath
[15:16:07] <edwarnicke>	 OK
[15:16:10] <edwarnicke>	 So lets talk through this
[15:16:15] <edwarnicke>	 Because:
[15:16:15] <flaviof>	edwarnicke: you can see that code here:https://github.com/opendaylight/ovsdb/blob/master/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/OF13Provider.java#L2698
[15:16:27] <edwarnicke>	 you understanding and deciding >> than me suggesting 'do x'
[15:16:46] <edwarnicke>	 So... on the config tree side
[15:17:00] <edwarnicke>	 Who do you expect to have created the node you are trying to config?
[15:17:01] <flaviof>	edwarnicke: oh no; i think your input is way more valuable here.
[15:17:35] <flaviof>	 that is a really good question. I think in Madhu_offline's mind it is somebody else -- not ovsdb.
[15:17:59] flaviof	 really wished Madhu_offline was here, so flaviof does not mis-speak
[15:18:03] <edwarnicke>	 OK... so were I you, I would want to understand two things:
[15:18:11] <edwarnicke>	 1)  Who do I expect to create the parent if not me
[15:18:25] <edwarnicke>	 2)  Do I expect anyone to delete the parent, and what would that delete mean
[15:18:32] <edwarnicke>	 3)  What should I do if someone deleted the parent
[15:18:37] <edwarnicke>	 Because you can only not have a parent if:
[15:18:41] <edwarnicke>	 1)  Nobody has created it yet
[15:18:46] <edwarnicke>	 2)  Somebody has deleted it
[15:19:08] <edwarnicke>	 In the case of StatsManager, because it is dealing with operational data, the answer to those questions is:
[15:19:23] <flaviof>	edwarnicke: 3) nobidy cares for having any configs for it (besides ovsdb)?!?
[15:19:24] <edwarnicke>	#1 StatsManager expected InventoryManager to have created the node
[15:20:01] <edwarnicke>	 Quick edit: #1 StatsManager expected InventoryManager to have created the node  *in the operational store*
[15:20:16] <flaviof>	 interesting...
[15:20:27] <edwarnicke>	#2 StatsManager expects InventoryManager to delete the node *from the operational store* if the switch has gone away
[15:20:53] <edwarnicke>	#3 StatsManager should not write stats for a disconnected switch, and so shouldn't write stats to a node that isn't already *in the operational store*
[15:21:31] <edwarnicke>	 Which means that
[15:21:31] <edwarnicke>	#1 If nobody has created a node, StatsManager should not create it in the operational store.
[15:21:49] <edwarnicke>	#2 Stats Manager should not recreate the node if someone has deleted it from the operational store
[15:21:53] <edwarnicke>	 flaviof: Make sense so far?
[15:22:06] <flaviof>	edwarnicke: yes!
[15:22:17] <edwarnicke>	 OK... your turn :)
[15:22:25] <edwarnicke>	 Can you answer those questions for your use case?
[15:22:34] <edwarnicke>	 (note: you are config store... so its interesting ;) )
[15:22:45] <flaviof>	edwarnicke: hehe... right.
[15:23:05] <edwarnicke>	 flaviof: So go for it :)  Lets see your answers :)
[15:23:14] <flaviof>	 so b/c this is about config store, what statsMgr does / does not do it really does not matter
[15:23:29] <flaviof>	 ok... let me git it a stab
[15:23:36] <flaviof>	 git/give
[15:24:01] <flaviof>	#1 inventoryManager?
[15:24:31] <flaviof>	 It seems that if nothing added node to the config tree, ovsdb shouln't either...
[15:24:39] <edwarnicke>	 InventoryManager only writes to the operational store
[15:24:48] <edwarnicke>	 Its just letting you know that operationally, there's a node there ;)
[15:24:51] <flaviof>	edwarnicke: yeah, that is news to me
[15:25:01] <edwarnicke>	 flaviof: And this is why we have these little chats ;)
[15:25:16] <flaviof>	 yeah. this is great
[15:26:03] <flaviof>	#2 i think that the deletion of node is also a blur in my head. I'll defer to the code on what it does. My gut tells me that it
[15:26:19] <edwarnicke>	 So config is quite different than operational
[15:26:31] <edwarnicke>	 And honestly, createParent==true makes a lot more sense there
[15:26:34] <edwarnicke>	 For example
[15:26:42] <flaviof>	 will only delete the portion of the tree on the flowId part. and its is intersting if the tree was completely gone by then.
[15:26:45] <edwarnicke>	 You guys may be the first folks to have ever written config data about a node
[15:27:16] <edwarnicke>	 flaviof: I would say it sort of rude for another app to delete the *whole* node and thus everyones configs... but I'm a courtesy freak ;)
[15:27:29] <flaviof>	edwarnicke: indeed. :)
[15:27:53] <flaviof>	 If you look at the deleteFlow code, can you tell that it is only deleting the 'upper section of the tree?
[15:27:58] <edwarnicke>	 So.. can you think of anyone who would create the *config tree* node for you?
[15:28:03] <flaviof>https://github.com/opendaylight/ovsdb/blob/master/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/OF13Provider.java#L2731
[15:28:48] <edwarnicke>	 flaviof: Looks like you are taking care to only delete your own stuff
[15:28:49] <flaviof>	edwarnicke: i honestly do not. now knowing what you told me. it could be that dave_tucker, Madhu_offline do, tho; I'm just still learning that code
[15:28:57] <flaviof>	edwarnicke: right.
[15:29:06] <flaviof>	 sorry for mixing #1 and #2 in the talk
[15:30:20] <edwarnicke>	 flaviof: No need to apologize at all :)
[15:30:24] <flaviof>	 on #3, I think is is nothing but a variant of #1, when no other app created the 'fixed' part of the tree
[15:30:54] <edwarnicke>	 flaviof: I broke things down the way I did to try to be clearer, not because my breakdown is magically right or wrong ;)
[15:31:11] <flaviof>	 it was very helpful
[15:31:19] <edwarnicke>	 flaviof: Yes, in many cases #3 and #1 are just the same thing
[15:31:26] <edwarnicke>	 (not all, but many)
[15:31:46] <edwarnicke>	 So what do you think then is the write thing for you guys?
[15:32:39] <flaviof>	 okay... so I guess the question standing is... what is the proper way of doing this? create all missing parents but only remove the part of the tree that the app has 'authority' over? or...?
[15:33:15] <edwarnicke>	 flaviof: So, I suspect the most common pattern for *config* data would be:
[15:33:27] <flaviof>	 i'd defer it to Madhu_offline + dave_tucker. but I also want to hear from md-sal gurus on what they would expect,agree?
[15:33:34] <edwarnicke>	 1)  Use createParent==true unless you know someone is magically managing your parent
[15:33:46] <edwarnicke>	 2)  Use the most granular delete that makes sense (so you don't get side effects)
[15:34:11] <edwarnicke>	 On #1, it really comes down to 'Is someone managing the parent?  Is that someone you?'
[15:34:11] <flaviof>	 right. bare in mind that can lead to some 'leak' overtime.
[15:34:23] <flaviof>	 leak as in java's memory leak
[15:34:37] <edwarnicke>	 On #2 it comes down to 'Could I screw somebody else up here' (note: other threads of yours are somebody else too ;) )
[15:35:09] <edwarnicke>	 flaviof: Tell me of your leak concerns
[15:35:43] <flaviof>	 it could be that the md-sal gurus have already thought about this. but if nobody deletes the 'fixed' portion of the tree. then it will
[15:35:51] <flaviof>	 remain 'forever', agree?
[15:36:41] <edwarnicke>	 flaviof: Yes
[15:36:44] <edwarnicke>	 That is a good point
[15:36:44] <flaviof>	 it's a variant of #2: i will make sure I do not screw anybody, but this can be creating litter. :)
[15:36:54] <edwarnicke>	 Sort of a 'last one to leave turns out the lights'
[15:37:02] <flaviof>	edwarnicke: right
[15:37:22] <edwarnicke>	 The MD-SALs atttiude is basically 'Dude, we don't know how to figure out the right thing to do here... you guys sort it out'
[15:37:34] <flaviof>	 lol
[15:37:36] <edwarnicke>	 (which is a bit of a cop out... but we don't have anything smarter)
[15:38:28] <flaviof>	 i gotta say it is a bit 'scary' to think that app A can blow away the config for app B, just because they share a piece of their config tree.
[15:40:24] <flaviof>	 ... and back to square 1: createMissingParents, while sounding really convenient creates a 'nobodys land' piece of the tree
[15:40:49] <flaviof>	 assuming the delete code is a gentleman (like Madhu_offline) of course. :)
[15:41:45] <flaviof>	edwarnicke: i need to take a call. do you think we can bag this topic for now?
[15:42:06] <edwarnicke>	 flaviof: Totally :)
[15:42:14] <edwarnicke>	 flaviof: We have authz coming :)
[15:42:27] <edwarnicke>	 flaviof: Think unix file permissions (or ACLs) for the datatree
[15:42:42] <flaviof>	edwarnicke: lol
[15:42:52] <edwarnicke>	 flaviof: You have a good point on the delete though
[15:42:57] <flaviof>	edwarnicke: or ref counting?!?
[15:43:00] <edwarnicke>	 We need to think of a reasonable way to handle that for Lithium
[15:43:09] <edwarnicke>	 flaviof: I don't so much like refcounting
[15:43:29] <edwarnicke>	 But maybe a way to register a DataChangeListener to clean up empty trees
[15:43:39] <flaviof>	edwarnicke: me either... it feels like a pick your poison situation.
[15:43:52] <edwarnicke>	 If a node has no children in config... then for certain use cases, it could probably be deleted
[15:44:10] <edwarnicke>	 (I would say most, but I haven't thought deeply about it... so take that with a grain of salt)
[15:44:11] <flaviof>	edwarnicke: sounds reasonable