Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-09
15:34:01 johnthetubaguy mdbooth: yeah, we totally need that bit
15:34:04 mdbooth However, I suspect it could similarly exist in a new form
15:34:09 johnthetubaguy yeah, that
15:34:56 mdbooth johnthetubaguy: It would be great if the new form included a flag for 'we didn't do the copy'
15:35:21 mdbooth So that in cases where the driver can't do it, eg because the instance is stopped, cinder knows it needs to do it
15:35:26 mdbooth And we don't need to fail the operation
15:36:23 johnthetubaguy mdbooth: my problem is how do we know what to do with the attachments, does it alway delete the correct attachment for us?
15:37:05 mdbooth Well cinder will have its own migration record which it hasn't exposed to Nova, which iirc is how it decides which 'mode' it's in
15:37:14 mdbooth Can cinder just work it out?
15:37:38 johnthetubaguy mdbooth: I think it knows, I just wonder if it always does the same thing from our view of the world
15:37:57 johnthetubaguy like it always deletes the correct attachment
15:38:05 johnthetubaguy so we don't need to delete any attachments
15:38:29 mdbooth johnthetubaguy: We say: we're done with the swap volume of att_A to att_b for instance X, and we did the copy
15:38:53 mdbooth That's enough info for cinder in all cases, right?
15:39:05 mdbooth And I don't think we need a return value any more
15:39:09 johnthetubaguy what I mean is, what do we have to do?
15:39:14 mdbooth Because we don't need to do connection_info munging
15:39:27 johnthetubaguy cinder gives us the volume-uuid of the finally attached volume to update in the BDM
15:39:31 mdbooth I think the only thing Nova has to do is callback cinder with ^^^
15:39:34 johnthetubaguy maybe we just don't delete any attachments
15:40:01 mdbooth It actually occurred to me that all we really needed to do was delete the attachment
15:40:05 mdbooth the source attachment, that is
15:40:11 mdbooth And that's actually enough info for cinder
15:40:24 mdbooth However, I vastly prefer to be explicit
15:40:42 mdbooth I think we delete the attachment, then make the callback
15:40:47 gibi dansmith, melwitt: Hi! could you please look at the bugfix of the instance.delete regression? https://review.openstack.org/#/c/437222/
15:40:48 johnthetubaguy Oh, true, cinder could delete the volume if it knows it has to
15:41:08 johnthetubaguy but something nice about migrate being explicit, in case that gets messed up
15:41:13 mdbooth Yeah
15:41:19 asettle jaypipes: thanks for reviewing the doc :) were you able to test or just take a look?
15:41:34 johnthetubaguy mdbooth: I have no idea how this could work with multi-attached volumes?? who does the delete?
15:41:40 jaypipes asettle: lol, just on a hangout with cdent talking about that right now :)
15:41:54 jaypipes asettle: apparently there *is* an openstack-nova-api-placement package...
15:41:56 asettle jaypipes: wow you guys have all the fun don't you :p
15:42:00 mdbooth johnthetubaguy: Whoa, I hadn't even considered that
15:42:06 mdbooth johnthetubaguy: Who does the copy?
15:42:13 asettle jaypipes: oh. so. what's next?
15:42:31 jaypipes asettle: well, other than that one comment from me, I think all my other comments were correct ;)
15:43:15 johnthetubaguy mdbooth: everyone? oh dear... yeah.
15:43:33 mdbooth johnthetubaguy: Ok, I think that problem can be solved if we add a 'please do the copy' flag to nova's swap_volume call
15:43:53 mdbooth I think cinder then has enough tools in the box to do this
15:43:53 asettle jaypipes: yeah just reading htem all now
15:44:04 mdbooth Hmm, no, you'd need a quiesce operation too
15:44:05 johnthetubaguy mdbooth: well we would also have to call back with the attachment_ids and not the volume ids right?
15:44:41 mdbooth johnthetubaguy: That case requires a lot of thought
15:44:57 mdbooth All attached instances would have to participate
15:45:09 openstackgerrit Dan Smith proposed openstack/nova master: Change MQ targeting to honor only what is in the context https://review.openstack.org/438023
15:45:10 openstackgerrit Dan Smith proposed openstack/nova master: Teach os-migrations about cells https://review.openstack.org/442685
15:45:11 openstackgerrit Dan Smith proposed openstack/nova master: Add cell field to Destination object https://review.openstack.org/438024
15:45:12 openstackgerrit Dan Smith proposed openstack/nova master: Teach simple_tenant_usage about cells https://review.openstack.org/441388
15:45:13 openstackgerrit Dan Smith proposed openstack/nova master: Teach os-aggregates about cells https://review.openstack.org/442200
15:45:46 mdbooth johnthetubaguy: So attachment_ids are going to make that case simpler, but it's also going to involve coordinating a simultaneous volume switch amongst all attached instances.
15:45:48 johnthetubaguy mdbooth: its assume the user ensures only one is writing, but doesn't tell cinder right now, this is messed up, I think we should just fail this for multi-attach, for the moment at least
15:45:52 mdbooth The latter problem is going to be funky
15:46:01 johnthetubaguy mdbooth: +1 its hard
15:46:24 dansmith gibi: was that a regression in ocata?
15:46:33 gibi dansmith: yes it was
15:46:38 gibi dansmith: I will backport it
15:46:54 dansmith mriedem1: so we should rebase the quota fix on this one probably
15:46:55 dansmith because they will conflict
15:47:16 mdbooth johnthetubaguy: Yeah, I think it would be cinder's job to refuse to retype multi-attach volumes until support for that is added.
15:47:49 mdbooth Nova shouldn't concern itself, just do the swap, and copy if requested
15:48:24 johnthetubaguy mdbooth: well, Nova can't do the copy either right, when its Nova initiated
15:49:18 mdbooth I mean, Nova gets an api call with (att_A, att_b, do_copy=True)
15:49:41 mdbooth That call may come direct from a user, or from cinder which has a migration record
15:49:51 mdbooth Either way, Nova's behaviour is the same:
15:49:56 mdbooth Attach to att_b
15:50:00 mdbooth Copy if requested
15:50:04 mdbooth Detach from att_a
15:50:26 mdbooth call cinder with finished(att_a, att_b, instance_x, did_copy=True)
15:50:29 dansmith gibi: question inthere
15:51:04 mdbooth And we update the connection_info to be that for att_b, in all cases
15:51:24 dansmith gibi: unfortunately there's more cleanup in that patch than just is necessary for the backport, AFAICT
15:51:48 mdbooth johnthetubaguy: And we trust the caller not to do anything which is going to cause data corruption
15:51:49 dansmith gibi: I'm kinda conflicted on whether to just do that whole thing vs. two patches, one that is a smaller backport, followed by the cleanup
15:53:11 mdbooth johnthetubaguy: I think instance is redundant in that callback, btw, because it's a property of both attachments
15:53:19 mriedem dansmith: which one?
15:53:20 mdbooth It shouldn't be there
15:53:29 dansmith mriedem: https://review.openstack.org/#/c/437222/5
15:53:47 mriedem oh yeah i knew about that, just hadn't gone through it yet
15:54:11 gibi dansmith: you mean do the minimal thing first, backport that then do some cleanup in a followup on master?
15:54:17 dansmith gibi: yeah
15:54:28 mriedem smaller backports are much easier and nicer
15:55:03 cdent jaypipes: I had to -1 get_invetory, but it is a very soft -1
15:55:50 mriedem dansmith: gibi: what's the cleanup? the context manager?
15:56:03 dansmith mriedem: yeah, and the use of it in a few places that aren't affected, AFAICT
15:57:13 mriedem jaypipes: https://review.openstack.org/#/c/442200/ - that's the bottom of the series and you were +2 yesterday
15:57:21 gibi mriedem: I think this is the only place where it wasn't needed to restore original behavior https://review.openstack.org/#/c/437222/1/nova/compute/api.py@1868
15:58:08 gibi mriedem, dansmith: I can put this ^^ to a separate patch
15:58:18 dansmith gibi: that's PS1
15:58:32 gibi ups sorry
15:58:55 dansmith gibi: there are only two places we're missing the notification,
15:58:56 dansmith and two places you convert to the new one, right?
15:59:09 jaypipes mriedem: k, on it. just +2'd https://review.openstack.org/#/c/345138 BTW
15:59:36 mriedem ok, haven't reviewed that yet at all
15:59:43 dansmith I have I'll circle back
15:59:55 gibi dansmith: true. I looked at the wrong ps
16:00:11 gibi dansmith: I can split the patch
16:00:14 dansmith gibi: I think splitting those up would be good for backport cleanliness if you're cool with it
16:00:28 dansmith gibi: sweet, thanks for finding, fixing :)

Earlier   Later