| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-30 | |||
| 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 | mriedem | i don't think we need additional extra log messages b/c the decorator itself does that | |
| 20:11:05 | melwitt | I think INFO was suggested since most operators run at INFO log level | |
| 20:11:22 | mriedem | i wouldn't consider this INFO | |
| 20:11:25 | melwitt | you won't know your deallocations are retrying unless you enable debug | |
| 20:11:33 | mriedem | do you need to know that | |
| 20:11:33 | mriedem | ? | |
| 20:11:37 | mriedem | unless you're hitting failures | |
| 20:11:47 | mriedem | and if you're hitting failures, you're probably gonna enable debug on compute | |
| 20:12:10 | melwitt | based on what sean-k-mooney said, it could be a hint that neutron is under load. thought it would be good to know about that | |