| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-26 | |||
| 02:20:00 | alex_xu | gmann: i'm not sure about the spec. but give th time i'm not sure we can get all thing done. maybe worth a email also to get widely suggestion | |
| 02:20:14 | mriedem | dansmith: bauzas: jaypipes: well it looks like the scheduler + placement stuff is failing grenade multinode because of this test: | |
| 02:20:15 | mriedem | https://github.com/openstack/tempest/blob/8bdd42766a7f02180d07e6ab75f0914e5d876a91/tempest/scenario/test_server_multinode.py#L49 | |
| 02:20:16 | gmann | alex_xu: i did not fully understood your comment about capability discovery on this https://review.openstack.org/#/c/415330/24 | |
| 02:20:26 | mriedem | i believe because we have 2 computes, but one is newton and one is ocata, and the newton one doesn't have placement configured, | |
| 02:20:45 | mriedem | so when the scheduler tries to place an instance on the newton compute, the scheduler pukes because there isn't a resource provider for that node | |
| 02:21:22 | gmann | alex_xu: you mean 403->200 is ok to change and discover with capabilities of all_tenant ? | |
| 02:21:36 | gmann | alex_xu: correct me if other way around | |
| 02:21:41 | alex_xu | gmann: whether the user is allowed to execute on a API, that should be discovered by capabilities discovery https://review.openstack.org/386555 | |
| 02:21:49 | owalsh | dansmith, mriedem, melwitt: big thanks for https://review.openstack.org/424243 | |
| 02:22:33 | dansmith | mriedem: with my backported change the newton one should keep trying | |
| 02:22:42 | dansmith | mriedem: this: https://review.openstack.org/#/c/419217/ | |
| 02:22:47 | dansmith | owalsh: np | |
| 02:23:03 | alex_xu | gmann: no, I agree with johnthetubaguy. We change a enforce from hard to soft, that is a change on the API contract. so yes, we need microversion | |
| 02:23:11 | mriedem | dansmith: but do we ever configure the newton subnode in grenade with placement creds? | |
| 02:23:32 | mriedem | dansmith: nope http://logs.openstack.org/61/417961/26/check/gate-grenade-dsvm-neutron-multinode-ubuntu-xenial/15545e4/logs/subnode-2/etc/nova/nova.conf.txt.gz | |
| 02:23:41 | dansmith | mriedem: oh right, damn | |
| 02:24:08 | dansmith | mriedem: so it fails because we don't have resources for multiple computes, right? | |
| 02:24:23 | dansmith | mriedem: that's farther than it was getting before, right? so that | |
| 02:24:25 | gmann | alex_xu: yea, so we all are on same page. thanks | |
| 02:24:27 | dansmith | mriedem: is progress | |
| 02:24:37 | alex_xu | gmann: yea | |
| 02:25:00 | gmann | alex_xu: i ll push spec + patch updates and both can be reviewed in parallel | |
| 02:25:10 | alex_xu | gmann: thanks! | |
| 02:25:21 | mriedem | dansmith: correct, this test uses AZs to specifically place an instance on each compute node | |
| 02:25:32 | mriedem | so the forced_host thing or whatever | |
| 02:26:29 | mriedem | we could change the test to not be a smoke test and it wouldn't be run with grenade, but i could see people freaking out about that | |
| 02:27:08 | dansmith | mriedem: well, it's either that or backport cred setup to newton devstack right? | |
| 02:27:18 | mriedem | cred setup is in newton devstack, | |
| 02:27:29 | mriedem | it's just grenade isn't configuring placement creds into nova.conf on the subnode | |
| 02:27:58 | dansmith | oh | |
| 02:28:27 | mriedem | the other alternative, which you're not going to probably like, is for ocata we fallback to the compute node get_all and drop that in pike | |
| 02:28:40 | mriedem | because ocata computes will have placement setup for grenade from ocata->pike | |
| 02:28:40 | dansmith | mriedem: I guess it's an important thing to check that the subnode is still live after the upgrade, assuming proper config | |
| 02:29:06 | dansmith | mriedem: seems strange to push this hard and then not try to fix grenade at the last minute | |
| 02:33:40 | mriedem | yeah i know, i'm not sure how hard this is to make grenade config the newton subnode for placement right now | |
| 02:34:08 | dansmith | melwitt: can you see my response to you on my patch just now? | |
| 02:34:28 | mriedem | i have an emergency toenail situation to deal with that's much easier to fix, will check grenade after that | |
| 02:34:36 | melwitt | dansmith: yeah just opened it. replying | |
| 02:41:06 | dansmith | melwitt: might be easier to chat here | |
| 02:41:45 | melwitt | dansmith: okay. that's setting the _context on the BuildRequest object though, not the BuildRequest.instance object | |
| 02:42:18 | melwitt | I don't see anywhere in _load_instance that it sets instance._context | |
| 02:42:42 | melwitt | self.instance._context | |
| 02:43:15 | dansmith | melwitt: you're right, I remember arguing with alaski about this specifically | |
| 02:43:29 | dansmith | melwitt: so what do you want me to change it to? | |
| 02:43:31 | melwitt | yeah, I was not sure if it's intentional not to do it | |
| 02:43:42 | dansmith | I wanted him to, and he wanted it to be orphaned | |
| 02:44:02 | melwitt | hrm. | |
| 02:44:41 | melwitt | assuming he had good reason for that (we should ask him next time we catch him in the channel), I would think to go back to what you had, sans the refresh() call | |
| 02:45:04 | dansmith | but your comment above the refresh explains why that's important | |
| 02:46:51 | melwitt | why what's important? | |
| 02:47:05 | dansmith | we also have to handle the case where we didn't actually look it up right? | |
| 02:47:11 | dansmith | why the refresh is important | |
| 02:47:24 | dansmith | or you want to keep the instance we got from _lookup? | |
| 02:47:57 | melwitt | oh, I was saying that's another more complicated option if doing a proper refresh() is important | |
| 02:48:16 | dansmith | okay, I'm totally confused | |
| 02:49:07 | melwitt | sorry. I was trying to say I think what we had with the _lookups makes sense considering that the Instance could have been a half-baked object generated from a BuildRequest | |
| 02:49:27 | dansmith | to get the newer instance with .host set, right? | |
| 02:49:40 | melwitt | and only to do something more (like check whether it's orphaned or not to determine whether to _lookup vs refresh()) but I'm not sure that matters | |
| 02:49:44 | melwitt | yeah | |
| 02:50:24 | dansmith | okay but PS79 wouldn't bail if we fail to do that lookup, so I also have to include the bit from PS81 that bails if we fail to lookup because we raced with another delete | |
| 02:51:32 | melwitt | if it's important to do a refresh if real Instance, then we'd have to check for that and do either or depending. and I was going to defer to you on whether we should do that here | |
| 02:52:18 | dansmith | we have to refresh if your comment above the refresh is correct | |
| 02:52:28 | dansmith | otherwise we'll fail to send the delete request to the compute | |
| 02:53:41 | melwitt | I thought the _lookup would serve as the refresh to send it. and then like you said also need the bail out part if it was a race with another delete | |
| 02:54:36 | dansmith | that's what I'm saying | |
| 02:55:05 | dansmith | however, it seemed like I was getting an orphaned instance from that lookup in the previous patchset because I was getting orphaned errors in jenkins, which I no longer am here | |
| 02:57:07 | dansmith | melwitt: you happy with this? http://pastebin.com/QgsXKH1f | |
| 02:57:11 | melwitt | that's unexpected | |
| 02:57:40 | dansmith | melwitt: what is unexpected? the orphaned warnings before? I can link you to the logs :) | |
| 02:58:16 | melwitt | yeah. I believe you, it's just unexpected since I just looked through _lookup_instance and didn't think I saw a way it could get an orphaned instance loaded there | |
| 02:59:09 | dansmith | actually, let me go back one more rev hang on | |
| 02:59:14 | dansmith | but, is the above what you're thinking? | |
| 02:59:17 | melwitt | dansmith: and yes that code is what I was thinking. but I'm concerned about the orphaned errors (although in the IBM CI it shows we still have them, maybe less of a window). I'm going to look through the logs now | |
| 03:00:08 | dansmith | melwitt: okay I got it | |
| 03:00:29 | dansmith | melwitt: PS79 we were getting failure trying to do instance.uuid from the return of _lookup, which can be None | |
| 03:00:48 | dansmith | since I wasn't bailing there, I was using that instance (which had _context) if it was good, and hitting None.uuid if not | |
| 03:00:51 | dansmith | then in the next one, | |
| 03:00:54 | melwitt | dansmith: oh, because you lacked the bailout code | |
| 03:00:57 | dansmith | I used _instance and was getting the orphaned errors | |
| 03:01:04 | dansmith | but maybe got lucky on the race | |
| 03:01:21 | dansmith | so I think the above code, using the _lookup for the refresh and bailing if we get None is probably right | |
| 03:01:24 | melwitt | okay, so the code in the paste should catch all of those cases. I think | |
| 03:01:27 | dansmith | yeah | |
| 03:01:48 | dansmith | the dep chain for this is like five deep so I just can't keep the sequencing of the errors in my head I guess | |
| 03:02:10 | openstackgerrit | Dan Smith proposed openstack/nova: Move instance creation to conductor https://review.openstack.org/319379 | |
| 03:03:31 | dansmith | and looks like the lowest dep is going to get kicked out of the gate.. yay | |
| 03:03:36 | dansmith | #fml | |
| 03:03:53 | melwitt | guh | |
| 03:11:53 | dansmith | I'm still kinda unsure.. this must be a fantastic race that we only hit occasionally | |
| 03:12:18 | dansmith | because I think it ran a bunch of times in a row without hitting it when I was dicking with the listing patch | |
| 03:12:29 | dansmith | but whatever | |
| 03:15:35 | melwitt | back when I was working on the patch to pass the cells v1 job, it failed often enough with the ObjectActionError in that block that I knew to add something. guess I was "lucky" | |
| 03:16:00 | dansmith | right but that was a race, | |
| 03:16:08 | dansmith | this is a race in the race handler :) | |
| 03:16:43 | melwitt | yes. fwiw it got really stable after adding the catch block, it's just the instance.refresh() wasn't really doing anything | |
| 03:16:51 | dansmith | I mean, I'm not saying this doesn't make sense, I'm just wondering why murphy hates me | |
| 03:17:04 | melwitt | yeah, I get you | |
| 03:17:31 | dansmith | I think it was, but only in the cases where we get there from a buildrequest and when we didn't race with another delete right? | |
| 03:19:08 | melwitt | hm, yeah. I guess it would have had an orphaned error itself when it tried to reload itself if it came from a build request | |