Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-11-21
17:03:47 mriedem efried: you'll like this ^ let's not fail with NoneType errors during re-auth
17:04:01 mriedem niraj_singh had a real issue and then left
17:04:43 efried mriedem https://review.openstack.org/#/c/512329/
17:05:05 sean-k-mooney stephenfin: so looking at that you infer the request for pinning by checking "self.cpu_pinning is not None" what dose self.cpu_pinning contain? will that check work if i explcitly set the policy to shared in the flavour
17:05:36 stephenfin sean-k-mooney: We _used_ to do that, then we added a field to actually store the policy
17:05:47 efried mriedem And yeah, I just sent him an email.
17:06:19 stephenfin With recent object versions, we check the policy field. However, that patch ensure the older object versions continue to work
17:06:22 efried mriedem I bet he didn't realize the conf split that happened a couple months ago, and has his stuff in the wrong conf file.
17:06:50 mriedem efried: replied on yours
17:06:54 mriedem i don't like raising a random exception here
17:07:20 efried mriedem Having seen yours, I suspected that would be the case.
17:07:30 mriedem well,
17:07:35 mriedem it punishes the user for the operator screwing up
17:07:41 efried mriedem I would rather fail early.
17:07:49 efried mriedem Because otherwise they probably get the original bug
17:07:50 mriedem plus, it could leak 500s out of the api
17:08:23 mriedem the original bug is the user token times out and you can't re-auth, which is no different from not using the service user stuff
17:08:41 mriedem with your change, the operator screws up and the api user is punished, plus we probably get 500s for this now
17:09:00 mriedem because i'm sure there is REST API code calling glance/cinder/neutron client stuff and not handling the error you're raising
17:09:01 efried mriedem The service will fail very early, likely before the user even got his hands on it.
17:09:52 mriedem you're assuming operators are doing full api test coverage, including all of the proxy apis which are going to hit tis code
17:10:19 openstackgerrit Stephen Finucane proposed openstack/nova master: test: Store the OutputStreamCapture fixture https://review.openstack.org/515146
17:10:19 openstackgerrit Stephen Finucane proposed openstack/nova master: nova-status: Migrate to cliff https://review.openstack.org/515147
17:10:19 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Rename 'policy_check' -> 'policy' https://review.openstack.org/515148
17:10:20 openstackgerrit Stephen Finucane proposed openstack/nova master: nova-policy: Migrate to cliff https://review.openstack.org/515149
17:10:23 efried mriedem It only has to be hit once from one API. Like if they create a flavor or an image or whatever.
17:10:42 efried mriedem Which admins tend to do early on before they hand their cloud off to users, true?
17:10:58 efried mriedem So I guess we don't cover the case where they've got an existing cloud and they just decide to enable this thing.
17:11:58 mriedem i also wouldn't backport yours
17:12:14 mriedem because if i'm on stable and pick that up, and all of a sudden i start getting 500s, i'd be annoyed
17:14:29 efried mriedem Is backporting a consideration? The service user thing is experimental, right?
17:14:42 mriedem i don't consider it experimental
17:14:49 mriedem it's been in since ocata and we run with it enabled in our nova-next job
17:14:54 mriedem so yes i was going to backport
17:15:06 mriedem i've also marketed this feature during 2 summit project update talks
17:15:17 efried mriedem https://github.com/openstack/nova/blob/master/nova/conf/service_token.py#L43
17:15:27 efried So I guess we oughtta take that line out ^
17:15:37 jaypipes sean-k-mooney: that series touches both traits and shared resource providers
17:15:48 jaypipes sean-k-mooney: it's a combo of me, efried and gibi.
17:16:54 mriedem efried: yeah i'd be fine with that
17:16:58 mriedem i'd leave it disabled by default
17:21:17 openstackgerrit Eric Fried proposed openstack/nova master: Service token is not experimental https://review.openstack.org/521955
17:21:19 efried mriedem ^
17:23:13 mriedem wanna remove the 'this is disabled by default' line?
17:23:16 mriedem then i'm +2
17:23:58 dansmith artom_: good comments, thanks for those
17:24:10 dansmith mriedem: you might want to look at those before you push up a rev and see if you have opinions
17:24:14 openstackgerrit Stephen Finucane proposed openstack/nova master: conf: Remove deprecated 'multi_instance_display_name_template' opt https://review.openstack.org/499612
17:24:15 openstackgerrit Stephen Finucane proposed openstack/nova master: Simplify instance name generation https://review.openstack.org/516573
17:24:47 artom_ dansmith, yey, my brain works!
17:27:22 sean-k-mooney stephenfin: sorry had to step away for a minute. well what i was wondering is wether "is not none" is enough e.g. can self.cpu_pinning contain "shared"
17:29:46 mriedem efried: +2 on https://review.openstack.org/#/c/490057 - thanks for the quick updates
17:29:59 stephenfin sean-k-mooney: cpu_pinning contains a dict of host to guest CPU mappings
17:30:11 efried mriedem Thanks.
17:30:13 stephenfin You're thinking of 'cpu_policy', which would contain 'shared' or 'dedicated'
17:30:48 openstackgerrit Merged openstack/nova master: Merge flavor extensions controller code https://review.openstack.org/516104
17:33:49 sean-k-mooney stephenfin: yes i just noticed self.cpu_pinning is a proxy field for cpu_pinning_raw whic is a dict of integers presumable the vCPU to pCPU mappings
17:34:06 efried mriedem I dup'd my bug (https://bugs.launchpad.net/nova/+bug/1724689) to yours and abandoned my patch.
17:34:06 openstack Launchpad bug 1733642 in OpenStack Compute (nova) "duplicate for #1724689 AttributeError: 'NoneType' object has no attribute 'get_token'" [Medium,In progress] - Assigned to Matt Riedemann (mriedem)
17:34:42 mriedem efried: ok, i was going to dupe my bug against yours but ok :)
17:34:47 artom_ I feel like naming it "policy" is confusing us
17:35:21 artom_ As I writing an internal email about it, I came up with "qualitative" (what we're calling policy) vs "quantitative" (ie, resources)
17:35:29 artom_ Does that work better? (Or at all?)
17:36:36 openstackgerrit Eric Fried proposed openstack/nova master: Service token is not experimental https://review.openstack.org/521955
17:36:48 efried mriedem That's done ^
17:37:06 mriedem +2
17:38:14 efried sdague Service token-y stuff while you're in the mood: https://review.openstack.org/#/c/521947/ https://review.openstack.org/521955
17:40:20 dansmith artom_: I don't really think that has much more meaning to me, but I'm obviously biased
17:40:29 dansmith artom_: I'll let mriedem make the call on what the name should be
17:40:44 artom Ugh, it'll be some obscure 80s rock band
17:41:01 mriedem i agree it's confusing that _GroupAntiAffinityFilter is a policy filter but _GroupAffinityFilter isn't
17:41:01 dansmith fine by me :P
17:41:36 sdague efried: looking
17:41:49 mriedem i mean, we could just call the damn thing RUN_FOR_REBUILD but that doesn't help a ton with the reasoning behind which filters should be run during rebuild or not
17:42:14 dansmith and I expect we need to re-use much of the logic for some other things where we need to check with the scheduler
17:42:40 mriedem example?
17:42:41 dansmith although I guess resize does need to check the resourcey things
17:42:44 dansmith I dunno
17:42:51 dansmith feels far too targeted to just say it's for resize
17:42:52 mriedem i think rebuild is just the odd duck
17:43:12 dansmith well, if that's really the case, then maybe we should just be specific until we have a counterexample
17:43:12 mriedem resize runs through the scheduler because it's going to do a claim on the chosen host
17:43:15 dansmith yeah
17:43:25 mriedem live migration doesn't do a claim
17:43:40 mriedem but live migration does allocate on the dest host
17:43:42 mriedem using the same flavor
17:43:42 sdague efried: so, on https://review.openstack.org/#/c/490057/27/nova/api/openstack/compute/servers.py, while it's fine to pass context around, did you look into just pulling it from thread local storage?
17:44:05 sdague efried: https://github.com/openstack/oslo.context/blob/18aa6ec496e54b2403c2cc65234ef6447021fdee/oslo_context/context.py#L490-L495
17:44:07 efried sdague I didn't.
17:44:38 efried sdague tbh, mordred did the code-side work on that patch. I just did the test, and kept it up to date.
17:44:51 sdague ok, the long pass through of some of these variables gets a little spidery. It's probably fine here, but something to think about in the future
17:45:17 mriedem that's why i worried about something doing context.get_admin_context() and that eventually going through here
17:45:21 mriedem but apparently we don't do that with images
17:45:28 efried sdague Yeah, that would have made things a lot easier. Is that the same thing?
17:49:00 sdague mriedem: yeh, I could see that
17:49:20 sdague it's one of those things where context was written originally to not have to pass it around
17:49:29 sdague but that got forgotten at some point
17:49:38 sdague so, meh
17:50:05 sdague efried: +A on that patch, I'll look at the rest of the conversions on top once I get some lunch
17:50:24 efried sdague Thank you sir.

Earlier   Later