| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-25 | |||
| 18:18:42 | mriedem | i just don't want it munged into attachment_ref itself | |
| 18:18:53 | mriedem | per https://review.openstack.org/#/c/486194/2/nova/volume/cinder.py@220 | |
| 18:18:55 | jgriffith | mriedem that's reasonable | |
| 18:20:15 | jgriffith | mriedem ildikov I think I see the problem here... | |
| 18:20:41 | jgriffith | mriedem ildikov the attachment_create sadly returns a dict, while the attachment_update returns an attachment_ref object | |
| 18:21:10 | ildikov | jgriffith: don't we "play" with both? | |
| 18:21:33 | jgriffith | ildikov I don't know what you mean, but regardless... | |
| 18:21:39 | mriedem | jgriffith: yes the cinderclient code is confusing as well | |
| 18:21:55 | ildikov | jgriffith: that we translate both after calling to_dict() | |
| 18:22:12 | mriedem | ildikov: i think jgriffith is just talking about the cinderclient attachments code itself | |
| 18:22:13 | jgriffith | ildikov yes | |
| 18:22:18 | jgriffith | mriedem correct | |
| 18:22:19 | mriedem | create returns an object and update returns a dict | |
| 18:22:32 | mriedem | which i got wrong when i originally wrote the nova side code | |
| 18:22:37 | jgriffith | mriedem ildikov and the comment in the review asks "This is a VolumeAttachment object, yes?" | |
| 18:22:41 | jgriffith | I'm answering "no" | |
| 18:22:42 | jgriffith | it's not | |
| 18:23:02 | jgriffith | mriedem you had a 50/50 shot | |
| 18:23:07 | jgriffith | which is sad | |
| 18:24:15 | mriedem | jangutter: small issues inline | |
| 18:26:56 | jangutter | mriedem: I'm running out of 80 characters!!! | |
| 18:26:57 | ildikov | mriedem: jgriffith: ok, so we conclude to the format of: attachment_ref: {connection_info: {'data': {'foo': 'bar'}}} | |
| 18:27:13 | mriedem | jangutter: drop a line | |
| 18:27:24 | mriedem | ildikov: yes | |
| 18:27:26 | ildikov | mriedem: jgriffith: I will upload an update hopefully soon | |
| 18:27:36 | jgriffith | ildikov yeah; I think the whole point is to just make it a 1:1 mapping, I can put together the exact structures for you if you like? | |
| 18:27:51 | ildikov | mriedem: another thing, there's a test failure in the attach patch, which I cannot figure out | |
| 18:28:08 | jgriffith | ildikov I'm also happy to work on the patch if you want, but I'll send you a git-diff because I want nothing to do with your rebase magic :) | |
| 18:28:10 | ildikov | mriedem: if you happen to have a view on what to do that that would be pretty great :) | |
| 18:28:29 | ildikov | mriedem: otherwise I will duplicate a bit more code for now and optimize later when I learn Python a bit more... | |
| 18:29:04 | ildikov | jgriffith: it's ok if only one of us is messing with rebase, I can do that :) | |
| 18:29:13 | jgriffith | ildikov roger that | |
| 18:30:39 | mriedem | ildikov: this http://logs.openstack.org/85/330285/104/check/gate-nova-tox-functional-ubuntu-xenial/231f3e5/testr_results.html.gz ? | |
| 18:31:26 | ildikov | mriedem: yes | |
| 18:31:58 | mriedem | TestInstanceNotificationSample is using the CinderFixture | |
| 18:32:03 | ildikov | mriedem: it gets the service version mock and I have no idea why | |
| 18:32:45 | ildikov | mriedem: here's the relevant change: https://review.openstack.org/#/c/330285/104/nova/tests/functional/notification_sample_tests/test_instance.py | |
| 18:33:10 | ildikov | mriedem: I created a new fixture for the new flow and had the TestInstanceNotificationSample class using that one | |
| 18:33:32 | jangutter | mriedem: newbie question: I should check for the exception ... raised ... in the unit test, not trying to go check for the ideal output of the function? | |
| 18:33:33 | ildikov | mriedem: and created another class which uses the old fixture and gets the mock for the service version to switch back to the old flow | |
| 18:33:35 | mriedem | ildikov: we shouldn't have to mess with the notification sample tests at all | |
| 18:33:49 | ildikov | mriedem: you mean then to duplicate them? | |
| 18:33:51 | mriedem | ildikov: really we shouldn't have to mess with anything under nova/tests/functional | |
| 18:33:58 | ildikov | mriedem: why not? | |
| 18:34:03 | mriedem | leave nova/tests/functional for the old flows | |
| 18:34:13 | mriedem | ildikov: we can test the new flows with unit tests | |
| 18:34:52 | mriedem | jangutter: yes, basically the same as the test as you have except dev_type='generic' and then the test uses self.assertRaises | |
| 18:35:07 | ildikov | mriedem: it works nicely except that one test, which I can figure out in another way which means more duplication though | |
| 18:35:16 | jangutter | mriedem: roger | |
| 18:35:35 | ildikov | mriedem: or I can just delete that part and leave the mocks only to switch back to the old flow, that works too | |
| 18:35:49 | ildikov | mriedem: I guess we should run with that for now | |
| 18:36:19 | mriedem | ildikov: sorry i'm in a meeting and doing a few things at once and i don't have all of your changes in my head right now, | |
| 18:36:28 | mriedem | but in general, we shouldn't have to touch nova/tests/functional for the new stuff, | |
| 18:36:49 | mriedem | since that is mostly all for api samples and notification samples, which are stubbing out cinder in specific ways for the old flows | |
| 18:37:05 | mriedem | i mostly care about test coverage for the *new* flows using unit tests | |
| 18:37:19 | mriedem | we can worry about changing the various fixtures and functional tests over when we actually drop the *old* flows | |
| 18:37:23 | mriedem | which is not going to be anytime soon | |
| 18:37:39 | ildikov | mriedem: I figured out the swap stuff already and the rest is simple, the test failure is a mock and inheritance issue, and has nothing to do with Cinder | |
| 18:38:39 | ildikov | mriedem: but anyway, if you don't want changes there I will remove it and I will add mocks to the places which fails due to having the highest service version but no new Cinder calls available to switch back to the old flow and these tests in a follow up patch | |
| 18:39:30 | ildikov | mriedem: anyway, sorry for eating up this much of your time, I will go and do the updates we agreed on and then we can check if there's anything else to fix | |
| 18:39:49 | mriedem | ildikov: just leave the tests you have then in the api change, i can look into them at some point | |
| 18:40:13 | ildikov | mriedem: ok | |
| 18:40:19 | ildikov | mriedem: thanks | |
| 18:50:45 | sdague | melwitt: if you could comment on the bug, and move it to Fix Released, that would be cool | |
| 18:53:07 | sdague | mriedem: back for a bit, you look through those results yet? | |
| 18:53:26 | sdague | also, anyone, this is a pretty easy deprecation of a conf variable - https://review.openstack.org/#/c/486623/ | |
| 18:54:10 | mriedem | sdague: melwitt: the quotas bug for ocata pointed out earlier is not necessarily fix released | |
| 18:54:16 | mriedem | you can't backport counting quotas to ocata | |
| 18:54:31 | mriedem | but the fix might already be available, which i pointd out in the bug report and marked it incomplete since they didn't provide the version | |
| 18:54:42 | sdague | mriedem: sure, but fixed in master does count as fixed | |
| 18:54:55 | sdague | then it's a backport question | |
| 18:55:08 | mriedem | it's incomplete either way at this point | |
| 18:55:09 | sdague | but that doesn't make it not fixed | |
| 18:55:16 | sdague | incomplete doesn't mean that | |
| 18:55:52 | sdague | sure, as long as we're talking about incomplete just meaning "question back to reporter that needs an answer" | |
| 18:56:42 | sdague | but if it's fixed in master, then I'd say it's probably correct to comment as such and put Fix Released on it | |
| 19:07:14 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add oslo_concurrency=INFO to default log levels for nova-manage https://review.openstack.org/487179 | |
| 19:09:32 | openstackgerrit | Jan Gutter proposed openstack/nova master: Add VIFHostDevice support to libvirt driver https://review.openstack.org/486426 | |
| 19:18:11 | mriedem | sdague: dansmith: i've dumped my debug notes in https://review.openstack.org/#/c/477556/ | |
| 19:18:31 | mriedem | we discover and map the compute node on the primary host as part of the devstack stack.sh run on the primary host, | |
| 19:18:42 | mriedem | we discover and map the subnodes (2 of them) after the subnodes are stacked | |
| 19:18:45 | openstackgerrit | Jan Gutter proposed openstack/nova master: Netronome SmartNIC Enablement https://review.openstack.org/483459 | |
| 19:18:57 | mriedem | but it looks like when discover_hosts runs, we only discover and map the subnode-2, but miss subnode-3 | |
| 19:19:10 | mriedem | so this might just be a latent issue in 3-node jobs | |
| 19:19:21 | mriedem | but makes me wonder why we don't hit this more often in 2-node jobs | |
| 19:24:53 | openstackgerrit | Dan Smith proposed openstack/nova master: [WIP] Add some more cellsv2 doc goodness https://review.openstack.org/487183 | |
| 19:25:42 | dansmith | mriedem: okay I really hadn't done any 3 node thinking yet | |
| 19:25:59 | mriedem | dansmith: as far as i can tell, the 3rd node is being setup the same as the 2nd node | |
| 19:26:00 | dansmith | mriedem: are there actual production 3-node jobs that we'll break with this? | |
| 19:26:15 | mriedem | i don't know if there are any 3 node voting jobs, but i can dig | |
| 19:26:31 | dansmith | okay | |
| 19:26:34 | mriedem | i think we're basically getting lucky in the 2 node jobs | |
| 19:26:42 | openstackgerrit | Robert Ellis proposed openstack/nova master: Clarifying node_uuid usage in ironic driver. https://review.openstack.org/485803 | |
| 19:27:11 | mriedem | for example, this is a normal 2 node job | |
| 19:27:17 | mriedem | we discover subnode host here | |
| 19:27:18 | mriedem | http://logs.openstack.org/66/483566/10/check/gate-tempest-dsvm-neutron-multinode-full-ubuntu-xenial-nv/770b47e/console.html#_2017-07-24_16_27_34_062884 | |
| 19:27:21 | dansmith | meaning we're getting the first subnode from the main node? | |
| 19:27:23 | mriedem | 2017-07-24 16:27:34.062884 | + /opt/stack/new/devstack-gate/devstack-vm-gate.sh:main:L777: discover_hosts | |
| 19:28:00 | mriedem | and that subnode compute node was actually created after that | |
| 19:28:00 | mriedem | http://logs.openstack.org/66/483566/10/check/gate-tempest-dsvm-neutron-multinode-full-ubuntu-xenial-nv/770b47e/logs/subnode-2/screen-n-cpu.txt.gz#_Jul_24_16_27_35_403617 | |