| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-30 | |||
| 18:46:49 | efried | jaypipes: But it's not about whether the patch is correct (it clearly isn't quite, yet). It's about whether we should allow it at all. Or if we do, if it needs to have microversion paperwork, a bp, a spec, etc. | |
| 18:47:12 | melwitt | efried: understood. at the very least I'd think we'd want tempest tests to cover unicode metadata key cases to verify it for now and keep it working in the future. and I'd think we'd want to change all of the user-settable keys to support unicode at the same time (rather than pick only one). and that's a bigger change and one I'd expect to have a spec about that explains all of the changes needed to properly support unicode in keys | |
| 18:47:28 | dansmith | and tests to cover metadata server and configdrive, amirite? | |
| 18:47:35 | melwitt | that's just my opinion. any other opinions? | |
| 18:47:53 | dansmith | personally I do not think unicode is worth the change to this, no surprises there | |
| 18:48:06 | efried | you anglocentric, you | |
| 18:49:03 | mriedem | i wouldn't ask me, i'd ask alex_xu, ken'ichi, takashi and Kevin_Zheng | |
| 18:49:20 | mriedem | i asked kevin long ago if huawei had any need for unicode in flavor extra specs and i believe he said no | |
| 18:49:38 | mriedem | passing unicode metadata values through to the guest is probably an easier case to make | |
| 18:49:49 | melwitt | I didn't mean opinions on the validity of the change, just whether it would need a spec and review | |
| 18:49:50 | dansmith | resources:<pileofpoo>=1 | |
| 18:50:57 | mriedem | normal microversion behavior state diagram says that if i can make a request to cloud A and it works and make the same request to cloud B and it doesn't work, w/o microversions, it should probably be versioned - unless it's just a bug / something broken in existing support | |
| 18:51:15 | mriedem | but this has never worked as far as i know | |
| 18:51:22 | mriedem | so it's not a bug/regression, it's an RFE | |
| 18:51:25 | dansmith | what do you do for keys that are unicode if retrieved from an older microversion? | |
| 18:51:30 | dansmith | detect if they have unicode and exclude them? | |
| 18:51:56 | mriedem | i thought you couldn't do that b/c of the existing schema? | |
| 18:52:06 | mriedem | oh you mean, | |
| 18:52:15 | efried | hypothetically if this change was made, even with a microversion | |
| 18:52:16 | mriedem | set with unicode on newer microversion, retrieve with older microversion? | |
| 18:52:23 | dansmith | mriedem: yeah | |
| 18:52:37 | mriedem | dansmith: idk, detecting that would suck | |
| 18:52:42 | dansmith | mriedem: yup | |
| 18:52:46 | jaypipes | efried: I asked a question on the patch. | |
| 18:52:47 | dansmith | mriedem: and be super confusing for people | |
| 18:53:28 | mriedem | this is generally why we have specs for api behavior changes.... :) | |
| 18:53:39 | dansmith | ahyup | |
| 18:53:43 | efried | jaypipes: In the discussion from a few weeks ago (linked in the bug report) we talked about it not being a good idea for extra specs. I think it was in reaction to that that they reverted that part. | |
| 18:53:45 | jaypipes | although I do like dansmith's pile of poo resource. | |
| 18:53:56 | dansmith | jaypipes: one pile of poo please, affined to numa node #2 | |
| 18:54:07 | jaypipes | side of fries with that, dansmith? | |
| 18:54:17 | dansmith | jaypipes: only after some hand sanitizer | |
| 18:54:21 | jaypipes | :) | |
| 18:54:29 | efried | affinitizations for 914 points | |
| 18:55:25 | sean-k-mooney | mriedem: does the api activly reject unicode? | |
| 18:55:46 | efried | sean-k-mooney: We're talking about in metadata/extra_specs keys, where the schema is patters | |
| 18:55:51 | efried | pattern-limited to ascii. | |
| 18:55:58 | efried | so yeah | |
| 18:56:24 | melwitt | efried: I'm going to comment on the patch | |
| 18:56:41 | efried | melwitt: Okay, I was about to update the bug. | |
| 18:56:42 | sean-k-mooney | efried: oh ok i was going to say we dont mandate a coralation type for the db so someone could have created a db with utf-8 set and would be able to store it | |
| 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. | |