| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-13 | |||
| 14:47:15 | mriedem | thanks | |
| 14:47:26 | dtantsur | what's the patch? | |
| 14:47:33 | mriedem | https://review.openstack.org/#/c/436094/35 | |
| 14:47:43 | mriedem | it's a devstack patch that gets us to multiple cells testing | |
| 14:47:50 | mriedem | with 2 levels of nova conductor | |
| 14:47:59 | mriedem | one in the top level controller node, and one in each cell | |
| 14:48:05 | mriedem | dansmith: ^ is that accurate? | |
| 14:48:12 | dtantsur | hmm, I wonder how this all works with ironic | |
| 14:48:25 | dansmith | mriedem: the two levels thing doesn't really matter to him, but yeah | |
| 14:49:36 | dansmith | mriedem: dtantsur I'm poking through lib/ironic now | |
| 14:49:46 | mriedem | i see cell1 created here http://logs.openstack.org/94/436094/35/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial-nv/1df37be/logs/devstacklog.txt.gz#_2017-06-12_20_10_32_098 | |
| 14:50:18 | mriedem | this isn't a multinode job, so it should just be a single cell right? | |
| 14:50:18 | dtantsur | I think what we do, or at least what we used to do, is to restart nova-compute for some reason | |
| 14:50:27 | dtantsur | not sure if it's related | |
| 14:50:56 | mriedem | i'm not seeing simple_cell_setup or discover_hosts run anywhere in the devstack log | |
| 14:51:25 | dtantsur | we don't seem to run it indeed.. | |
| 14:51:30 | dtantsur | but then we would break long ago, no? | |
| 14:52:05 | mriedem | dansmith: keep in mind they have an ironic devstack plugin https://github.com/openstack/ironic/tree/master/devstack | |
| 14:52:14 | dansmith | mriedem: right | |
| 14:52:18 | dansmith | mriedem: just pushed a change | |
| 14:52:27 | dansmith | https://review.openstack.org/473868 | |
| 14:52:46 | dtantsur | thanks dansmith, let's try this | |
| 14:53:02 | mriedem | s/.///? in there? | |
| 14:53:10 | dansmith | gah yes | |
| 14:53:10 | dtantsur | to be honest, I have hard time understanding the recent changes around cells, etc.. | |
| 14:53:35 | dansmith | mriedem: fixed | |
| 14:54:37 | dtantsur | dansmith: shouldn't the dependency be the other way around? i.e. the devstack patch to depend on the ironic change? | |
| 14:54:45 | mriedem | yes | |
| 14:55:14 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: use context mgr in instance.delete https://review.openstack.org/443764 | |
| 14:55:14 | mriedem | i'm also not sure how any of the ironic jobs have worked w/o discovering the hosts by now | |
| 14:55:15 | mriedem | since ocata | |
| 14:55:24 | dtantsur | this is what I wonder as well | |
| 14:55:32 | dtantsur | we had to do it in tripleo | |
| 14:56:20 | dansmith | dtantsur: not for testing | |
| 14:56:52 | dansmith | dtantsur: I'll swap the order when this shows the job working on this patch | |
| 14:57:00 | dtantsur | ack | |
| 15:05:01 | mriedem | this is what normally runs discover_hosts https://github.com/openstack-dev/devstack/blob/master/stack.sh#L1412 | |
| 15:05:32 | mriedem | maybe the ironic thing happens before that? | |
| 15:05:33 | openstackgerrit | Vladyslav Drok proposed openstack/nova master: Remove only compute ports in deallocate_for_instance https://review.openstack.org/473873 | |
| 15:05:35 | dansmith | right | |
| 15:05:36 | dansmith | right | |
| 15:05:42 | dansmith | that is the late run to pick up stragglers I think, | |
| 15:05:53 | dansmith | and this is earlier | |
| 15:06:07 | dansmith | I don't think the devstack change makes anything like db init happen in any different order, | |
| 15:06:21 | dansmith | so I'm kinda suspecting that we're using the wrong config file somewhere and never installing the maps at the top | |
| 15:06:38 | dansmith | but I can chase that down.. just want to do this as the easy first stab and see where we are | |
| 15:06:49 | dansmith | if this doesn't work, it's definitely a config file thing I think | |
| 15:06:59 | dansmith | definitely I think | |
| 15:09:14 | huanxie | stack.org/#/c/462017/ ? And TBH, I'm not quit sure which one is better now, so could you help give some suggestions? Thanks a lot:) | |
| 15:09:14 | huanxie | Hi jaypipes, for VM lost connection issue during live migration with XenServer, I saw your comments with this patch https://review.openstack.org/#/c/451657/ and I think it's reasonable to hide detailed bridge operation, so I made another patch https://review.openstack.org/#/c/462017/ for this issue. Could you help have a look at https://review.open | |
| 15:10:40 | cfriesen | mriedem: dansmith: If you have a few minutes, I addressed some of mriedem's comments on https://review.openstack.org/#/c/435141/ but I'm stuck on how to mock the unit tests to do the api version. | |
| 15:11:14 | dansmith | cfriesen: that's not a thing I'm going to be helpful with. ask sdague or alex_xu | |
| 15:12:48 | jaypipes | huanxie: yup, it's on my queue :) sorry, in three hour meeting right now :( | |
| 15:13:08 | mriedem | cfriesen: andreykurilin might be able to help, otherwise i can look later | |
| 15:13:25 | andreykurilin | hi | |
| 15:13:29 | cfriesen | hey | |
| 15:13:53 | andreykurilin | cfriesen: what had happend? need help?) | |
| 15:13:54 | huanxie | That's fine, please leave your comments and concern and I will address your comments my next Monday:) | |
| 15:14:06 | cfriesen | andreykurilin: see https://review.openstack.org/#/c/435141/ | |
| 15:14:12 | andreykurilin | looking | |
| 15:14:41 | cfriesen | andreykurilin: I added a microversion such that the "nova list" command now needs to check microversions to determine how to format the output, and it's breaking a bunch of novaclient tests. | |
| 15:15:01 | huanxie | Hi jaypipes, sorry for my mistack, I mean I will fix your comments my next morning :) | |
| 15:15:21 | jaypipes | huanxie: no problem! | |
| 15:16:01 | cfriesen | andreykurilin: the tests in question use mock.MagickMock for novaclient.client.Client, and then later on some of the tests do @mock.patch('novaclient.client.Client'), and I can't figure out how to get them to report api_version properly | |
| 15:16:09 | huanxie | Thanks a lot jaypipes | |
| 15:16:36 | andreykurilin | cfriesen: are you talking about unit or functional tests? | |
| 15:17:06 | cfriesen | andreykurilin: unit. novaclient/tests/unit/test_shell.py | |
| 15:17:52 | mriedem | cfriesen: wouldn't you just put an api_version attribute on the mock? | |
| 15:19:42 | cfriesen | mriedem: the code in question does this in setUp(): self.useFixture(fixtures.MonkeyPatch('novaclient.client.Client', mock.MagicMock())) | |
| 15:20:12 | cfriesen | mriedem: I can't figure out how to put an api_version attribute on that | |
| 15:20:20 | andreykurilin | cfriesen: it looks like just that module requires some cleanup | |
| 15:20:38 | mriedem | cfriesen: mock.MagicMock(api_version=api_versions.APIVersion('2.1')) | |
| 15:20:40 | mriedem | or something | |
| 15:20:41 | andreykurilin | if setUp method uses fixtures, no @mock.patch is required | |
| 15:22:36 | andreykurilin | cfriesen: I suppose, we need to same mock.MagicMock() object as self.mock_client before transmitting to MonkeyPatch, and than you will able to do `self.mock_client.api_version = api_versions.APIVersion("2.22")` in your test | |
| 15:24:58 | cfriesen | andreykurilin: if you don't @mock.patch on the test itself, how do you do stuff like "client_kwargs = mock_client.call_args_list[0][1]" ? | |
| 15:25:31 | andreykurilin | cfriesen: as I mentioned previously, we need to store object transmitted to MockPatch in setUp method | |
| 15:25:59 | cfriesen | andreykurilin: I think I see. will try that | |
| 15:29:10 | cfriesen | andreykurilin: you are a genius. Looks like that'll work. let me respin a new version for review. | |
| 15:29:47 | andreykurilin | cfriesen: If you have time, please submit a patch with removing all redundant @mock.patch there | |
| 15:30:03 | cfriesen | andreykurilin: whoops, spoke too soon. TypeError: '<MagicMock name='mock().api_version' id='69241360'>' should be an instance of '<class 'novaclient.api_versions.APIVersion'>' | |
| 15:30:53 | andreykurilin | cfriesen: in your test, you should do something like `self.mock_client.api_version = api_versions.APIVersion("2.22")`` where api_versions is a module novaclient.api_versions | |
| 15:31:42 | mriedem | _CellProxy | |
| 15:31:42 | cfriesen | andreykurilin: this is what I have now: http://paste.openstack.org/show/612435/ | |
| 15:31:43 | mriedem | oh _CellProxy | |
| 15:31:46 | mriedem | why oh why | |
| 15:33:18 | cfriesen | andreykurilin: and this is what I get when running it: http://paste.openstack.org/show/612437/ | |
| 15:33:47 | mriedem | cfriesen: needs to be an object, | |
| 15:33:49 | mriedem | not a string | |
| 15:34:38 | mriedem | cfriesen: https://github.com/openstack/python-novaclient/blob/master/novaclient/client.py#L59 | |
| 15:34:43 | mriedem | ^ is what you're mocking out ultimately | |
| 15:34:57 | cfriesen | mriedem: what needs to be an object? | |
| 15:34:58 | mriedem | so it needs to be of type novaclient.api_versions.APIVersion | |
| 15:35:09 | mriedem | cfriesen: self.mock_client.api_version = novaclient.API_MAX_VERSION | |
| 15:35:13 | mriedem | that's a string | |
| 15:35:15 | mriedem | needs to be an object | |
| 15:35:22 | mriedem | an APIVersion object | |
| 15:35:26 | mriedem | which is constructed with a string value | |
| 15:36:02 | mriedem | i would probably also use API_MIN_VERSION as the default | |
| 15:36:06 | mriedem | for existing tests | |