| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 19:44:24 | cdent | it is public by virtue of being python | |
| 19:44:47 | efried | What I mean is, is it used by some known external entity that we care about? | |
| 19:44:52 | cdent | and in the early days of placement was the entry point to the wsgi application stack until sean added the stuff in wsgi.py | |
| 19:44:57 | cdent | the point is that we don’t know | |
| 19:45:09 | efried | But we care? | |
| 19:45:13 | cdent | yes, of course | |
| 19:45:18 | efried | Oh, not of course. | |
| 19:45:33 | cdent | the whole point of a wsgi application is to be able to integrate it in whatever way you want | |
| 19:45:36 | efried | Nova changes "public APIs" like ComputeDriver all the time without worrying about external consumption. | |
| 19:45:47 | sean-k-mooney2 | was it ever the entrypoint in a version of the placement api that shipped in a release | |
| 19:46:07 | efried | And when I've asked about that before, it was basically explained away to "we don't care". | |
| 19:46:15 | efried | So in this case, why do we care? | |
| 19:46:16 | cdent | sean-k-mooney2: a) I _think_ so but am not certain, b) it has always been my intent that deploy be available as a public interface | |
| 19:46:28 | cdent | because placement is not nova? | |
| 19:46:46 | cdent | it has a _much_ different attitude towards building the wsgi application than the nova-api ever did | |
| 19:47:13 | sean-k-mooney2 | cdent: well if that was the intent then we should treat it as public and document that fact. | |
| 19:48:13 | cdent | I think I probably made the mistake of assuming that was somehow obvious, by virtue of its position in the wsgi stack, but I think I may be one of the few people around here who think of the wsgi stack in that fashion | |
| 19:48:23 | cdent | we can certainly document it as such | |
| 19:49:09 | cdent | efried: would your preference be that I add that documentation to that patch now, or is it cool to worry about it later? | |
| 19:49:32 | efried | Later is fine. | |
| 19:50:50 | efried | cdent Changed sides. | |
| 19:51:25 | cdent | thanks, I’ll do the documenting likely tomorrow, I think I’m out of code mode for today | |
| 19:51:36 | cdent | also, all my promises to myself to not work late tonight seem to be for naught | |
| 19:52:21 | sean-k-mooney2 | cdent: your based in the uk right? | |
| 19:52:24 | cdent | yeah | |
| 19:52:33 | cdent | and I often work this late, but last night I worked past midnight | |
| 19:52:40 | cdent | so was going to give myself a break | |
| 19:52:50 | cdent | but everybody here was just too interesting today | |
| 19:53:14 | sean-k-mooney2 | i have a daily reminder to head home at 8 so im normally here till around now too. | |
| 19:56:00 | cdent | as it is 5 to 8 I think I’ll go, have a good night every one | |
| 20:09:16 | melwitt | mriedem: in case you didn't see, I had posted a follow up for the old quotas code removal that we talked about some days ago https://review.openstack.org/#/c/524234 | |
| 20:12:29 | melwitt | although I think I just realized an issue with it and commented | |
| 20:12:43 | mriedem | lgtm | |
| 20:13:19 | mriedem | oh right | |
| 20:13:30 | mriedem | gawd | |
| 20:14:21 | melwitt | yeah. guh | |
| 20:15:33 | mriedem | https://review.openstack.org/#/c/400384/ | |
| 20:15:58 | melwitt | I need to add some kind of equivalent test to the API tests somewhere. the api samples test cover the presence of the 'reserved' key but not the expected value if no-op quotas | |
| 20:16:13 | mriedem | well, | |
| 20:16:27 | mriedem | easiest is probably an api samples test with the noop quota driver and it's own samples | |
| 20:23:56 | melwitt | lemme see if I can do that | |
| 20:25:33 | openstackgerrit | Hongbin Lu proposed openstack/nova master: Add support for listing hosts in cellv2 https://review.openstack.org/524755 | |
| 20:41:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add new style volume attachment support to block_device.py https://review.openstack.org/525787 | |
| 20:41:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: libvirt: Allow multiple volume attachments https://review.openstack.org/267587 | |
| 20:41:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 20:41:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Avoid stashed connector lookup for new style detach https://review.openstack.org/526182 | |
| 20:41:23 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Allow multi-attach in compute api https://review.openstack.org/271047 | |
| 20:46:51 | efried | jaypipes I seem to have flubbed https://review.openstack.org/#/c/521605/ which was supposed to be two separate commits (one to add agg stuff to ProviderTree, one to add traits stuff). | |
| 20:47:05 | efried | jaypipes I was about to split it up, but thought I should ask you whether it's worth bothering | |
| 20:47:53 | efried | jaypipes i.e. should I just do both in one change set? | |
| 21:03:46 | mriedem | ildikov: ok, final comments in https://review.openstack.org/#/c/330285/173 | |
| 21:03:54 | mriedem | ildikov: easy things to address and then i'm ready to go on that | |
| 21:04:03 | ildikov | mriedem: ok | |
| 21:04:05 | mriedem | we'll have to decide if we hold it for the shared_targets microversion in cinder | |
| 21:04:32 | ildikov | mriedem: let me ask Jay about the client | |
| 21:05:00 | ildikov | mriedem: as for the new attach patch, did you mean I should take over on those? | |
| 21:05:18 | mriedem | ildikov: i was thinking, in the api change to support multiattach, if/when we check the volume to see if it's multiattach=True, couldn't we just check the cinder microversion at that point if the volume already has >0 attachments? | |
| 21:05:43 | ildikov | mriedem: I haven't touched the API patch in almost years now | |
| 21:05:49 | mriedem | basically, nova api can't attach a multiattach volume to >1 instance until we are sure we can get shared_targets info | |
| 21:05:55 | ildikov | mriedem: so I will take a look shortly and figure out | |
| 21:05:58 | mriedem | ok | |
| 21:06:06 | ildikov | I didn't want to keep those checks, those are from the ancient times | |
| 21:06:14 | mriedem | i can update https://review.openstack.org/#/c/330285/173 quick | |
| 21:06:18 | mriedem | and then i'm done with it i think | |
| 21:06:27 | ildikov | another question is whether it's ok to do the lock for detach in a separate patch? | |
| 21:06:42 | ildikov | so we could land the libvirt change and then the lock and then the API | |
| 21:06:48 | mriedem | if it's a separate patch, it has to come before we start multiattaching things | |
| 21:06:53 | mriedem | oh, | |
| 21:06:54 | ildikov | also, we will bump a microversion for this, right? | |
| 21:07:04 | mriedem | as long as we don't turn on multiattach until the parts in the compute are ready | |
| 21:07:06 | mriedem | i think we can do that | |
| 21:07:20 | mriedem | i think we will bump a microversion as an indication of multiattach yes | |
| 21:07:31 | mriedem | i thought we updated the spec about that | |
| 21:07:55 | ildikov | ok, so we will need an API level check whether the volume is attached already or not independently from Cinder | |
| 21:07:57 | mriedem | oh it's a todo https://specs.openstack.org/openstack/nova-specs/specs/queens/approved/cinder-volume-multi-attach.html#rest-api-impact | |
| 21:08:06 | mriedem | "Whether or not a new compute API microversion is needed will be determined during implementation and code review. API users will need some way to discover if they can perform volume multiattach and a microversion might be the signal, but it is unclear if Nova would block those requests on a lower microversion, e.g. 2.1. It probably makes sense to do a microversion like 2.49 for tagged attach capabilities." | |
| 21:08:48 | ildikov | yeah, we were a bit hesitant | |
| 21:09:24 | ildikov | and it'll kinda require another check_attach too as basically that's what turns multi-attach on on the Nova side | |
| 21:09:28 | ildikov | every other check is in CInder | |
| 21:10:18 | ildikov | the libvirt change is supposed to be harmless as it plays with things only for multi-attach volumes on intention, but it doesn't do any attaching related stuff | |
| 21:10:31 | ildikov | just turns on the shareable flag and adds the capabilities info | |
| 21:25:54 | ildikov | mriedem: so will you upload any more versions, or it's on me now? I got lost on that one. | |
| 21:28:29 | mriedem | i'm doing it | |
| 21:29:07 | ildikov | mriedem: cool, tnx | |
| 21:29:20 | ildikov | mriedem: replied to your comments | |
| 21:30:51 | ildikov | mriedem: as we're adding a new API microversion I think we can land this and deal with the new Cinder microversion in the multi-attach change | |
| 21:31:20 | ildikov | mriedem: as we will need to do some extra checks for that anyway therefore we might as well add the new Cinder microversion to that one | |
| 21:31:39 | mriedem | i tend to agree, | |
| 21:31:47 | ildikov | mriedem: we can do a version check now independently from the attachment_create call so that's not an issue either | |
| 21:31:51 | mriedem | especially since the point of microversions is that the client can opt into whatever they want when it's available (or not) | |
| 21:31:54 | mriedem | and in this case, nova is the client | |
| 21:32:21 | mriedem | so we just won't support a multiattach volume to >1 instance if the new cinder microversion for shared_targets isn't htere | |
| 21:32:26 | mriedem | because we won't be able to safely detach | |
| 21:32:32 | ildikov | yeah, true, I just wanted to avoid the check_attach-like checks as much as possible, but the API microversion in Nova will require that anyway | |
| 21:32:55 | ildikov | yep | |
| 21:33:03 | mriedem | well, the compute microversoin is mainly just a signal | |
| 21:33:24 | ildikov | I mean the Nova API microversion | |
| 21:33:25 | mriedem | 'i want to attach this multiattach volume to >1 instance and i'm opting into that, but i know it might not work too' | |
| 21:34:00 | mriedem | might not work b/c (1) old cinder (2) old computes (3) policy (4) virt driver doesn't support it (5) volume type doens't support it, etc | |
| 21:34:17 | mriedem | (6) uranus is not aligned with pluto | |