Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-23
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
17:42:59 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Remove legacy '_from_dict' functions https://review.opendev.org/537414
17:43:00 openstackgerrit Stephen Finucane proposed openstack/nova master: claims: Remove useless caching https://review.opendev.org/671791
17:43:00 openstackgerrit Stephen Finucane proposed openstack/nova master: Add '[compute] cpu_dedicated_set' option https://review.opendev.org/671792
17:43:01 openstackgerrit Stephen Finucane proposed openstack/nova master: libvirt: Start reporting PCPU inventory to placement https://review.opendev.org/671793
17:43:02 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Rename exception argument https://review.opendev.org/671795
17:43:02 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Remove unused function parameter https://review.opendev.org/671796
17:43:03 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove 'hardware.get_host_numa_usage_from_instance' https://review.opendev.org/671797
17:43:04 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove 'hardware.host_topology_and_format_from_host' https://review.opendev.org/671798
17:43:04 openstackgerrit Stephen Finucane proposed openstack/nova master: Remove 'hardware.instance_topology_from_instance' https://review.opendev.org/671799
17:43:05 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: hardware: Differentiate between shared and dedicated CPUs https://review.opendev.org/671800
17:43:05 openstackgerrit Stephen Finucane proposed openstack/nova master: Add support translating CPU policy extra specs, image meta https://review.opendev.org/671801
17:43:06 openstackgerrit Stephen Finucane proposed openstack/nova master: tests: Split NUMA object tests https://review.opendev.org/672336
17:44:15 mriedem efried: they were written independenty and artom had a bug i pointed out in his change which prompted me to write the functional test since the unit tests were just asserting the broken mock call,
17:44:27 mriedem he rebased on top of the functional test to assert that it would hit the issue in his patch,
17:44:35 efried okay
17:44:50 mriedem regardless of his patch, i think it's a useful functional test for that init host restart code since we don't test that stuff in gate runs and unit tests for that kind of code kind of suck....

Earlier   Later