Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-07-25
17:28:42 jaypipes k
17:28:43 mriedem should be done this afternoon
17:30:20 openstackgerrit Robert Ellis proposed openstack/nova master: Clarifying node_uuid usage in ironic driver. https://review.openstack.org/485803
17:31:58 openstackgerrit Balazs Gibizer proposed openstack/nova master: Add 'updated_at' field to InstancePayload in notifications https://review.openstack.org/475276
17:32:49 gibi mriedem: rebased the update_at bugfix ^^
17:33:14 gibi mriedem: I mean I've rebased
17:33:54 mriedem thanks
17:42:01 openstackgerrit Mark Giles proposed openstack/nova master: Do not attempt volume swap when guest is stopped/suspended https://review.openstack.org/389798
17:58:04 openstackgerrit Ken'ichi Ohmichi proposed openstack/nova master: Remove the code related to extension loading from APIRouterV21 https://review.openstack.org/486414
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?

Earlier   Later