| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-12 | |||
| 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? | |
| 18:37:41 | ildikov | melwitt: is your question on shelve offload about the case when we shelve an instance with volume attached? | |
| 18:38:17 | dansmith | cburgess: this is the magic that causes object operations to go via some other service (i.e. conductor): https://github.com/openstack/nova/blob/master/nova/cmd/compute.py#L56 | |
| 18:38:27 | dansmith | cburgess: without that, they always default to direct | |
| 18:38:38 | cburgess | Where direct = DB? | |
| 18:38:44 | dansmith | cburgess: yes | |
| 18:38:55 | cburgess | dansmith I'm going to go cry in the corner now. Thank you. | |
| 18:39:06 | dansmith | cburgess: uh, why? you wanted via conductor? | |
| 18:39:39 | cburgess | dansmith Well conductor would be better then the DB yes. Though they shouldn't be loading the objects directly like that. They should be using the API. | |
| 18:39:42 | melwitt | ildikov: yes, I think so. it's not really related to that patch but something I noticed during review | |
| 18:40:22 | dansmith | cburgess: why would conductor be better? you'd have to setup all the rpc stuff to do that thing | |
| 18:40:28 | cburgess | dansmith As in this isn't code in nova some place. This is something entirely external to nova that wants some data and its choosing to do so loading the objects directly. | |
| 18:40:33 | dansmith | cburgess: but agree, if you can get it efficiently from the api that's better | |
| 18:40:34 | ildikov | melwitt: this is how it looks like for attaching a volume to a shelved offloaded instance: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3720 | |
| 18:41:17 | cburgess | dansmith My real issue is they should not be loading the objects at all. That needs to be via the API. Clients of the cloud don't get to load internal object code and muck about in the DB. They should talk to the API. | |
| 18:41:18 | dansmith | cburgess: ack, you're crying because someone who is paying you money is doing a scary thing and you have to figure out how to fix that and keep getting paid, is that right? | |
| 18:41:26 | ildikov | melwitt: we call attachment_create as part of the check_attach_and_reserve_volume call and in addition we call attachment_complete in _attach_volume_shelved_offloaded to put the volume to 'in-use' | |
| 18:41:29 | dansmith | cburgess: ack, yes | |