This [1] adds tests for swap volume with admin and non-admin case. I want to check behaviour with current code so not adding Depends-On on your fix. Once we have gate results then feel free to add Depends-On or I will add.
I think virt driver does not do attach/detach but we can re-verify that on tempest patch. ..[1] https://review.openstack.org/#/c/299830/1 On Thu, Mar 31, 2016 at 11:29 AM, Matt Riedemann <[email protected]> wrote: > > > On 3/30/2016 9:14 PM, GHANSHYAM MANN wrote: >> >> On Thu, Mar 31, 2016 at 10:39 AM, Matt Riedemann >> <[email protected]> wrote: >>> >>> >>> >>> On 3/30/2016 8:20 PM, Matt Riedemann wrote: >>>> >>>> >>>> >>>> >>>> On 3/30/2016 7:56 PM, Matt Riedemann wrote: >>>>> >>>>> >>>>> >>>>> >>>>> On 3/30/2016 7:38 PM, Matt Riedemann wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 2/25/2016 5:31 AM, Takashi Natsume wrote: >>>>>>> >>>>>>> >>>>>>> Hi Nova and Cinder developers. >>>>>>> >>>>>>> As I reported in a bug report [1], nova swap volume >>>>>>> (updating an attached volume) fuction does not work >>>>>>> in the case of non admin users by default. >>>>>>> (Volumes are stuck.) >>>>>>> >>>>>>> Before I was working for fixing another swap volume bug [2][3]. >>>>>>> But Ryan fixed it on the Cinder side [4]. >>>>>>> As a result, admin users can execute swap volume function, >>>>>>> but it was not fixed in the case of non admin users. >>>>>>> So I reported the bug report [1]. >>>>>>> >>>>>>> In the patch[5], I tried to change the default cinder's policy >>>>>>> to allow non admin users to execute migrate_volume_completion API. >>>>>>> But it was rejected by the cinder project ('-2' was voted). >>>>>>> >>>>>>> In the patch[5], it was suggested to make the swap volume API admin >>>>>>> only >>>>>>> on the Nova side. >>>>>>> But IMO, the swap volume function should be allowed to non admin >>>>>>> users >>>>>>> because attaching a volume and detaching a volume can be performed >>>>>>> by non admin users. >>>>>> >>>>>> >>>>>> >>>>>> I agree with this. DuncanT said in IRC that he didn't think non-admin >>>>>> users should be using the swap-volume API in nova because it can be >>>>>> problematic, but I'm not sure why, is there more history or detail >>>>>> there? I'd think it shouldn't be any worse than doing a detach/attach >>>>>> in >>>>>> quick succession (like in a CI test for example). >>>>>> >>>>>>> >>>>>>> If migrate_volume_completion is only allowed to admin users >>>>>>> by default on the Cinder side, attaching a new volume and >>>>>>> detaching an old volume should be performed on the Nova side >>>>>>> when swapping volumes. >>>>>> >>>>>> >>>>>> >>>>>> My understanding of the problem is as follows: >>>>>> >>>>>> 1. Admin-initiated volume migration in Cinder calls off to Nova to >>>>>> perform the swap-volume, and then Nova calls back to Cinder's >>>>>> migrate_volume_completion API. This is fine since it's an admin that >>>>>> initiated this series of operations on the Cinder side (that's by >>>>>> default, however, this is broken if the policy file for Cinder is >>>>>> change >>>>>> to allow non-admins to migrate volumes). >>>>>> >>>>>> 2. A non-admin swap-volume API call in Nova fails because Nova blindly >>>>>> makes the migrate_volume_completion call to Cinder which fails with a >>>>>> 403 because the Cinder API policy has that as an admin action by >>>>>> default. >>>>>> >>>>>> I don't know the history around when the swap-volume API was added to >>>>>> Nova, was it specifically for this volume migration scenario in >>>>>> Cinder? >>>>>> Are there other use cases? Knowing those would be good to >>>>>> determine >>>>>> if Nova should change it's default policy for swap-volume, although, >>>>>> again, that's only a default and can be changed per deployment so we >>>>>> probably shouldn't rely on it. >>>>>> >>>>>> Ideally we would have implemented this like the nova/neutron server >>>>>> events callback API in Nova during vif plugging (nova does the vif >>>>>> plug >>>>>> on the host then waits for neutron to update it's database for the >>>>>> port >>>>>> status and sends an event (API call) to nova to continue booting the >>>>>> server). That server events API in nova is admin-only by default and >>>>>> neutron is configured with admin credentials for nova to use it. >>>>>> >>>>>> Another option would be for Nova to handle a 403 response when calling >>>>>> Cinder's migrate_volume_completion API and ignore it if we don't have >>>>>> an >>>>>> admin context. This is pretty hacky though. It assumes that it's a >>>>>> non-admin user initiating the swap-volume operation. It wouldn't be a >>>>>> problem for the volume migration operation initiated in Cinder since >>>>>> by >>>>>> default that's admin-only, so nova shouldn't get a 403 when calling >>>>>> migrate_volume_completion. The trap would be if the cinder policy for >>>>>> volume migration was changed to allow non-admins, but if someone did >>>>>> that, they should also change the policy for migrate_volume_completion >>>>>> to allow non-admin too. >>>>>> >>>>>>> >>>>>>> If you have a good idea, please let me know it. >>>>>>> >>>>>>> [1] Cinder volumes are stuck when non admin user executes nova swap >>>>>>> volume API >>>>>>> https://bugs.launchpad.net/cinder/+bug/1522705 >>>>>>> >>>>>>> [2] Cinder volume stuck in swap_volume >>>>>>> https://bugs.launchpad.net/nova/+bug/1471098 >>>>>>> >>>>>>> [3] Fix cinder volume stuck in swap_volume >>>>>>> https://review.openstack.org/#/c/207385/ >>>>>>> >>>>>>> [4] Fix swap_volume for case without migration >>>>>>> https://review.openstack.org/#/c/247767/ >>>>>>> >>>>>>> [5] Enable volume owners to execute migrate_volume_completion >>>>>>> https://review.openstack.org/#/c/253363/ >>>>>>> >>>>>>> Regards, >>>>>>> Takashi Natsume >>>>>>> NTT Software Innovation Center >>>>>>> E-mail: [email protected] >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> __________________________________________________________________________ >>>>>>> >>>>>>> >>>>>>> >>>>>>> OpenStack Development Mailing List (not for usage questions) >>>>>>> Unsubscribe: >>>>>>> [email protected]?subject:unsubscribe >>>>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >>>>>>> >>>>>> >>>>> >>>>> I also just checked Tempest and apparently we have no coverage for the >>>>> swap-volume API in Nova, we should fix that as part of this. >>>>> >>>> >>>> I've done some more digging. The swap-volume functionality was added to >>>> nova here [1]. The cinder use of it for volume migration was added here >>>> [2]. >>>> >>>> Looking at the cinder volume API for migrate_volume_completion, it >>>> expects the source (old) volume to have migration_status set [3]. >>>> >>>> So, I think we can easily fix this in Nova by simply not calling >>>> volume_migration_completion if old_volume['migration_status'] is None. >>>> >>>> [1] >>>> >>>> >>>> https://github.com/openstack/nova/commit/8f51b120b430c7c21399256f37e1d8f75d030484 >>>> >>>> [2] >>>> >>>> >>>> https://github.com/openstack/nova/commit/0e4bd7f93b9bfadcc2bb6dfaeae7bb5ee00c194b >>>> >>>> [3] >>>> >>>> >>>> http://git.openstack.org/cgit/openstack/cinder/tree/cinder/volume/api.py#n1358 >>>> >>>> >>> >>> Of course that had to be too easy to be true. The volume >>> 'migration_status' >>> is only returned for the volume details if you're calling with an admin >>> context [1]. >>> >>> I think we can still use this, we just can't expect >>> volume['migration_status'] to be in the response from the volume GET. If >>> it's not there, we can assume we're not doing a migration and we're not >>> an >>> admin anyway, so we can't call migrate_volume_completion. >> >> >> So in that case, we need to attach, detach volume on nova side right? >> I mean if migrate_volume_completion is not being called then >> new volume attachment and old volume detachment should be initiated >> explicitly. >> >> Can we make Nova default policy same as cinder, i mean swap volume >> allowed only for admin? Because if there is simple swap initiated from >> nova(not cinder migration), Nova allow that operation for non-admin >> user and get stuck to attaching/detaching status. >> >>> >>> [1] >>> >>> http://git.openstack.org/cgit/openstack/cinder/tree/cinder/api/v2/views/volumes.py#n82-L84 >>> >>> -- >>> >>> Thanks, >>> >>> Matt Riedemann >>> >>> >>> >>> __________________________________________________________________________ >>> OpenStack Development Mailing List (not for usage questions) >>> Unsubscribe: >>> [email protected]?subject:unsubscribe >>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> >> >> __________________________________________________________________________ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: [email protected]?subject:unsubscribe >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> > > Good question, I'm assuming the virt driver does the attach/detach on the > nova side, but I haven't dug into it or verified the fix myself. I wanted to > write a Tempest test for the non-admin swap-volume case in Nova. > > Changing the nova policy to be admin-only by default for swap-volume was > another easy option, I just wasn't sure if it was the right thing to do > since this was not initially added as an admin-only operation, and it seems > the only thing that's made it become that is the cinder volume migration > callback case. > > -- > > Thanks, > > Matt Riedemann > > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
