Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-04-09
16:25:25 openstackgerrit Sylvain Bauza proposed openstack/nova-specs master: Proposes NUMA affinity for vGPUs https://review.openstack.org/650963
16:25:46 bauzas stephenfin: thanks for reviewing https://review.openstack.org/#/c/650963/, just wrote PS2
16:33:46 tssurya mordred, mriedem, dansmith: after the back and forths on changing the type of the "locked" response key since all of you are for the seperate key style and no change on type I will just update the spec then to that approach.
16:34:02 dansmith tssurya: thanks
16:34:06 tssurya separate*
16:34:06 mordred ++
16:34:18 dansmith I was shocked to have mordred come in and agree with me
16:34:40 dansmith he might not have realized what he was doing though
16:39:32 dansmith stephenfin: melwitt bauzas mriedem: I didn't see any discussion about how stephenfin only created five placeholders for the main db this time, but I do think that's probably more than enough and would be good for the future
16:39:59 bauzas yeah, I just feel 5 is enough given the history
16:39:59 stephenfin dansmith: I _think_ I called it out in the commit message or a comment, so I figured everyone was ok with that
16:40:14 dansmith stephenfin: I didn't see it anywhere
16:40:14 stephenfin (did it because you mentioned that we'd never used them anyway)
16:40:19 mordred dansmith: wait. I change my mind. whatever the opposite things is please
16:40:30 dansmith mordred: :)
16:41:06 stephenfin dansmith: Yeah, it wasn't as obvious as it should have been but it's there alright https://review.openstack.org/#/c/650964/1/nova/tests/unit/db/test_migrations.py@183
16:41:24 dansmith ack, okay
16:44:11 mriedem he put it in a comment
16:44:17 mriedem oh yeah i'm late
16:44:52 dansmith I didn't read that deep since it was already merged, just saw the fewer file count
17:06:10 gmann dansmith: mriedem mordred can we remove 'locked' field in favor of 'locked_by' in https://review.openstack.org/#/c/638629/9/specs/train/approved/add-locked-reason.rst@198
17:06:40 dansmith I'd rather we didn't
17:06:43 mordred gmann: it would be nicer to not remove the field
17:06:44 gmann it will be change for old user so no strong opinion just a thought.
17:06:50 dansmith it's just senseless change for no real gain
17:06:51 gmann yeah
17:07:09 mriedem pretty sure that was discussed in some of the earlier patch sets
17:07:19 gmann ok.
17:09:39 bauzas stephenfin: I'm a terrible man, I left you a -1 on https://review.openstack.org/#/c/555081 after you left
17:10:07 bauzas but I feel it's worth discussing on the upgrade path
17:10:09 openstackgerrit Ghanshyam Mann proposed openstack/nova-specs master: Spec for API inconsistency cleanup https://review.openstack.org/603969
17:10:31 bauzas I'm in favor of just saying "heh dude, migrate your workloads first before touching anything or your compute will blow up"
17:10:52 gmann mriedem: mordred updated the API cleanup spec with only 3 cleanups to target. https://review.openstack.org/603969
17:11:17 dansmith bauzas: um, really?
17:11:38 dansmith bauzas: you want cern to have to migrate all their workloads before/during an upgrade?
17:11:49 bauzas dansmith: no, that's not what I meant
17:12:05 bauzas dansmith: I meant, before changing the opts
17:12:26 bauzas if you upgrade and just keep the same conf, you're not impacted
17:12:41 bauzas actually, I left another comment saying this
17:13:04 bauzas ie. keep existing behaviour in Train exactly like in Stein if untouched
17:13:12 dansmith okay let me re-read
17:13:36 bauzas dansmith: context being https://review.openstack.org/#/c/555081/22
17:13:54 dansmith right I know :)
17:14:13 bauzas https://review.openstack.org/#/c/555081/22/specs/train/approved/cpu-resources.rst@171 is the existing backwards compat concern I have => don't change anything
17:14:38 mriedem tssurya: gmann: i left some comments on the locked reason spec regarding filtering and sorting, but i'm not sure why locked is not a valid sort/filter parameter but locked_by is - even though we dont currently expose the latter
17:14:47 mordred gmann: all of those fields with OS-EXT prefixes are readonly aren't they?
17:14:49 bauzas https://review.openstack.org/#/c/555081/22/specs/train/approved/cpu-resources.rst@181 is the "what happens when you start playing with those new opts"
17:15:09 tssurya mriedem: I was just about to push :)
17:15:26 dansmith bauzas: okay, so what happens if you do make those changes on a compute node with instances?
17:15:28 bauzas oh man, I really thought the locked reason spec was not controversial :)
17:15:28 tssurya oh had just added filtering based on the new keys will be allowed
17:15:30 mordred gmann: in any case - we expose all of those without the prefix in sdk already - so I'm definitely in favor of that cleanup :)
17:16:02 mriedem tssurya: you can't filter on something that's stored in system_metadata
17:16:05 dansmith bauzas: as long as the same config means you don't have to move stuff that's okay, I thought you were kindof implying that they would have to move everything at some point though
17:16:06 bauzas dansmith: that's my point, in the spec, we say we reshape
17:16:18 bauzas when we start using those opts
17:16:22 tssurya bauzas: I thought so too! (when I initially proposed it)
17:16:30 mordred (although I think I need to add a way to express inside of sdk "this property comes from whichever of OS-EXT-AZ:availability_zone or availability_zone exists"
17:16:48 bauzas dansmith: nope, nope, keep same things in Train if config unchanged
17:16:56 dansmith bauzas: I would expect someone like CERN to really care about enabling this sort of thing and requiring everyone to move many gigs of data around just so we don't have to reshape is kinda icky
17:17:01 dansmith I mean, really icky
17:17:11 bauzas dansmith: I totally agree
17:17:21 tssurya mriedem: oh yea forgot ! I guess we discussed this too earlier in the spec when we were deciding where to put it
17:17:26 mriedem mordred: not necessarily, OS-DCF:diskConfig is on server create
17:17:40 gmann mriedem: they are all in response so did not get readonly thing completely
17:17:46 bauzas dansmith: that's why I'm in favor of a full service stop if operators start playing with new config opts in Train and have existing instances
17:17:58 mordred mriedem: ah - so it is. ok - that one will take a little extra effort - but shouldn't be too bad
17:18:04 mriedem gmann: OS-DCF:diskConfig is in server create and resize requests
17:18:28 mordred consuming whichever field name happens to be in the response is easy - knowing which field name one is supposed to send to the api is more effort
17:18:36 mordred but still doable and fine
17:19:03 mriedem i don't know that gmann was thinking about request params, and i just thought of OS-DCF:diskConfig since you saked
17:19:05 mriedem *asked
17:19:15 mriedem there could be others, it's a crusty api
17:19:19 gmann ok, user data is one case?
17:19:19 dansmith bauzas: but I'm saying I think that we should be able to provide support for them stopping a compute, tweaking the params to enable, and then restart... with instances.
17:19:40 mriedem user_data is not prefixed in the create request
17:19:40 mordred mriedem: I was mostly looking at the list in https://review.openstack.org/#/c/603969/10/specs/train/approved/api-consistency-cleanup.rst
17:19:49 bauzas dansmith: with a reshape ?
17:19:53 gmann OS-DCF:diskConfig will not be changed right
17:19:58 bauzas dansmith: sorry if I'm not getting you
17:20:01 dansmith bauzas: we can abort on startup if they have specified something that can't be reshaped, requiring them to move some stuff off before doing it, but requiring clearing every single compute node (over time) to turn that on really sucks
17:20:44 bauzas dansmith: okay so you're in favor of some automation
17:20:50 bauzas lemme reconsider that
17:20:52 mordred dansmith: just adopt cloud native design principles and stop writing files to disk. solves all your problems
17:21:15 dansmith mordred: heh
17:21:38 gmann mriedem: tssurya locked_by in valid sort/filter list was mistake. we can improve that in tssurya spec i think.
17:22:06 mriedem mistake because it's a field we don't even expose today?
17:22:11 tssurya gmann, mriedem: I will add locked" as a sort/filter key
17:22:36 mriedem i know at one point there was a comment in the code near these fields that said "don't use locked anymore" becaues locked_by implied locked
17:22:38 tssurya but as for the existing "locked_by" key in the whitelist, isn't that just a refactor of code ? since we don't expose it anyways ?
17:23:18 gmann mriedem: yes, we did not expose that and had in valid sort/fitler
17:23:34 mriedem tssurya: refactor of code?
17:23:45 gmann i tried to check the reason in original change but did not find any rational to add in whitelist
17:23:47 mriedem tssurya: as i mentioned in the spec, supporting filtering on locked_by could get weird,
17:24:11 tssurya mriedem: no I don't want to support locked_by based filtering as well
17:24:14 tssurya its useless IMO
17:24:18 mriedem becaues if we expose locked_by=admin or other, but only store admin or owner, then filtering on locked_by=other means we have to do something like "select * from instances where locked_by!='admin'"
17:24:38 dansmith filtering in the api on a non-db field isn't *super* terrible, but sorting is
17:24:47 gmann this one - https://review.openstack.org/#/c/408571/
17:24:57 bauzas dansmith: just to make sure https://review.openstack.org/#/c/555081/22/specs/train/approved/cpu-resources.rst@725 requires a reshape, right?

Earlier   Later