| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-19 | |||
| 13:40:48 | mdbooth | lyarwood: Do we have a test for adding native encryption to xml which doesn't currently have it? | |
| 13:40:56 | mdbooth | lyarwood: I think the answer's no. | |
| 13:41:52 | lyarwood | mdbooth: during LM? No. | |
| 13:42:31 | mdbooth | lyarwood: Should be mostly cut/paste hopefully | |
| 13:44:41 | mdbooth | lyarwood: Couple more nits. If you're respinning, you probably ought to remove the unrelated whitespace change. | |
| 13:45:32 | Spazmotic | And before I goto bed will ask again for good luck, if anyone knows XenAPI well and has some free cycles away from their important reviews, feel free to take a look at https://review.openstack.org/#/c/533168/ :) | |
| 13:46:10 | lyarwood | mdbooth: ack, already have :) | |
| 13:47:00 | Spazmotic | I think this eventaully needs to be changed to use nova.objects.block_device.is_volume() but that's something that can be resolved a bit later to pull in CinderV3 into VMOps | |
| 13:47:04 | Spazmotic | Have a good night everyone :) | |
| 13:59:41 | rabel | is the default quota-class deprecated by now? it seems not to be considered by nova in any way | |
| 14:04:03 | claudiub | lyarwood: sorry, i was in a meeting. did you need any help with mock autospec? | |
| 14:04:44 | lyarwood | claudiub: no sorry, I'm fine, I was just talking to mdbooth about them earlier and I fried_rice was offering help :) | |
| 14:04:56 | lyarwood | I think* | |
| 14:05:05 | claudiub | cool. :) | |
| 14:05:36 | claudiub | anyways, it's been a lingering issue since last year. recently, we've merged a patch to oslotest which should help with it. you can take a look here: https://github.com/openstack/oslotest/blob/master/doc/source/user/mock-autospec.rst | |
| 14:05:56 | claudiub | and this is the 1st patch addressing autospec issues in nova: https://review.openstack.org/#/c/447505/37 | |
| 14:06:14 | mdbooth | lyarwood: It's not a high priority given that we almost never use autospec, btw. I have had autospec catch a couple of bugs for me in the past, though. | |
| 14:06:51 | mdbooth | lyarwood: I was thinking of it as I thought I'd seen you using it. | |
| 14:08:43 | claudiub | i do recommend using autospecs whenever possible, especially when you're using external libraries which might change over time. mocking them normally will make your unit tests pass even if the method signatures changed, which is not ok. :) | |
| 14:10:40 | fried_rice | mdbooth Yeah, one of the reasons we don't use autospec as much as we could/should is because there were bugs in it. claudiub just fixed some of those, so let the floodgates open! | |
| 14:11:13 | artom_ | bauzas, https://bugs.launchpad.net/nova/+bug/1744325 | |
| 14:11:15 | openstack | Launchpad bug 1744325 in OpenStack Compute (nova) "If a rebuild is refused by the scheduler, the instance's imageref is not rolled back" [Undecided,New] | |
| 14:11:39 | artom | mdbooth, ^^ if you care | |
| 14:11:41 | bauzas | artom: yup, I saw your internal discussion | |
| 14:11:46 | mdbooth | artom: I do, thanks | |
| 14:12:45 | TheJulia | Greetings nova folks! Over in the land of ironic, we've been encountering a condition in our multinode grenade job where while underlying libraries are being upgraded and nova is not upgraded from pike to master. What we're seeing is the nova conductor exiting with SEGV, and then looping causing all sorts of other issues. Has anyone seen anything like this? | |
| 14:14:32 | leakypipes | Spazmotic: we don't "zero out the resources" in allocations. | |
| 14:14:33 | TheJulia | Likewise :\ | |
| 14:14:43 | leakypipes | fried_rice: the consistency marker is the resource provider's generation. | |
| 14:15:13 | fried_rice | leakypipes Is it updated when allocations are made? | |
| 14:15:20 | leakypipes | fried_rice: of course. | |
| 14:15:34 | openstackgerrit | David Rabel proposed openstack/nova master: Fix format in flavors.rst https://review.openstack.org/535777 | |
| 14:15:55 | fried_rice | leakypipes But the documentation says the RP generation is ignored in alloc requests. | |
| 14:16:45 | fried_rice | leakypipes ...for PUT. And it appears to be entirely absent for POST. | |
| 14:17:18 | fried_rice | leakypipes Bug? | |
| 14:17:32 | leakypipes | fried_rice: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L2085 | |
| 14:17:34 | artom | I can't even begin to think about the fix | |
| 14:17:47 | artom | We're setting a whole bunch of instance attributes in the compute API | |
| 14:17:54 | artom | And *then* doing the rebuild | |
| 14:17:55 | leakypipes | fried_rice: there is no POST. | |
| 14:18:02 | artom | With no checks whether it passed the scheduler or not | |
| 14:18:13 | fried_rice | leakypipes https://developer.openstack.org/api-ref/placement/#manage-allocations | |
| 14:18:16 | leakypipes | fried_rice: there is only PUT /allocations/{consumer_uuid} | |
| 14:18:34 | artom | Maybe the easiest would be to just revert the CVE "fix" and leave it as WONTFIX :/ | |
| 14:18:37 | TheJulia | fried_rice: my guess is some piece of compiled c is getting loaded in from the upgraded underlying modules, and the conductor goes sideways at that point | |
| 14:19:04 | fried_rice | TheJulia Fo sho. I was just being glib, though - I certainly haven't seen the behavior you describe. | |
| 14:19:07 | leakypipes | fried_rice: oh, that is for when we move instances. | |
| 14:19:30 | leakypipes | fried_rice: it is not used anywhere else. | |
| 14:19:44 | fried_rice | By nova. At the moment. But not the point. | |
| 14:20:23 | leakypipes | fried_rice: what is your point? | |
| 14:20:37 | fried_rice | leakypipes Getting there, finding code, one sec. | |
| 14:21:01 | fried_rice | leakypipes Yeah, so link above convinces me we're incrementing the generation when we do allocationey stuff, which is dandy. | |
| 14:21:18 | fried_rice | leakypipes But if the allocation APIs don't accept a generation, how are we detecting conflicts? | |
| 14:21:28 | fried_rice | leakypipes Or reporting them (409s)? | |
| 14:21:34 | rabel | can anyone please help me with quotas? is the default quota class used for anything? a GET call to /os-quota-sets/{tenant_id}/defaults does not seem to return it, but i'm not sure what it returns instead. | |
| 14:21:39 | leakypipes | fried_rice: we detect conflicts in terms of whether capacity is exceeded, that's all. | |
| 14:22:14 | leakypipes | fried_rice: i.e. does the original constraint that there was existing capacity for the allocated resources on these set of resource providers still hold. | |
| 14:23:07 | fried_rice | leakypipes That's cool. But if two threads are trying to e.g. modify allocations for the same consumer_uuid at the same time, the second one will "win". | |
| 14:23:54 | fried_rice | leakypipes Not a problem in today's world where everything is funneled through report client, and there's one compute node RP, and the consumer is an instance. | |
| 14:24:25 | leakypipes | fried_rice: no, that's not possible. one of those threads would get a 409 because the provider generation increment would fail. | |
| 14:24:43 | fried_rice | leakypipes How and why would the increment fail if we're not passing the generation into the API? | |
| 14:26:12 | leakypipes | fried_rice: because the provider generations are *read* at the start of the transaction that inserts/deletes allocation records. after those allocation records are changed, the provider generations are incremented for all providers involved in the transaction. if another thread had inserted/deleted allocation records (for that consumer or another consumer) against those same resource providers, the increment would fail. | |
| 14:26:57 | leakypipes | fried_rice: specifically, this would happen: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L281-L282 | |
| 14:27:30 | fried_rice | leakypipes I'm not talking about multiple threads in the placement code at the same time. I'm talking about multiple consumer threads making single individual atomic calls to completion. | |
| 14:29:10 | leakypipes | fried_rice: I'm going to go back to reviewing the patch series I need to get reviewed today :) you are welcome to walk through the code paths in there and identify a particular concurrency bug. | |
| 14:30:03 | leakypipes | fried_rice: you are also welcome to put a discussion on all the concurrency topics up at the PTG. I think that would be a good discussion. it's just I need to focus today on these reviews. | |
| 14:30:39 | fried_rice | leakypipes Here's the flow: Existing instance. Shared disk manager A wants to add some disk resource. sean-k-mooney's neutron thingy B wants to add a vif or whatever. A does a GET of the current allocations. B does a GET of the current allocations. A adds his disk and PUTs back. Placement happily replaces the allocations, which now include the disk, and increments the generation. B adds his vif to the original (d | |
| 14:30:39 | fried_rice | isk-less) payload and PUTs back. Placement happily replaces with B's version of the allocation, which has the vif but doesn't have the disk. | |
| 14:30:59 | leakypipes | alex_xu: are you still around? that patch you wanted a review on is dependent on this one which needs a rebase: https://review.openstack.org/#/c/479776/. are you planning on getting to that? | |
| 14:32:18 | sean-k-mooney | fried_rice: why would you not request both the disk and vif in the same allocation request? | |
| 14:32:45 | fried_rice | sean-k-mooney Because different entities are "owning" those resources. | |
| 14:32:53 | leakypipes | alex_xu: never mind. it's not a dependent patch, it's just in the same topic. damn gerrit... :( | |
| 14:33:32 | sean-k-mooney | fried_rice: differnet entityes are creating the resouce providers but the nova sechduler/conductor will be claiming both on behalf of the instance | |
| 14:33:48 | fried_rice | sean-k-mooney Sure, you *could* have it set up that way. Or, you know, not. | |
| 14:33:58 | fried_rice | sean-k-mooney At the moment, this is not a danger. | |
| 14:34:05 | fried_rice | sean-k-mooney In the future.... | |
| 14:34:28 | fried_rice | Certainly for the general case usage of the placement API. | |
| 14:34:36 | sean-k-mooney | we could technically make the neutron port the owner of the vif which would then result in two owners | |
| 14:35:09 | sean-k-mooney | *vif allocation | |
| 14:35:22 | fried_rice | The solution would be fairly simple I think: just add the generation for each RP to PUT /allocations/{consumer_uuid} and POST /allocations | |
| 14:37:43 | openstackgerrit | Merged openstack/nova master: libvirt: create vGPU for instance https://review.openstack.org/528832 | |
| 14:38:05 | fried_rice | Basically, it works as long as all the allocations for a given consumer are always only managed by one thread at a time. Which is a constraint that happens to be true today, but certainly isn't documented as a limitation of using the API. | |
| 14:38:49 | sean-k-mooney | leakypipes: alex_xu https://review.openstack.org/#/c/535642 seams to add suport for required to get /allocation_candiates but i dont see support for the Resouce1..,Required1 syntax needed for different traits per resouce. is there a followup patch for that | |
| 14:39:16 | leakypipes | sean-k-mooney: yes, we're not targeting that for queens. fried_rice has a patch up for that. | |
| 14:40:17 | fried_rice | leakypipes Quit saying that. We *are* targeting it for Queens, until something changes. We have an approved blueprint and 95% of the code merged. | |
| 14:41:05 | leakypipes | fried_rice: I won't hold up non-granular requests with traits for granular requests. | |
| 14:41:15 | leakypipes | fried_rice: does that sound more accurate? | |
| 14:41:23 | sean-k-mooney | leakypipes: ya i saw fried_rice's patch i assumed it superceeded alex's instead of building on it.thats problematic but ok. ill cherry pick fried_rice patch if i need it. | |
| 14:41:24 | fried_rice | leakypipes Agree with that (see -dev ml) | |
| 14:42:20 | fried_rice | sean-k-mooney ditto (http://lists.openstack.org/pipermail/openstack-dev/2018-January/126393.html) | |
| 14:43:11 | sean-k-mooney | so how does required work wiht nrp | |
| 14:43:28 | fried_rice | sean-k-mooney The reason my patch doesn't build on alex_xu's is because mine is 2.5 months older :) | |
| 14:44:31 | fried_rice | sean-k-mooney You get candidates such that a) all resources come from providers in the same tree, and b) the union of all the traits on providers you actually get resources from is a superset of the traits you asked for. | |
| 14:45:11 | sean-k-mooney | fried_rice: ok the union is the important bit | |
| 14:45:13 | fried_rice | sean-k-mooney Which is obviously limited e.g. you can't say I want one VIF with trait CUSTOM_PHYSNET_A and one with CUSTOM_PHYSNET_B -- for that you would need granular, period. | |
| 14:46:17 | sean-k-mooney | yes but i can say one vif and one cpu with CUSTOM_PHYSNET_A and cpu_trait_x | |
| 14:46:43 | fried_rice | sean-k-mooney Yes. And you'd better hope that your CPU doesn't have trait CUSTOM_PHYSNET_A | |
| 14:46:54 | fried_rice | (silly example in this case) | |
| 14:47:12 | sean-k-mooney | why what would happen if it did? | |
| 14:47:19 | fried_rice | You could still get a vif from physnet B and placement will think it did its job. | |