| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-30 | |||
| 13:00:54 | mriedem | gibi: GRENADE_(NEW|OLD)_BRANCH should work yeah, we've used that before https://github.com/openstack/nova/blob/stable/pike/nova/tests/live_migration/hooks/run_tests.sh#L52 | |
| 13:00:58 | luyao | mriedem: Welcome to review this patch series if you would like, I'll appreciate that. Current design is NOT virt driver specific. https://review.opendev.org/#/q/topic:bp/virtual-persistent-memory+(status:open) | |
| 13:02:08 | mriedem | gibi: ok you're going to drop that env dump yeah? | |
| 13:02:10 | mriedem | that's just WIP? | |
| 13:02:25 | gibi | mriedem: yes, I just noticed it | |
| 13:02:31 | gibi | mriedem: new PS is coming | |
| 13:02:56 | mriedem | luyao: i'll be honest with you, that series is likely not something i'm going to be focusing on in the next 2 weeks to feature freeze, i'm already committed to reviewing some other series that have been around longer | |
| 13:03:33 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add cold migrate and resize to nova-grenade-multinode https://review.opendev.org/679210 | |
| 13:03:38 | mriedem | sean-k-mooney: btw, totally unrelated, i realize our docs for AggregateInstanceExtraSpecsFilter kind of suck https://docs.openstack.org/nova/latest/admin/configuration/schedulers.html?highlight=aggregate_instance_extra_specs#aggregateinstanceextraspecsfilter | |
| 13:04:00 | mriedem | meaning, what the hell is aggregate_instance_extra_specs supposed to mean to a user trying to configure flavors or aggregates for that filter? | |
| 13:04:10 | mriedem | looking at the code it doesn't even really make sense to me | |
| 13:04:35 | sean-k-mooney | i know i hate that filter and tried to replaces it | |
| 13:04:59 | mriedem | should at least have an example of how to use it | |
| 13:05:08 | sean-k-mooney | yes | |
| 13:05:08 | mriedem | it came up while triaging this bug https://bugs.launchpad.net/nova/+bug/1841932 | |
| 13:05:09 | openstack | Launchpad bug 1841932 in OpenStack Compute (nova) "hide_hypervisor_id extra_specs in nova flavor cannot pass AggregateInstanceExtraSpecsFilter" [Undecided,Incomplete] | |
| 13:05:20 | sean-k-mooney | i can proably add one if you want | |
| 13:05:49 | luyao | mriedem: Yes, I fully understand you, but any comments are welcome, even small and short. :) | |
| 13:07:47 | sean-k-mooney | mriedem: it bassicaly asserts that that every extraspec that is in the fiter (optionally that starts with aggregate_instance_extra_specs:) is set in at least 1 aggreate the current host is a member of | |
| 13:07:52 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 13:08:06 | aspiers | alex_xu, stephenfin: Your feedback addressed ^^^ | |
| 13:09:50 | sean-k-mooney | mriedem: looking at the bug report its working as intended | |
| 13:10:05 | sean-k-mooney | well actuly | |
| 13:11:07 | sean-k-mooney | it might not handel un namepaced extra specs properly ill check | |
| 13:12:10 | sean-k-mooney | mriedem: no its working as indended | |
| 13:12:38 | sean-k-mooney | its not working as they want in https://bugs.launchpad.net/nova/+bug/1841932 | |
| 13:12:38 | openstack | Launchpad bug 1841932 in OpenStack Compute (nova) "hide_hypervisor_id extra_specs in nova flavor cannot pass AggregateInstanceExtraSpecsFilter" [Undecided,Incomplete] | |
| 13:12:52 | sean-k-mooney | but that is worrking the way that filter has always worked | |
| 13:12:57 | mriedem | sean-k-mooney: i've reported a docs bug if you you're keen https://bugs.launchpad.net/nova/+bug/1842061 | |
| 13:12:58 | openstack | Launchpad bug 1842061 in OpenStack Compute (nova) "Compute schedulers in nova - AggregateInstanceExtraSpecsFilter docs are not clear" [Undecided,New] | |
| 13:15:01 | sean-k-mooney | ill see if i can add it to my list. but we create https://github.com/openstack/nfv-filters/blob/master/nfv_filters/nova/scheduler/filters/aggregate_instance_type_filter.py to replace it for several reason. also with docs https://github.com/openstack/nfv-filters/blob/master/doc/source/scheduler_filters/aggregate-instance-type-filter.rst | |
| 13:15:16 | aspiers | stephenfin: I also split off those fixups | |
| 13:29:08 | efried | aspiers: why is textwrap.dedent necessary https://review.opendev.org/#/c/679339/3/nova/tests/unit/virt/libvirt/fake_libvirt_data.py ? | |
| 13:29:16 | openstackgerrit | Merged openstack/nova-specs master: Create specs directory for Ussuri https://review.opendev.org/679364 | |
| 13:29:20 | sean-k-mooney | efried: its not | |
| 13:29:21 | efried | This is xml, not python - it doesn't care about whitespace internally, does it? | |
| 13:29:32 | sean-k-mooney | actully it does | |
| 13:29:32 | aspiers | efried: it's necessary to keep stephenfin happy, and I kinda like it too | |
| 13:29:51 | sean-k-mooney | but we are using xml parsing libs and we dont care | |
| 13:30:03 | aspiers | I could live without, but he suggested it and I like keeping things as readable as possible | |
| 13:30:10 | aspiers | sean-k-mooney: it's about readability not parsing | |
| 13:30:14 | efried | I'm not asking why you're indenting the lines with the xml on them. Visually prettier. I'm asking why textwrap.dedent is necessary. | |
| 13:30:17 | stephenfin | hmm, yeah, we could forget about the textwrap piece, actually. It was the indentation of the XML itself that I cared about | |
| 13:30:23 | aspiers | Oh right | |
| 13:30:28 | sean-k-mooney | aspiers: yes but we dont read the xml we read the code | |
| 13:30:30 | stephenfin | efried: I suggested that out of habit | |
| 13:30:37 | aspiers | sean-k-mooney: strongly disagree | |
| 13:30:39 | stephenfin | You're right, it's not needed in this case | |
| 13:30:43 | sean-k-mooney | and that makes the code mroe readable | |
| 13:30:49 | aspiers | I can drop dedent if that's the consensus | |
| 13:30:49 | sean-k-mooney | e.g. leaving it out | |
| 13:31:20 | stephenfin | I'm happy with it. I'm happy without it. Just so long as I get my block indentation :) | |
| 13:31:25 | sean-k-mooney | aspiers: i read the xml all the time just not that xml its alway after its been parsed or pretty printed | |
| 13:32:25 | aspiers | sean-k-mooney: OK you meant "we don't read the XML as it is at *run-time*, we read it in the source"? I would mostly agree with that | |
| 13:32:26 | efried | aspiers: it's just entirely unnecessary. And in fact if you folded the result into the surrounding xml doc, it would look weird, because it's completely left-justifying | |
| 13:32:54 | sean-k-mooney | aspiers: yes | |
| 13:33:04 | aspiers | efried: what's entirely unnecessary, the dedent or the whole fixup? | |
| 13:33:08 | sean-k-mooney | aspiers: if im reading it at runtime its in a debugger or a traceback | |
| 13:33:27 | aspiers | another classic bikeshed topic :) | |
| 13:33:41 | sean-k-mooney | obviously | |
| 13:33:42 | aspiers | Is there a mandatory minimum of one per day? ;-p | |
| 13:34:05 | efried | well, both aspiers, and if it weren't for the fact that all these lines already belong to two very recent commits, I would -1 it for the sake of git history. I'm tempted to do that anyway. | |
| 13:34:16 | efried | but I have an actual problem with the dedent. | |
| 13:34:42 | aspiers | efried: I only submitted this because stephenfin asked for it - please negotiate with him not me :) | |
| 13:34:42 | efried | it's unnecessary processing (test only, but still) and the result is actually technically too dedented. | |
| 13:34:56 | aspiers | I really don't mind what you guys agree on | |
| 13:34:56 | efried | I hear ya. | |
| 13:35:41 | aspiers | thanks :) | |
| 13:36:53 | stephenfin | efried: straight up indentation fixes aren't an issue, tbh - git blame -w | |
| 13:37:01 | sean-k-mooney | mriedem: i may have ranted a bit but hopefully that helps. https://bugs.launchpad.net/nova/+bug/1842061 | |
| 13:37:03 | openstack | Launchpad bug 1842061 in OpenStack Compute (nova) "Compute schedulers in nova - AggregateInstanceExtraSpecsFilter docs are not clear" [Undecided,New] | |
| 13:37:16 | efried | stephenfin: where's the -w option in pycharm? | |
| 13:37:36 | sean-k-mooney | ill try and come up with a doc friendly way to say that | |
| 13:37:55 | stephenfin | efried: Google tells me it's on by default? https://stackoverflow.com/questions/37868225/ignore-whitespace-identation-in-annotate-mode-with-intelliji-and-git | |
| 13:38:08 | sean-k-mooney | efried: you can get the history of the lines you have slected and go back though each commit | |
| 13:38:38 | stephenfin | Also, am I the only sucker still working in Vim? | |
| 13:38:45 | sean-k-mooney | yes | |
| 13:38:50 | efried | sean-k-mooney: Yes, of course I can, but is that extra work worth it? IMO it is not. | |
| 13:38:54 | efried | especially for this | |
| 13:39:01 | efried | stephenfin: I believe cdent is with you. | |
| 13:39:17 | sean-k-mooney | efried: sooy i was not following the converstion | |
| 13:39:45 | sean-k-mooney | can you do git blame -w with pycharm | |
| 13:39:55 | sean-k-mooney | and yes if you want too | |
| 13:40:35 | efried | well, that's interesting, stephenfin you're right about -w being on by default. That takes some of the sting out. | |
| 13:40:50 | sean-k-mooney | how did i miss a full sentence... | |
| 13:40:53 | efried | I'll just abstain. | |
| 13:40:57 | stephenfin | \o/ | |
| 13:41:08 | stephenfin | that's a sane default | |
| 13:41:19 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/rocky: Add useful error log when _determine_version_cap raises DBNotAllowed https://review.opendev.org/679449 | |
| 13:41:23 | sean-k-mooney | litally ment to type "all i saw was" before "can you do..." | |
| 13:41:38 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/rocky: Add useful error log when _determine_version_cap raises DBNotAllowed https://review.opendev.org/679449 | |
| 13:41:51 | sean-k-mooney | stephenfin: proably because white space in lanagues like python matters | |
| 13:42:00 | sean-k-mooney | so changing indention could change behavior | |
| 13:42:09 | sean-k-mooney | or no one added it | |
| 13:42:11 | stephenfin | sean-k-mooney: Right, but make it toggleable | |
| 13:42:28 | stephenfin | Maybe 'git blame -W' to override whatever and re-enable it | |
| 13:42:35 | stephenfin | Hmm, that would be a decent patch | |
| 13:42:40 | stephenfin | Possible weekend project there | |
| 13:42:56 | sean-k-mooney | stephenfin: as long as you dont change the default behavior | |
| 13:43:05 | stephenfin | oh yeah, of cours | |