)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b90edd2ed3f228d392927ed27b66d3c34abccfb0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"65bb7957_6cb4ef29","updated":"2025-11-20 17:01:09.000000000","message":"I appreciate you creating this to appease me, but it doesn\u0027t seem like this proposes enough extra detail to really help prevent the sort of confusion that we apparently have.\n\nIf nobody else is interested in either changing the way things work or documenting what behavior belongs in each of the two init methods (and why), then feel free to just abandon this and forget I asked.","commit_id":"24469e371968962f030e050a6f26f3baf65534f3"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"37b399ea816fb301cdd7b63c07a952aba9d2becd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a1a3b679_7106cf5a","in_reply_to":"11a027a3_4a7f08ff","updated":"2025-12-01 15:08:52.000000000","message":"\u003e I\u0027m only opposing changing how things work without proper planning, during a bugfix. Especially as the change feels complicated to me. But I\u0027ve never opposed to making a doc patch. I\u0027m now confused why you are suggesting abandoning this doc patch.\n\nBecause it was adding nothing but a couple of redundant sentences that seemed like a diversion instead of an attempt to define a useful justification/explanation for why we have the split and what the rules are. If we weren\u0027t going to do anything more than that (it was already +2d), then I saw no point. It\u0027s shaping up to be more useful now, though, which I appreciate.","commit_id":"24469e371968962f030e050a6f26f3baf65534f3"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e3424ae9f420036a54f1829fd111655fed1be7d8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"11a027a3_4a7f08ff","in_reply_to":"65bb7957_6cb4ef29","updated":"2025-12-01 12:06:06.000000000","message":"\u003e  it doesn\u0027t seem like this proposes enough extra detail to really help prevent the sort of confusion that we apparently have\n\nThanks for the feedback. I expanded the docs text. Hope this better now.\n\n\u003e If nobody else is interested in either changing the way things work or documenting what behavior belongs in each of the two init methods (and why), then feel free to just abandon this and forget I asked.\n\nI\u0027m only opposing changing how things work without proper planning, during a bugfix. Especially as the change feels complicated to me. But I\u0027ve never opposed to making a doc patch. I\u0027m now confused why you are suggesting abandoning this doc patch.","commit_id":"24469e371968962f030e050a6f26f3baf65534f3"}],"nova/virt/driver.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b90edd2ed3f228d392927ed27b66d3c34abccfb0","unresolved":true,"context_lines":[{"line_number":358,"context_line":""},{"line_number":359,"context_line":"        Note that the virt driver is not fully initialized until init_host()"},{"line_number":360,"context_line":"        is called."},{"line_number":361,"context_line":"        \"\"\""},{"line_number":362,"context_line":"        self.virtapi \u003d virtapi"},{"line_number":363,"context_line":"        self._compute_event_callback \u003d None"},{"line_number":364,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9a27c6f3_49d185bf","line":361,"updated":"2025-11-20 17:01:09.000000000","message":"I think you previously asserted that you expect that a caller still can\u0027t make any other calls to the virt driver until after `init_host()` right? Like, the only thing you can do with the virt driver after instantiating it is call `init_host()`? If so, I think we need to say that here. \"fully initialized\" could mean at lot of things and also leaves a lot of ambiguity over what partial initialization means.\n\nAlso, I was hoping to get a little more definition here of _why_ we need this split so that other drivers know what goes where. To me, the version checking being in `init_host()` seems wrong, because that happens after we\u0027ve created a service record in the database and have written things to disk. What\u0027s the purpose/benefit of doing that stuff so late?\n\nAnd are there things we don\u0027t want virt drivers to do in `__init__()` that are okay in `init_host()`? No database calls in the latter maybe? The way I read this and the below docstring, I almost wonder if the driver isn\u0027t supposed to connect to the hypervisor at all until the latter, which definitely doesn\u0027t match the current state of things.","commit_id":"24469e371968962f030e050a6f26f3baf65534f3"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e3424ae9f420036a54f1829fd111655fed1be7d8","unresolved":true,"context_lines":[{"line_number":358,"context_line":""},{"line_number":359,"context_line":"        Note that the virt driver is not fully initialized until init_host()"},{"line_number":360,"context_line":"        is called."},{"line_number":361,"context_line":"        \"\"\""},{"line_number":362,"context_line":"        self.virtapi \u003d virtapi"},{"line_number":363,"context_line":"        self._compute_event_callback \u003d None"},{"line_number":364,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3501e619_c698d5c9","line":361,"in_reply_to":"9a27c6f3_49d185bf","updated":"2025-12-01 12:06:06.000000000","message":"\u003e I think you previously asserted that you expect that a caller still can\u0027t make any other calls to the virt driver until after ``init_host()`` right? Like, the only thing you can do with the virt driver after instantiating it is call ``init_host()``? If so, I think we need to say that here. \"fully initialized\" could mean at lot of things and also leaves a lot of ambiguity over what partial initialization means.\n\n\u003e Also, I was hoping to get a little more definition here of why we need this split so that other drivers know what goes where.\n\nI expanded on what \"fully initialized\" means. \n\nBased on how the compute manager uses the driver and how the driver support callbacks to the compute manager, it turns out that the main reason of the two steps init is to allow the compute manager not to miss those callbacks. Callbacks can happen right after the driver connects to the hypervisor as some of those callbacks are initiated based on events from the hypervisor. So having a separate ``__init__`` and ``init_host`` calls allow the manager to register / initialize the handlers for those callbacks. This implies that if the driver are sending callbacks then it should only start sending them after ``driver.init_host()`` is called otherwise the manager will miss callbacks. In case the libvirt driver it also means that the libvirt driver should not connect to the hypervisor before ``init_host``.\n\n\u003e To me, the version checking being in ``init_host()`` seems wrong, because that happens after we\u0027ve created a service record in the database and have written things to disk. What\u0027s the purpose/benefit of doing that stuff so late?\n\nThe version checking in the libvirt driver is implemented by asking from libvirt about its version. So it needs a hypervisor connection. Therefore the compute manager needs to be set up already to be able to handle libvirt events as those starts flowing when the connection to the hypervisor is opened. \n\n\u003e And are there things we don\u0027t want virt drivers to do in ``__init__()`` that are okay in ``init_host()``? No database calls in the latter maybe? The way I read this and the below docstring, I almost wonder if the driver isn\u0027t supposed to connect to the hypervisor at all until the latter, which definitely doesn\u0027t match the current state of things.\n\nBetween driver ``__init__`` and ``init_host`` the driver should not try to send events to the event handler in compute manager as it is not registered yet so event would be missed, and probably should not try to call virtapi methods as those might eventually call back to the driver that did not executed ``init_host`` yet.\n\n\u003e I almost wonder if the driver isn\u0027t supposed to connect to the hypervisor at all until the latter, which definitely doesn\u0027t match the current state of things.\n\nThose drivers that emit events should make sure that events are not sent to the manager until after ``driver.init_host()`` is called. For libvirt driver it is ensured by not connecting to the hypervisor until ``driver.init_host()``. I\u0027m not sure if other virt drivers has such event handling at all and therefore might not be limited to connect only in init_host.\n\n----\n\nI\u0027m not saying there could not be other ways to handle all this manager - driver interactions. But also I don\u0027t think that it is safe or easy to refactor all the init_host logic.","commit_id":"24469e371968962f030e050a6f26f3baf65534f3"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"37b399ea816fb301cdd7b63c07a952aba9d2becd","unresolved":true,"context_lines":[{"line_number":358,"context_line":""},{"line_number":359,"context_line":"        Note that the virt driver is not fully initialized until init_host()"},{"line_number":360,"context_line":"        is called. Only a limited set of function can be called on the driver"},{"line_number":361,"context_line":"        before init_host(). Calls that result in virtapi or event callbacks"},{"line_number":362,"context_line":"        is not allowed before init_host() is called."},{"line_number":363,"context_line":""},{"line_number":364,"context_line":"        This potentially means that a virt driver (like libvirt) that starts"}],"source_content_type":"text/x-python","patch_set":2,"id":"4b02bfdb_5709bc88","line":361,"range":{"start_line":361,"start_character":0,"end_line":361,"end_character":75},"updated":"2025-12-01 15:08:52.000000000","message":"Does this mean libvirt API or _our_ ComputeVirtAPI? When I see \"virtapi\" I think the latter, FWIW. I think we should be clearer if possible.","commit_id":"72197f368bc3d7ebbd6435c05fb7c1c3165e6d43"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c30ddf5a6a2b330c02b7e13f1cd94b37b50daf6f","unresolved":true,"context_lines":[{"line_number":358,"context_line":""},{"line_number":359,"context_line":"        Note that the virt driver is not fully initialized until init_host()"},{"line_number":360,"context_line":"        is called. Only a limited set of function can be called on the driver"},{"line_number":361,"context_line":"        before init_host(). Calls that result in virtapi or event callbacks"},{"line_number":362,"context_line":"        is not allowed before init_host() is called."},{"line_number":363,"context_line":""},{"line_number":364,"context_line":"        This potentially means that a virt driver (like libvirt) that starts"}],"source_content_type":"text/x-python","patch_set":2,"id":"c29f16d0_1518f79c","line":361,"range":{"start_line":361,"start_character":0,"end_line":361,"end_character":75},"in_reply_to":"4b02bfdb_5709bc88","updated":"2025-12-05 12:51:58.000000000","message":"I referring to the argument named virtapi. I will clarify.","commit_id":"72197f368bc3d7ebbd6435c05fb7c1c3165e6d43"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"37b399ea816fb301cdd7b63c07a952aba9d2becd","unresolved":true,"context_lines":[{"line_number":372,"context_line":"        2. Then register event handlers via driver.register_event_listener()"},{"line_number":373,"context_line":"        3. And finish the initialization of the virtapi instance (e.g. if it"},{"line_number":374,"context_line":"           needs a driver instance to function)"},{"line_number":375,"context_line":"        4. Finally call driver.init_host()"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"        In return the client can expect that no events will be missed and"},{"line_number":378,"context_line":"        no virtapi calls like update_compute_provider_status() is made before"}],"source_content_type":"text/x-python","patch_set":2,"id":"21ef3440_e8886fd6","line":375,"updated":"2025-12-01 15:08:52.000000000","message":"AFAICT, we\u0027re calling `init_host()` before we call `register_event_handler()`, aren\u0027t we? Your use of \"client\" here is confusing me, but I assume you mean \"caller\" as in \"the compute manager\".\n\nHere, we call `driver.init_host()`:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1663\n\nThen later we call `self.init_virt_events()`:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1690\u0027\n\n...which is what calls `driver.register_event_listener()`:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1494\n\n(side note: we\u0027re doing that with a default-true workaround flag...wtf?)\n\nSo I\u0027m not sure that I see that the above sequence is correct, unless I\u0027m missing something. That said, perhaps the InstanceEvents is reason for the split.. anything that does something and waits for an InstanceEvent would require going through a partially-initialized compute manager object, since those come in via RPC and get dispatched to the waiter. But I don\u0027t think that\u0027s what you\u0027re talking about here and also definitely wouldn\u0027t be affected by doing things against `self.driver._host`.\n\nI\u0027m also wondering if we actually need to be connected to the Host/libvirt before we register the libvirt event handler? The comment now in `Host.initialize` mentions we need to register it \"before it is used for the first time\" but that seems a big risky (as we just saw). Are we not able to register that ahead of time (say, in `driver.__init__` where/before we create `self._host`? It seems like all the work it does is independent of `self._host` and that having it always setup would make this a lot less fragile.","commit_id":"72197f368bc3d7ebbd6435c05fb7c1c3165e6d43"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c30ddf5a6a2b330c02b7e13f1cd94b37b50daf6f","unresolved":true,"context_lines":[{"line_number":372,"context_line":"        2. Then register event handlers via driver.register_event_listener()"},{"line_number":373,"context_line":"        3. And finish the initialization of the virtapi instance (e.g. if it"},{"line_number":374,"context_line":"           needs a driver instance to function)"},{"line_number":375,"context_line":"        4. Finally call driver.init_host()"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"        In return the client can expect that no events will be missed and"},{"line_number":378,"context_line":"        no virtapi calls like update_compute_provider_status() is made before"}],"source_content_type":"text/x-python","patch_set":2,"id":"909c1216_955275a5","line":375,"in_reply_to":"21ef3440_e8886fd6","updated":"2025-12-05 12:51:58.000000000","message":"\u003e AFAICT, we\u0027re calling init_host() before we call register_event_handler(), aren\u0027t we? Your use of \"client\" here is confusing me, but I assume you mean \"caller\" as in \"the compute manager\".\n\nYou are correct. That is yet another bug probably. When we connect to libvirt via init_host we start getting lifecycle events from libvirt. If we don\u0027t have an event handler registered those events are dropped by the libvirt driver. So we will miss VM shutdown event this way. Now with eventlet, probably manager.init_host run to competion before any libvirt event is picked up from the queue by the event dispatcher greenlet in the libvirt driver, so in reality with eventlet we are not missing such events due to run to completion semantics allows manager.init_host to also register the manager level handler functions. With native threading we don\u0027t have such protections\n\n\u003e (side note: we\u0027re doing that with a default-true workaround flag...wtf?)\n\nA 10 year old race condition mitigation move as per \nhttps://github.com/openstack/nova/commit/d09785b97a282e8538642f6f8bcdd8491197ed74\n\n\u003e I\u0027m also wondering if we actually need to be connected to the Host/libvirt before we register the libvirt event handler? The comment now in Host.initialize mentions we need to register it \"before it is used for the first time\" but that seems a big risky (as we just saw). Are we not able to register that ahead of time (say, in driver.__init__ where/before we create self._host? It seems like all the work it does is independent of self._host and that having it always setup would make this a lot less fragile.\n\nAs far as I understand today:\n* we need to add a manager level event handler before we call driver.init_host otherwise we potentially miss events that the libvirt driver gets from libvirt as soon as the libvirt driver connects to libvirt.\n* the libvirt driver needs to register event handlers and start an event poller native thread before connects to libvirt otherwise libvirt logs that it cannot initialize the event handling and will never deliver events for this connection.\n\nTechnically it would be possible to change the ``driver.__init__`` to not just take virtapi but also take the manager level event handler callback(s). And then change the implementation of the libvirt (and other virt drivers) ``driver.__init__`` to store the manager level handlers, register the libvirt level handlers, and start the poller thread, and then connect to libvirt. This way the libvirt driver will be fully usable right after the ``driver.__init__`` way before ``driver.init_host``. However it requires a signature change here in the interface and probably has a big unknown fallout radius that makes me nerves.\n\nIs this something you would like to see?","commit_id":"72197f368bc3d7ebbd6435c05fb7c1c3165e6d43"}]}