| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-07 | |||
| 11:48:26 | cdent | building in cache from the outset almost always leads to harder to maintain code | |
| 11:48:29 | mdbooth | Returning no values or some values doesn't necessarily mean that you're not out of date | |
| 11:50:06 | mdbooth | cdent: How often will we hit this thing? Would an index optimised for a startswith query be appropriate? | |
| 11:50:47 | cdent | (thus my concerns about early optimizations and speculations before measuring, etc) | |
| 11:50:53 | mdbooth | From reading the context, the expectation seems to be a lot. Whose was that expectation? | |
| 11:51:02 | alex_xu | cdent: in the poc, actually I query the db directly. but I try to get the API "GET /traits?name=in:TRAIT1,TRAIT2" to use the cache, since that is the API used to validate whether the traits is valid or not | |
| 11:51:18 | cdent | mdbooth: it will be alot, eventually | |
| 11:51:31 | cdent | but that's based on abstractions, not measurement | |
| 11:52:03 | mdbooth | Are we carefully avoiding the db throughout this whole code path? | |
| 11:52:36 | alex_xu | wait, no, that is for "GET /traits?prefix=xxx", that can be from the db directly. "GET /traits?name=in:TRAIT1,TRAIT2" can get benefit from the single trait lookup | |
| 11:53:24 | cdent | but I haven't had a chance yet | |
| 11:53:29 | mdbooth | alex_xu: Yeah, I think the concrete lookup is ok to cache. Either you get a hit (but wait... what if it was deleted?), or you don't, in which case you can refresh. | |
| 11:53:30 | cdent | (been concentrating on the spec) | |
| 11:54:33 | alex_xu | mdbooth: yea, if user query a non-existed trait, that will trigger a db call | |
| 11:54:55 | mdbooth | alex_xu: Deleting a cached trait/resource class is also an interesting case. | |
| 11:55:38 | mdbooth | Does the user have an expectation that after a delete call completes, a subsequent lookup will fail? | |
| 11:55:43 | mdbooth | What if it doesn't? | |
| 11:55:59 | mdbooth | I suspect that probably doesn't matter as much as the other case. | |
| 11:56:48 | alex_xu | yes, I remember I point out that problem from the resource class patch, but I forget what answer I got now | |
| 11:57:07 | cdent | you guys are proving my point | |
| 11:57:10 | cdent | first make it right | |
| 11:57:12 | cdent | then add the cache | |
| 11:57:49 | mdbooth | alex_xu: Would cdent's suggestion fly? | |
| 11:57:52 | jvarlamova__ | rpodolyaka: by the way, some instances were created successfully while other got an exception. Is it a known issue of parallel instances creation? | |
| 11:58:10 | alex_xu | cdent: all the cache? even for single trait lookup? | |
| 11:59:03 | cdent | theoretically, yes: that's just good practice: make code that does the right thing | |
| 11:59:10 | cdent | only once it is doign the right thing consider adding cache | |
| 11:59:22 | cdent | the mix between custom traits and the os-traits library makes things a bit weird in all this | |
| 11:59:53 | cdent | but since the number of traits is, let's say, less than 10,000 python can happily work with lists of strings of that size | |
| 12:00:23 | cdent | but again: I haven't looked at the codet yet! :) | |
| 12:00:49 | cdent | I just hear a lot of early optimizating, and feel like that's worth flagging before we get too caught up in the details of that when we don't even have a fully working api yet | |
| 12:00:56 | alex_xu | cdent: yea, that sounds make sense. hope we can catch jay also. I feel he will say something when I remove the cache :) | |
| 12:01:14 | cdent | I'm sure he will, jay is a notorious early optimizer :) | |
| 12:01:39 | alex_xu | cdent: heh :) | |
| 12:01:47 | mdbooth | alex_xu: Anyway, I'm going to remind you again that I don't have nearly enough context here. I can only point out potential problems and ask if they're real problems. | |
| 12:02:02 | mdbooth | alex_xu: Don't mean to be a pita. | |
| 12:02:22 | cdent | if what we're really concerned about is performance of the api then we should, _later_, consider a namespaced memcached layer on top of the placement api that allows marking collections as dirty | |
| 12:02:35 | alex_xu | mdbooth: appreciate your review same, it is really helpful :) | |
| 12:02:54 | cdent | alex_xu, mdbooth: I think we should have lots more of this type of conversation, so I think this is good | |
| 12:03:10 | alex_xu | mdbooth: btw, the spec is at https://review.openstack.org/345138, in case you are interesting :) | |
| 12:03:29 | mdbooth | alex_xu: Thanks, I should probably have read that first. | |
| 12:04:12 | cdent | I think jay's concerns are at the database layer, in which case we can address that by simply having all the traits in the same table, and isn't that being done when we sync from os-traits? | |
| 12:04:56 | alex_xu | cdent: yes, all the traits in one table | |
| 12:05:07 | cdent | then why have a cache at all? | |
| 12:05:11 | alex_xu | by 'placement-manage os-traits sync' | |
| 12:06:16 | alex_xu | cdent: I guess jay think the trait/resource class won't update frequently, then to same some db call... | |
| 12:06:37 | cdent | :) | |
| 12:06:37 | cdent | -1 | |
| 12:06:43 | alex_xu | :) | |
| 12:06:58 | cdent | with the resource class stuff the cache is because the classes are _not_ all int he same table | |
| 12:07:10 | cdent | with traits, if they are, then... | |
| 12:07:32 | alex_xu | ah... | |
| 12:07:40 | alex_xu | yea a abstraction layer | |
| 12:09:40 | alex_xu | that is why I put the traits sync in the cache before, now we pull that sync out.. | |
| 12:09:47 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: List/show all server migration types https://review.openstack.org/430608 | |
| 12:10:44 | cdent | mdbooth: in a round about way you have done a good deed. if your quota is one a day, you're done, good job, time for a beer. | |
| 12:22:30 | openstackgerrit | Takashi NATSUME proposed openstack/nova-specs master: List/show all server migration types https://review.openstack.org/407237 | |
| 12:24:25 | openstackgerrit | Danil Akhmetov proposed openstack/nova master: Use proper user and tenant in the owner section of libvirt.xml. https://review.openstack.org/399679 | |
| 12:27:28 | gibi | bauzas: Hi! thanks for looking at the scheduler hint API spec. I suggested a way to handle missing hints in the review. | |
| 12:28:50 | rfolco | cdent, review_begging++ https://review.openstack.org/#/c/415885/ | |
| 12:31:12 | cdent | rfolco: yup that's on my list for today | |
| 12:31:23 | rfolco | cdent, thx :) | |
| 12:32:56 | lyarwood | mdbooth: have you played around with lvm2 and sparse files yet? | |
| 12:33:14 | lyarwood | mdbooth: we are already creating thinp volumes AFAICT | |
| 12:34:25 | lyarwood | mdbooth: http://paste.openstack.org/show/601745/ | |
| 12:34:39 | openstackgerrit | Steven Webster proposed openstack/nova master: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/382853 | |
| 13:15:17 | jsheeren | hi, i'm having issues with nova-spiceproxy and spice-html5 using Windows instances. Everything is upside down, which makes it a little difficult to use the horizon console. Has anyone seen this as well? | |
| 13:16:21 | jsheeren | with using spice-html5 from git, i'm seeing the same upside down console | |
| 13:16:37 | jsheeren | with linux instances there is no problem at all, spice console works as expected | |
| 13:22:29 | mnaser | hi nova team, is there plans of cutting a stable/newton release soon? | |
| 13:22:47 | mnaser | we had a lot of problems with upgrades re this: https://review.openstack.org/#/c/408725/ -- we manually worked around it but it would be nice to not have to | |
| 13:28:35 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Use Cinder API v3 as default https://review.openstack.org/420201 | |
| 13:52:15 | Dinesh_Bhor | Vek: Hi, Could you please take a look at it? https://review.openstack.org/#/c/441750/ | |
| 13:55:44 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add test for inventory with invalid ResourceClass https://review.openstack.org/442498 | |
| 13:59:11 | tdurakov | paul-carlton2 eliqiao shaohe_feng pkoniszewski yuntongjin mdbooth kashyap kmartin claudiub tobasco tdurakov abhishekk mrhillsman raj_singh wznoinsk siva_krishnan live migration subteam meeting in 2 minutes on #openstack-meeting-3 | |
| 14:09:07 | cdent | edleafe gets the pedant of the day award | |
| 14:09:38 | edleafe | cdent: After all, I used to head the Department of Redundancy Department | |
| 14:10:31 | cdent | You sure you weren't the head chairperson? | |
| 14:10:49 | sfinucan | sdague: Could I ask you to take a look at this patch and dependencies? There are some merge conflicts in later patches and I don't want to push a 15 patch series rebase again https://review.openstack.org/#/c/431022 | |
| 14:11:16 | sfinucan | it's focused on deprecating remaining nova-net options (of which there are many) | |
| 14:12:14 | sdague | sfinucan: ok, I guess I wonder why it's in 8 patches? | |
| 14:12:38 | sfinucan | sdague: I figured that it would be easier to review in chunks. I can combine if not | |
| 14:14:36 | markus_z | Anyone here has a vagrant multinode setup with Ubuntu1604 and likes to share it? | |
| 14:15:00 | sdague | honestly, I think that all the deprecations in nova/conf/network.py as one patch might make it simpler | |
| 14:15:16 | sfinucan | sdague: Sure thing. Gimme two | |
| 14:15:38 | sdague | I also threw a quick comment in here - https://review.openstack.org/#/c/430914/3 | |
| 14:15:53 | sdague | just took me a second to realize how that could be not used any more | |
| 14:21:17 | sfinucan | sdague: Ah, I remember why I broke it up. The options were used by different components of nova-network and I gave that reason in the commit message (to avoid forcing people to go root that out themselves) | |
| 14:21:37 | sdague | sfinucan: ok, well it could just be a long commit message | |
| 14:21:45 | sdague | I'm fine with long commit messages | |
| 14:21:49 | sfinucan | Aye, that's what I'll do now :) | |
| 14:32:44 | johnthetubaguy | lyarwood mdbooth: thanks for you great comments on this one, would be awesome if you could take another peak at that cinder API spec: https://review.openstack.org/#/c/373203/ | |
| 14:32:58 | mdbooth | johnthetubaguy: Will do | |
| 14:33:20 | lyarwood | johnthetubaguy: ack will try to later today | |
| 14:35:06 | mriedem | edleafe: on https://review.openstack.org/#/c/442254/ i'm not using nova/tests/functional/api/openstack/placement | |
| 14:35:09 | johnthetubaguy | lyarwood: mdbooth: awesome, thank you | |
| 14:35:12 | mriedem | i'm testing the scheduler report client | |
| 14:35:44 | mriedem | well the gabbits anyway | |
| 14:37:21 | mriedem | edleafe: your test is testing the placement API directly, which is good, but the functional test i added is testing the report client, which is needed for the logic in there, as noted here https://review.openstack.org/#/c/441543/2/nova/tests/unit/scheduler/client/test_report.py@1242 | |
| 14:37:35 | cdent | mriedem, edleafe yeah seems we need both | |