| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-26 | |||
| 01:39:24 | oomichi | gmann: yeah I see "For the non-admin user, the sort key ‘host’ and ‘node’ will be excluded. Those two columns are about the cloud internal. It can’t be leaked to the end user." | |
| 01:41:56 | oomichi | gmann: if we consider it as security issue, the bug should be fixed on stable branches. But we need different patch for fixing it because it is difficult to backport the whole patches related to the bp | |
| 01:43:03 | gmann | oomichi: hummm | |
| 01:46:07 | gmann | alex_xu: johnthetubaguy oomichi on policy all_tenant changes, should we first change the spec ? - https://specs.openstack.org/openstack/nova-specs/specs/ocata/approved/add-whitelist-for-server-list-filter-sort-parameters.html#rest-api-impact | |
| 02:04:44 | melwitt | dansmith: I left a comment on https://review.openstack.org/#/c/319379/ it looks like the IBM CI failure might be legit, OrphanedObjectError raised from L1790 | |
| 02:05:12 | melwitt | I don't yet understand how the instance could be orphaned at that point | |
| 02:19:04 | alex_xu | oomichi: yea, just as gmann said, this probably is a fix for key we didn't want to expose. I feel that is different with all_tenants | |
| 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" | |