| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 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 | |
| 21:34:37 | ildikov | yeah, if Cinder is mid-old then they will not be able to create a multi-attach volume on the first place | |
| 21:34:45 | ildikov | lol | |
| 21:34:55 | ildikov | I hope I | |
| 21:35:06 | ildikov | 'll never need to debug (6) :) | |
| 21:36:35 | ildikov | anyway, so if we don't want to hold the new attach patch now we can just check whether the volume is already attached and continue if the Cinder microversion is high enough | |
| 21:38:43 | ildikov | however the new flow currently doesn't allow multi-attach in general so we might not need that check | |
| 21:39:27 | ildikov | I need to think about it, but the point is still that I think we all had enough of this patch by now... | |
| 21:41:04 | mriedem | incoming | |
| 21:41:35 | mriedem | or not | |
| 21:41:39 | mriedem | anyway, new patches are up | |
| 21:41:42 | mriedem | https://review.openstack.org/#/c/330285/174/releasenotes/notes/bp-cinder-new-attach-apis-eca854e27a255e3e.yaml is the reno i added | |
| 21:43:21 | ildikov | mriedem: looks good to me | |
| 21:43:43 | ildikov | not that I would've worded anything so far good enough to judge, but still :) | |
| 21:45:15 | mriedem | +2 on the new attach flow patch | |
| 21:45:41 | mriedem | i think johnthetubaguy is on paternity leave | |
| 21:45:51 | mriedem | so we'll likely need gibi and/or melwitt to go through this | |
| 21:46:09 | ildikov | mriedem: wow, we haven't even reach 200... :) | |
| 21:46:38 | edleafe | so it looks like I need to rebase my series. Will that kill the patches that are in the gate? | |
| 21:46:49 | mriedem | edleafe: i can re-approve | |
| 21:47:08 | ildikov | mriedem: gibi is familiar with the patch, but he might not familiar with the whole Cinder-side new flow to +A this | |
| 21:47:33 | mriedem | ildikov: i don't think anyone from nova besides john will be | |
| 21:47:43 | ildikov | mriedem: can check with him tomorrow or look at melwitt with cat eyes from Shrek :) | |
| 21:48:03 | ildikov | mriedem: I know, I'm here if anyone wants to ask questions | |
| 21:48:18 | mriedem | at this point i think i can answer any questions on the nova patches | |
| 21:48:26 | edleafe | mriedem: sure, but won't that mean restarting the whole CI process? They are mostly through the gate now. | |
| 21:48:33 | mriedem | edleafe: yes if they are rebased | |
| 21:49:19 | edleafe | ok, then I'll wait | |
| 21:50:13 | ildikov | mriedem: if I could have just 1/10 of your brain :) | |
| 21:55:32 | melwitt | ildikov: if gibi is already familiar with the patch, check with him first. if he won't be able to look, let me know and I'll look at it | |
| 21:55:54 | mriedem | melwitt: the bottom 2 patches in the series are non-destructive. | |