| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-30 | |||
| 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) | |
| 15:13:36 | efried | I'm confused as to why it's not `host1` | |
| 15:13:40 | mdbooth | stephenfin: How sure are you of assert(), btw? | |
| 15:13:53 | mriedem | efried: because the fake driver is pullin the node name from CONF.host | |
| 15:13:58 | mriedem | if you don't use fake.set_nodes | |
| 15:14:09 | stephenfin | mdbooth: Not hugely sure. I just recall being told not to use it at some point in the past | |
| 15:14:11 | efried | But as far as I can see, CONF.host *should* be `host1` and `host2` | |
| 15:14:55 | mdbooth | stephenfin: I think I'm using it correctly in that patch, but there may be practical considerations. e.g. if we disable it in CI. | |
| 15:14:57 | mriedem | efried: ala https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L1283 | |
| 15:15:18 | mdbooth | If it's disabled in production, we don't really care. | |
| 15:15:32 | efried | mriedem Okay, thanks, trying... | |
| 15:15:59 | cdent | efried: I can't remember, is the rp ending up named fake-mini or 'host?' ? | |
| 15:16:07 | efried | cdent fake-mini | |
| 15:16:17 | mdbooth | mriedem: Do you happen to remember, btw: all(el + 1 == l[i + 1] for i, el in enumerate(l[:-1])) ? | |
| 15:16:25 | mriedem | melwitt: so i've got this code now http://paste.openstack.org/show/627877/ | |
| 15:16:37 | mdbooth | You chose to add a comment to it for some reason :) | |
| 15:16:44 | mriedem | melwitt: which fails with this http://paste.openstack.org/show/627878/ | |
| 15:16:54 | mriedem | so i'm creating host2 in cell2 but when i try to pull host2 out of cell2, it's not found | |
| 15:17:06 | mriedem | i'm assuming that's because of the CellDatabase fixture defaulting to cell1 | |
| 15:17:15 | mriedem | which is weird since i'm targeting the context specifically to cell2 | |
| 15:17:26 | mriedem | but i can replay this on top of your fixture cleanup patch and see if that fixes it | |
| 15:18:04 | mriedem | mdbooth: ? | |
| 15:18:35 | mriedem | mdbooth: if i added a comment it's because i oculdn't understand ndipanov's code and had to figure it out at one point, and left a comment for posterity | |
| 15:19:02 | mdbooth | mriedem: Not important. I was doing something else and came across it. It made my head hurt :) I submitted this throw-away: https://review.openstack.org/#/c/524208/ | |
| 15:20:09 | mdbooth | I assume this was very X of this patch for some large value of X, and this complexity made sense at some version <X | |
| 15:20:16 | mdbooth | s/very/version/ | |
| 15:21:27 | mdbooth | I was actually trying to work out where we set the boot index of a root disk to 0 if it wasn't done explicitly in the api call | |
| 15:21:30 | mdbooth | still haven't found it | |
| 15:21:50 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add instance action record for shelve_offload instances https://review.openstack.org/523653 | |
| 15:23:31 | openstackgerrit | Merged openstack/nova stable/pike: Set regex flag on ostestr command for osprofiler tests https://review.openstack.org/510226 | |
| 15:23:45 | mriedem | melwitt: that didn't seem to fix it, still can't find host2 in cell2 | |
| 15:24:31 | tetsuro | Hi all, I'd like you to review https://review.openstack.org/#/c/465160/ | |
| 15:26:11 | tetsuro | Currently, VM with NUMA goes into a silent error if the virt_type doesn't support it. | |
| 15:26:21 | tetsuro | So, this patch is to raise an error explicitly if NUMA Topology not supported, | |
| 15:49:06 | mriedem | 2017-11-30 10:47:07,831 ERROR [nova.tests.functional.test_servers] Failed to find host host2 in cell cell2 | |