| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-25 | |||
| 17:58:39 | openstackgerrit | Ken'ichi Ohmichi proposed openstack/nova master: Remove the useless FakeExt https://review.openstack.org/486415 | |
| 17:58:47 | openstackgerrit | Ken'ichi Ohmichi proposed openstack/nova master: Remove the useless extension block_device_mapping_v1 object https://review.openstack.org/486069 | |
| 17:59:45 | ildikov | mriedem: I added a comment to the translation patch | |
| 18:00:09 | ildikov | mriedem: I think the base for the confusion there is that the namings there are pretty confusing | |
| 18:00:31 | oomichi | alex_xu: re: https://review.openstack.org/#/c/486414/ yeah, that is an important one. +2 | |
| 18:00:37 | ildikov | mriedem: would that be fine to make that human readable or you want the structure change too? | |
| 18:02:23 | melwitt | sdague: yeah, that shouldn't be a thing with counting quotas in Pike. but maybe probably there needs to be a fix on stable only? I can't remember if we ever do that cc mriedem | |
| 18:02:27 | mriedem | ildikov: i said in https://review.openstack.org/#/c/486194/2/nova/volume/cinder.py@218 that we should rename that data_keys variable to connection_info | |
| 18:02:42 | mriedem | ildikov: however, _translate_attachment_ref doesn't return a connection_info dict | |
| 18:03:01 | mriedem | it mangles the attachment dict, | |
| 18:03:07 | mriedem | and adds a ['data'] key in it | |
| 18:03:17 | mriedem | and shoves the attachment['connection_info'] stuff in there | |
| 18:03:20 | mriedem | as far as i can tell | |
| 18:04:15 | ildikov | as connection_info is a free form data structure having stuff under the 'data' key and 'driver_volume_type' on top level with it basically fulfills the criteria as far as I can tell | |
| 18:05:21 | ildikov | but jgriffith is smarter than me on this front | |
| 18:06:06 | mriedem | ewww yeah i don't like that | |
| 18:06:10 | mriedem | and i just got now what it's doing | |
| 18:06:26 | mriedem | it's the attachment ref PLUS all of the crap from the connection_info, mainlined into the attachment ref body resp | |
| 18:06:35 | mriedem | that's super confusing and i don't think we should do that | |
| 18:06:49 | mriedem | let's just translate the connection_info within the attachment ref as i said in there | |
| 18:07:16 | ildikov | well, connection_info goes under 'data' as how it used to be before | |
| 18:07:32 | mriedem | thios https://review.openstack.org/#/c/486194/2/nova/volume/cinder.py@220 | |
| 18:07:37 | mriedem | *this | |
| 18:08:03 | mriedem | yes i get that | |
| 18:08:13 | mriedem | but it's also mangled into the attachment response body | |
| 18:09:04 | mriedem | if the cinder API returns attachment: {connection_info: {'foo': 'bar'}} and now we turn that into attachment: {'data': {'foo': 'bar'}} that gets confusing | |
| 18:10:01 | mriedem | i'd prefer to just see attachment: {connection_info: 'data': {{'foo': 'bar'}}} at the end | |
| 18:10:16 | mriedem | well ^ is busted, but you know | |
| 18:11:22 | jangutter | mriedem, jaypipes: https://review.openstack.org/#/c/486426 got the checkmark from Jenkins.... but did I throw my first exception correctly? | |
| 18:11:52 | mriedem | jangutter: omfg you don't throw anything! | |
| 18:12:02 | mriedem | :P | |
| 18:12:02 | jgriffith | mriedem ok, well this is why I dumped all that crap in the first place (which got us to that point) | |
| 18:12:04 | mriedem | you RAISE! | |
| 18:12:25 | jgriffith | mriedem the issue being that in my opinion that original return info was a confusing free form mess | |
| 18:12:39 | jangutter | mriedem: what the heck? is this Poker or Python? | |
| 18:12:43 | jgriffith | but then *we* decided we wanted to keep it consistent | |
| 18:12:44 | mriedem | jangutter: seems ok | |
| 18:12:58 | jgriffith | so that's why it's now stuffing in the way it is | |
| 18:13:38 | mriedem | jgriffith: i'm fine if cinder 3.27 wanted to flatten the connection_info dict and drop the 'data' subkey | |
| 18:14:01 | mriedem | but what i don't like is doing attachment_ref.update(attachment_ref.pop('connection_info', {})) basically | |
| 18:14:04 | mriedem | sans the driver_volume_type key | |
| 18:14:23 | mriedem | er attachment_ref.update(dict(data=attachment_ref.pop('connection_info', {}))) | |
| 18:14:47 | ildikov | mriedem: the removal of the 'data' key on the Nova side is quite an amount of code line change apparently :( | |
| 18:14:58 | mriedem | because then am i dealing with an attachment representation, or a connection_info, or some weird hybrid? | |
| 18:15:01 | ildikov | mriedem: that's why we thought to do that at another time | |
| 18:15:07 | mriedem | ildikov: yes it should be done another time | |
| 18:15:50 | jgriffith | mriedem so wait... do you have a better idea on how to translate that other than popping it out into a new struct? | |
| 18:15:58 | ildikov | mriedem: ok, at least one thing we agree at :) | |
| 18:18:33 | mriedem | jgriffith: i'm fine with popping out the original one and writing it back into attachment_ref['connection_info'] | |
| 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 | |