| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 14:58:57 | mdbooth | jaypipes: Incidentally, I was looking for validation of this approach for handling RequestContexts for cells: https://review.openstack.org/#/c/443627/ | |
| 14:59:14 | mdbooth | Or invalidation, obviously, which is why I haven't tried to do the whole job yet. | |
| 14:59:25 | jaypipes | mdbooth: k, queued. | |
| 14:59:37 | mdbooth | jaypipes: Thanks | |
| 15:01:30 | openstackgerrit | Gábor Antal proposed openstack/nova master: Add json style checking for sample notifications https://review.openstack.org/443677 | |
| 15:03:05 | dansmith | mdbooth: I skimmed it briefly | |
| 15:03:28 | dansmith | mdbooth: I think we can go with that approach for things for object contexts, where we iterate over multiple cells | |
| 15:03:36 | dansmith | mdbooth: I don't like it for clearly single-cell-targeted operations on an instance or a host | |
| 15:04:03 | dansmith | mdbooth: because after getting all of this to work, I feel confident we'll have some leaks where we don't realize that we've now got two copies of the context | |
| 15:04:06 | mdbooth | dansmith: Ok. So it's worth adding context.for_cell() and fixing up say get_all() and its users? | |
| 15:04:12 | dansmith | mdbooth: the context _is_ gobal and by reference everywhere | |
| 15:04:21 | dansmith | mdbooth: we get it by reference from the request | |
| 15:05:31 | dansmith | mdbooth: maybe, I also would want to see that change on top of a fully working tempest run of the current stack to prove it doesn't break anything else | |
| 15:05:59 | jaypipes | sfinucan: so https://review.openstack.org/#/c/399653/is going to require the gate to have libvirt 1.2.9 | |
| 15:06:01 | mdbooth | dansmith: That was another reason I didn't want to go all the way down this rabbit hole. I expect there are a bunch of tricky cases. | |
| 15:06:12 | dansmith | mdbooth: there are a ton | |
| 15:06:31 | mdbooth | dansmith: However, I was hoping that by removing the whole side-effect of changing the global context there might be significantly less | |
| 15:06:41 | dansmith | mdbooth: I think you might be surprised | |
| 15:07:03 | sfinucan | jaypipes: I thought the point of those tests was to fake out libvirt? | |
| 15:07:12 | mdbooth | dansmith: Ok, I'll limit to get_all() initially and see how it looks. | |
| 15:07:14 | mdbooth | Thanks for looking. | |
| 15:07:28 | jaypipes | sfinucan: see the functional test failure :) | |
| 15:07:30 | cdent | coreycb: I think I've got another one. Is it considered a bug for the service.conf files to have uncommented config settings? | |
| 15:08:11 | coreycb | cdent, not necessarily. some of the packages set defaults. | |
| 15:08:24 | sfinucan | jaypipes: Well that doesn't look right :D | |
| 15:08:36 | cdent | coreycb: in this case it is that os_region_name in the [placement] section is being set | |
| 15:08:50 | sfinucan | Guess we can mock out that version check too | |
| 15:08:55 | dansmith | mdbooth: fwiw, we already had a discussion about whether or not to fork the context in the objects that are tied to cells and decided on this approach to start | |
| 15:09:09 | dansmith | mdbooth: one thing to remember is that if you send an object over rpc, it resets the context on all the objects in the stack | |
| 15:09:25 | dansmith | mdbooth: so if you make a call over to conductor, for anything like boot or resize, contexts get reset anyway | |
| 15:09:33 | coreycb | cdent, that may not need to be set. we set defaults so that the service will can start after the package is installed. | |
| 15:10:13 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403 | |
| 15:10:21 | cdent | coreycb: the reason I noticed was because the docs I was testing changed it, i made all those changs at the top of the section, and then realized it was going to conflict with something that's set further below | |
| 15:10:47 | cdent | coreycb: ah, okay, yeah, that makes sense then | |
| 15:10:53 | cdent | it won't start without that | |
| 15:10:57 | cdent | (there's no default) | |
| 15:11:42 | mriedem | dansmith: ftr, i did give you the business, yes | |
| 15:11:44 | mriedem | :) | |
| 15:11:55 | dansmith | mriedem: yeah, I just totally wasn't paying attention when I wrote that :) | |
| 15:11:59 | dansmith | mriedem: see my rev | |
| 15:12:00 | dansmith | ? | |
| 15:13:02 | mriedem | yes | |
| 15:13:08 | mriedem | wait, what? | |
| 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 | coreycb | cdent, yeah :) | |
| 15:17:00 | mriedem | the tricky thing is going to be, | |
| 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 | |