| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-22 | |||
| 14:29:39 | dansmith | because I was thinking that the rest of our unit tests are apparently running *with* greendns, but our prod code does not, which is a bit wonky | |
| 14:29:48 | dansmith | if we split it thusly, | |
| 14:30:12 | dansmith | we could also put a call to the set-and-import call in the functional init to make sure those tests also run with eventlet configured in the same way as runtime | |
| 14:30:51 | artom | dansmith, to be fair, unit tests should not depend on name resolution... should they? | |
| 14:31:02 | artom | Also, wouldn't that be changing our code to make unit tests happy? | |
| 14:31:23 | dansmith | wouldn't what be changing our code? | |
| 14:31:32 | artom | Splitting monkey_patch() into 2 | |
| 14:31:59 | dansmith | no, because the runtime code would be the same exact thing it is now | |
| 14:32:02 | artom | Also, in the case of greendns at least, we kinda need it to be monkeypatched to test it properly | |
| 14:32:14 | dansmith | it's totally cool to design code so it's easily testable right? | |
| 14:32:18 | artom | True | |
| 14:32:36 | artom | In fact, easy unit tests are usually an indication of good design | |
| 14:32:39 | dansmith | this is my point.. we're testing all our code with eventlet configured differently than we actually run it with | |
| 14:32:57 | dansmith | that's dumb and maybe there shouldn't be any behavior change as a result in the kind of tests we do, but.. there could be | |
| 14:33:13 | sean-k-mooney | dansmith: well i think the rational is unittest should never do io | |
| 14:33:19 | dansmith | so facilitating testing that it's configured as we expect, and running tests with it configured that way seems pretty sane | |
| 14:33:22 | sean-k-mooney | dansmith: so eventlets shoudl never context swtich | |
| 14:33:27 | dansmith | sean-k-mooney: sure I understand | |
| 14:33:50 | dansmith | sean-k-mooney: but all our functional tests used to be under unit/ and we still have plenty that do ... stuff :) | |
| 14:34:03 | artom | Yeah, functional tests definitely write to temp files | |
| 14:34:18 | sean-k-mooney | ya but they are monkeypatched | |
| 14:34:20 | artom | But then they import nova.monkey_path properly | |
| 14:34:54 | sean-k-mooney | the api used to not be monkeypatch in the past if it was run under uwsgi too | |
| 14:35:08 | sean-k-mooney | the conductor and comptue agent are always patched | |
| 14:35:31 | artom | Is eventlet just there for historical reasons, btw? Have we ever measured performance with and without it? | |
| 14:35:36 | dansmith | ? | |
| 14:35:40 | sean-k-mooney | had have been since twisted was removed form the code bas although that might predate the conductor. its before i started workign on nova | |
| 14:35:47 | dansmith | I forgot it was actually using our monkey patch | |
| 14:36:12 | artom | Or are we just so certain eventlet's monkeypatching makes our IO so much faster that we don't bother | |
| 14:36:34 | sean-k-mooney | artom: we have some explcit cases where we spawn thing directly too | |
| 14:36:38 | sean-k-mooney | its notall implcit | |
| 14:36:44 | dansmith | artom: eventlet is not a matter of speed (at all) and all the non-API services require it for doing more than one thing at a time :) | |
| 14:36:46 | sean-k-mooney | but i know we have some infinity loops | |
| 14:36:57 | sean-k-mooney | if you dont patch it will eventually stop worrking | |
| 14:37:17 | dansmith | we also have plenty of code that it entirely thread unsafe, and just converting to regular threads will definitely create a million bugs we have to track down :) | |
| 14:37:59 | sean-k-mooney | artom: we basically realy on it for coperative processing | |
| 14:38:11 | artom | Fair enough | |
| 14:38:24 | dansmith | sean-k-mooney: not basically.. we DO rely on it :) | |
| 14:38:39 | sean-k-mooney | so unless we used explcit corutines we would not really be able to do things as we do today and even then we woudl need an event loop | |
| 14:39:37 | sean-k-mooney | dansmith: correct me if im wrong but we used twisted in the really realy days instead of eventlet right | |
| 14:39:57 | sean-k-mooney | as in pre catus maybe | |
| 14:39:58 | dansmith | sean-k-mooney: not that I ever knew.. old RAX stuff did I think , or something | |
| 14:40:09 | dansmith | artom: so I just put your test in functional/ and it passes just fine | |
| 14:40:12 | sean-k-mooney | maybe not once it was open sourced | |
| 14:40:19 | dansmith | because functional runs our monkey_patch routine right? | |
| 14:40:27 | dansmith | not sure why we lost sight of this yesterday | |
| 14:40:34 | sean-k-mooney | dansmith: its in the functionl test __init__.py | |
| 14:41:04 | dansmith | sean-k-mooney: right, I had thought it was doing bare eventlet monkey patching for some reason, but it calls our handler, so I don't see what the problem is | |
| 14:41:12 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/__init__.py | |
| 14:41:23 | artom | dansmith, yeah, I said that last night :) | |
| 14:41:27 | artom | In functional it's fine | |
| 14:41:34 | sean-k-mooney | no i might of before mdbooth refactored this 3 release ago | |
| 14:41:49 | artom | But we'd have to add code similar to osprofiler to skip it with an env var | |
| 14:41:51 | sean-k-mooney | yes it did https://github.com/openstack/nova/blame/59f1f187e5dceb5841a711f265280346d70a972b/nova/tests/functional/__init__.py | |
| 14:41:59 | artom | (Actually, wait, would we? Don't think we would) | |
| 14:42:01 | dansmith | right, which artom eh? | |
| 14:42:16 | artom | But... it's not a functional test, is it? | |
| 14:42:24 | sean-k-mooney | this https://github.com/openstack/nova/commit/3c5e2b0e9fac985294a949852bb8c83d4ed77e04 reworkd most of our monkey patching | |
| 14:42:33 | dansmith | artom: yeah, I'm not getting what you're saying.. in the functional set, we can just run it natively and it will test what we want with that environment as it is already | |
| 14:42:53 | artom | dansmith, true | |
| 14:43:18 | artom | But... it's not a functional test :) It doesn't start any services, access the DB, etc | |
| 14:43:31 | dansmith | sean-k-mooney: okay I totally looked at this file yesterday, but maybe I just "saw" the older bare monkey patching and that stuck in my head | |
| 14:43:41 | artom | Like, if you want it in there I don't care | |
| 14:43:42 | dansmith | artom: you're joking right? | |
| 14:43:48 | artom | No? | |
| 14:44:07 | dansmith | I think it's fine in functional. | |
| 14:44:31 | artom | Sure, I'll move it there | |
| 14:45:08 | dansmith | apologies if I missed something previous about that, but I wasn't chatting with you "last night" about this, so maybe I wasn't around when it came up | |
| 14:45:31 | artom | dansmith, last afternoon? | |
| 14:45:35 | artom | I guess it was morning for you | |
| 14:45:59 | dansmith | okay, well, sorry if I missed it | |
| 14:46:12 | artom | (Let's be honest, https://i.imgur.com/aWxtlff.jpg is my go-to reaction for this kind of thing :P ) | |
| 14:47:12 | artom | But because I get a chance to prove dansmith publicly wrong (however insignificant the subject actually is), I'll still do it: http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2020-09-21.log.html#t2020-09-21T18:48:23 | |
| 14:48:20 | dansmith | ah, I see, right at the end | |
| 14:48:54 | dansmith | I had pretty much checked out, but I thought you were saying you'd need a way to turn it off and on (which I guess you were) and didn't go back to check or read what you were saying to grasp the reason | |
| 14:48:59 | dansmith | ANYWAY | |
| 14:49:07 | dansmith | I'd just do it there and then we can be done with it :) | |
| 14:49:11 | sean-k-mooney | to defend dansmith that also was not really night :P even in ireland | |
| 14:49:21 | dansmith | heh | |
| 14:49:43 | sean-k-mooney | by ya lets channel nike and just do it | |
| 14:49:44 | artom | sean-k-mooney, you're in no position to lecture people about time of day :P | |
| 14:49:54 | sean-k-mooney | :) | |
| 14:57:21 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Test for disabling greendns https://review.opendev.org/753072 | |
| 15:11:44 | openstackgerrit | Merged openstack/nova master: doc: Fix rendering in the accelerator support doc https://review.opendev.org/751574 | |
| 15:22:25 | openstackgerrit | Merged openstack/nova master: doc: correct the link to user/flavor.rst https://review.opendev.org/740176 | |
| 15:27:52 | openstackgerrit | Merged openstack/nova master: docs: fix aggregate weight multiplier property names https://review.opendev.org/746128 | |
| 15:55:13 | openstackgerrit | Mark Goddard proposed openstack/nova stable/ussuri: docs: fix aggregate weight multiplier property names https://review.opendev.org/753392 | |
| 16:12:57 | kashyap | Is there a code name for the "x" release yet? I don't see anything on the list ... so presumably no | |
| 19:32:51 | mnaser | kashyap: nope don't think we are there yet :) | |
| 20:06:13 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/queens: Fix overcommit for NUMA-based instances https://review.opendev.org/726868 | |
| #openstack-nova - 2020-09-23 | |||
| 06:37:24 | openstackgerrit | Brin Zhang proposed openstack/nova master: WIP: Cyborg suspend/resume support https://review.opendev.org/729945 | |
| 06:54:40 | openstackgerrit | Brin Zhang proposed openstack/nova master: WIP: Cyborg suspend/resume support https://review.opendev.org/729945 | |
| 08:34:20 | brinzhang0 | frickler: hope you can review https://review.opendev.org/#/c/675304/22 while you have free time | |
| 08:34:45 | brinzhang0 | this patch "add server migration list CLI" | |
| 08:34:45 | brinzhang0 | frickler: | |
| 08:48:47 | kashyap | lyarwood: (or anyone): Have we ever deprecated / removed *allowed values* for a given image metadata property? | |
| 09:31:05 | stephenfin | kashyap: Not that I'm aware of, no | |
| 09:31:21 | kashyap | stephenfin: Okay; I'm just posting a little patch; we can discuss there | |
| 09:31:40 | stephenfin | It's technically possible, but no one has bothered. You'd need data migrations to remove DB entries using the old values | |
| 09:32:26 | stephenfin | bauzas: Am I correct in saying that AggregateInstanceExtraSpecsFilter and AggregateImagePropertiesIsolation have the same purpose but for extra specs and image metadata props? | |