| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-04 | |||
| 18:33:22 | sean-k-mooney | it will still take the topic if you do git checkout -b whatever | |
| 18:33:36 | dansmith | sean-k-mooney: only if there is no other tag in the commit message | |
| 18:33:55 | dansmith | sean-k-mooney: go lie down :) | |
| 18:33:55 | sean-k-mooney | a recent version of git-review removed some of the auto topic stuff | |
| 18:34:12 | sean-k-mooney | ya i will but stephenfin change how this works recently | |
| 18:34:20 | melwitt | aw man | |
| 18:34:27 | sean-k-mooney | i think they weanted to remvoe the auto topic form the commit message | |
| 18:34:29 | melwitt | I need to downgrade then probably | |
| 18:34:38 | sean-k-mooney | but leave the auto topic form branch name | |
| 18:34:39 | mriedem | i thought sean-k-mooney was going to lie down? | |
| 18:34:47 | melwitt | git-review version 1.28.0 | |
| 18:34:48 | sean-k-mooney | ok o/ | |
| 18:35:09 | mriedem | now that he's gone, | |
| 18:35:22 | dansmith | https://opendev.org/opendev/git-review/commit/af955c932eb97433199781b5d4f7fb5fb5af8a6f | |
| 18:35:40 | dansmith | https://opendev.org/opendev/git-review/commit/03768832c4a73c3079ce24d84fecae1da39b2d71 | |
| 18:35:57 | dansmith | "frequently does the wrong thing" ... not that I ever saw. | |
| 18:36:07 | melwitt | yeah like, what? | |
| 18:36:16 | dansmith | melwitt: that second commit is likely what you've noticed I guess | |
| 18:36:27 | melwitt | stephenfin you're killin me | |
| 18:36:56 | melwitt | and yeah, I think you're right dansmith | |
| 18:37:01 | melwitt | I'm gonna downgrade | |
| 18:37:13 | dansmith | the story links in that commit are broken, | |
| 18:37:29 | dansmith | and no gerrit links so I dunno what kind of review that stuff gets | |
| 18:37:50 | melwitt | fun | |
| 18:38:00 | dansmith | I pretty much always name my local branches with bug/X and bp/Y anyway, so I probably wouldn't notice | |
| 18:38:13 | dansmith | because I actually don't like that I end up with them different in gerrit vs. local | |
| 18:38:15 | melwitt | I'm a renegade | |
| 18:39:18 | melwitt | https://en.wikipedia.org/wiki/Renegade_(TV_series) | |
| 18:39:24 | mriedem | was just thinking that | |
| 18:39:38 | mriedem | i had a friend that couldn't leave the house all summer in middle school because his parents made him babysit his sister, | |
| 18:39:43 | mriedem | and he'd be watching renegade every day | |
| 18:40:15 | melwitt | lol, an excellent choice | |
| 18:40:19 | mriedem | so we had to go to his house b/c he coudn't leave, but then we had to watch renegade | |
| 18:42:01 | mriedem | who is lorenzo roundhouse kicking today? | |
| 18:43:36 | melwitt | heh | |
| 18:54:09 | zbr | efried: does https://review.opendev.org/#/c/674057/ look ok now? | |
| 18:59:17 | artom | I'm a dumbass | |
| 18:59:42 | artom | I spent literally hours, at this point, trying to stub, mock, or hack __dict__ to pin a compute RPC object | |
| 19:00:09 | artom | (Because def router is an @property that can't be set/stubbed, and it's using a global singleton) | |
| 19:00:30 | artom | When what I *should* have done is just inherited from ComputeAPI and overwrote def router >_< | |
| 19:02:58 | dansmith | can't you just kill router on the instance that each one gets? | |
| 19:03:09 | dansmith | service.manager.rpcapi.router = mock.Mock() ? | |
| 19:03:14 | dansmith | or whatever | |
| 19:03:54 | artom | @property makes it un-settable | |
| 19:04:09 | dansmith | can you delattr it? | |
| 19:04:35 | artom | Huh, didn't think to try that | |
| 19:04:39 | dansmith | anyway, doesn't matter | |
| 19:30:36 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: NUMA live migration support https://review.opendev.org/634606 | |
| 19:30:37 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Deprecate CONF.workarounds.enable_numa_live_migration https://review.opendev.org/640021 | |
| 19:30:37 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Functional tests for NUMA live migration https://review.opendev.org/672595 | |
| 19:31:51 | artom | dansmith, ^^ | |
| 19:32:20 | artom | (FWIW, I had the -patch27 in my local branch name for some reason, fixed that) | |
| 19:36:16 | dansmith | artom: one more thing I didn't catch last time, but it's easy | |
| 19:36:28 | dansmith | artom: otherwise I think it looks good, probably time for mriedem to have a go at it | |
| 19:41:07 | aspiers | can anyone point me at where the fake server instance object comes from in https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/compute/test_migrate_server.py#L579 ? | |
| 19:41:18 | aspiers | I've trawled through the test setup code and can't find it | |
| 19:43:04 | aspiers | ah | |
| 19:43:12 | aspiers | admin_only_action_common.py | |
| 19:53:09 | aspiers | still don't get it | |
| 19:53:38 | aspiers | if I use _stub_instance_get() then I get "Exception: This test uses methods that set internal oslo_db state, but it does not claim to use the database. This will conflict with the setup of tests that do use the database and cause failures later." | |
| 19:59:20 | melwitt | aspiers: is there a link to a patch you're working on that will give more context? my first guess is maybe your new test is accessing an attribute on instance that isn't populated, so it's trying to lazy-load it from the database, which is flagging your test as USES_DB = True | |
| 19:59:27 | openstackgerrit | Merged openstack/nova master: Remove unused methods https://review.opendev.org/679918 | |
| 20:00:17 | aspiers | melwitt: I'll upload | |
| 20:01:06 | melwitt | that error comes from a test inheriting from NoDBTestCase and then accessing the database | |
| 20:05:16 | aspiers | melwitt: does this test avoid it by patching out the meat of the code path (i.e. resize)? https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/compute/test_migrate_server.py#L124 | |
| 20:05:34 | aspiers | so it's effectively only testing the HTTP stuff in the top layer? | |
| 20:07:06 | melwitt | probably, that stub will fill in most of the instance objects attributes. so if anything in the code path accesses the attributes, they are already filled in, | |
| 20:07:42 | melwitt | with objects, if an attribute is *not* filled in and it is accessed, the object will fall back on a read from the database to get the value | |
| 20:08:07 | openstackgerrit | Merged openstack/nova master: Convert nova-lvm job to zuul v3 https://review.opendev.org/674345 | |
| 20:08:29 | melwitt | so it will test the full code path, just not the database accesses (if there were going to be any) | |
| 20:08:30 | efried | sean-k-mooney: in case this doesn't show up in your inbox/dashboard/whatever, would you please respond to this os-vif release patch: https://review.opendev.org/#/c/680094/ | |
| 20:09:10 | aspiers | melwitt: ahhh maybe that's it | |
| 20:10:09 | aspiers | melwitt: maybe it's missing the flavor or image_meta | |
| 20:10:39 | melwitt | aspiers: so sometimes what happens is you're working on a new test whose code path will access an attribute that's not stubbed out/filled in, and then it falls back on a read from the db, and if you're inherited from NoDBTestCase, that error will raise | |
| 20:11:18 | aspiers | no, just adding a call to self._stub_instance_get() is enough to trigger this db-related failure, *even* if I don't use the returned fake instance | |
| 20:11:31 | aspiers | so it's not my test code which is triggering the error | |
| 20:11:54 | melwitt | yeah if you look in https://github.com/openstack/nova/blob/master/nova/tests/unit/fake_instance.py#L45 you can see what's been filled in | |
| 20:11:59 | aspiers | yeah I spotted that | |
| 20:12:04 | melwitt | oh, hm | |
| 20:12:12 | aspiers | but this makes no sense, because other tests happily call that same method | |
| 20:13:40 | aspiers | melwitt: uploading now | |
| 20:13:52 | openstackgerrit | Adam Spiers proposed openstack/nova master: Reject live migration and suspend on SEV guests https://review.opendev.org/680158 | |
| 20:13:56 | aspiers | there ^^^ | |
| 20:14:40 | aspiers | melwitt: https://review.opendev.org/#/c/680158/1/nova/tests/unit/api/openstack/compute/test_migrate_server.py@592 causes this db error | |
| 20:14:51 | aspiers | why do other identical lines in the same file not cause it? | |
| 20:16:22 | melwitt | at a glance, I notice the other tests calling _stub_instance_get are also mocking the live_migrate method in compute_api | |
| 20:16:35 | aspiers | right, that's what I was saying earlier | |
| 20:16:43 | aspiers | they mock the meat of the code path | |
| 20:17:19 | aspiers | but the call to _stub_instance_get() isn't inside the mocking?! | |
| 20:17:49 | aspiers | so it can't be that | |
| 20:17:57 | aspiers | super weird | |
| 20:18:06 | openstackgerrit | Adam Spiers proposed openstack/nova master: Reject live migration and suspend on SEV guests https://review.opendev.org/680158 | |
| 20:18:28 | melwitt | you're saying the _stub_instance_get call itself will not return and raises that error? | |
| 20:18:33 | aspiers | yes | |
| 20:18:39 | melwitt | O.o | |
| 20:18:53 | melwitt | ok, that is unexpected | |
| 20:19:24 | aspiers | oh no wait | |
| 20:19:32 | aspiers | OK I get it now | |
| 20:19:42 | aspiers | _stub_instance_get() has side effects of course | |
| 20:19:47 | aspiers | it patches stuff | |