Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-23
14:42:06 dansmith and your comment on the config option
14:43:04 stephenfin coolness
14:48:54 aspiers sean-k-mooney: I'm about to review your https://review.opendev.org/#/c/670189/ - thanks for working on that
14:49:52 openstackgerrit Dan Smith proposed openstack/nova master: Remove deprecated Core/Ram/DiskFilter https://review.opendev.org/672065
14:49:52 openstackgerrit Dan Smith proposed openstack/nova master: Remove deprecated CPU, RAM, disk claiming in resource tracker https://review.opendev.org/551026
14:49:53 openstackgerrit Dan Smith proposed openstack/nova master: Remove 'nova.virt.driver.ComputeDriver.estimate_instance_overhead' https://review.opendev.org/672106
14:50:59 sean-k-mooney aspiers: nice to see you around again. and i need it for my own stuff too so its not entily un selfish :)
14:56:27 aspiers sean-k-mooney: :)
14:56:44 aspiers sean-k-mooney: I am not sure why I thought that defaulting to q35 would be OK :-/
15:02:17 mriedem ralonsoh: jangutter: efried: stephenfin: i'm late but have some concerns in https://review.opendev.org/#/c/641670/ that should be addressed
15:03:04 efried mriedem: should I unapprove the bp?
15:03:19 mriedem efried: probably not right now
15:03:23 efried k
15:03:58 efried mriedem: if you're in a spec reviewing mood, this one could use some love https://review.opendev.org/#/c/608696/
15:04:18 ralonsoh mriedem, I'll check your comments today
15:05:22 mriedem efried: i'm not really
15:05:27 mriedem i just saw that one was merged and short
15:10:31 openstackgerrit sean mooney proposed openstack/nova master: libvirt: delegate ovs plug to os-vif https://review.opendev.org/602432
15:14:31 stephenfin efried: It looks like we've gone from zero to two implementations of the cpu-resources spec in less than a week \o/
15:15:14 stephenfin I'm working on comparing and likely merging the two, but I'd like your thoughts on this particular difference of implementation when you can https://review.opendev.org/#/c/672223/ and https://review.opendev.org/#/c/671801/
15:16:03 stephenfin They're the two patches to convert legacy 'hw:cpu_(thread_)policy' extra specs to PCPU requests and they've taken very different approaches. I can't decide which one I prefer
15:20:33 stephenfin TIL '(x or y) in foo' is a valid Python statement
15:30:19 stephenfin Actually, it does (thank ralonsoh)
15:30:23 stephenfin >>> (0 or None) in [0]
15:30:24 stephenfin False
15:33:26 openstackgerrit Matt Riedemann proposed openstack/nova master: Convert nova-next to a zuul v3 job https://review.opendev.org/670196
15:38:33 mriedem is there a follow up needed for surya's spec here https://review.opendev.org/#/c/636132/ ? before digging into the code series i was going to go over the spec again but don't really want to if it's a mess that needs an update.
15:44:21 stephenfin sean-k-mooney: Question: do we want to allow users to request 'resources:VCPU' or 'resources:PCPU' in their flavor extra specs?
15:44:50 stephenfin as opposed to 'hw:cpu_policy=dedicated' and whatever the new mask option to allow instances with both types of CPU ends up being
15:44:59 stephenfin or in addition to, I should say
15:46:33 openstackgerrit Merged openstack/nova master: Translatable output strings in heal allocation https://review.opendev.org/668925
15:59:21 efried stephenfin: you may be expecting too much of me wrt those PCPU impls, but I'll do what I can.
15:59:42 stephenfin That's all I can ask (y)
16:00:08 stephenfin They both "work", I'm more interested in which approach is preferable
16:01:13 efried stephenfin: also, to answer "do we want to allow users to request 'resources:VCPU' or 'resources:PCPU' in their flavor extra specs?" -- I think we need to *allow* it, because downstream thingies are going to want to override the meanings of the RCs for things like frequency/priority (per ptg direction) --
16:01:13 efried but we should definitely *support* (and encourage as the primary usage) flavor-ese lingo translated by request_filter to placement-ese.
16:02:00 stephenfin Aye, so allow it unless the 'hw:cpu_policy' extra spec/image meta prop is present
16:02:05 stephenfin In which case we spit the dummy out?
16:18:09 mriedem efried: dansmith: melwitt: you were all +2 on the spec for https://review.opendev.org/#/c/645611/ so i might be off base on my comments,
16:18:19 mriedem nothing major but i'm not sure why the driver even needs to implement this
16:18:54 dansmith mriedem: what do you mean?
16:19:12 dansmith why we're plumbing this to the driver instead of just telling the compute about the change?
16:20:29 mriedem all the virt driver does is set the values on the instance and save them to the db,
16:20:38 mriedem which is what stop/start_instance in the compute manager does for normal stop/start,
16:20:52 mriedem so why not just do the db updates in the compute manager code rather than muck with driver plumbing
16:20:56 mriedem just seems weird to me,
16:21:08 mriedem if it's "we don't want non-ironic drivers to do this" then add a driver capability flag
16:21:12 dansmith mriedem: compute start/stop will call the driver and tell it what to do right?
16:21:35 mriedem to actually power on / off a guest yeah
16:21:37 mriedem this is just db muckery
16:21:45 dansmith the point of this is to decide what needs to happen when some external change happens to the instance, just like how libvirt gets direct events
16:22:00 dansmith the point being, ironic needs to know this should be swallowed and the db updated,
16:22:03 mriedem none of that logic is in the actual driver
16:22:10 dansmith but that's not necessarily what should happen for vmware
16:22:29 openstackgerrit Merged openstack/nova master: Update api-ref location https://review.opendev.org/672077
16:22:39 mriedem ok i guess "it's just a simple abstraction" is the answer then
16:22:57 dansmith remember the first version of this *did* just call start/top,
16:23:00 mriedem i don't expect any other drivers to ever implement this, but i guess it's generic enough that they could
16:23:08 dansmith but had a "don't actually call the virt driver=True" if we're ironic
16:23:28 dansmith so the change to delgate "what should I do with this external change?" to the driver was to make that cleaner
16:23:33 mriedem yeah i agree that was gross
16:23:46 dansmith didn't we discuss all this during spec review?
16:23:50 dansmith I thought you were involved with that
16:23:54 mriedem not really
16:24:07 mriedem that's why i asked earlier in here if the spec was going to be updated so i could sanely read it before reviewing the code
16:24:13 mriedem but i just dove in anyway
16:24:35 dansmith okay you did review it at one point, but I see not in the later revs
16:24:39 mriedem anyway, that's fine when you explain it that way
16:24:45 dansmith okay
16:45:37 efried mriedem, dansmith: what's your take on the race condition & lock? Add it to the code, or take it out of the spec?
16:47:07 mriedem lock....
16:47:17 mriedem the api is validating the vm/task/power state right?
16:47:21 mriedem and the compute manager is making the change
16:47:28 efried mriedem: The concern I raised top-level, which was apparently also discussed in the spec.
16:47:58 efried that we could start the power sync before the event hits
16:48:51 mriedem if the sync task runs it would call api.stop() which would set the task_state and then the event should be ignored,
16:49:07 mriedem but there could be a window where we passed the event from the api to compute before compute calls api.stop
16:49:29 mriedem the task_state is generally our lock for stuff, i.e. the task state ignores instances with a non-None task_state
16:49:49 mriedem i'm not sure how you're going to impose a lock otherwise, unless you lock on the instance uuid in the compute manager or something
16:50:28 mriedem note that stop_instance in compute manager already locks on instance uuid
16:50:30 efried I'm okay accepting the risk, but it should probably be noted in a code comment.
16:54:38 openstackgerrit dzogchen proposed openstack/nova master: Nova: node should be deleted when nova-compute is removed https://review.opendev.org/671731
16:56:00 mriedem efried: i left a comment in the code,
16:56:20 mriedem i *think* if we lose a race somewhere, chances are one of the threads is going to fail with UnexpectedTaskStateError
16:56:34 efried thanks
16:56:48 mriedem which is maybe ok - worst case is the node gets powered off when it shouldn't be, which is the original bug she's trying to fix right?
16:57:03 mriedem some lab monkey powered on the box when nova thinks it's off
16:57:11 mriedem so nova's task shuts it off
16:57:50 mriedem fwiw i think we also have races with the existing sync task and the libvirt lifecycle event callback stuff which is why we have a workaround option to ignore those
16:58:10 mriedem https://docs.openstack.org/nova/latest/configuration/config.html#workarounds.handle_virt_lifecycle_events
16:59:04 efried right, so like, fixing that bug is the whole reason this patch exists. So if we're leaving that bug open -- even if we're drastically reducing the probability -- we need to own that somehow.
17:05:41 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove super old unnecessary TODO from API start() method https://review.opendev.org/672330
17:32:49 sean-k-mooney lyarwood: mriedem: can ye add this depercation warning backport to your review queue https://review.opendev.org/#/c/671532/
17:39:48 mriedem sean-k-mooney: done
17:40:31 mriedem note that issue is also in cinder
17:40:34 mriedem i'll add cinder to the bug
17:42:52 efried artom, mriedem: Why is https://review.opendev.org/#/c/668631/ (pass migration to finish_revert_migration) on top of https://review.opendev.org/#/c/670393/ (func test for crash of same)?
17:42:52 efried Are we making sure the former doesn't change any of the (broken) behavior demonstrated by the latter?
17:42:58 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Rename 'nova.objects.instance_numa_topology' https://review.opendev.org/671789
17:42:58 openstackgerrit Stephen Finucane proposed openstack/nova master: libvirt: Remove unnecessary try-catch around 'getCPUMap' https://review.opendev.org/671790

Earlier   Later