| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-30 | |||
| 14:39:25 | efried | jaypipes Is there a reason we don't do that? | |
| 14:39:36 | jaypipes | efried: just never did it. I blame cdent. | |
| 14:39:50 | cdent | jaypipes: hah, I validated the api guideline that says it should happen | |
| 14:39:53 | cdent | I blame jaypipes | |
| 14:40:13 | efried | Okay, if there's no reason we're avoiding it, we could get started on that as a long-term solution. | |
| 14:40:13 | jaypipes | cdent: :) | |
| 14:40:26 | gibi | gmann: looking... | |
| 14:40:31 | jaypipes | efried: ack, though there are more pressing concerns at this point, no? | |
| 14:40:39 | efried | In the meantime: in PS2 I actually made us raise an exception: https://review.openstack.org/#/c/523932/2 | |
| 14:40:44 | cdent | efried: I think it was mostly just a kind of distraction in the early days and then forgotten | |
| 14:40:50 | cdent | however, it wouldn't make any difference in this bug | |
| 14:41:06 | efried | jaypipes That broke a functional test that runs _init_compute_host twice. cdent was helping me look at that last night. | |
| 14:41:28 | cdent | the 409 is the same in either case: uniqueness is violated and we require uniqueness. the bug isn't so much the response code as the return of None | |
| 14:41:35 | jaypipes | efried: sorry, I didn't mean the bug wasn't pressing. I meant reworking things so we have proper error codes. | |
| 14:41:45 | jaypipes | efried: as opposed to grepping through an error string | |
| 14:41:46 | efried | jaypipes Yeah, I'm with you. | |
| 14:42:00 | efried | jaypipes Hold on, are you saying we could grep the error string? | |
| 14:42:05 | efried | as an interim solution? | |
| 14:42:33 | jaypipes | efried: we've already done that in a couple places, yeah. | |
| 14:42:43 | cdent | the error string doesn't matter in this bug | |
| 14:43:12 | cdent | or rather the fact that we hide the error response in the expection message made it hard to debug | |
| 14:43:16 | jaypipes | cdent: well, the whole "the error is saying I used a bad UUID, when I actually used a bad name" is problematic. | |
| 14:43:26 | jaypipes | cdent: since they both have the same error message | |
| 14:43:26 | cdent | that's a different bug: | |
| 14:43:30 | jaypipes | cdent: ack | |
| 14:43:42 | cdent | https://bugs.launchpad.net/nova/+bug/1735405 | |
| 14:43:44 | openstack | Launchpad bug 1735405 in OpenStack Compute (nova) "Error message from placement when creating resource provider uses ambiguous identifier" [Undecided,New] | |
| 14:43:44 | efried | So let's say we do grep the error string - how would report client react that would be appropriate here? | |
| 14:43:53 | jaypipes | ok, agreed, diff bug | |
| 14:43:55 | cdent | the db duplicate response message does include which columns were duplicated | |
| 14:44:03 | cdent | I changed that some months ago | |
| 14:44:10 | cdent | but we hide that in the report client, and shouldn't | |
| 14:44:15 | jaypipes | efried: raise an exception back to the caller. | |
| 14:44:29 | jaypipes | cdent: gotcha | |
| 14:44:39 | cdent | why do we need to grep the error response? | |
| 14:44:48 | cdent | if uniqueness fails, uniqueness fails | |
| 14:44:58 | cdent | you can only get a 409 from post to /resource_providers for uniqueness fails | |
| 14:45:00 | jaypipes | cdent: in order to log an appropriate message, that's all | |
| 14:45:17 | jaypipes | cdent: it's just misleading to write an error saying bad UUID, when it was the name that was problematic | |
| 14:45:17 | cdent | pass on the response's error message, no need to parse it, just use it | |
| 14:45:40 | jaypipes | cdent: sure, can do that | |
| 14:46:09 | efried | Hold on | |
| 14:46:13 | efried | I don't agree | |
| 14:46:45 | efried | Right now we assume it's UUID conflict, so we GET the provider by UUID and return it (or None) | |
| 14:47:03 | cdent | the code is incomplete | |
| 14:47:16 | efried | Right | |
| 14:47:28 | cdent | if we want to assume that a 409 _could_ be "another thread created" | |
| 14:47:33 | cdent | then yes, parsing it required | |
| 14:47:52 | cdent | (or adding more detailed error codes, eventually) | |
| 14:48:36 | gibi | gmann_afk: +W | |
| 14:49:47 | cdent | but, then we have 409 meaning potentially 3 things: uuid uniqueness violation (good luck!), name uniqueness violation, concurrent violation | |
| 14:50:09 | cdent | and while telling some of those apart is gravy, the uuid ones, unclear | |
| 14:50:12 | jaypipes | cdent: right. thus the need for more specific error codification. | |
| 14:50:35 | cdent | right, but even with better error codification how do you distinguish conficts 1 and 3? | |
| 14:50:43 | bauzas | oh shit, DST shift impact :( | |
| 14:50:46 | bauzas | mriedem: ^ | |
| 14:50:53 | efried | cdent We have to assume #1 doesn't happen. | |
| 14:51:02 | efried | Or rather, assume the risk | |
| 14:51:51 | efried | Realistically, we don't get to that point if there's a UUID "conflict" - we would have just returned that provider from the first GET in _ensure_resource_provider. | |
| 14:51:57 | mriedem | bauzas: ? | |
| 14:52:38 | cdent | brb | |
| 14:52:49 | mriedem | sdague: i think you will enjoy this https://review.openstack.org/#/c/507693/ | |
| 14:53:20 | efried | cdent jaypipes IMO the first thing we should do is the simplest: raise exceptions instead of returning None in those various paths. | |
| 14:54:03 | efried | Today that 404 will be indicating name conflict 99% of the time. | |
| 14:55:07 | efried | The only excepton would be concurrent create-followed-by-delete. So I'd even go so far as to say 99.99999% | |
| 14:55:09 | cdent | efried: yeah, I agree None is not right | |
| 14:55:13 | jaypipes | efried: go for it. | |
| 14:55:27 | cdent | you'll have to have the fun of figuring out what's up with the api_samples test :( | |
| 14:55:47 | efried | Exactly. jaypipes it looks like this: http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz | |
| 14:56:24 | efried | It happens because those test cases are starting the fake compute host twice. Each one seems to be getting its own database. But still just one placement service. | |
| 14:56:57 | sdague | mriedem: nice, those test cleanups are especially nice | |
| 14:57:01 | efried | We create ComputeNode with a name and let the DB assign its UUID. Then we push that guy to placement. So the second time around, it has a different UUID. | |
| 14:57:13 | cdent | efried: i don't think "own database" is the right analysis. something else is going on, like the second node (not host), clobbering the first | |
| 14:57:51 | efried | cdent Okay. FWIW, I pdb'd it, and the compute host definitely ain't there the second time around. | |
| 14:58:42 | efried | I don't really have context on how these fixtures do their business. Who would be an expert there? | |
| 14:59:22 | cdent | sdague might have some insight into efried's problem? | |
| 15:01:55 | mriedem | efried: same cell? | |
| 15:02:00 | mriedem | i.e. it's a single-cell test right? | |
| 15:02:10 | takashin | nova-specs cores, would you take a look at a patch to create specs directory for Rocky https://review.openstack.org/#/c/514101/ ? | |
| 15:02:11 | mriedem | could be an issue with not using fake.set_nodes() and fake.restore_nodes | |
| 15:02:12 | efried | mriedem I don't know that answer. | |
| 15:02:28 | efried | mriedem http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz | |
| 15:05:37 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.56 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:06:28 | takashin | a patch to create specs directory for Rocky https://review.openstack.org/#/c/514101/ | |
| 15:09:13 | openstackgerrit | Matthew Booth proposed openstack/nova master: Simplify BDM boot index checking https://review.openstack.org/524208 | |
| 15:10:17 | sdague | what's up? | |
| 15:10:32 | sdague | efried: can you summarize the issue, maybe I can help | |
| 15:11:25 | efried | sdague If report client tries to ensure a RP exists, and gets any error, it returns None (rather than raising an exception) but nobody is checking for that. | |
| 15:11:45 | mriedem | efried: these are a single cell test, using the SingleCellSimple | |
| 15:11:45 | efried | sdague In this test: http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz we're hitting that code path | |
| 15:12:04 | mriedem | efried: looking at HypervisorsSampleJson233Tests, it's creating a 2nd service | |
| 15:12:10 | mriedem | but not setting unique nodes | |
| 15:12:42 | efried | sdague As long as we keep the `return None` in there, the test works; but if I raise an exception, the test fails because, as mriedem is saying, it's creating a second service, which creates a second ComputeNode with the same name but different UUID. | |
| 15:12:51 | mriedem | _IntegratedTestBase, the base class, creates a compute service, and the HypervisorsSampleJson test creates a compute service, but i think they have the same node name | |
| 15:13:01 | efried | sdague When we try to create that in placement, we get that 409 because the *name* conflicts. | |
| 15:13:10 | sdague | mriedem: yeh, that's probably something we should prevent on the test side | |
| 15:13:21 | efried | mriedem Yes, they're both called fake-mini. | |
| 15:13:22 | mriedem | so you need to put this in HypervisorsSampleJson when creating the 2nd service: | |
| 15:13:30 | mriedem | fake.set_nodes(['host1']) | |
| 15:13:35 | mriedem | self.addCleanup(fake.restore_nodes) | |