| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-30 | |||
| 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 | sean-k-mooney | e.g. leaving it out | |
| 13:30:49 | aspiers | I can drop dedent if that's the consensus | |
| 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 | efried | it's unnecessary processing (test only, but still) and the result is actually technically too dedented. | |
| 13:34:42 | aspiers | efried: I only submitted this because stephenfin asked for it - please negotiate with him not me :) | |
| 13:34:56 | efried | I hear ya. | |
| 13:34:56 | aspiers | I really don't mind what you guys agree on | |
| 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 | |
| 13:43:06 | stephenfin | *course | |
| 13:43:16 | sean-k-mooney | efried: speaking of which | |
| 13:43:44 | sean-k-mooney | efried: care to take a look at |https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:master+topic:bp/image-metadata-prefiltering | |
| 13:44:07 | sean-k-mooney | there are more more important reviews but im hoping those merge soon | |
| 13:44:21 | efried | sean-k-mooney: been on the list, but got some stuff in front of it. | |
| 13:44:32 | sean-k-mooney | cool | |
| 13:45:38 | sean-k-mooney | stephenfin: by they way i tested and fixed https://review.opendev.org/#/c/675776/ | |
| 13:45:50 | stephenfin | the revert? | |
| 13:45:52 | sean-k-mooney | stephenfin: we do need that fix to avoid out issue with libvirt | |
| 13:45:54 | stephenfin | yeah | |
| 13:45:54 | sean-k-mooney | yes | |
| 13:47:09 | sean-k-mooney | i guess we are still waiting for the melonox ci to run on it too | |
| 13:49:10 | mdbooth | mriedem dansmith: I don't think I'm going to get better confirmation of the fix in https://review.opendev.org/#/c/677736/ . It *has* run on a real system, though, and didn't break it. | |
| 13:49:50 | sean-k-mooney | artom: https://review.opendev.org/#/c/678887/ failed because its using qemu again | |
| 13:50:45 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 13:51:27 | aspiers | alex_xu, stephenfin, efried: need your take on this debate https://review.opendev.org/#/c/636318/49/nova/virt/libvirt/config.py@2701 | |
| 13:52:12 | mriedem | mdbooth: dan is out today, | |
| 13:52:16 | mriedem | but are you going to address https://review.opendev.org/#/c/677736/3/nova/virt/libvirt/host.py@a236 ? | |
| 13:52:22 | mriedem | otherwise i'll +2 it | |
| 13:53:09 | aspiers | stephenfin, efried: if the question isn't easy to parse from those comments, I can help explain here | |
| 13:53:33 | efried | aspiers: Give me 10 mins | |
| 13:53:38 | aspiers | efried: np | |
| 13:53:49 | sean-k-mooney | mdbooth: you know the py37 hang i was seeing before when we tryied to boot vms could also be fixed by https://review.opendev.org/#/c/677736/3 | |
| 13:54:41 | sean-k-mooney | i had assumed when i saw it it was due to eventlest and python37 not playing nicely but that looks like it is more likely the actual cause | |
| 13:57:25 | mdbooth | sean-k-mooney: Why would the behaviour be different on py37, though? | |
| 13:58:36 | mdbooth | Bug call time | |
| 14:02:10 | sean-k-mooney | well i was febuary, eventlet did not offically test/support py37 and we have see issue with it on python3 in the pasted that looked like it | |
| 14:02:44 | sean-k-mooney | but honestly it was just a guess and it happend on python 3 but python 2.7 | |
| 14:09:25 | efried | So this might be the first time we are cross-checking Thing A and Thing B for compatibility. | |
| 14:09:25 | efried | As of yet, I think all of these ^ checks are just checking whether Thing A is on in flavor and off in image meta, kind of thing. | |
| 14:09:25 | efried | We have a place where we validate flavor+image compatibility for things like this: nova.compute.api.API._validate_flavor_image | |
| 14:09:25 | efried | If it's possible for Thing A (via extra spec, image meta, config, etc) to ask for a switch to be flipped on, and Thing B (likewise) for it to be off, that's a conflict, and should be an error. | |
| 14:09:25 | efried | aspiers: I don't have the bandwidth to dig into this all the way right now, so the following is a bit from the hip: | |
| 14:09:26 | efried | Now, that's at the API layer. Once you get down to the compute, you can start vetting against things like config or whatever. Hopefully any conflicts there would have already been filtered out based on placement; but if not, late fail the spawn. | |
| 14:09:26 | efried | But that's fine - I believe that was one of the long-term goals of that infrastructure. stephenfin might be able to say more there. | |
| 14:09:27 | efried | So if all of that were in place, having a single function that checks sev *and anything else relevant* and either sets iommu on or raises an exception: sounds like the right plan. | |
| 14:13:30 | mriedem | artom: so https://review.opendev.org/#/c/678887/18 we're just waiting for another run after your rebase right? | |
| 14:21:49 | aspiers | efried: thanks, I think that's enough justification for me to start switching the driver iommu flags so they all get enabled from a single place | |