Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-12
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
17:43:11 mriedem it doesn't *have* to change, but it's really clunky API behavior the way it is
17:45:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass mountpoint to volume attachment_create with connector https://review.openstack.org/527479
17:45:14 mriedem and ^
17:47:01 dansmith gibi: are you planning to hit this? https://review.openstack.org/#/c/517119/
17:47:24 dansmith aside from the comment jaypipes is going to add, it looks okay to me, but I'm admittedly glossing over a lot of the database details
17:51:01 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass mountpoint to volume attachment_update https://review.openstack.org/527468
17:51:01 openstackgerrit Matt Riedemann proposed openstack/nova master: Update and complete volume attachments during resize https://review.openstack.org/527228
17:51:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass bdms to versioned notifications during finish_revert_resize https://review.openstack.org/527446
17:51:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Pass mountpoint to volume attachment_create with connector https://review.openstack.org/527479
17:55:56 jaypipes dansmith: I already added my comment.
17:56:13 dansmith jaypipes: not that I see
17:56:47 jaypipes dansmith: it's inline in https://review.openstack.org/#/c/517119/17/nova/objects/resource_provider.py
17:58:25 dansmith jaypipes: your last comment on that patch asks if you can just add a note to satisfy the concern.. doesn't that mean you're going to respin and, you know, add a note?
17:58:34 dansmith L3111
17:59:48 jaypipes dansmith: heh, sorry, I thought you were referring to my review comment as "the comment jaypipes is going to add". lol, sorry
18:00:10 dansmith ...no :)
18:00:57 jaypipes dansmith: I understand ya now. I havent' updated the series yet because I already recheck'd a couple in the series and didn't want to needlessly reset the gates for a small thing like that
18:01:25 dansmith I think the gate is fairly toast at the moment.. it's failing most status calls for me at least
18:02:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove unnecessary connector stash in attachment_update https://review.openstack.org/527484
18:02:38 melwitt mriedem: patches look good to me, just waiting on test results. added a comment on the top patch about using a helper function
18:03:09 mriedem yeah i thought about that, but figured i'd do a refactor later on top if i'm going to do one
18:03:28 mriedem test results will probably be a few days coming...
18:03:38 mriedem i will kill some time with lunch
18:05:34 jgriffith mriedem: not sure why that's confusing?
18:12:54 jgriffith mriedem: added notes/questions to the bug report
18:24:54 cdent dansmith: do you have any feelings, pro or con, on starting to use the new POST /allocations functionality? now or rocky?
18:26:21 dansmith cdent: which new post behavior? the atomic multiple thing?
18:26:31 cdent aye
18:26:39 dansmith why would we wait?
18:27:01 cdent mostly because of coding and reviewing bandwidth, I guess?
18:28:12 dansmith cdent: well, we have a race right now, so I'd tend to think we should be fixing that and not delaying longer than necessary
18:29:30 cdent fine by me
18:30:08 cdent persistent
18:32:01 cburgess dansmith: Random object question for you if you are around.
18:32:15 dansmith cburgess: random object answer readied
18:32:56 cburgess dansmith Let me make a gist.. that will be simpler to show you.
18:36:33 cburgess dansmith I have found a script someone wrote (lets not ask to many question) that does something like the following... https://gist.github.com/cburgess/bbedb9038a79e8dfc92375b383b1fc4a
18:37:20 dansmith uhm, okay
18:37:32 cburgess So they are importing the objects directly. My question is... is that going direct to the DB, or making a call to conductor in this case?

Earlier   Later