| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-18 | |||
| 10:09:35 | gibi | I can call both but I think apply is a superset of support | |
| 10:09:35 | sean-k-mooney | have not looked in a while | |
| 10:10:05 | gibi | support is basically _filter_pools, apply is _filter_pools + _decrease_pool_count | |
| 10:10:10 | sean-k-mooney | ya im just wondering about cost but i guess apply is already called for multi create | |
| 10:10:21 | gibi | apply is called at the end yes | |
| 10:10:25 | gibi | anyhow | |
| 10:10:37 | sean-k-mooney | ack so i guess that is fine | |
| 10:11:02 | sean-k-mooney | will apply decresase the pools if they all dont fit | |
| 10:11:20 | sean-k-mooney | i.e. can it handel partial cases | |
| 10:11:45 | sean-k-mooney | im just wonderign do we need to do an atomic swap | |
| 10:12:10 | gibi | I can call apply on a local copy of stats | |
| 10:12:14 | gibi | then drop the copy | |
| 10:12:23 | sean-k-mooney | ack | |
| 10:12:24 | gibi | so no interference between parallel requests | |
| 10:12:42 | sean-k-mooney | well i was thinking swap the copy with the orginal if it succeeeds | |
| 10:12:51 | sean-k-mooney | so that multi create works | |
| 10:13:16 | sean-k-mooney | althogh | |
| 10:13:18 | sean-k-mooney | maybe not | |
| 10:13:29 | sean-k-mooney | droping it might be better to not break the numa toplogy filters | |
| 10:13:33 | gibi | we have two phases for multicreate 1) running filters (this will apply on a copy) 2) consume the selected host (this already apply on a shared stats) | |
| 10:13:57 | sean-k-mooney | right we will need to do apply 3 times | |
| 10:14:08 | sean-k-mooney | pci filter, numa toplogy filter and host manager | |
| 10:14:16 | gibi | yes | |
| 10:14:20 | sean-k-mooney | the first two should be copies to not break each other | |
| 10:14:23 | gibi | yes | |
| 10:14:34 | gibi | so I will keep the support call to do the apply on a copy | |
| 10:14:49 | gibi | and keep the apply work on the shared stats | |
| 10:14:56 | sean-k-mooney | you could make supports call apply internally on a copy | |
| 10:15:23 | gibi | but then I need to signal to apply when to copy and when not to copy | |
| 10:15:47 | sean-k-mooney | i have not looked at the signiture but do we pass in the pools to apply | |
| 10:15:55 | sean-k-mooney | or does it get them itself | |
| 10:16:03 | sean-k-mooney | i was assumign we passed them in | |
| 10:16:11 | sean-k-mooney | so supports could do the copy for it | |
| 10:17:02 | sean-k-mooney | _apply takes the pools https://github.com/openstack/nova/blob/master/nova/pci/stats.py#L622-L653 | |
| 10:17:48 | sean-k-mooney | its up to you which you think is cleaner | |
| 10:17:50 | gibi | yes, but apply_requests doesnt | |
| 10:17:58 | gibi | anyhow I will push the code soon | |
| 10:17:59 | sean-k-mooney | let me know when its ready to review | |
| 10:18:02 | gibi | and we can look at that | |
| 10:18:03 | sean-k-mooney | cool | |
| 10:18:09 | gibi | thanks for the discussion | |
| 10:24:40 | sean-k-mooney | im going to try an power through and finish the vdpa patches today just an fyi | |
| 10:24:48 | sean-k-mooney | its mainly just tests and docs at this point | |
| 10:24:50 | opendevreview | efineshi proposed openstack/python-novaclient master: Fix nova host-evacuate won't work with hostname is like a.b.c https://review.opendev.org/c/openstack/python-novaclient/+/853465 | |
| 10:25:15 | sean-k-mooney | althouhg i have one downstream bug to look at first... | |
| 10:27:07 | gibi | sean-k-mooney: sure I can review the rest of the vdpa series when it is ready | |
| 10:29:29 | opendevreview | Balazs Gibizer proposed openstack/nova master: Trigger reschedule if PCI consumption fail on compute https://review.opendev.org/c/openstack/nova/+/853611 | |
| 10:35:24 | opendevreview | Balazs Gibizer proposed openstack/nova master: Trigger reschedule if PCI consumption fail on compute https://review.opendev.org/c/openstack/nova/+/853611 | |
| 12:31:16 | opendevreview | Merged openstack/nova master: imagebackend: default by_name image_type to config correctly https://review.opendev.org/c/openstack/nova/+/826526 | |
| 12:31:23 | opendevreview | Merged openstack/nova master: image_meta: Add ephemeral encryption properties https://review.opendev.org/c/openstack/nova/+/760454 | |
| 12:31:31 | opendevreview | Merged openstack/nova master: BlockDeviceMapping: Add encryption fields https://review.opendev.org/c/openstack/nova/+/760453 | |
| 12:31:39 | opendevreview | Merged openstack/nova master: BlockDeviceMapping: Add is_local property https://review.opendev.org/c/openstack/nova/+/764485 | |
| 12:31:47 | opendevreview | Merged openstack/nova master: compute: Update bdms with ephemeral encryption details when requested https://review.opendev.org/c/openstack/nova/+/764486 | |
| 12:47:18 | opendevreview | sean mooney proposed openstack/nova master: add sorce dev parsing for vdpa interfaces https://review.opendev.org/c/openstack/nova/+/841016 | |
| 12:52:10 | opendevreview | sean mooney proposed openstack/nova master: add sorce dev parsing for vdpa interfaces https://review.opendev.org/c/openstack/nova/+/841016 | |
| 13:49:52 | dansmith | gibi: can you look at my reply here real quick? https://review.opendev.org/c/openstack/nova/+/852900 | |
| 13:50:13 | dansmith | if you agree that I need to chase down the tests. that fail because of shared state, I'll give that a shot | |
| 13:50:33 | gibi | dansmith: sure I will look in 5 | |
| 13:50:35 | dansmith | but if you have some other idea about why that might be, I'll be glad to have it before getting into that rabbit hole :) | |
| 13:50:37 | dansmith | thx | |
| 13:53:15 | gibi | OK I need to run it locally to see the issues | |
| 13:55:12 | dansmith | the libvirt reshape test is the one I remember | |
| 13:58:35 | dansmith | hmm, maybe resetting the client during restart_compute_service is all I need | |
| 13:58:46 | dansmith | I need to run a full set now to see if the reshape ones are the only ones | |
| 14:00:13 | dansmith | what I really wanted to say in that comment was something like "this compute service stack is where we use a client with all the complex internal state and thus maybe we shouldn't share that with anything else that isn't part of this set of objects" | |
| 14:00:23 | dansmith | which is maybe still a good idea, I dunno | |
| 14:00:43 | dansmith | but re-using the same init code has the benefit of the similar error messages and things | |
| 14:02:58 | gibi | hm restart_compute_service could will trigger a creation of a new ComputeManager instance | |
| 14:03:05 | gibi | -could | |
| 14:03:51 | gibi | so you are right that behaves differently in the test where the two ComputeManager will share report client state, from the reality where a compute restart will reset the client | |
| 14:03:58 | gibi | s/reset/recreate/ | |
| 14:04:26 | dansmith | yeah, so I put a call there to reset the global state and it passed the few reshape tests I had in my history | |
| 14:04:29 | dansmith | running the full set now | |
| 14:05:32 | gibi | buut, in func test we run all of our computes in the same process, so now they will share the same report client across compute hosts | |
| 14:06:09 | dansmith | will that work because of the multi-root functionality you mentioned? | |
| 14:06:23 | sean-k-mooney | i kind of feel like we shoudl seperate the singelton changes form the lazy loading | |
| 14:06:41 | sean-k-mooney | if you have not already done that | |
| 14:06:50 | dansmith | but as I said above, I'm also fine keeping them separate for the compute manager part if you think that's better | |
| 14:06:52 | sean-k-mooney | just so there is less change to condier | |
| 14:06:54 | gibi | dansmith: I'm not sure about the scope of the multi root functionality | |
| 14:07:12 | dansmith | and I'll try to write something more concise than my verbose sentence above, but more useful than the one I put in there that caused the confusion | |
| 14:07:49 | dansmith | sean-k-mooney: they're already together, and can't be separated without losing some of the behavior that gibi wanted to keep | |
| 14:07:50 | gibi | I think in short term keep a separate client per ComputeManager instance and note why we are doing it. | |
| 14:08:05 | dansmith | gibi: ack, sounds good | |
| 14:08:40 | gibi | I dont like the global but I checked and in most cases it is safe based on how we use it | |
| 14:08:52 | gibi | the compute manager is an exception | |
| 14:09:14 | gibi | at least due to the func test, but also might be due to ironic multiple node per compute | |
| 14:09:52 | dansmith | the global results in fewer hits to keystone and also fewer places we could fail if a call to keystone fails, and mirrors our other client behaviors | |
| 14:09:59 | dansmith | (the internal state does not, of course, but...) | |
| 14:10:16 | gibi | yeah I accept the compromise | |
| 14:10:24 | gibi | the global has pros and cons | |
| 14:10:35 | dansmith | if you really hate the global I can switch it to per-use lazy load | |
| 14:10:48 | dansmith | but aside from the state thing, I don't know why we would perfer that | |
| 14:11:02 | gibi | yeah, only the share state that makes it complicated. | |
| 14:11:05 | dansmith | (or prefer even) | |
| 14:11:45 | dansmith | ack, so we could also make each call to get the singleton generate a new local state object so that that part is not shared everywhere | |
| 14:11:57 | dansmith | I could leave a node in there with the idea in case it becomes problematic in the future | |
| 14:12:05 | gibi | if I had time I would also trim the report client to have only those methods there that depend on the shared state and move the independent functions somehere else to make it clear what is problematic | |
| 14:12:30 | dansmith | ack, there are several things we *could* do to make this cleaner for sure | |
| 14:12:49 | gibi | the extra note in the singletone works for me | |