| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-25 | |||
| 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 | |
| 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 | |