Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-05-16
15:36:28 dansmith cfriesen: maybe I can't see enough of your change.. are you saying you see a single request to refresh an instance multiple times in the same go?
15:36:30 dansmith sdague: yeah
15:36:51 sdague dansmith: I'm fine fixing it just for the systemd case
15:36:58 sdague we're default there everywhere
15:37:07 dansmith sdague: okay, but it'll only be for mysql
15:37:19 sdague oh ... let me go look again
15:37:26 cfriesen dansmith: we are coming into this code and there are multiple network-changed events for the same instance, so we end up calling self.network_api.get_instance_nw_info() multiple times
15:37:45 dansmith cfriesen: ah, okay I assumed this was in the periodic
15:37:49 dansmith cfriesen: why is that happening?
15:39:36 cfriesen dansmith: let me get more context, this was proposed by one of our other guys.
15:39:44 dansmith *facepalm*
15:39:54 sdague dansmith: what about DefaultLimitNOFILE= in /etc/systemd/system.conf
15:39:58 sdague then a daemon restart
15:40:11 dansmith sdague: will that get picked up by a daemon restart?
15:40:16 dansmith I mean reload
15:40:21 dansmith restart is a full re-exec right?
15:40:24 bauzas dansmith: quick question, should you think we would need a new RPC version for adding a "alternatives" key to the returned dict when calling the scheduler API method select_destinations() ?
15:40:29 sdague dansmith: I don't know
15:40:42 sdague but that's where the google's are leading me to
15:40:56 dansmith sdague: okay will try that
15:41:15 bauzas dansmith: my own opinion is that given we say to first upgrading the conductor, then the conductor is anyway newer that the scheduler, so it would know the new key
15:41:31 sdague dansmith: daemon-reload probably should be tried first
15:41:37 dansmith sdague: yeah
15:41:44 openstackgerrit dane-fichter proposed openstack/nova master: Add trusted certificates to InstanceExtras https://review.openstack.org/457711
15:41:49 dansmith bauzas: of course, if you're changing the rpc api... you should version that change :)
15:41:56 bauzas dansmith: but I could just add a new RPC API version for not returning the new key in case the asked version is not cool
15:42:16 bauzas dansmith: I'm not modifying the interface
15:42:23 bauzas dansmith: so it's not needing a version
15:42:36 bauzas dansmith: I'm just adding some new key for what we return
15:42:55 bauzas but lemme just upload the change
15:43:00 bauzas and you'll see
15:43:39 dansmith changing the return is changing the api, period.
15:44:25 bauzas okay
15:50:56 cfriesen dansmith: with regards to the multiple network-changed events for the same instance, this is in the context of a live-migration on a busy system. Adding to the pain is that the "with lockutils.lock" took a long time to be acquired (which seems to be due to the lack of a "fair" lock in oslo).
15:54:46 dansmith cfriesen: not sure I get it, but if that is just avoiding processing the same instance in a single go, then I guess it makes sense
15:54:55 dansmith (but should use a set() of course)
16:02:38 cfriesen dansmith: yes, it's just to try to process each instance once. I guess neutron must be batching multiple events in a single call.
16:02:53 dansmith I'm pretty sure they don't
16:03:30 cfriesen dansmith: then why is the code written to handle multiple events per call?
16:03:35 dansmith tbh, I think I'd want an analysis of that first.. like "neutron started batching things in commit <foo> therefore we need this"
16:03:36 dansmith otherwise I'd be highly suspect that something else is going on
16:04:00 dansmith cfriesen: because the whole thing was written to be high bandwidth, but AFAIK, they never used it that way
16:04:24 dansmith and multiple events of the same instance in a single batch hardly makes sense
16:04:47 dansmith if they're doing that, then there needs to be a fix on both sides, but if you just paper over it here, we'll never find that
16:08:44 melwitt dansmith, sdague: I did some tests where I ran a subset of tempest locally with concurrency of 4 with and without caching oslo.db transaction context managers and with, it topped out around 90 Threads_connected in mysql and without it was around 140. so it did seem to make a difference. this is the patch I tried https://review.openstack.org/#/c/465042
16:09:16 openstackgerrit Gage Hugo proposed openstack/nova master: Admin API Policy contingent on is_admin_project https://review.openstack.org/384148
16:10:26 sdague melwitt: ok, cool, so that's definitely helpful
16:10:55 dansmith sdague: melwitt: I'm not sure I'm surprised by that, up to the limit of the connection pool
16:11:07 sdague melwitt: it feels like we should do that in addition to the raise of the open fds, right?
16:11:08 dansmith the question would be if that continues to grow forever
16:11:33 melwitt the subset I ran was like 'ostestr run --regex tempest.api.compute.servers --concurrency 4' which is about 206 tests
16:11:56 dansmith sdague: melwitt: we also need to keep in mind that we need to be able to flush this cache on SIGHUP if/when we implement that for the list-of-cells cache, else we'll never notice a connection change
16:14:13 cfriesen dansmith: I think on the neutron side they're using BatchNotifier, calling send_events() (https://git.openstack.org/cgit/openstack/neutron/tree/neutron/notifiers/nova.py#n242) which just blindly fires the events over to nova.
16:15:14 dansmith cfriesen: sure, but what does that have to do with anything?
16:15:28 openstackgerrit Balazs Gibizer proposed openstack/nova master: use context mgr in instance.delete https://review.openstack.org/443764
16:15:45 cfriesen dansmith: neutron has multiple network changes over live migration, queues up multiple events for the same instance, and fires them off in a single API call to nova
16:15:58 cfriesen dansmith: nova then updates the cache multiple times for the same instance
16:16:37 dansmith cfriesen: it has multiple plug events, but it should not send multiple refresh-cache events
16:16:54 cfriesen dansmith: it's just a "network-changed" event
16:17:01 dansmith yeah, that
16:19:03 melwitt dansmith: okay, just looked through the code and I see what you mean. so that one does a load-all-cells once, so it would need to be flushed itself if a new cell is added, right?
16:19:21 cfriesen dansmith: looks like it might send a network-changed event each time a port is updated or a floating ip is updated.
16:19:45 dansmith cfriesen: ...right
16:20:05 cfriesen dansmith: so if you've got 16 ports on an instance...
16:20:45 dansmith melwitt: we've built multiple lists of cells that will have to be flushed, and this is another, is my only point
16:21:06 melwitt ack
16:21:13 dansmith cfriesen: I understand, but that still doesn't mean that neutron should send a single request with 16 identical events for the same instance
16:23:16 cfriesen dansmith: oh, I agree. was just trying to show how we could end up in this scenario since earlier you said you were pretty sure neutron didn't do that.
16:24:18 dansmith cfriesen: I meant for multiple instances
16:25:42 cfriesen dansmith: with the neutron BatchNotifier code I think neutron just sends all queued notifications every couple seconds
16:26:00 cfriesen dansmith: so I think it could include events for multiple instances
16:27:01 dansmith cfriesen: that would be a change from how it originally behaved, and would delay instance booting unnecessarily
16:27:16 dansmith I don't see any timing in that code
16:27:35 cfriesen look at neutron/notifiers/batch_notifier.py
16:28:53 dansmith yeah, that's unfortunate
16:30:09 cfriesen we'll try pushing a neutron change to remove duplicate network-changed events for the same instance from the batch
16:31:00 dansmith in your nova dupe check, you should log a warning or something so we can see if this ever regresses once you fix the sending side
16:31:16 cfriesen was just going to ask if we wanted a nova dupe check as well
16:41:50 openstack Launchpad bug 1691178 in OpenStack Compute (nova) "virt_type=qemu does not support cpu affinity." [Undecided,New] - Assigned to sean mooney (sean-k-mooney)
16:41:50 sean-k-mooney sfinucan: https://bugs.launchpad.net/nova/+bug/1691178 this is the bug for the qemu cpu affinity ill upload the patch WIP patch in a few minutes but if you have in put on the scheduler question feel free to advise.
16:43:00 sean-k-mooney sfinucan: there isnt much in the bug but i will add you to the review of the code change when i split it out of my other changes.
16:47:01 cfriesen sean-k-mooney: would we maybe want to make this configurable? otherwise you're going to need nested kvm to test any of the "dedicated" cpu policy stuff
16:47:16 cfriesen (or real hardware)
16:48:48 sean-k-mooney cfriesen: i was going to make it condtional based on the config value of virt_type
16:49:08 sean-k-mooney cfriesen: cpu pinning is currently only tested in intels nfv ci
16:50:16 sean-k-mooney cfriesen: i basically looks like this https://pastebin.com/afp0M4Rm
16:50:26 sean-k-mooney im currently rebasing to master
16:51:14 sean-k-mooney cfriesen: sfinucan basically just adding if(CONF.libvirt.virt_type != "qemu"): in 2 places in the libvirt dirver and then i need to check unit tests and update if needed
16:53:00 dansmith melwitt: your comment about stale objects is about conf change? or are you worried that seldom-used ones will actually become stale in some un-usable way?
16:54:53 sean-k-mooney cfriesen: sfinucan that change should be in /nova/virt/libvirt/driver.py not /nova/virt/libvirt/config.py ... this is why i never edit diffs/patches
16:55:00 efried I just discovered this (apologies if everyone else knew it already): Add `-a` to journalctl and you get COLORS!
16:56:00 melwitt dansmith: I was thinking more of the latter, though I can't imagine it being much of a problem in practice. it would be things like, removing cells from the fleet or if a cell went temporarily offline
16:56:26 dansmith hmm
16:56:28 melwitt conf change is important but I wasn't thinking of that yet
16:56:40 cfriesen sean-k-mooney: I know at least some of our devs are testing with virtualbox which doesn't do nested KVM. You'd basically require nested KVM or real hardware to exercise the cpu pinning logic. I think a "permissive_qemu_affinity" config option might make sense, though I know we're trying to get rid of config options in general.
16:56:43 dansmith we don't have to do something if we restart the db today, right?
16:56:55 dansmith it reconnects for us I think, so I'm not sure we need to actually worry about things going stale
16:57:03 dansmith and by conf change, I mean changing a cell record or something
16:57:10 melwitt I don't think so, no. good point
16:57:27 melwitt yeah, understood. like changing a connection string

Earlier   Later