Posted	Nick	Remark
#openstack-nova - 2017-03-09
15:13:26	mriedem	oh i see
15:13:53	dansmith	actually, I have to fix target_cell too
15:13:58	mriedem	dansmith: you missed on
15:13:59	mriedem	*one
15:14:00	dansmith	oh no
15:14:01	dansmith	it works
15:14:07	mriedem	you missed one in the exception block
15:14:09	dansmith	ah yeah
15:14:13	dansmith	just a sec
15:14:31	openstackgerrit	Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403
15:14:48	openstackgerrit	Balazs Gibizer proposed openstack/nova master: Using max api version in notificationt sample test https://review.openstack.org/443686
15:15:32	coreycb	cdent, ok. we're considering loosening the restriction of having to start services by default because it can cause issues particularly with deployment tools.
15:15:54	dansmith	mriedem: so we should be able to write a functional test for this that checks the quota in each after delete right?
15:15:56	coreycb	cdent, but we haven't done that yet. if we did i think we'd just use the default config files.
15:16:20	cdent	coreycb: it's a reasonable thing to do for machines being directly used by humans but not so much in this case :)
15:16:40	mriedem	dansmith: yes
15:16:55	mriedem	that's the plan
15:17:00	mriedem	the tricky thing is going to be,
15:17:00	coreycb	cdent, yeah :)
15:17:16	mriedem	we only need one of these fixes for the backport to ocata
15:17:24	mriedem	so the functional test won't pass until the top patch in pike
15:17:29	mriedem	but it will work for the bottom patch in ocata
15:17:42	mriedem	but re-ordering the changes in master doesn't make sense to me
15:17:49	dansmith	okay
15:18:02	mriedem	so i'm thinking maybe the functional test is in a separate patch
15:18:03	mriedem	not sure
15:18:09	dansmith	yeah
15:22:06	mriedem	so i think the easy thing i can do first is create a functional regression patch that goes at the bottom of the series and recreates the failure, and asserts the error,
15:22:10	mriedem	then the fix patches on top of that,
15:22:31	mriedem	and the top fix patch changes the functional test to show it working
15:22:35	mriedem	that was my plan anyway
15:22:39	dansmith	I was just looking at the tests
15:22:44	dansmith	give me a few minutes
15:23:00	dansmith	we don't really have a way to query into the reservation stuff to look, AFAICT, but I think I can hack something in at least to check
15:23:09	mriedem	well,
15:23:16	mriedem	for a functional test i think this is pretty straightforward,
15:23:19	mriedem	we have cell1 and cell0,
15:23:25	mdbooth	dansmith: Incidentally, https://review.openstack.org/#/c/443403/ is pretty much what I was talking about with the side-effect thing.
15:23:29	mriedem	we disable nova-compute or do something to make scheduling fail,
15:23:39	mriedem	create the server and show it goes into ERROR state,
15:23:47	dansmith	mdbooth: it's not at all, since this is a problem across multiple requests
15:23:49	mriedem	assert we have quota of instances=9 (default-1)
15:23:55	mriedem	then delete the instance,
15:24:03	mriedem	and assert that quota of instances=10 (but it won't be until the fix)
15:24:12	mriedem	it's basically the same recreate steps as in the bug report
15:24:35	dansmith	mriedem: I guess I'm lost in the db api on terminology, knowing which things are returning limits and which are calculated by limit and usage
15:24:44	dansmith	mriedem: but yeah, that's what I'm doing
15:24:48	mriedem	the functional test just hits the rest api
15:24:54	mriedem	like novaclient would
15:25:04	mriedem	i really don't want to be going into the actual databases
15:25:32	mriedem	i was going to do this in nova/tests/functional/regressions
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...

Earlier   Later