Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-12
15:47:13 openstack Launchpad bug 1737779 in OpenStack Compute (nova) "Volume attach sets mountpoint as /dev/na in Cinder attachment" [Undecided,New]
15:48:24 cdent jaypipes: I responded on the AttributeError question, I think it's right as is.
15:48:41 jaypipes ack
15:51:02 openstackgerrit Balazs Gibizer proposed openstack/nova master: Stabilize test_live_migration_abort func test https://review.openstack.org/527440
15:51:38 ildikov smatzek: thanks for the detailed bug report!
15:53:52 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Add cache headers to placement api requests https://review.openstack.org/521640
15:53:52 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Add info about last-modified to contrib docs https://review.openstack.org/526084
15:54:25 cdent jaypipes: with luck that ^ gets it. The expected follow to do the unit tests Takashi asked for is on my list for this week.
15:55:26 mriedem ildikov: smatzek: i wonder if that's what this was for https://review.openstack.org/#/c/525787/3/nova/virt/block_device.py@448 ?
15:56:20 jaypipes cdent: k.
15:56:40 mriedem smatzek: i'll push a patch to add that back in so we can see if trove ci is happy with it
15:57:01 mriedem smatzek: is this recreatable in trove ci or are you testing it manually?
15:57:18 smatzek trove CI.
15:57:23 mriedem ok good
15:57:46 ildikov mriedem: yeah, that would make a lot of sense
15:59:00 ildikov mriedem: I thought we didn't remove that finally, mistake on my side
15:59:33 smatzek is the connector an object or a dict?
15:59:46 mriedem smatzek: dict
16:00:09 smatzek on the cinder side it's doing a .get('mountpoint') on the connector, so I'm not sure mount_device in the connector on the Nova side would help unless there is some key translation
16:01:46 jaypipes cdent: +W
16:01:51 melwitt mriedem: ack, will take a look
16:01:55 cdent jaypipes: rad
16:02:06 cdent only one more microversion conflict to go!
16:03:15 jaypipes cdent: :)
16:03:24 jaypipes cdent: I know the feeling very well.
16:03:45 openstackgerrit Balazs Gibizer proposed openstack/nova master: Wait for live_migration_rollback.start notification https://review.openstack.org/527444
16:07:46 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass bdms to versioned notifications during finish_revert_resize https://review.openstack.org/527446
16:08:49 mriedem smatzek: where are you seeing that in the cinder code?
16:09:05 smatzek https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L4396
16:10:01 mriedem ok that makes sense - with the old flow, we'd pass mountpoint to the os-attach API
16:10:10 mriedem and it looks like attachment_update needs the same thing
16:10:44 mriedem eh which sucks now because i have to handle attachment_update in multiple places
16:13:21 ildikov mriedem: can't we handle that centrally somehow?
16:14:16 mriedem i plan on adding a mountpoint kwarg to the attachment_update method and passing in self['mount_device'] from the DriverVolumeBlockDevice code (normal attach, bfv) and from the resize code
16:14:26 mriedem from the resize code, the mountpoint is i think just the bdm.device_name
16:15:03 ildikov mriedem: how did it work with the old flow?
16:15:24 mriedem but...the 'mount_device' in the driver block device code is after the blockinfo stuff has massaged the device_name, so i'm not sure if they will actually be the same thing
16:15:46 mriedem with the old flow, during resize, we didn't call attach again
16:16:22 jaypipes dansmith, mriedem: do we not have any functional tests for db schema migrations against MySQL in the gate?
16:16:50 mriedem jaypipes: the walk_migrations tests should run through mysql and postgresql
16:16:55 mriedem in the python unit test jobs
16:17:01 jaypipes that's what I thought too...
16:17:20 ildikov mriedem: ah ok, we passed it in the volume_api.attach() call
16:17:29 mriedem http://logs.openstack.org/87/525787/4/check/openstack-tox-py27/5a1b999/job-output.txt.gz#_2017-12-08_14_36_36_831356
16:17:31 mriedem jaypipes: ^
16:17:35 mriedem ildikov: yes
16:17:50 mriedem and for resize old flow we don't call block_device.attach() again
16:17:58 mriedem we just use init_connection and terminate_connection
16:18:06 mriedem with the new flow, we're doing attachment_updates now
16:19:00 mriedem actually we should be ok just pulling the bdm.device_name during resize https://review.openstack.org/#/c/525787/3/nova/virt/block_device.py@246
16:19:05 mriedem because the driver block device code uses that
16:19:29 mriedem https://review.openstack.org/#/c/525787/3/nova/virt/block_device.py@408
16:19:33 ildikov mriedem: yeah, I was just wondering whether we could have it updated and stored with the connector
16:19:49 ildikov mriedem: but can just use it from the BDM too, maybe less confusing that way
16:20:01 mriedem the connector is per host
16:20:04 mriedem the mountpoint is per volume
16:20:08 mriedem so doesn't really work that way
16:20:37 mriedem so, actually, it's really confusing that the attachment_update API expects the mountpoint in the connector dict rather than a top-level parameter on the PUT /attachments/{id}/ call itself
16:20:39 mriedem jgriffith: ^
16:20:44 mriedem that seems like a bug in the API
16:20:59 ildikov I think it's because we're handling and saving things now per attachment
16:21:01 mriedem but of course is a microversion to fix
16:21:15 mriedem sure, it should still be a top-level field on the attachment though
16:21:23 mriedem not hidden down in the nebulous connector dict
16:21:55 ildikov it might've been intentional, but if it needs to be fixed we can do that with the microversion of course
16:23:26 mriedem we can workaround it for now
16:23:29 mriedem on the nova side
16:23:41 mriedem i added cinder to https://bugs.launchpad.net/cinder/+bug/1737779 with the explanation of the cinder API issue
16:23:42 openstack Launchpad bug 1737779 in OpenStack Compute (nova) "Volume attach sets mountpoint as /dev/na in Cinder attachment" [High,Triaged] - Assigned to Matt Riedemann (mriedem)
16:26:13 ildikov mriedem: ok, thanks, I will try to reach jgriffith and see what to do with this longer term
17:00:12 mriedem melwitt: fyi i'll have to update that patch for the resize thing, i'll need to rebase it on the fix for bug 1737779
17:00:14 openstack bug 1737779 in OpenStack Compute (nova) "Volume attach sets mountpoint as /dev/na in Cinder attachment" [High,Triaged] https://launchpad.net/bugs/1737779 - Assigned to Matt Riedemann (mriedem)
17:01:57 melwitt mriedem: k
17:23:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Update and complete volume attachments during resize https://review.openstack.org/527228
17:23:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass bdms to versioned notifications during finish_revert_resize https://review.openstack.org/527446
17:23:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass mountpoint to volume attachment_update https://review.openstack.org/527468
17:23:22 mriedem ildikov: smatzek: melwitt: ^ passing mountpoint to attachment_update now - note that i also need to do the same for attachment_create during live migration when we create the attachment with the connector, but i'm going to handle that in a separate patch
17:23:25 mriedem for review sanity
17:23:38 mriedem https://github.com/openstack/nova/blob/32c8ac6b7dfe4ca0c211cbce7c5a67d88558126f/nova/compute/manager.py#L5786-L5790
17:26:06 mriedem smatzek: should just be able to recheck that same trove ci test patch from yesterday
17:28:43 smatzek mriedem: thanks, but I should add a depends-on for the review above as well, correct?
17:28:53 mriedem not necessary
17:28:57 mriedem you have a depends-on the patch above it
17:29:01 mriedem so it'll pull in the series
17:30:30 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Add PCI NUMA policy fields https://review.openstack.org/527470
17:30:31 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Pass InstanceNUMATopology to consume_request https://review.openstack.org/527471
17:30:31 openstackgerrit Stephen Finucane proposed openstack/nova master: Add PCI NUMA policies https://review.openstack.org/527472
17:30:32 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Modify signature of _filter_non_requested_pfs https://review.openstack.org/527473
17:32:06 stephenfin jaypipes: I've gone and broken that PCI NUMA policy patch into smaller components so it should be easier to review. Hopefully it helps somewhat
17:39:30 ildikov mriedem: the series looks good for the first glance, hopefully tests will confirm as well
17:39:33 ildikov mriedem: thanks
17:40:17 mriedem stephenfin: replied in https://review.openstack.org/#/c/526329/
17:40:31 mriedem stephenfin: seems you can make that work for both old and new glanceclient so it could be backported
17:41:15 melwitt mriedem: so what would be in the new cinder microversion in the future? cinder handling mountpoint automatically?
17:41:32 mriedem melwitt: no, make it a top-level field on the attachment resource
17:41:50 melwitt okay
17:42:03 mriedem so PUT /attachments/{id}/ with a body that has {'mountpoint': '/dev/vdb', ...}
17:42:29 smcginnis mriedem: Why does that need to change?
17:42:30 mriedem the connector dict is host detail stuff from brick
17:42:47 mriedem smcginnis: see https://review.openstack.org/527468

Earlier   Later