Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-30
18:56:59 mriedem sean-k-mooney: can you answer this question to stephen? "where does the libvirt driver actually translate hw_cpu_policy and hw_cpu_thread_policy into something that goes in the guest xml?"
18:57:04 melwitt efried: feel free to do that
18:57:13 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Use oslotest CaptureOutput fixture https://review.openstack.org/587129
18:57:14 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Use a non-nova log capture fixture https://review.openstack.org/587130
18:57:15 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Use a simplified WarningsFixture https://review.openstack.org/587131
18:57:35 efried melwitt: Procedurally, if we've deemed this to need a bp/spec, do I set the bug to Won't Fix?
18:57:55 mriedem i don't think so, it would be a wishlist bug
18:58:01 mriedem invalid -> wishlist or something
18:58:05 sean-k-mooney mriedem: let me see if i can find it. i can i can give you the relevent xml snipit it generates
18:58:12 efried ight
18:58:37 mriedem all i mostly see is the giant hardware.py methods,
18:58:43 mriedem but can't link those up to where it's used by a driver
18:59:14 mriedem maybe it's not directly set in the guest xml? maybe it's just used to determine which cpus to pin?
19:00:01 sean-k-mooney mriedem: its burried in the numa code
19:00:19 sean-k-mooney mriedem: yes it just used to determin the pinning
19:00:21 efried melwitt, mriedem: Do we have a helpful contributor link to the bp/spec process?
19:00:28 mriedem yes
19:00:34 sean-k-mooney it never gets into the xml itself
19:00:40 mriedem https://docs.openstack.org/nova/latest/contributor/blueprints.html
19:00:48 mriedem sean-k-mooney: ok then, that answers that, thanks
19:01:11 efried ack
19:02:14 sean-k-mooney mriedem: bassicaly we generate teh pinning here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4471-L4480
19:07:13 MultipleCrashes Anyone free to take up this review further : https://review.openstack.org/#/c/563418/
19:21:04 openstackgerrit Eric Fried proposed openstack/nova master: Updated AggregateImagePropertiesIsolation filter illustration https://review.openstack.org/586317
19:25:45 openstackgerrit karim proposed openstack/nova master: Updated AggregateImagePropertiesIsolation filter illustration https://review.openstack.org/586317
19:26:55 melwitt MultipleCrashes: are you asking for review or help with updating the patch or both?
19:28:10 MultipleCrashes I am new to it, in my knowledge we need a +2 for a merge ..mostly looking for a possibility of merge
19:30:37 sean-k-mooney MultipleCrashes: just looking at the bug you are getting a keysonte error form calling neutronport delete in a bulk delete of nova instnaces.
19:31:01 sean-k-mooney this almost looks like we are ddosing the neuron api with too many requests at once
19:31:24 sean-k-mooney retry is certenly one want to solve it but perhaps we should be ratelimiting
19:33:04 MultipleCrashes yeah , apparently this happens when too many instances are simultaneously deleted , if we try a rate limiting there is possibility that the task of deleting the VMs might get interrupted.
19:33:38 MultipleCrashes Eg:lets say we are deleting 1000 instances and the problem occurs at 550th (say) instance
19:34:21 MultipleCrashes we would still like to continue the process, probably rate limit might stop further execution
19:34:30 melwitt MultipleCrashes: the last comment on the review suggests a change to avoid logging error per retry and instead log info for the retry and then if all retries have failed, log error. otherwise the operator gets a false log error if one of the retries succeeds
19:35:14 melwitt and by false I mean, operators consider "error" to mean action should be taken
19:36:06 sean-k-mooney MultipleCrashes: well the error is stemming from data = neutron.list_ports(**search_opts). so in your case we would be doing 1000 concurrent requets to netron to list the ports for the 1000 instnaces.
19:36:11 MultipleCrashes yeah, since with the retries we would also be having instance ids, so we would be able to figure out that the retires are being done for this particular intance
19:37:25 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix formatting for vcpu_pin_set and reserved_huge_pages https://review.openstack.org/587206
19:37:39 sean-k-mooney melwitt: we have a rate limit for creatign new instaces. do you know is there an equivalent for delete?
19:37:53 MultipleCrashes the deletion of instances would happen one-at-a-time and hence, only the retry will be for that particular instance.
19:38:20 dansmith we don't have rate limiting for any API methods anymore, that I know of
19:39:26 sean-k-mooney MultipleCrashes: im not sure about that. i would expect the api/conductor to call down to the compute nodes to do the delete and for those deleteions to work in paralle but i have not looked at that code path.
19:39:47 sean-k-mooney dansmith: well this would not be an api ratelimit it would be a limit in the conductor i guess
19:39:55 dansmith sean-k-mooney: MultipleCrashes means we have no bulk delete api call
19:39:57 mriedem nova-api does an rpc cast to the compute that is hosting the instance
19:40:00 dansmith sean-k-mooney: so of course, all of them happen in parallel
19:40:15 dansmith sean-k-mooney: we have no rate limits in conductor either
19:40:27 dansmith sean-k-mooney: we have the build and migrate counters in compute, but those aren't per-tenant
19:40:36 dansmith but definitely don't have any such limits on delete
19:40:53 sean-k-mooney dansmith: oh ok then ya i guess retry is the best we can currently do then.
19:41:08 MultipleCrashes yeah, mostly is done via autoscale feature
19:41:36 MultipleCrashes while scaling down
19:41:41 melwitt MultipleCrashes: the suggestion isn't to use instance ids to figure out whether it's a retry. the suggestion is to move the retry decorator to the inner method _deallocate_network, so that the log error in _try_deallocate_network won't happen each retry attempt
19:42:20 MultipleCrashes yeah, moving to _deallocate_network would mean we will have to save_and_reraise exception in that function
19:42:46 mriedem "otherwise the operator gets a false log error if one of the retries succeeds" is definitely annoying and a red herring when you're actually trying to debug something,
19:42:47 MultipleCrashes as we are doing retry based on exception which we are handling in _try_deallocate_network
19:42:59 mriedem i know there is a persistent case of that in cinder-volume during volume delete i think which always throws me off
19:43:05 mriedem because it logs an error, then retries and succeeds
19:44:13 MultipleCrashes this seems to be an intermittent issue, once in many time..possibly caused by transient network connectivity problem.
19:45:12 melwitt MultipleCrashes: why? won't the RetryDecorator catch the ConnectFailure and retry and once retries expire it will propagate ConnectFailure up to _try_deallocate_network?
19:46:02 sean-k-mooney MultipleCrashes: it could be connectivity but its more likely that its due to the number of neutron api calls.
19:46:46 mriedem i thought at one point the bug said it was a keystone issue?
19:47:06 sean-k-mooney mriedem: its a keysone connection failure on list port
19:47:19 mriedem does it re-use the same token to delete all 1000 instances and the token times out?
19:48:34 sean-k-mooney mriedem: i cant tell form "ConnectFailure: Unable to establish connection to http:/somehost:someport/v2.0/ports.json?device_id=someid"
19:48:35 MultipleCrashes yeah , tried the way with _deallocate_network RetryDecorator .. doesn't function properly.Yeah agree neutron load via no of api calls could be a likely reason
19:49:48 sean-k-mooney mriedem: the error is propagating form the keystone auth session _send_request method but i dont think its a keysone issue
19:50:17 melwitt MultipleCrashes: ok, it would be helpful to reply to the review comment and let the reviewer know why their suggestion doesn't work. fwiw, I thought it would have worked too
19:51:18 melwitt oh, my mistake, I guess the suggestion does say to add log info and reraise
19:52:25 melwitt in the inner method. what I said, it wouldn't be possible to log the info part to say "retrying"
19:53:24 melwitt it would be better to log the "retrying ..." so operators can know if they have retries going on for the network deallocation
19:54:27 sean-k-mooney dansmith: ya the compute node max_concurrent_builds config option is the one i was thinking of originally but i had tought that was in the conductor. i guess not.
19:59:12 sean-k-mooney melwitt: mriedem by the way do we care about https://review.openstack.org/#/c/584999/ for rocky or will i loop back to it in stien?
20:00:24 mriedem sean-k-mooney: i can -1 it for any release based on the commit message if you want
20:00:57 sean-k-mooney hehe well if you want any changes please do
20:01:18 mriedem done
20:01:32 melwitt sean-k-mooney: bugs can be fixed any time so you don't need to target it to a specific release. that said, I agree the commit message doesn't explain anything about what's wrong or how/why the patch fixes it
20:01:43 mriedem sean-k-mooney: max_concurrent_builds defaults to 10
20:01:45 mriedem not 100
20:02:04 sean-k-mooney mriedem: i know i used to think it wa for the conductor so used to locally set it to 100
20:02:17 sean-k-mooney since its for compute that is excessive
20:03:03 sean-k-mooney melwitt: yes ture it does not. i have a detailed summay in the bug. ill copy the relevent bit into the commit
20:03:13 melwitt sounds good
20:03:14 MultipleCrashes looks like getting state of the retry count would be tricky, as it comes from within the RetryDecorator
20:03:40 MultipleCrashes for logging
20:04:25 melwitt MultipleCrashes: we don't need the retry count. the suggestion was just to LOG.info('Network allocation failed, retrying') so that the fact that it was retried is captured
20:04:42 melwitt for the benefit of operators
20:05:18 melwitt it's totally fine to add a save and reraise exception in _deallocate_network
20:06:47 mriedem rather than monkey with _deallocate_network directly, just put the call to that in an inner method
20:07:17 melwitt that works too
20:07:18 MultipleCrashes yeah, that handler is a generic exception handler there..we would be retrying only in case we get this specific exception of ConnectFailure
20:09:07 MultipleCrashes so logging in there for 'retrying' would bring a log line even if we have a generic exception
20:09:20 mriedem i left a comment
20:09:23 melwitt in the inner method you catch ConnectFailure only and reraise that, right?
20:09:35 mriedem the RetryDecorator itself will log at debug
20:10:03 mriedem https://github.com/openstack/oslo.service/blob/master/oslo_service/loopingcall.py#L453
20:10:36 mriedem so it should be easy to do what i added in the review comment
20:10:47 mriedem so we only get 1 ERROR log if everything fails including the retries timing out
20:11:05 melwitt I think INFO was suggested since most operators run at INFO log level
20:11:05 mriedem i don't think we need additional extra log messages b/c the decorator itself does that

Earlier   Later