| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-30 | |||
| 14:11:36 | gibi | dansmith: I can push that change in a separate commit top of stephenfin's if that helps | |
| 14:12:42 | dansmith | well, I just don't want to end up without functional tests for this stuff, but I understand why you think this isn't working | |
| 14:12:48 | dansmith | so let me dig in a bit I guess | |
| 14:14:46 | gibi | dansmith: I'm affraid we are missing something as this functional test is pretty complex. | |
| 14:15:05 | dansmith | I understand | |
| 14:16:04 | gibi | dansmith: If we find nothing then I can accept to merge this as there are new tests that seems OK | |
| 14:17:17 | dansmith | gibi: your comment about L96.. is that new test using that? I don't think it calls _run_build_test() | |
| 14:17:35 | dansmith | because I thought these were stubbed way lower, which is why mriedem suggested just unstubbing in these couple new tests | |
| 14:18:03 | dansmith | I also thought that this was stubbing neutronapi methods, | |
| 14:18:17 | dansmith | but the fake_stub_network_methods() on L96 is stubbing out compute stuff | |
| 14:18:28 | dansmith | unset will unset any that have been stubbed | |
| 14:19:03 | gibi | dansmith: that could explain what I see. Let me dig in after the scheduler meeting | |
| 14:19:38 | gibi | dansmith: in general I think these tests should work with non stubbed neutronv2/api | |
| 14:19:58 | dansmith | gibi: I'm not sure how, we still have to return fake responses from neutron | |
| 14:20:25 | gibi | dansmith: yeah, that is the only thing that need to be faked | |
| 14:22:17 | dansmith | well, it's using the neutronfixture, which seems like the right thing to do... | |
| 14:23:52 | dansmith | ah | |
| 14:23:55 | dansmith | gibi: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L79-L79 | |
| 14:24:02 | dansmith | that is the test this inherits from, which stubs things out | |
| 14:24:25 | dansmith | I think if we don't unstub them we don't call into our neutron fixture to return the augmented nw_info, which is why we don't see the new stuff unless we unstub | |
| 14:24:48 | dansmith | so NUMAServersTestWithNetworks->NUMAServersTestBase->ServersTestBase->the stubbing | |
| 14:25:40 | gibi | dansmith: OK, then my expectation is that if I move the unstub to the NUMAServersTestBase.setup() then everything should pass | |
| 14:26:18 | dansmith | do you mean NUMAServersWithNetworksTest? | |
| 14:26:29 | dansmith | I don't think this patch needs to change the other tests that are here | |
| 14:27:36 | gibi | dansmith: I theory both NUMAServersTestBase but I agree that in _this_ patch we should not change the other tests, so the NUMAServersTestBase change would be a followup | |
| 14:27:46 | gibi | s/both// | |
| 14:27:53 | dansmith | I just moved it to setup and I think all these new ones pass.. will run the whole file to be sure | |
| 14:28:06 | sean-k-mooney | dansmith: for the numa aware vswitch spec what is the expect behavior if you do not enable the numatopology fileter | |
| 14:28:26 | dansmith | TBH, I'm not sure I see why moving it to setup matters, but.. | |
| 14:28:40 | dansmith | sean-k-mooney: are you asking? | |
| 14:29:18 | sean-k-mooney | dansmith: yes. with stephenfin code we do not validate the constratins on the compute node just in the schduler | |
| 14:29:32 | sean-k-mooney | at least that is what i am seeing locally. | |
| 14:29:33 | dansmith | sean-k-mooney: right, with the filter disabled this is just all ignored | |
| 14:29:47 | sean-k-mooney | ya is that correct however? | |
| 14:29:52 | dansmith | yes | |
| 14:30:18 | sean-k-mooney | ok cool | |
| 14:31:00 | sean-k-mooney | im guessing we dont want a late check like the anti affinity stuff to avoid retries? | |
| 14:31:01 | gibi | dansmith: I thought originally that L96 was the stubbing for every NUMAServer tests (and I was wrong) and in general I want to have as small amount of stub in the functional tests as possible. Now that I know that we get the stubbing from a lot further base it means If I need to follow my principle then I have to do a bit more surgery _outside_ of the current patch | |
| 14:31:01 | dansmith | gibi: another test fails if I do that | |
| 14:31:12 | dansmith | gibi: it's related to this series, but not this patch | |
| 14:31:25 | dansmith | so if I change this I will have to update that test too | |
| 14:31:46 | dansmith | gibi: totally agree with minimal stubbing in functional tests for sure | |
| 14:31:58 | dansmith | this is testing fake libvirt with fake neutron, so... lots of stubbing required regardless :/ | |
| 14:32:01 | gibi | dansmith: OK, I don't think any more that is worth to blow up the current patch | |
| 14:32:26 | dansmith | gibi: I will put the move and the related changes in a separate patch on top then if that's okay with you | |
| 14:32:28 | gibi | dansmith: let's merge this as is and I will spend time on a followup to reduce the stubbing if possible | |
| 14:32:45 | gibi | dansmith: that totally works for me | |
| 14:38:20 | mnaser | dansmith: whenever you're free, if you can add this to your queue -- https://review.openstack.org/#/c/586824/ (and the patch above it is more of a poc) -- with some info in the bug | |
| 14:39:12 | dansmith | alright | |
| 14:39:51 | kashyap | mnaser: Maybe it's just me, but first paragraph in the commit message is confusing | |
| 14:40:52 | kashyap | (And the patch above needs to be rebased I think) | |
| 14:40:56 | dansmith | makes sense to me | |
| 14:42:06 | kashyap | You have more context, though :-) But yeah, reading the whole message makes it clearer. | |
| 14:44:58 | mnaser | kashyap: yeah, it's still very much early and needs reviews :) | |
| 14:45:08 | mnaser | it was more of a rough "hey how's this to solve a problem" | |
| 14:46:37 | dansmith | gibi: the fix for the other test was in the create_port() handler of the fixture, so it was touched by this test and part of your other feedback anyway, | |
| 14:46:47 | dansmith | gibi: so I'll just squash and update this patch | |
| 14:46:53 | gibi | dansmith: OK, cool | |
| 14:47:10 | openstackgerrit | Dan Smith proposed openstack/nova master: Add additional functional tests for NUMA networks https://review.openstack.org/585385 | |
| 14:47:13 | dansmith | gibi: note that just moves it up one test level, not all the way to the top | |
| 14:48:14 | kashyap | mdbooth: Not sure if you'll be able to today, but this is ready and fixed all the tests (including the live migration job): https://review.openstack.org/#/c/567258/ | |
| 14:48:33 | mdbooth | kashyap: ack | |
| 14:48:37 | kashyap | Gentle ping because you've reviewed it before. (And I'm off for 2 weeks from Wed.) | |
| 14:48:41 | kashyap | mdbooth: Thanks! | |
| 14:50:13 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Updated AggregateImagePropertiesIsolation filter illustration https://review.openstack.org/586317 | |
| 14:51:55 | tetsuro_ | gibi: I'm going to rebase https://review.openstack.org/#/c/585672/ on gibi's 1.28 support one. | |
| 14:52:00 | openstack | Launchpad bug 1783635 in OpenStack Compute (nova) "Live migration failed.: AttributeError: 'Guest' object has no attribute 'migrate_configure_max_speed'" [Undecided,New] | |
| 14:52:00 | mriedem | lyarwood: looks like a backport to pike is using a method that doesn't even exist :( https://bugs.launchpad.net/nova/+bug/1783635 | |
| 14:52:09 | tetsuro_ | gibi: Before pushing them, I'm looking into the functional test in https://review.openstack.org/#/c/527728/18 to pass rebased on the two. | |
| 14:52:27 | tetsuro_ | gibi, lajos: Would you mind if I amend https://review.openstack.org/#/c/527728/18? | |
| 14:54:37 | mriedem | lyarwood: i guess you figured that out already https://review.openstack.org/#/c/586965/ | |
| 14:54:39 | gibi | tetsuro_: thanks for the heads-up. I have nothing agains the rebase. I talked to lajos before and he let me take over https://review.openstack.org/#/c/527728 so I think I can let you in his name to amend it :) | |
| 14:56:08 | tetsuro_ | gibi: Thanks! | |
| 14:56:11 | gibi | tetsuro_: regarding the rebase the patch adding support for 1.29 top of 1.28. That is also cool with me | |
| 14:56:45 | tetsuro_ | Yup. That would be the right way to go. | |
| 14:56:49 | gibi | tetsuro_: I will try to keep continue progressing support for 1.28 as my time allows during the RC period so we can merge it soon in Stein | |
| 14:58:51 | tetsuro_ | gibi: Cool. I can help the "change unit tests to pass" part if I finish the rebase and test work earlier than I think. | |
| 15:00:00 | gibi | tetsuro_: I consider unit test secondary in 1.28 but thanks for any help with those. I'm more affraid of how to add proper functional coverage for the consumer generation conflict cases | |
| 15:00:54 | tetsuro_ | gibi: yup, that sounds tough... | |
| 15:01:12 | mriedem | dansmith: https://review.openstack.org/#/c/586965/ to unbreak live migration in pike | |
| 15:01:36 | gibi | tetsuro_: I have some ideas but until I see them working I'm in doubt | |
| 15:01:51 | mriedem | well, live migration + pike + linuxbridge, which we don't test in CI | |
| 15:02:11 | dansmith | mriedem: is this because of that speed limit thing? | |
| 15:02:22 | openstackgerrit | Merged openstack/nova master: tox: Reuse envdirs https://review.openstack.org/534382 | |
| 15:02:22 | dansmith | for LB yeah | |
| 15:03:19 | tetsuro_ | gibi: But as many project (neutron, cyborg, blazar etc) are looking forward the feature we can just continue to work. | |
| 15:03:54 | tetsuro_ | gibi: Anyway, I should go to bed now, sorry. Thanks for your info. | |
| 15:04:17 | gibi | tetsuro_: yeah, not to mention that my work on bandwidth base scheduling also needs it :) | |
| 15:04:33 | gibi | tetsuro_: thanks for the info too. good night! | |
| 15:04:56 | tetsuro_ | gibi: Good day and night! | |
| 15:10:31 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Add unshelve instance error info to fault table https://review.openstack.org/583426 | |
| 15:16:56 | gibi | dansmith: thanks for the fix in https://review.openstack.org/#/c/585385 I'm +2 now | |
| 15:17:05 | gibi | mriedem: ^^ you were +2 before | |
| 15:17:26 | dansmith | gibi: thanks | |
| 15:35:05 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/ocata: Add unshelve instance error info to fault table https://review.openstack.org/583422 | |
| 16:11:59 | mnaser | well i sure don't run out of ways to find how broken this was :( | |
| 16:12:41 | mnaser | instances is shown when listing instances (with ACTIVE state), getting the specific instance (nova show/delete/etc) gives 404, instance_mapping exists, it exists in instances table in the appropriate cell | |
| 16:12:51 | mnaser | and no build request | |
| 16:12:53 | mnaser | so things.. should be right? | |
| 16:15:45 | Guest9714 | mnaser: instance_mapping with cell_id=None? | |