| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-22 | |||
| 14:27:40 | sean-k-mooney | apparently | |
| 14:27:43 | dansmith | so we don't end up patched in the end I guess? | |
| 14:27:54 | dansmith | so, there's something else we could do, | |
| 14:28:28 | dansmith | which is split monkey_patch() into two things.. one that sets the params and imports eventlet, and one that does the monkey patching (the latter would call the former).. then we could just call the former in unit test init and be able to check for it | |
| 14:29:01 | dansmith | that would make our unit tests behave more like real life, if it ever mattered, without changing the monkeypatched-ness | |
| 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: | |