| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-01-26 | |||
| 01:13:47 | mriedem | dansmith: simple change needed in https://review.openstack.org/#/c/424243/ and then i'm +2 | |
| 01:14:08 | dansmith | mriedem: we're waiting for a run I think | |
| 01:16:07 | gmann | alex_xu: | |
| 01:16:15 | gmann | alex_xu: humm, | |
| 01:17:22 | dansmith | mriedem: | |
| 01:17:34 | dansmith | gah | |
| 01:17:53 | mriedem | _LW _MFER | |
| 01:18:13 | openstackgerrit | Dan Smith proposed openstack/nova: Only warn about hostmappings during ocata upgrade https://review.openstack.org/424243 | |
| 01:18:18 | dansmith | there | |
| 01:21:05 | gmann | alex_xu: IMO, that as bug fix makes it valid | |
| 01:24:10 | oomichi | alex_xu: Does it mean the patch changes 403 to 400 ? | |
| 01:26:13 | oomichi | alex_xu: we are discussing error status code changes on https://review.openstack.org/#/c/421846 . I am ok to allow error status code changes anyways | |
| 01:26:30 | openstackgerrit | Dan Smith proposed openstack/nova: Only warn about hostmappings during ocata upgrade https://review.openstack.org/424243 | |
| 01:29:33 | gmann | oomichi: policy change for all_tenant will change 403->200 | |
| 01:29:53 | gmann | oomichi: so its kinnda behavior change | |
| 01:30:37 | gmann | oomichi: and on sort key change it is bug fix (not expose host and node to non admin user) and 200->400 | |
| 01:31:02 | gmann | alex_xu: correct me if am wrong on sort key which used to me 200 now? | |
| 01:32:54 | oomichi | gmann: humm, 200->400 seems against on changing policy: "The following types of changes are generally **not** considered acceptable:" | |
| 01:33:18 | oomichi | "A change such that a request which was successful before now results in an error response" | |
| 01:33:49 | oomichi | if that is not security bug | |
| 01:34:56 | oomichi | gmann: "not expose host and node to non admin user" seems a little bit security thing, which means abstraction doesn't work correctly for non-admin users | |
| 01:35:08 | mriedem | melwitt: this is an easy one https://review.openstack.org/#/c/424243/ | |
| 01:35:09 | oomichi | but I am not sure at this time | |
| 01:37:52 | gmann | oomichi: yea that seems security bug for me too. but johnthetubaguy alex_xu mriedem can give better feedback on this - last paragraph in https://specs.openstack.org/openstack/nova-specs/specs/ocata/approved/add-whitelist-for-server-list-filter-sort-parameters.html#proposed-change | |
| 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 | dansmith | mriedem: I guess it's an important thing to check that the subnode is still live after the upgrade, assuming proper config | |
| 02:28:40 | mriedem | because ocata computes will have placement setup for grenade from ocata->pike | |
| 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 | |