| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 15:25:54 | dansmith | mriedem: well, you're not really validating that exactly what you think is happening really is, but okay | |
| 15:26:09 | dansmith | I'll leave it to you then | |
| 15:26:36 | mriedem | as long as the regression test asserts the failure (the recreate), and the fix has to change the test to show it now passing, i think that's what we care about | |
| 15:27:02 | dansmith | I just want to see them in the wrong database, but okay | |
| 15:27:33 | dansmith | the quota stuff is so opaque to me that I'd want to see that we're out of sync for the reason we think and not one of the other reasons quotas gets out of sync | |
| 15:27:46 | dansmith | but that's fine, you're far more familiar, so go for it | |
| 15:28:04 | mriedem | why did you change set_target_cell? | |
| 15:28:09 | mriedem | we're calling target_cell and passing None | |
| 15:28:35 | dansmith | target_cell calls set_target_cell | |
| 15:29:42 | mriedem | gotcha | |
| 15:32:05 | johnthetubaguy | mdbooth: I am looking at your comments on migrate_volume_completion | |
| 15:32:30 | mdbooth | johnthetubaguy: I spent half a day or so reading the cinder side code for that, btw | |
| 15:32:32 | johnthetubaguy | mdbooth: I don't feel like I am understanding what that is trying to do, and if it should die/get replaced in this new model | |
| 15:32:43 | mdbooth | I'll never get that back | |
| 15:32:50 | johnthetubaguy | yeah, I did that about 6-8 months ago, and its wondered out my head again | |
| 15:33:36 | mdbooth | johnthetubaguy: I think it needs to continue to exist, because cinder needs to know that the copy is complete so it can cleanup the old volume. | |
| 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 | asettle | jaypipes: yeah just reading htem all now | |
| 15:43:53 | mdbooth | I think cinder then has enough tools in the box to do this | |
| 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 | |