)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"997a83fdd38a5d48afb442ca8bc75b44db0f6068","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Two use cases have emerged semi-recently which both require the"},{"line_number":10,"context_line":"libvirt driver to be able to invoke libvirt\u0027s"},{"line_number":11,"context_line":"virConnectGetDomainCapabilities() API and parse the results:"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- Automatic detection of AMD compute hosts which are capable of"},{"line_number":14,"context_line":"  providing SEV (Secure Encrypted Virtualization)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bfb3d3c7_d9c1b430","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":37},"updated":"2019-05-17 14:24:43.000000000","message":"A reference to this would be helpful. This one looks good\n\nhttps://libvirt.org/html/libvirt-libvirt-domain.html#virConnectGetDomainCapabilities","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Two use cases have emerged semi-recently which both require the"},{"line_number":10,"context_line":"libvirt driver to be able to invoke libvirt\u0027s"},{"line_number":11,"context_line":"virConnectGetDomainCapabilities() API and parse the results:"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"- Automatic detection of AMD compute hosts which are capable of"},{"line_number":14,"context_line":"  providing SEV (Secure Encrypted Virtualization)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bfb3d3c7_9a2d6b48","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":37},"in_reply_to":"bfb3d3c7_d9c1b430","updated":"2019-05-17 16:40:59.000000000","message":"Done","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1acb488b5eeca38854fb2905835e11bacd8c4b0d","unresolved":false,"context_lines":[{"line_number":21,"context_line":"the XML returned from libvirt, and corresponding tests."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Change-Id: I4aeac9b2397bb2f5e130d1e58829a5e549fcb191"},{"line_number":24,"context_line":"blueprint: gracefully-handle-qemu-machine-types"},{"line_number":25,"context_line":"blueprint: amd-sev-libvirt-support"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bfb3d3c7_343a55e7","line":24,"range":{"start_line":24,"start_character":0,"end_line":24,"end_character":47},"updated":"2019-05-17 14:26:28.000000000","message":"This isn\u0027t approved. I don\u0027t know if that matters or not","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"2624315e1ea5fcf3e1042295f902d919de0d07bf","unresolved":false,"context_lines":[{"line_number":21,"context_line":"the XML returned from libvirt, and corresponding tests."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Change-Id: I4aeac9b2397bb2f5e130d1e58829a5e549fcb191"},{"line_number":24,"context_line":"blueprint: gracefully-handle-qemu-machine-types"},{"line_number":25,"context_line":"blueprint: amd-sev-libvirt-support"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bfb3d3c7_40e38406","line":24,"range":{"start_line":24,"start_character":0,"end_line":24,"end_character":47},"in_reply_to":"bfb3d3c7_343a55e7","updated":"2019-05-17 16:34:45.000000000","message":"Oh, thanks for noticing that.\n\nI\u0027m going to say it\u0027s okay, since the sev one is approved, and this patch is also being used for that bp.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":21,"context_line":"the XML returned from libvirt, and corresponding tests."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Change-Id: I4aeac9b2397bb2f5e130d1e58829a5e549fcb191"},{"line_number":24,"context_line":"blueprint: gracefully-handle-qemu-machine-types"},{"line_number":25,"context_line":"blueprint: amd-sev-libvirt-support"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bfb3d3c7_407aa424","line":24,"range":{"start_line":24,"start_character":0,"end_line":24,"end_character":47},"in_reply_to":"bfb3d3c7_40e38406","updated":"2019-05-17 16:40:59.000000000","message":"Yup - I think there\u0027s still value in having the association, especially as it seems likely that this will land before the machine types one is approved.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"}],"nova/tests/unit/virt/libvirt/fakelibvirt.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":1490,"context_line":"    \u003cgic supported\u003d\u0027no\u0027/\u003e"},{"line_number":1491,"context_line":"  \u003c/features\u003e\u0027\u0027\u0027"},{"line_number":1492,"context_line":""},{"line_number":1493,"context_line":"    # This approach lets us patch this class variable with alternate values."},{"line_number":1494,"context_line":"    _domain_capability_features \u003d _default_domain_capability_features"},{"line_number":1495,"context_line":""},{"line_number":1496,"context_line":"    def getCapabilities(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_b91ac014","line":1493,"range":{"start_line":1493,"start_character":4,"end_line":1493,"end_character":76},"updated":"2019-05-17 14:29:59.000000000","message":"The two separate vars aren\u0027t necessary to achieve this result.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":1490,"context_line":"    \u003cgic supported\u003d\u0027no\u0027/\u003e"},{"line_number":1491,"context_line":"  \u003c/features\u003e\u0027\u0027\u0027"},{"line_number":1492,"context_line":""},{"line_number":1493,"context_line":"    # This approach lets us patch this class variable with alternate values."},{"line_number":1494,"context_line":"    _domain_capability_features \u003d _default_domain_capability_features"},{"line_number":1495,"context_line":""},{"line_number":1496,"context_line":"    def getCapabilities(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_5aaa9385","line":1493,"range":{"start_line":1493,"start_character":4,"end_line":1493,"end_character":76},"in_reply_to":"bfb3d3c7_b91ac014","updated":"2019-05-17 16:40:59.000000000","message":"I thought it felt cleaner conceptually to distinguish the default value from the \"current\" value, since two more default values get added later:\n\nhttps://review.opendev.org/#/c/633855/13/nova/tests/unit/virt/libvirt/fakelibvirt.py\n\nbut that was just a personal style preference; happy to remove it.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"}],"nova/tests/unit/virt/libvirt/test_fakelibvirt.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":280,"context_line":"    def test_getDomainCapabilities(self):"},{"line_number":281,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"},{"line_number":282,"context_line":"        etree.fromstring(conn.getDomainCapabilities("},{"line_number":283,"context_line":"            \u0027/usr/bin/qemu-kvm\u0027, \u0027x86_64\u0027, \u0027q35\u0027, \u0027kvm\u0027, 0))"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def test_nwfilter_define_undefine(self):"},{"line_number":286,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_f9e5980b","line":283,"updated":"2019-05-17 14:29:59.000000000","message":"What\u0027s going on here? Just making sure getDomainCapabilities doesn\u0027t puke and whatever it returns is parseable?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":280,"context_line":"    def test_getDomainCapabilities(self):"},{"line_number":281,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"},{"line_number":282,"context_line":"        etree.fromstring(conn.getDomainCapabilities("},{"line_number":283,"context_line":"            \u0027/usr/bin/qemu-kvm\u0027, \u0027x86_64\u0027, \u0027q35\u0027, \u0027kvm\u0027, 0))"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def test_nwfilter_define_undefine(self):"},{"line_number":286,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1a7dfb1b","line":283,"in_reply_to":"bfb3d3c7_f9e5980b","updated":"2019-05-17 16:40:59.000000000","message":"Exactly - same as the immediately preceding test, but for a different API Call.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"abfb12e2d4b035782d77b09ed105eccd6c37b788","unresolved":false,"context_lines":[{"line_number":279,"context_line":""},{"line_number":280,"context_line":"    def test_getDomainCapabilities(self):"},{"line_number":281,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"},{"line_number":282,"context_line":"        etree.fromstring(conn.getDomainCapabilities("},{"line_number":283,"context_line":"            \u0027/usr/bin/qemu-kvm\u0027, \u0027x86_64\u0027, \u0027q35\u0027, \u0027kvm\u0027, 0))"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def test_nwfilter_define_undefine(self):"},{"line_number":286,"context_line":"        conn \u003d self.get_openAuth_curry_func()(\u0027qemu:///system\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bfb3d3c7_43cdce5b","line":283,"range":{"start_line":282,"start_character":8,"end_line":283,"end_character":60},"updated":"2019-05-17 17:40:17.000000000","message":"nit: this is really jsut asserting that we get back some valid xml\nwe could probably have a better test but since that is all the rest do i think that is fine as is for now.","commit_id":"297f3ba687f974ec950bd462beb2c345c84a925f"}],"nova/tests/unit/virt/libvirt/test_host.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":678,"context_line":"        caps \u003d self._test_get_domain_capabilities()"},{"line_number":679,"context_line":"        self.assertEqual(vconfig.LibvirtConfigDomainCaps, type(caps))"},{"line_number":680,"context_line":"        features \u003d caps.features"},{"line_number":681,"context_line":"        self.assertEqual([], features)"},{"line_number":682,"context_line":""},{"line_number":683,"context_line":"    @mock.patch.object(fakelibvirt.virConnect, \"getHostname\")"},{"line_number":684,"context_line":"    def test_get_hostname_caching(self, mock_hostname):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_799448e9","line":681,"updated":"2019-05-17 14:29:59.000000000","message":"This is effectively the same as above, because \"we don\u0027t parse that\" for the default feature from the fixture. Shouldn\u0027t we have a test that patches the features to something we *do* parse to make sure they come through?\n\n[Later] Oh, we don\u0027t parse anything currently. So future changes will just enhance the previous test.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":678,"context_line":"        caps \u003d self._test_get_domain_capabilities()"},{"line_number":679,"context_line":"        self.assertEqual(vconfig.LibvirtConfigDomainCaps, type(caps))"},{"line_number":680,"context_line":"        features \u003d caps.features"},{"line_number":681,"context_line":"        self.assertEqual([], features)"},{"line_number":682,"context_line":""},{"line_number":683,"context_line":"    @mock.patch.object(fakelibvirt.virConnect, \"getHostname\")"},{"line_number":684,"context_line":"    def test_get_hostname_caching(self, mock_hostname):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_3ac8ff53","line":681,"in_reply_to":"bfb3d3c7_799448e9","updated":"2019-05-17 16:40:59.000000000","message":"Right.  This was the kind of suck which originally made me resistant to decoupling the SEV stuff from this.  But Kashyap kept asking, so I gave in ;-)  It looks a bit more sensible when viewed in the context of the follow-on patch:\n\nhttps://review.opendev.org/#/c/633855/13/nova/tests/unit/virt/libvirt/test_host.py","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"abfb12e2d4b035782d77b09ed105eccd6c37b788","unresolved":false,"context_lines":[{"line_number":637,"context_line":"            self.assertIsNone(caps.host.cpu.model)"},{"line_number":638,"context_line":"            self.assertEqual(0, len(caps.host.cpu.features))"},{"line_number":639,"context_line":""},{"line_number":640,"context_line":"    def _test_get_domain_capabilities(self):"},{"line_number":641,"context_line":"        caps \u003d self.host.get_domain_capabilities()"},{"line_number":642,"context_line":"        self.assertIn(\u0027x86_64\u0027, caps.keys())"},{"line_number":643,"context_line":"        self.assertEqual([\u0027q35\u0027], list(caps[\u0027x86_64\u0027]))"},{"line_number":644,"context_line":"        return caps[\u0027x86_64\u0027][\u0027q35\u0027]"},{"line_number":645,"context_line":""},{"line_number":646,"context_line":"    def test_get_domain_capabilities(self):"},{"line_number":647,"context_line":"        caps \u003d self._test_get_domain_capabilities()"},{"line_number":648,"context_line":"        self.assertEqual(vconfig.LibvirtConfigDomainCaps, type(caps))"},{"line_number":649,"context_line":"        # There is a \u003cgic supported\u003d\u0027no\u0027/\u003e feature in the fixture but"},{"line_number":650,"context_line":"        # we don\u0027t parse that because nothing currently cares about it."},{"line_number":651,"context_line":"        self.assertEqual(0, len(caps.features))"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    @mock.patch.object(fakelibvirt.virConnect, \u0027_domain_capability_features\u0027,"},{"line_number":654,"context_line":"                       new\u003d\u0027\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bfb3d3c7_031ef6b9","line":651,"range":{"start_line":640,"start_character":2,"end_line":651,"end_character":47},"updated":"2019-05-17 17:40:17.000000000","message":"ok so this works because you have modifed the fakelibvirtdriver to use the static xml https://review.opendev.org/#/c/655268/6/nova/tests/unit/virt/libvirt/fakelibvirt.py@1360\n\nand it is regiserted as a test fixture \nhttps://review.opendev.org/#/c/655268/6/nova/tests/unit/virt/libvirt/fakelibvirt.py@1360\n\nso this does not  actully use any host state so this should be portable +1","commit_id":"297f3ba687f974ec950bd462beb2c345c84a925f"}],"nova/virt/libvirt/config.py":[{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"cfc512a7f6f8a2a3fe29dc419b3629ed7125c1ee","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        for c in xmldoc.getchildren():"},{"line_number":149,"context_line":"            feature \u003d None"},{"line_number":150,"context_line":"            # TODO: add supported features here"},{"line_number":151,"context_line":"            if feature:"},{"line_number":152,"context_line":"                feature.parse_dom(c)"},{"line_number":153,"context_line":"                self.features.append(feature)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ffb9cba7_fb455b44","line":150,"range":{"start_line":150,"start_character":14,"end_line":150,"end_character":18},"updated":"2019-04-24 07:35:57.000000000","message":"The PEP 8 failure is due to the \u0027TODO\u0027 syntax:\n\n[...]\nRunning flake8 on all files\n./nova/virt/libvirt/config.py:150:15: H101  Use TODO(NAME)\nERROR: InvocationError for command /bin/bash tools/flake8wrap.sh (exited with code 1)\n[...]","commit_id":"5d3f8289003a5c6e3f722038be9a73c19f795ba9"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            root_name\u003d\"features\", **kwargs)"},{"line_number":143,"context_line":"        self.features \u003d []"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":146,"context_line":"        super(LibvirtConfigDomainCapsFeatures, self).parse_dom(xmldoc)"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        for c in xmldoc.getchildren():"},{"line_number":149,"context_line":"            feature \u003d None"},{"line_number":150,"context_line":"            # TODO(aspiers): add supported features here"},{"line_number":151,"context_line":"            if feature:"},{"line_number":152,"context_line":"                feature.parse_dom(c)"},{"line_number":153,"context_line":"                self.features.append(feature)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"            # There are many other features and domain capabilities,"},{"line_number":156,"context_line":"            # but we don\u0027t need to regenerate the XML (it\u0027s read-only"},{"line_number":157,"context_line":"            # data provided by libvirtd), so there\u0027s no point parsing"},{"line_number":158,"context_line":"            # them until we actually need their values."},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    # For the same reason, we do not need a format_dom() method, but"},{"line_number":161,"context_line":"    # it\u0027s a bug if this ever gets called and we inherited one from"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_74fe8d06","line":158,"range":{"start_line":145,"start_character":0,"end_line":158,"end_character":55},"updated":"2019-05-17 14:29:59.000000000","message":"IMO this method should go away - just keep the comments - until something is actually needed. Having this framework in place doesn\u0027t buy anything.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            root_name\u003d\"features\", **kwargs)"},{"line_number":143,"context_line":"        self.features \u003d []"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":146,"context_line":"        super(LibvirtConfigDomainCapsFeatures, self).parse_dom(xmldoc)"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        for c in xmldoc.getchildren():"},{"line_number":149,"context_line":"            feature \u003d None"},{"line_number":150,"context_line":"            # TODO(aspiers): add supported features here"},{"line_number":151,"context_line":"            if feature:"},{"line_number":152,"context_line":"                feature.parse_dom(c)"},{"line_number":153,"context_line":"                self.features.append(feature)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"            # There are many other features and domain capabilities,"},{"line_number":156,"context_line":"            # but we don\u0027t need to regenerate the XML (it\u0027s read-only"},{"line_number":157,"context_line":"            # data provided by libvirtd), so there\u0027s no point parsing"},{"line_number":158,"context_line":"            # them until we actually need their values."},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    # For the same reason, we do not need a format_dom() method, but"},{"line_number":161,"context_line":"    # it\u0027s a bug if this ever gets called and we inherited one from"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_9dfec503","line":158,"range":{"start_line":145,"start_character":0,"end_line":158,"end_character":55},"in_reply_to":"bfb3d3c7_74fe8d06","updated":"2019-05-17 16:40:59.000000000","message":"Well, if nothing else it makes the follow-on patch nice and small:\n\nhttps://review.opendev.org/#/c/633855/13/nova/virt/libvirt/config.py@150\n\nBut you could continue this argument further and say that if we\u0027re not parsing \u003cfeatures\u003e, there\u0027s no point in adding the class and associated tests.  And then you could say that since we\u0027re not instantiating that class, LibvirtConfigDomainCaps.parse_dom() and .features() are also pointless and should be removed, along with their tests.  And then you could argue that since LibvirtConfigDomainCaps now does nothing, that and its tests could also be removed ...  \n\nAnd then there\u0027d be nothing left in this patch ;-)\n\nThis is exactly why I originally resisted Kashyap\u0027s request to decouple this from the SEV-specific code, since without the SEV code which consumes it, the whole thing is a giant no-op, which only exists in order to be tested:\n\nhttps://review.opendev.org/#/c/633855/11/nova/virt/libvirt/host.py@680\n\nhttp://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-03-19.log.html#t2019-03-19T10:21:42\n\nIn the end we agreed that it was OK to have a foundational patch which is only exercised via tests, because the benefits are that\n\na) it potentially unblocks Kashyap\u0027s secure boot work, which would otherwise have an unwanted dependency on an SEV patch,\n\nb) lays a common foundation for both SEV and secure boot, and\n\nc) keeps the follow-on patches to a minimal size.\n\nSo I don\u0027t really see why it\u0027s better to draw the line at adding a constructor for LibvirtConfigDomainCapsFeatures, vs. adding parse_dom().  At this stage they\u0027re both no-ops, as are all the new chunks of code they rely on.\n\nIf I\u0027m missing something then I\u0027m very happy to be shown it, and of course I can fairly easily tweak it, but it feels to me like this is maybe starting to go round in circles a bit ;-p\n\nMaybe we should continue discussing on IRC.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"2c51dbd7f55ee327cdccd6b7e36d16831437442e","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            root_name\u003d\"features\", **kwargs)"},{"line_number":143,"context_line":"        self.features \u003d []"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":146,"context_line":"        super(LibvirtConfigDomainCapsFeatures, self).parse_dom(xmldoc)"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        for c in xmldoc.getchildren():"},{"line_number":149,"context_line":"            feature \u003d None"},{"line_number":150,"context_line":"            # TODO(aspiers): add supported features here"},{"line_number":151,"context_line":"            if feature:"},{"line_number":152,"context_line":"                feature.parse_dom(c)"},{"line_number":153,"context_line":"                self.features.append(feature)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"            # There are many other features and domain capabilities,"},{"line_number":156,"context_line":"            # but we don\u0027t need to regenerate the XML (it\u0027s read-only"},{"line_number":157,"context_line":"            # data provided by libvirtd), so there\u0027s no point parsing"},{"line_number":158,"context_line":"            # them until we actually need their values."},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"    # For the same reason, we do not need a format_dom() method, but"},{"line_number":161,"context_line":"    # it\u0027s a bug if this ever gets called and we inherited one from"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_a094c0e9","line":158,"range":{"start_line":145,"start_character":0,"end_line":158,"end_character":55},"in_reply_to":"bfb3d3c7_9dfec503","updated":"2019-05-17 16:58:09.000000000","message":"I\u0027m not as bothered by this one, as the next patch makes use of it nicely.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"997a83fdd38a5d48afb442ca8bc75b44db0f6068","unresolved":false,"context_lines":[{"line_number":162,"context_line":"    # the base class, so override that to watch out for accidental"},{"line_number":163,"context_line":"    # calls."},{"line_number":164,"context_line":"    def format_dom(self):"},{"line_number":165,"context_line":"        raise RuntimeError(_(\u0027BUG: tried to generate domainCapabilities XML\u0027))"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_545b492d","line":165,"range":{"start_line":165,"start_character":8,"end_line":165,"end_character":78},"updated":"2019-05-17 14:24:43.000000000","message":"As an aside, sahid had an effort to start generating the samples you included in the fakelibvirt module. I should probably pick that up again at some point","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":162,"context_line":"    # the base class, so override that to watch out for accidental"},{"line_number":163,"context_line":"    # calls."},{"line_number":164,"context_line":"    def format_dom(self):"},{"line_number":165,"context_line":"        raise RuntimeError(_(\u0027BUG: tried to generate domainCapabilities XML\u0027))"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1d12d559","line":165,"range":{"start_line":165,"start_character":8,"end_line":165,"end_character":78},"in_reply_to":"bfb3d3c7_545b492d","updated":"2019-05-17 16:40:59.000000000","message":"Oh?  Interesting - what would that ever be used for?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        raise RuntimeError(_(\u0027BUG: tried to generate domainCapabilities XML\u0027))"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_74752d98","line":168,"range":{"start_line":168,"start_character":6,"end_line":168,"end_character":36},"updated":"2019-05-17 14:29:59.000000000","message":"currently unused, nix","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        raise RuntimeError(_(\u0027BUG: tried to generate domainCapabilities XML\u0027))"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_dd1bdd70","line":168,"range":{"start_line":168,"start_character":6,"end_line":168,"end_character":36},"in_reply_to":"bfb3d3c7_74752d98","updated":"2019-05-17 16:40:59.000000000","message":"See my essay above ;-)  This is deliberately no-op code intended to serve as a foundation on top of which SEV and secure boot code can sit.  That said, happy to move it to the follow-up patch if you\u0027re convinced that\u0027s better.  Let\u0027s discuss on IRC.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_f4ad9dea","line":170,"range":{"start_line":170,"start_character":23,"end_line":170,"end_character":27},"updated":"2019-05-17 14:29:59.000000000","message":"Not sure how you\u0027re planning to use this class (because you\u0027re not yet) but if you instantiate it using the root_name kwarg, you can do away with overriding __init__ entirely.\n\n[Later] Yeah, I looked at the patch above this, and it doesn\u0027t look like there\u0027s any real reason for this class to exist - see comment there.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"03bf3f6e0118526a217bc6f4114ed055cfc22bc1","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_43f5ceca","line":170,"range":{"start_line":170,"start_character":23,"end_line":170,"end_character":27},"in_reply_to":"bfb3d3c7_80bc1c6a","updated":"2019-05-17 17:18:20.000000000","message":"Gotcha. Removed in PS5 (and rebased in PS6).","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_fd182176","line":170,"range":{"start_line":170,"start_character":23,"end_line":170,"end_character":27},"in_reply_to":"bfb3d3c7_f4ad9dea","updated":"2019-05-17 16:40:59.000000000","message":"Ditto.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"2c51dbd7f55ee327cdccd6b7e36d16831437442e","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"class LibvirtConfigDomainCapsFeature(LibvirtConfigObject):"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def __init__(self, name, **kwargs):"},{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_80bc1c6a","line":170,"range":{"start_line":170,"start_character":23,"end_line":170,"end_character":27},"in_reply_to":"bfb3d3c7_fd182176","updated":"2019-05-17 16:58:09.000000000","message":"This one was my main beef. You don\u0027t need this class to exist at all, now or later. See https://review.opendev.org/#/c/633855/13/nova/virt/libvirt/config.py@183","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":175,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).parse_dom(xmldoc)"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"class LibvirtConfigCapsNUMATopology(LibvirtConfigObject):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_140f112f","line":175,"range":{"start_line":174,"start_character":0,"end_line":175,"end_character":69},"updated":"2019-05-17 14:29:59.000000000","message":"redundant, nix","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":171,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).__init__(root_name\u003dname,"},{"line_number":172,"context_line":"                                                             **kwargs)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":175,"context_line":"        super(LibvirtConfigDomainCapsFeature, self).parse_dom(xmldoc)"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"class LibvirtConfigCapsNUMATopology(LibvirtConfigObject):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_fd3101ee","line":175,"range":{"start_line":174,"start_character":0,"end_line":175,"end_character":69},"in_reply_to":"bfb3d3c7_140f112f","updated":"2019-05-17 16:40:59.000000000","message":"Ditto.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"abfb12e2d4b035782d77b09ed105eccd6c37b788","unresolved":false,"context_lines":[{"line_number":119,"context_line":"            root_name\u003d\"domainCapabilities\", **kwargs)"},{"line_number":120,"context_line":"        self._features \u003d None"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    def parse_dom(self, xmldoc):"},{"line_number":123,"context_line":"        super(LibvirtConfigDomainCaps, self).parse_dom(xmldoc)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        for c in xmldoc.getchildren():"},{"line_number":126,"context_line":"            if c.tag \u003d\u003d \"features\":"},{"line_number":127,"context_line":"                features \u003d LibvirtConfigDomainCapsFeatures()"},{"line_number":128,"context_line":"                features.parse_dom(c)"},{"line_number":129,"context_line":"                self._features \u003d features"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    @property"},{"line_number":132,"context_line":"    def features(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"bfb3d3c7_8360a634","line":129,"range":{"start_line":122,"start_character":3,"end_line":129,"end_character":41},"updated":"2019-05-17 17:40:17.000000000","message":"ok so this chagne only provides support for extracting the\nfeatures form the domain capablities but it should be simple to extend this to retirive the other aspect as needed.\n\ni can reuse this for the device model scheduling support for video modeld  and disk bus relatively simply so this look good to me.","commit_id":"297f3ba687f974ec950bd462beb2c345c84a925f"}],"nova/virt/libvirt/host.py":[{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"db85b26e21c5197f07b85e1d83900d91f91c57ca","unresolved":false,"context_lines":[{"line_number":694,"context_line":"        phase of the driver lifecycle rather than just before booting"},{"line_number":695,"context_line":"        an instance."},{"line_number":696,"context_line":""},{"line_number":697,"context_line":"        This causes an additional complication since the Python"},{"line_number":698,"context_line":"        binding for this libvirt API call requires the architecture"},{"line_number":699,"context_line":"        and machine type to be provided.  So in order to gain a full"},{"line_number":700,"context_line":"        picture of the hypervisor\u0027s capabilities, technically we need"},{"line_number":701,"context_line":"        to call it with the right parameters, once for each"},{"line_number":702,"context_line":"        (architecture, machine_type) combination which we care about."}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_45c0127c","line":699,"range":{"start_line":697,"start_character":47,"end_line":699,"end_character":39},"updated":"2019-04-26 08:38:02.000000000","message":"Nit: it implies as if the C API itself or other bindings don\u0027t require it @arch and @machine parameters :-)\n\nHow about:\n\n\"since the Python bindings for this libvirt API call\" --\u003e \"since the libvirt API call\"","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"6000f382daac3754ab5b870721d34056a09af2db","unresolved":false,"context_lines":[{"line_number":694,"context_line":"        phase of the driver lifecycle rather than just before booting"},{"line_number":695,"context_line":"        an instance."},{"line_number":696,"context_line":""},{"line_number":697,"context_line":"        This causes an additional complication since the Python"},{"line_number":698,"context_line":"        binding for this libvirt API call requires the architecture"},{"line_number":699,"context_line":"        and machine type to be provided.  So in order to gain a full"},{"line_number":700,"context_line":"        picture of the hypervisor\u0027s capabilities, technically we need"},{"line_number":701,"context_line":"        to call it with the right parameters, once for each"},{"line_number":702,"context_line":"        (architecture, machine_type) combination which we care about."}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_a3b6a3f9","line":699,"range":{"start_line":697,"start_character":47,"end_line":699,"end_character":39},"in_reply_to":"ffb9cba7_1151fc0e","updated":"2019-04-26 14:57:56.000000000","message":"\u003e \u003e Nit: it implies as if the C API itself or other bindings don\u0027t require it @arch and @machine parameters :-)\n\n... which is true, currently.\n\n \u003e Actually, the Python API *does* mistakenly allow skipping the @emulatorbin parameter, while the C API mandates it.\n\nUmm ... ?  AFAICS it\u0027s the other way around.\n\nThe C API in qemuConnectGetDomainCapabilities() actually calls virQEMUCapsCacheLookupDefault():\n\nhttps://libvirt.org/git/?p\u003dlibvirt.git;a\u003dblob;f\u003dsrc/qemu/qemu_driver.c;h\u003df48d9256e4cc0f6282dcd7aa13bb7a1b53508eb3;hb\u003dHEAD#l19898\n\nwhich handles cases such as emulatorbin being NULL:\n\nhttps://libvirt.org/git/?p\u003dlibvirt.git;a\u003dblob;f\u003dsrc/qemu/qemu_capabilities.c;h\u003da0b2ca73fbdf986609d37892c5cb36f912fdad15;hb\u003dHEAD#l4781\n\n    * Looks up the QEMU binary specified by @binary and @archStr, checks it can\n    * provide the required @virttypeStr and @machine and returns its capabilities.\n    * Sensible defaults are used for any argument which is NULL (the function can\n    * even be called with all NULL arguments).\n\nIn contrast the Python API doesn\u0027t provide a default value for any of the parameters except flags\u003d0, so e.g. emulatorbin can\u0027t be skipped.  It *appears* that passing None may work, but I haven\u0027t tried that yet, and anyway in Python land that doesn\u0027t technically count as an optional argument; it\u0027s a mandatory argument which is allowed to be None amongst other values.\n\n \u003e I notified libvirt upstream, and they\u0027re fixing the Python bindings as I write this. :-)\n\nYep, thanks a lot for doing that!  I don\u0027t quite understand how the fix in https://www.redhat.com/archives/libvir-list/2019-April/msg01418.html would make the Python arguments optional however, since they\u0027re still missing defaults.  Does something magically spot the mention of \"optional\" in the info attributes and then auto-generate Python code which defaults to None?  That seems far-fetched, but it\u0027s the only explanation I can think of.\n\n \u003e But meanwhile, we can pass \u0027None\u0027 to @emulatorbin for the Python API.\n\nThere\u0027s no need to do that because I\u0027ve already obtained the correct value via https://review.opendev.org/#/c/640483/, plus AFAICS there is no documentation on how the libvirt API would actually pick an emulatorbin value if none was provided.  Sure, I can see what it\u0027s doing in the source:\n\nhttps://libvirt.org/git/?p\u003dlibvirt.git;a\u003dblob;f\u003dsrc/qemu/qemu_capabilities.c;h\u003da0b2ca73fbdf986609d37892c5cb36f912fdad15;hb\u003dHEAD#l4839\n\nand this does seem to tie in with the \"Sensible defaults\" comment above.  But for now I\u0027d prefer to stick with something which is known to work in a documented and well-understood way.\n\nIf libvirt upstream can add a bit of clarity around that then I guess we can revisit this and remove the three lines of code below which calculate emulator_bin:\n\n                emulator_bin \u003d guest.emulator\n                if virt_type in guest.domemulator:\n                    emulator_bin \u003d guest.domemulator[virt_type]\n\nbut before that point it doesn\u0027t feel to me like a big enough win to be worth it.\n\nBTW I originally spotted this discrepancy between the Python and C APIs by noticing that running \"virsh domcapabilities\" works - it doesn\u0027t actually require *any* arguments.\n\nI didn\u0027t check the bindings for other languages.","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"5a2547974c364ea74ea34618c1783e29a843ce07","unresolved":false,"context_lines":[{"line_number":694,"context_line":"        phase of the driver lifecycle rather than just before booting"},{"line_number":695,"context_line":"        an instance."},{"line_number":696,"context_line":""},{"line_number":697,"context_line":"        This causes an additional complication since the Python"},{"line_number":698,"context_line":"        binding for this libvirt API call requires the architecture"},{"line_number":699,"context_line":"        and machine type to be provided.  So in order to gain a full"},{"line_number":700,"context_line":"        picture of the hypervisor\u0027s capabilities, technically we need"},{"line_number":701,"context_line":"        to call it with the right parameters, once for each"},{"line_number":702,"context_line":"        (architecture, machine_type) combination which we care about."}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_8b513b65","line":699,"range":{"start_line":697,"start_character":47,"end_line":699,"end_character":39},"in_reply_to":"ffb9cba7_45c0127c","updated":"2019-04-26 12:35:25.000000000","message":"\u003e Nit: it implies as if the C API itself or other bindings don\u0027t\n \u003e require it @arch and @machine parameters :-)\n\nActually, the Python API *does* mistakenly allow skipping the @emulatorbin parameter, while the C API mandates it.\n\nI notified libvirt upstream, and they\u0027re fixing the Python bindings as I write this. :-)\n\nBut meanwhile, we can pass \u0027None\u0027 to @emulatorbin for the Python API.\n\n \u003e How about:\n \u003e \n \u003e \"since the Python bindings for this libvirt API call\" --\u003e \"since\n \u003e the libvirt API call\"","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"7756b8f085184d674de985d6e6444016530951fc","unresolved":false,"context_lines":[{"line_number":694,"context_line":"        phase of the driver lifecycle rather than just before booting"},{"line_number":695,"context_line":"        an instance."},{"line_number":696,"context_line":""},{"line_number":697,"context_line":"        This causes an additional complication since the Python"},{"line_number":698,"context_line":"        binding for this libvirt API call requires the architecture"},{"line_number":699,"context_line":"        and machine type to be provided.  So in order to gain a full"},{"line_number":700,"context_line":"        picture of the hypervisor\u0027s capabilities, technically we need"},{"line_number":701,"context_line":"        to call it with the right parameters, once for each"},{"line_number":702,"context_line":"        (architecture, machine_type) combination which we care about."}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_1151fc0e","line":699,"range":{"start_line":697,"start_character":47,"end_line":699,"end_character":39},"in_reply_to":"ffb9cba7_8b513b65","updated":"2019-04-26 13:21:33.000000000","message":"[...]\n\n \u003e Actually, the Python API *does* mistakenly allow skipping the\n \u003e @emulatorbin parameter, while the C API mandates it.\n \u003e \n \u003e I notified libvirt upstream, and they\u0027re fixing the Python bindings\n \u003e as I write this. :-)\n\nThere it is: https://www.redhat.com/archives/libvir-list/2019-April/msg01418.html (\"[python PATCH] Allow virConnect.getDomainCapabilities() to have no arguments\")\n\n[...]","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"db85b26e21c5197f07b85e1d83900d91f91c57ca","unresolved":false,"context_lines":[{"line_number":712,"context_line":"            https://bugzilla.redhat.com/show_bug.cgi?id\u003d1683471#c7"},{"line_number":713,"context_line":""},{"line_number":714,"context_line":"        However, future domain capabilities might report SEV in a more"},{"line_number":715,"context_line":"        fine-grained manner, and we also expect to use this to detect"},{"line_number":716,"context_line":"        other features, such as for gracefully handling machine types"},{"line_number":717,"context_line":"        and potentially for detecting OVMF binaries.  Therefore we"},{"line_number":718,"context_line":"        memoize the results of the API calls in a nested dict where"}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_253b9e9c","line":715,"range":{"start_line":715,"start_character":55,"end_line":715,"end_character":59},"updated":"2019-04-26 08:38:02.000000000","message":"Nit: s/this/_get_domain_capabilities()/","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"6000f382daac3754ab5b870721d34056a09af2db","unresolved":false,"context_lines":[{"line_number":712,"context_line":"            https://bugzilla.redhat.com/show_bug.cgi?id\u003d1683471#c7"},{"line_number":713,"context_line":""},{"line_number":714,"context_line":"        However, future domain capabilities might report SEV in a more"},{"line_number":715,"context_line":"        fine-grained manner, and we also expect to use this to detect"},{"line_number":716,"context_line":"        other features, such as for gracefully handling machine types"},{"line_number":717,"context_line":"        and potentially for detecting OVMF binaries.  Therefore we"},{"line_number":718,"context_line":"        memoize the results of the API calls in a nested dict where"}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_d09cc25e","line":715,"range":{"start_line":715,"start_character":55,"end_line":715,"end_character":59},"in_reply_to":"ffb9cba7_253b9e9c","updated":"2019-04-26 14:57:56.000000000","message":"Done","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"d818161df17aa3e0f05f80b56ae73214d0d3ca23","unresolved":false,"context_lines":[{"line_number":745,"context_line":"                if arch not in domain_caps:"},{"line_number":746,"context_line":"                    domain_caps[arch] \u003d {}"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"                domain_caps[arch][machine_type] \u003d \\"},{"line_number":749,"context_line":"                    self._get_domain_capabilities(emulator_bin, arch,"},{"line_number":750,"context_line":"                                                  machine_type, virt_type)"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"            # NOTE(aspiers): Use a temporary variable to update the"},{"line_number":753,"context_line":"            # instance variable atomically, otherwise if some API"}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_d05da2e9","line":750,"range":{"start_line":748,"start_character":0,"end_line":750,"end_character":74},"updated":"2019-04-26 09:58:24.000000000","message":"Adam, following our IRC chat[1], writing down some note here (it\u0027s probably worth documenting some of them in the doc string):\n\n- Whenever libvirt/QEMU are updated, domCapabilities get outdated (because QEMU will contain new features and the capabilities will vary).\n- However, the above point should not be a problem in Nova\u0027s case, because whe libvirt/QEMU gets updated, the Compute agent needs restarting, and the memoization vanishes.\n- domCapabilities are MachineType-dependent.\n- You\u0027ve already documented in the doc string that we need to know the capabilities at the time of placement / scheduling and not just at instance launch time.\n- We should bear in mind that host settings play a role here too (e.g. KVM/IOMMU availability), so if you have two hosts with the same libvirt+QEMU packages and one has IOMMU disabled then you\u0027ll get different domCapabilities on each of the hosts.\n\nAlso see the latest comment[2]. from Michal Privoznik in the bug you filed.  (I presume the libvirt dev is thinking from a single hypervisor point of view, and not considering the multi-host scheduling.)\n\n[1] http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-04-23.log.html#t2019-04-23T12:42:11)\n[2] https://bugzilla.redhat.com/show_bug.cgi?id\u003d1683471#c11","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"6000f382daac3754ab5b870721d34056a09af2db","unresolved":false,"context_lines":[{"line_number":745,"context_line":"                if arch not in domain_caps:"},{"line_number":746,"context_line":"                    domain_caps[arch] \u003d {}"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"                domain_caps[arch][machine_type] \u003d \\"},{"line_number":749,"context_line":"                    self._get_domain_capabilities(emulator_bin, arch,"},{"line_number":750,"context_line":"                                                  machine_type, virt_type)"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"            # NOTE(aspiers): Use a temporary variable to update the"},{"line_number":753,"context_line":"            # instance variable atomically, otherwise if some API"}],"source_content_type":"text/x-python","patch_set":2,"id":"ffb9cba7_a35c637e","line":750,"range":{"start_line":748,"start_character":0,"end_line":750,"end_character":74},"in_reply_to":"ffb9cba7_d05da2e9","updated":"2019-04-26 14:57:56.000000000","message":"\u003e Adam, following our IRC chat[1], writing down some note here (it\u0027s probably worth documenting some of them in the doc string):\n\u003e \n\u003e - Whenever libvirt/QEMU are updated, domCapabilities get outdated (because QEMU will contain new features and the capabilities will vary).\n\u003e - However, the above point should not be a problem in Nova\u0027s case, because whe libvirt/QEMU gets updated, the Compute agent needs restarting, and the memoization vanishes.\n\nDone.\n\n\u003e - domCapabilities are MachineType-dependent.\n\nWell this is already explicit both in the comments and the code.\n\n\u003e - We should bear in mind that host settings play a role here too (e.g. KVM/IOMMU availability), so if you have two hosts with the same libvirt+QEMU packages and one has IOMMU disabled then you\u0027ll get different domCapabilities on each of the hosts.\n\nSure but this should go without saying really.  Every compute host is of course inventoried independently of the other hosts, not just domCapabilities but all the other resources tracked by nova.\n\n\u003e Also see the latest comment[2]. from Michal Privoznik in the bug you filed.  (I presume the libvirt dev is thinking from a single hypervisor point of view, and not considering the multi-host scheduling.)\n\u003e\n\u003e [2] https://bugzilla.redhat.com/show_bug.cgi?id\u003d1683471#c11\n\nThanks for the pointer.  I\u0027ll reply to that too.","commit_id":"9b9aaa3c2e52283ad1be5816f61ad212519e2d33"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":685,"context_line":"        In this context the fuzzy word \"hypervisor\" implies QEMU"},{"line_number":686,"context_line":"        binary, libvirt itself and the host config.  libvirt provides"},{"line_number":687,"context_line":"        this in order that callers can determine what the underlying"},{"line_number":688,"context_line":"        emulator and/or libvirt is capable of, prior creating a domain"},{"line_number":689,"context_line":"        (for instance via virDomainCreateXML or virDomainDefineXML)."},{"line_number":690,"context_line":"        However nova needs to know the capabilities much earlier, when"},{"line_number":691,"context_line":"        the host\u0027s compute service is first initialised, in order that"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_d48df97b","line":688,"range":{"start_line":688,"start_character":47,"end_line":688,"end_character":52},"updated":"2019-05-17 14:29:59.000000000","message":"prior to","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":685,"context_line":"        In this context the fuzzy word \"hypervisor\" implies QEMU"},{"line_number":686,"context_line":"        binary, libvirt itself and the host config.  libvirt provides"},{"line_number":687,"context_line":"        this in order that callers can determine what the underlying"},{"line_number":688,"context_line":"        emulator and/or libvirt is capable of, prior creating a domain"},{"line_number":689,"context_line":"        (for instance via virDomainCreateXML or virDomainDefineXML)."},{"line_number":690,"context_line":"        However nova needs to know the capabilities much earlier, when"},{"line_number":691,"context_line":"        the host\u0027s compute service is first initialised, in order that"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_dd62fdd8","line":688,"range":{"start_line":688,"start_character":47,"end_line":688,"end_character":52},"in_reply_to":"bfb3d3c7_d48df97b","updated":"2019-05-17 16:40:59.000000000","message":"Done","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":729,"context_line":"        Note: The result is cached in the member attribute"},{"line_number":730,"context_line":"        _domain_caps."},{"line_number":731,"context_line":""},{"line_number":732,"context_line":"        :returns: a nested dict of dicts which maps architectures to"},{"line_number":733,"context_line":"        machine types to instances of config.LibvirtConfigDomainCaps"},{"line_number":734,"context_line":"        representing the domain capabilities of the host for that arch"},{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_f4cefd81","line":733,"range":{"start_line":732,"start_character":18,"end_line":733,"end_character":68},"updated":"2019-05-17 14:29:59.000000000","message":"This is fairly clear, but a little picture wouldn\u0027t hurt\n\n { arch:\n      { machine_type: LibvirtConfigDomainCaps }\n }","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":729,"context_line":"        Note: The result is cached in the member attribute"},{"line_number":730,"context_line":"        _domain_caps."},{"line_number":731,"context_line":""},{"line_number":732,"context_line":"        :returns: a nested dict of dicts which maps architectures to"},{"line_number":733,"context_line":"        machine types to instances of config.LibvirtConfigDomainCaps"},{"line_number":734,"context_line":"        representing the domain capabilities of the host for that arch"},{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1dae55e7","line":733,"range":{"start_line":732,"start_character":18,"end_line":733,"end_character":68},"in_reply_to":"bfb3d3c7_f4cefd81","updated":"2019-05-17 16:40:59.000000000","message":"Done","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""},{"line_number":737,"context_line":"        \"\"\""},{"line_number":738,"context_line":"        if not self._domain_caps:"},{"line_number":739,"context_line":"            domain_caps \u003d {}"},{"line_number":740,"context_line":"            caps \u003d self.get_capabilities()"},{"line_number":741,"context_line":"            virt_type \u003d CONF.libvirt.virt_type"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_34aa5585","line":738,"range":{"start_line":738,"start_character":8,"end_line":738,"end_character":33},"updated":"2019-05-17 14:29:59.000000000","message":"based on the docstring, this is going to be run synchronously in a single thread (init_host), so no locking is needed, yah?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"997a83fdd38a5d48afb442ca8bc75b44db0f6068","unresolved":false,"context_lines":[{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""},{"line_number":737,"context_line":"        \"\"\""},{"line_number":738,"context_line":"        if not self._domain_caps:"},{"line_number":739,"context_line":"            domain_caps \u003d {}"},{"line_number":740,"context_line":"            caps \u003d self.get_capabilities()"},{"line_number":741,"context_line":"            virt_type \u003d CONF.libvirt.virt_type"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_547109e0","line":738,"range":{"start_line":738,"start_character":8,"end_line":738,"end_character":33},"updated":"2019-05-17 14:24:43.000000000","message":"nit:\n\n  if self._domain_caps:\n      return self._domain_caps\n\n  domain_caps \u003d {}\n  ...\n\nJust to avoid unnecessary indentation","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""},{"line_number":737,"context_line":"        \"\"\""},{"line_number":738,"context_line":"        if not self._domain_caps:"},{"line_number":739,"context_line":"            domain_caps \u003d {}"},{"line_number":740,"context_line":"            caps \u003d self.get_capabilities()"},{"line_number":741,"context_line":"            virt_type \u003d CONF.libvirt.virt_type"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_fd69018b","line":738,"range":{"start_line":738,"start_character":8,"end_line":738,"end_character":33},"in_reply_to":"bfb3d3c7_34aa5585","updated":"2019-05-17 16:40:59.000000000","message":"Exactly, it\u0027s added to init_host() here:\n\nhttps://review.opendev.org/#/c/638680/10/nova/virt/libvirt/driver.py@519","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":735,"context_line":"        and machine type."},{"line_number":736,"context_line":""},{"line_number":737,"context_line":"        \"\"\""},{"line_number":738,"context_line":"        if not self._domain_caps:"},{"line_number":739,"context_line":"            domain_caps \u003d {}"},{"line_number":740,"context_line":"            caps \u003d self.get_capabilities()"},{"line_number":741,"context_line":"            virt_type \u003d CONF.libvirt.virt_type"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1dc5f58e","line":738,"range":{"start_line":738,"start_character":8,"end_line":738,"end_character":33},"in_reply_to":"bfb3d3c7_547109e0","updated":"2019-05-17 16:40:59.000000000","message":"Done","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":742,"context_line":""},{"line_number":743,"context_line":"            for guest in caps.guests:"},{"line_number":744,"context_line":"                arch \u003d guest.arch"},{"line_number":745,"context_line":"                machine_type \u003d \\"},{"line_number":746,"context_line":"                    libvirt_utils.get_default_machine_type(arch) or \u0027q35\u0027"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"                emulator_bin \u003d guest.emulator"},{"line_number":749,"context_line":"                if virt_type in guest.domemulator:"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_5404a97b","line":746,"range":{"start_line":745,"start_character":16,"end_line":746,"end_character":73},"updated":"2019-05-17 14:29:59.000000000","message":"Is this always different? Or could you add a short-circuit for:\n\n if machine_type in domain_caps[arch]:\n     continue","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":742,"context_line":""},{"line_number":743,"context_line":"            for guest in caps.guests:"},{"line_number":744,"context_line":"                arch \u003d guest.arch"},{"line_number":745,"context_line":"                machine_type \u003d \\"},{"line_number":746,"context_line":"                    libvirt_utils.get_default_machine_type(arch) or \u0027q35\u0027"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"                emulator_bin \u003d guest.emulator"},{"line_number":749,"context_line":"                if virt_type in guest.domemulator:"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_400f042e","line":746,"range":{"start_line":745,"start_character":16,"end_line":746,"end_character":73},"in_reply_to":"bfb3d3c7_5404a97b","updated":"2019-05-17 16:40:59.000000000","message":"Impressively thorough question!  So you are worried about the case where two \u003cguest\u003e entries have the same arch, and therefore the same default for machine type?  IIUC this won\u0027t happen; at least it certainly doesn\u0027t look like it from the hypervisors I checked.  I also checked with SUSE\u0027s libvirt guru before writing this:\n\n2019-02-28 23:49:47     aspiers in getCapabilities(), can each \u003cguest\u003e have more than one \u003carch\u003e?\n2019-02-28 23:52:43     jimfehlig       no\n2019-02-28 23:52:55     aspiers great, that makes things simpler :)\n2019-02-28 23:52:57     jimfehlig       should only be one\n\n... although admittedly that is a slightly different question.\n\nHopefully Jim or some other libvirt expert can chime in here.  Maybe it *can* happen if some \u003cguest\u003e entries have \u003cos_type\u003ehvm\u003c/os_type\u003e and others have another value?\n\nIn any case for now I\u0027ve added your short-circuit suggestion to avoid any superfluous API calls.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":749,"context_line":"                if virt_type in guest.domemulator:"},{"line_number":750,"context_line":"                    emulator_bin \u003d guest.domemulator[virt_type]"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"                if arch not in domain_caps:"},{"line_number":753,"context_line":"                    domain_caps[arch] \u003d {}"},{"line_number":754,"context_line":""},{"line_number":755,"context_line":"                domain_caps[arch][machine_type] \u003d \\"},{"line_number":756,"context_line":"                    self._get_domain_capabilities(emulator_bin, arch,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_149f1151","line":753,"range":{"start_line":752,"start_character":16,"end_line":753,"end_character":42},"updated":"2019-05-17 14:29:59.000000000","message":"nit: could make domain_caps a defaultdict(dict) and get rid of this","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":749,"context_line":"                if virt_type in guest.domemulator:"},{"line_number":750,"context_line":"                    emulator_bin \u003d guest.domemulator[virt_type]"},{"line_number":751,"context_line":""},{"line_number":752,"context_line":"                if arch not in domain_caps:"},{"line_number":753,"context_line":"                    domain_caps[arch] \u003d {}"},{"line_number":754,"context_line":""},{"line_number":755,"context_line":"                domain_caps[arch][machine_type] \u003d \\"},{"line_number":756,"context_line":"                    self._get_domain_capabilities(emulator_bin, arch,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_8082dc4e","line":753,"range":{"start_line":752,"start_character":16,"end_line":753,"end_character":42},"in_reply_to":"bfb3d3c7_149f1151","updated":"2019-05-17 16:40:59.000000000","message":"Nice idea, thanks! Done.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":759,"context_line":"            # NOTE(aspiers): Use a temporary variable to update the"},{"line_number":760,"context_line":"            # instance variable atomically, otherwise if some API"},{"line_number":761,"context_line":"            # calls succeeded and then one failed, we might"},{"line_number":762,"context_line":"            # accidentally memoize a partial result."},{"line_number":763,"context_line":"            self._domain_caps \u003d domain_caps"},{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1424f10c","line":762,"updated":"2019-05-17 14:29:59.000000000","message":"If something failed, wouldn\u0027t that raise an exception, thereby blowing up init_host and killing the compute service?\n\nNot a problem to use a temp var, but is it actually necessary?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":759,"context_line":"            # NOTE(aspiers): Use a temporary variable to update the"},{"line_number":760,"context_line":"            # instance variable atomically, otherwise if some API"},{"line_number":761,"context_line":"            # calls succeeded and then one failed, we might"},{"line_number":762,"context_line":"            # accidentally memoize a partial result."},{"line_number":763,"context_line":"            self._domain_caps \u003d domain_caps"},{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_600ec8e6","line":762,"in_reply_to":"bfb3d3c7_1424f10c","updated":"2019-05-17 16:40:59.000000000","message":"You\u0027re probably right for now.  I guess I was thinking that something might catch such an exception.  Even if it doesn\u0027t now, maybe future enhancements might be made to make nova-compute more robust in the face of loss of connection to libvirt?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"2c51dbd7f55ee327cdccd6b7e36d16831437442e","unresolved":false,"context_lines":[{"line_number":759,"context_line":"            # NOTE(aspiers): Use a temporary variable to update the"},{"line_number":760,"context_line":"            # instance variable atomically, otherwise if some API"},{"line_number":761,"context_line":"            # calls succeeded and then one failed, we might"},{"line_number":762,"context_line":"            # accidentally memoize a partial result."},{"line_number":763,"context_line":"            self._domain_caps \u003d domain_caps"},{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_00e10c9b","line":762,"in_reply_to":"bfb3d3c7_600ec8e6","updated":"2019-05-17 16:58:09.000000000","message":"sfine","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"},{"line_number":766,"context_line":""},{"line_number":767,"context_line":"    def _get_domain_capabilities(self, emulator_bin, arch, machine_type,"},{"line_number":768,"context_line":"                                 virt_type, flags\u003d0):"},{"line_number":769,"context_line":"        xmlstr \u003d self.get_connection().getDomainCapabilities("},{"line_number":770,"context_line":"            emulator_bin,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_b4e3a5a4","line":767,"range":{"start_line":767,"start_character":8,"end_line":767,"end_character":32},"updated":"2019-05-17 14:29:59.000000000","message":"any particular reason this isn\u0027t just inlined above?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"},{"line_number":766,"context_line":""},{"line_number":767,"context_line":"    def _get_domain_capabilities(self, emulator_bin, arch, machine_type,"},{"line_number":768,"context_line":"                                 virt_type, flags\u003d0):"},{"line_number":769,"context_line":"        xmlstr \u003d self.get_connection().getDomainCapabilities("},{"line_number":770,"context_line":"            emulator_bin,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_60aa4886","line":767,"range":{"start_line":767,"start_character":8,"end_line":767,"end_character":32},"in_reply_to":"bfb3d3c7_1a941b44","updated":"2019-05-17 16:40:59.000000000","message":"Oh dear, you accidentally triggered one of my favourite soapboxes ;-)\n\nMany coding gurus say that methods / functions should \"do one thing only, and do it well\".  I don\u0027t just take this blindly on trust; my personal experience has repeatedly confirmed it.  Overly long methods are the most common cause of bad code I\u0027ve seen, so I try to avoid them wherever possible :)\n\nIn this case, get_domain_capabilities() does one thing (build a memoized _domain_caps dict of dicts), and _get_domain_capabilities() does another (return the results of a single libvirt API as a LibvirtConfigDomainCaps object).  This is not the best example of the principle (arguably they could be split up further), but IMHO the encapsulation promotes encapsulation, readability, and perhaps most importantly, reduces the risk of \"broken windows\" effect caused by a method which is already too long to easily refactor continually growing over time.  Technical debt tends to beget more technical debt... OK, you get the idea, I\u0027ll shut up now.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2479b899ee8d3a9c6b9e2d576000ed8b10ce7568","unresolved":false,"context_lines":[{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"},{"line_number":766,"context_line":""},{"line_number":767,"context_line":"    def _get_domain_capabilities(self, emulator_bin, arch, machine_type,"},{"line_number":768,"context_line":"                                 virt_type, flags\u003d0):"},{"line_number":769,"context_line":"        xmlstr \u003d self.get_connection().getDomainCapabilities("},{"line_number":770,"context_line":"            emulator_bin,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_fa46e74f","line":767,"range":{"start_line":767,"start_character":8,"end_line":767,"end_character":32},"in_reply_to":"bfb3d3c7_b4e3a5a4","updated":"2019-05-17 14:34:49.000000000","message":"I was thinking the same thing but figured it was for UT reasons (though I didn\u0027t go and check /o\\)","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"1a7e7815d7efab463f09d8d230f4be53323e25a3","unresolved":false,"context_lines":[{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        return self._domain_caps"},{"line_number":766,"context_line":""},{"line_number":767,"context_line":"    def _get_domain_capabilities(self, emulator_bin, arch, machine_type,"},{"line_number":768,"context_line":"                                 virt_type, flags\u003d0):"},{"line_number":769,"context_line":"        xmlstr \u003d self.get_connection().getDomainCapabilities("},{"line_number":770,"context_line":"            emulator_bin,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_1a941b44","line":767,"range":{"start_line":767,"start_character":8,"end_line":767,"end_character":32},"in_reply_to":"bfb3d3c7_fa46e74f","updated":"2019-05-17 14:50:36.000000000","message":"I did, and no. I imagine mocking getDomainCapabilities would be adequate for UT.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":770,"context_line":"            emulator_bin,"},{"line_number":771,"context_line":"            arch,"},{"line_number":772,"context_line":"            machine_type,"},{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_7461ad39","line":773,"range":{"start_line":773,"start_character":12,"end_line":773,"end_character":34},"updated":"2019-05-17 14:29:59.000000000","message":"virt_type (or don\u0027t bother passing that in)","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":770,"context_line":"            emulator_bin,"},{"line_number":771,"context_line":"            arch,"},{"line_number":772,"context_line":"            machine_type,"},{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_a0892027","line":773,"range":{"start_line":773,"start_character":12,"end_line":773,"end_character":34},"in_reply_to":"bfb3d3c7_5a6ff3c4","updated":"2019-05-17 16:40:59.000000000","message":"Good spot indeed, thanks.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2479b899ee8d3a9c6b9e2d576000ed8b10ce7568","unresolved":false,"context_lines":[{"line_number":770,"context_line":"            emulator_bin,"},{"line_number":771,"context_line":"            arch,"},{"line_number":772,"context_line":"            machine_type,"},{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_5a6ff3c4","line":773,"range":{"start_line":773,"start_character":12,"end_line":773,"end_character":34},"in_reply_to":"bfb3d3c7_7461ad39","updated":"2019-05-17 14:34:49.000000000","message":"Good spot","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"997a83fdd38a5d48afb442ca8bc75b44db0f6068","unresolved":false,"context_lines":[{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""},{"line_number":777,"context_line":"                 \"machine_type\u003d%s:\\n%s\", arch, machine_type, xmlstr)"},{"line_number":778,"context_line":"        caps \u003d vconfig.LibvirtConfigDomainCaps()"},{"line_number":779,"context_line":"        caps.parse_str(xmlstr)"},{"line_number":780,"context_line":"        return caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_b47f25aa","line":777,"range":{"start_line":776,"start_character":0,"end_line":777,"end_character":68},"updated":"2019-05-17 14:24:43.000000000","message":"This is going to be a _lot_ of information. Are we sure we want to dump it all? I know we do for capabilities but that\u0027s not a reason to arbitrarily do it here too","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a72510a69311180a15099ecf560245e8996a086","unresolved":false,"context_lines":[{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""},{"line_number":777,"context_line":"                 \"machine_type\u003d%s:\\n%s\", arch, machine_type, xmlstr)"},{"line_number":778,"context_line":"        caps \u003d vconfig.LibvirtConfigDomainCaps()"},{"line_number":779,"context_line":"        caps.parse_str(xmlstr)"},{"line_number":780,"context_line":"        return caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_f4235dbd","line":777,"range":{"start_line":776,"start_character":0,"end_line":777,"end_character":68},"in_reply_to":"bfb3d3c7_b47f25aa","updated":"2019-05-17 14:29:59.000000000","message":"at least bust down to debug level?","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"},{"author":{"_account_id":2394,"name":"Adam Spiers","email":"aspiers@suse.com","username":"adam.spiers"},"change_message_id":"65969143d83752ca3ae35d0ed78d016731e3f4a2","unresolved":false,"context_lines":[{"line_number":773,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":774,"context_line":"            flags"},{"line_number":775,"context_line":"        )"},{"line_number":776,"context_line":"        LOG.info(\"Libvirt host hypervisor capabilities for arch\u003d%s and \""},{"line_number":777,"context_line":"                 \"machine_type\u003d%s:\\n%s\", arch, machine_type, xmlstr)"},{"line_number":778,"context_line":"        caps \u003d vconfig.LibvirtConfigDomainCaps()"},{"line_number":779,"context_line":"        caps.parse_str(xmlstr)"},{"line_number":780,"context_line":"        return caps"}],"source_content_type":"text/x-python","patch_set":3,"id":"bfb3d3c7_4047041f","line":777,"range":{"start_line":776,"start_character":0,"end_line":777,"end_character":68},"in_reply_to":"bfb3d3c7_f4235dbd","updated":"2019-05-17 16:40:59.000000000","message":"It\u0027s a lot of information, but it only happens during init_host() rather than regularly.\n\nI don\u0027t mind too much either way, but danp deliberately chose to log capabilities at info level to avoid bug reports with insufficient data:\n\nhttps://review.opendev.org/#/c/158334/\n\nand I thought a similar approach with domain capabilities seemed reasonable.","commit_id":"5017156f5713d9621ce94564b47c1b209221dd04"}]}