| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-12 | |||
| 21:24:06 | bauzas | mriedem: you mean https://review.openstack.org/#/c/392569/26/nova/api/openstack/placement/handlers/resource_provider.py@a170 ? | |
| 21:24:24 | mriedem | bauzas: yup | |
| 21:25:01 | bauzas | mriedem: accepted | |
| 21:25:12 | ildikov | mriedem: that contains the API microversion | |
| 21:25:19 | dansmith | rpodolyaka: but we want it to roll back all the way in this case | |
| 21:25:24 | mriedem | ildikov: can you put it into a paste? | |
| 21:25:25 | dansmith | rpodolyaka: so no savepoint, as you said | |
| 21:25:53 | melwitt | dansmith: I thought we just wanted to skip duplicates? and commit all the non-duplicates in one transaction | |
| 21:26:17 | bauzas | mriedem: so, about the member_of query | |
| 21:26:18 | dansmith | melwitt: sure, but if something else happens you want to unwind all of them | |
| 21:26:28 | melwitt | oh | |
| 21:26:47 | bauzas | mriedem: we first added the member_of field in allowed_filters without microversing it | |
| 21:27:02 | dansmith | melwitt: otherwise you've got some moved and not others, which will make the code start to look at just the subset, instead of going back to the main db while you figure out what to do | |
| 21:27:14 | ildikov | mriedem: here it is: http://paste.openstack.org/show/594772/ | |
| 21:27:21 | bauzas | mriedem: so, before that, we were not accepting member_of and then we accepted it | |
| 21:27:40 | melwitt | dansmith: yeah, I guess I'm confused about what happens then if we don't use a savepoint and get a duplicate, that it would rollback the entire transaction? | |
| 21:27:46 | openstackgerrit | Sarafraj Singh proposed openstack/nova: Adopts keystoneauth with glance client. https://review.openstack.org/412634 | |
| 21:27:53 | bauzas | mriedem: for both 1.0 and 1.1 | |
| 21:28:04 | openstackgerrit | Sarafraj Singh proposed openstack/nova: Adopts keystoneauth with glance client. https://review.openstack.org/412634 | |
| 21:28:14 | dansmith | melwitt: you're ignoring duplicates, right? | |
| 21:28:32 | melwitt | dansmith: yeah, but rpodolyaka said it would still roll back everything? | |
| 21:28:34 | bauzas | mriedem: and then, cdent added the v1.3 microversion for still accepting member_of even if that was not this version | |
| 21:28:45 | dansmith | melwitt: no, he said it would roll back everything if you allow an exception to be raised, | |
| 21:28:47 | bauzas | which is IMO terrible but okay | |
| 21:28:56 | dansmith | which is what we want for everything other than dupes, which you catch and swallow | |
| 21:29:03 | cdent | we discussed it at length and decide that it impacted approximately zero real people | |
| 21:29:39 | rpodolyaka | dansmith: the problem is that you can't have one transcation and swallow duplicate errors | |
| 21:29:40 | melwitt | dansmith: yeah, I actually asked if I still need savepoint considering I'm catching and ignoring DBDuplicateEntry and the answer was yes. maybe there's a misunderstanding | |
| 21:29:42 | ildikov | mriedem: this is the n-api log for the same attachment-create call: http://paste.openstack.org/show/594773/ | |
| 21:29:46 | rpodolyaka | unless you use savepoints | |
| 21:29:58 | rpodolyaka | but the rollback in the outer transcation will still rollback everything | |
| 21:30:23 | bauzas | cdent: mriedem: anyway, I'll -W it and work tomorow morning my time for a next PS | |
| 21:30:24 | dansmith | rpodolyaka: I don't know why that is | |
| 21:30:36 | mriedem | bauzas: yeah so up until 1.4 we always accepted member_of, | |
| 21:30:47 | mriedem | but if < 1.3 we don't filter on member_of | |
| 21:31:08 | smcginnis | ildikov: I don't see "Openstack-Api-Version: volume 3.24" in there. | |
| 21:31:10 | bauzas | mriedem: not really but yeah | |
| 21:31:10 | mriedem | but it's not an error to put it in the query string, because of the aforementioned missing of the versioning | |
| 21:31:25 | bauzas | mriedem: for example, Newton is not accepting it for 1.0 | |
| 21:31:29 | bauzas | AFAIK | |
| 21:31:31 | ildikov | smcginnis: me neither, but I also don't know what should put it there :) | |
| 21:31:47 | rpodolyaka | dansmith: lemme give it a try on a small snippet | |
| 21:31:51 | smcginnis | ildikov: :) | |
| 21:31:55 | mriedem | bauzas: oh because member_of wasn't in allowed_filters until ocata | |
| 21:32:02 | bauzas | zactly | |
| 21:32:13 | bauzas | just verifying it | |
| 21:32:18 | bauzas | for stable/newton | |
| 21:32:18 | mriedem | https://github.com/openstack/nova/blob/stable/newton/nova/objects/resource_provider.py#L413 | |
| 21:32:25 | jaypipes | mriedem: responded to your review comments on the patch. | |
| 21:32:29 | bauzas | mriedem: yup that | |
| 21:33:02 | bauzas | jaypipes: lemme work on a next patchset tomorrow | |
| 21:33:08 | dansmith | rpodolyaka: my understanding is that an insert will still fail with DBDuplicate when you do it, and the thing that does the abort/commit of the transaction is the top-level facade decorator, | |
| 21:33:12 | openstackgerrit | Jay Pipes proposed openstack/nova: placement: RT now adds proper Ironic inventory https://review.openstack.org/404472 | |
| 21:33:13 | openstackgerrit | Jay Pipes proposed openstack/nova: rt: explicitly pass compute node to _update() https://review.openstack.org/398472 | |
| 21:33:16 | openstackgerrit | Jay Pipes proposed openstack/nova: rt: use a single ResourceTracker object instance https://review.openstack.org/398473 | |
| 21:33:23 | bauzas | jaypipes: given it's the end of your afternoon :) | |
| 21:33:30 | jaypipes | bauzas: no problemo. I'm flying all day :) | |
| 21:33:44 | dansmith | rpodolyaka: so if you appear to exit your function cleanly, it will commit and you save anything you successfully inserted, unless you hit *another* dupe because you raced with another transaction | |
| 21:33:55 | bauzas | jaypipes: heh, seems terrible :) | |
| 21:34:28 | bauzas | mriedem: so, confirmed, stable/newton is not accepting member_of https://github.com/openstack/nova/blob/stable/newton/nova/api/openstack/placement/handlers/resource_provider.py#L187 | |
| 21:34:47 | rpodolyaka | dansmith: the problem is that it's the very same transaction - once you have an integrity error you must roll it back at this point | |
| 21:34:52 | bauzas | cdent: ^ see that | |
| 21:34:59 | mriedem | yeah ok... | |
| 21:35:08 | dansmith | rpodolyaka: why must you roll back? | |
| 21:35:13 | cdent | bauzas: what are you trying to tell me? | |
| 21:35:16 | mriedem | so it might be fine to bend the rules and make the schema validation strict again | |
| 21:35:25 | bauzas | cdent: I think that if we silently accept member_of, it's not zero folks impacted | |
| 21:35:33 | mriedem | i.e. we didn't allow it, then we allowed it for a short window, then we didn't again | |
| 21:35:37 | mriedem | and the only user is internal to nova right now | |
| 21:35:43 | mriedem | excluding whatever mlavalle is doing | |
| 21:35:48 | dansmith | rpodolyaka: we did another migration like this that I think we've since purged, so if this doesn't work then that didn't either | |
| 21:35:49 | rpodolyaka | dansmith: mysql will tell you the transaction is invalid and must be rolled back. just like with any other error in a transaction | |
| 21:36:04 | rpodolyaka | you sure that were not independent transactions? | |
| 21:36:07 | bauzas | mriedem: either way, you have lots of other comments, will respond to those | |
| 21:36:13 | dansmith | rpodolyaka: yeah | |
| 21:36:16 | rpodolyaka | hmm | |
| 21:36:36 | dansmith | rpodolyaka: well, I don't know anything about mysql making you do that, but surely willing to believe you know more than me | |
| 21:37:07 | openstackgerrit | Sivasathurappan Radhakrishnan proposed openstack/nova: Refactor _post_live_migratiion https://review.openstack.org/413021 | |
| 21:37:17 | bauzas | anyway, calling it a end of day | |
| 21:37:19 | rpodolyaka | dansmith: heh, I'm a bit sleepy, so maybe I'm missing something obvious :P | |
| 21:37:24 | dansmith | rpodolyaka: not sure we were filtering for dupes before, which I guess is the wrinkle | |
| 21:37:49 | dansmith | melwitt: either way, if rpodolyaka is right, a save point before each insert and a revert in a generic handler is not hard right? | |
| 21:38:08 | melwitt | yeah, I couldn't find a current migration that does a loop in one transaction, to use as an example | |
| 21:38:18 | dansmith | melwitt: also, do you need the dupe handling if you apply them in a transaction? | |
| 21:38:55 | mriedem | jaypipes: what do you mean this is unreadable?! (?:[A-Z0-9_]+\:[0-9]+),? | |
| 21:39:02 | melwitt | dansmith: hm, I dunno. maybe not | |
| 21:40:07 | openstackgerrit | Sarafraj Singh proposed openstack/nova: Add service_token for nova-glance interaction https://review.openstack.org/419662 | |
| 21:42:48 | mlavalle | mriedem, bauzas: I am not using any filters with resource providers in my core. Feel free to go ahead | |
| 21:42:48 | mlavalle | code^^^ | |
| 21:47:06 | mriedem | bauzas: jaypipes: ok i replied to the replies. :) i still think that we can enforce the format of the uuid parameter to be a uuid, as it's not being used with in: | |
| 21:47:13 | mriedem | "in:" is only used with member_of | |
| 21:48:07 | mriedem | https://review.openstack.org/#/c/392569/26/nova/api/openstack/placement/handlers/resource_provider.py@289 | |
| 21:48:33 | openstackgerrit | Sarafraj Singh proposed openstack/nova: DNM: Test service token in Glance https://review.openstack.org/419666 | |
| 21:49:10 | mriedem | ildikov: ok so "-H "OpenStack-API-Version: volume 3.24"" | |
| 21:49:22 | dansmith | rpodolyaka: okay, I found some references to what you're describing | |
| 21:49:26 | rpodolyaka | dansmith: shit, it works on mysql - but fails on postgresql without savepoints... http://paste.openstack.org/show/594775/ | |
| 21:49:37 | dansmith | hah | |
| 21:50:10 | dansmith | rpodolyaka: seems like enginefacade could manage this for us.. automatically doing savepoints or something, if we nest calls | |
| 21:50:14 | mriedem | ildikov: yeah so the "OpenStack-API-Version" header isn't in the paste from the nova-api call | |
| 21:50:29 | dansmith | rpodolyaka: but regardless, gotta handle it for now I guess | |