| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-30 | |||
| 17:54:42 | openstackgerrit | Merged openstack/nova stable/pike: Make host_aggregate_map dictionary case-insensitive https://review.openstack.org/585335 | |
| 17:54:50 | openstackgerrit | Merged openstack/nova stable/pike: Fix unbound local when saving an unchanged RequestSpec https://review.openstack.org/580951 | |
| 17:54:56 | openstackgerrit | Merged openstack/nova stable/queens: Refactor _build_device_metadata https://review.openstack.org/581398 | |
| 17:55:05 | openstackgerrit | Merged openstack/nova stable/queens: Consider hostdev devices when building metadata https://review.openstack.org/581399 | |
| 17:55:13 | openstackgerrit | Merged openstack/nova master: Add additional functional tests for NUMA networks https://review.openstack.org/585385 | |
| 17:59:20 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Use of set_middleware_defaults https://review.openstack.org/587183 | |
| 18:00:32 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Use own set_middleware_defaults https://review.openstack.org/587183 | |
| 18:32:33 | efried | Hey folks, we're back with https://bugs.launchpad.net/nova/+bug/1737711 | |
| 18:32:33 | openstack | Launchpad bug 1737711 in OpenStack Compute (nova) "nova boot failed when use the chinese metadata key and value" [Undecided,In progress] - Assigned to huanhongda (hongda) | |
| 18:32:59 | efried | They've restricted the patch https://review.openstack.org/#/c/536236/ to just metadata keys (not extra_specs keys anymore). | |
| 18:34:20 | efried | The bug still doesn't give a whole lot of detail as to the use case for this, but maybe it makes more sense to y'all, dansmith mriedem jaypipes | |
| 18:38:21 | melwitt | efried: fwiw, my initial read on that is we have more than one place where unicode keys won't work (as you mentioned extra_specs is another one) and IMHO it feels like it would be a small spec to propose to change all of them together in a consistent way to support unicode | |
| 18:38:39 | melwitt | is the schema validation really the only thing preventing unicode from working? | |
| 18:39:14 | efried | melwitt: Good question. No idea if they've tried... whatever it is they wanted to try, with their fix in place. | |
| 18:39:59 | efried | melwitt: Anyway, my position on this is that I don't have the background to be able to make a compelling argument to shut down the bug, or ask for it to be converted to a bp, or whatever. I need cavalry support (that would be you). | |
| 18:40:41 | jaypipes | efried: I'm curious why the patch includes a change to add maxLength: 255 | |
| 18:41:40 | efried | jaypipes: Yeah, that's redundant (already covered by the regex), but not really important to the question of whether we should do it at all... | |
| 18:42:25 | efried | jaypipes: I added a comment for ya :) | |
| 18:42:36 | jaypipes | efried: right, but AFAICT, the regex doesn't change. | |
| 18:42:49 | mriedem | stephenfin: where does the libvirt driver actually translate hw_cpu_policy and hw_cpu_thread_policy into something that goes in the guest xml? | |
| 18:42:58 | efried | jaypipes: Right, that part of the delta is them making sure the extra_specs pattern does *not* change. | |
| 18:43:16 | mriedem | stephenfin: _get_guest_cpu_config ? | |
| 18:44:11 | efried | jaypipes: Actually, they've correctly carried over the redundant maxLength from the metadata key pattern... | |
| 18:44:13 | openstackgerrit | karim proposed openstack/nova master: Updated AggregateImagePropertiesIsolation filter illustration https://review.openstack.org/586317 | |
| 18:44:45 | efried | jaypipes: I suppose if you were being really particular, you could say they did the right thing, because you could get back a different schema error. | |
| 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 | |