)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"0c935da246283a2d1a3269998cf010bb62c064c3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"d03839f6_6ed63105","updated":"2022-11-30 14:00:39.000000000","message":"Looking at [1] that does not look like the fix as it uses GlanceApiEdgeNetwork in ServiceNetMap and glance_api_edge_node_ips.\n\n[1] https://github.com/openstack/tripleo-heat-templates/blob/master/deployment/haproxy/haproxy-edge-container-puppet.yaml#L100-L102","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":18002,"name":"John Fulton","email":"fulton@redhat.com","username":"fultonj"},"change_message_id":"fe619dc5049bf4ff2c2dede5f2b2dd2f77f4cfa4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e154756c_e5909ad7","updated":"2022-12-01 19:17:25.000000000","message":"Thank you Rabi and Alan for THT patches 866099 and 866165 to fix LP 1998227. Both fix the bug in the standard use case but merging both is unnecessary so we should only merge one. I\u0027m +2\u0027ing 866165 for the following reasons:\n\n- I agree w/ Marios when he said \"if this is really caused by a typo then lets fix that instead of adding an override if this is not needed?\"\n- It has been verified in an edge deployment\n- It\u0027s true we can override a ServiceNetMap entry but we\u0027d need another update to fix the typo or remove the line completely but we want to merge quickly so the bug no longer blocks\n\n","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"210732f1b9ad8121e0726faba0555760e3e20c71","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"76c1683e_6b9be68d","updated":"2022-12-02 01:19:56.000000000","message":"recheck\n\ntripleo-ci-centos-9-content-provider-zed","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"47a43a33743afc33700337de31173adc69e1d1e7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7d657d9f_536cc359","updated":"2022-12-01 19:13:24.000000000","message":"recheck\n\ntripleo-ci-centos-9-undercloud-upgrade (unrelated failure)","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":6796,"name":"Giulio Fidente","email":"gfidente@redhat.com","username":"gfidente"},"change_message_id":"9ccfe9d683306895fae81be2e84cd2363f81b74b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f96e9e36_7e106529","in_reply_to":"06549bdf_ef07c681","updated":"2022-12-06 18:57:32.000000000","message":"yes I can see that configuring a specific network for glance_api_internal, a network different than that set for glance_api, will not work :( I believe there are other parts in the code (eg. the keystone endpoint configuration and haproxy frontend listener) also assuming for both glance_api and glance_api_internal to be on the same network; it would probably be challenging to get it to work, especially considering implications in more complex scenarios like edge\n\nI think the override would make deployment pass but ignoring whichever parameter is set anyway so if we were to try fix, I would probably remove the entry from the service_net_map entirely; I agree we can try that in a follow up patch to avoid confusion","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"4c6b4cd90f26e3f1df7526d56e97a6155983bc30","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1352874c_fd42d471","in_reply_to":"0f0fb402_a12a582f","updated":"2022-11-30 18:12:47.000000000","message":"That is also a THT patch. Not sure what you mean. However thats not a valid justification IMO.\n\nI would like to understand how edge cases would be broken with that patch tomorrow..","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"2e0e1b54a2a3fde249482cf287a4d4243d0af56f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"18f981cf_05e23d53","in_reply_to":"1352874c_fd42d471","updated":"2022-11-30 21:54:24.000000000","message":"Rabi, I owe you an apology. While I was chatting with John Fulton, I suddenly realized I\u0027ve been mixing up two different patches that want to fix the LP. Indeed, yours is a THT patch, and what\u0027s been sticking in my head all day is [1], which Takashi has since abandoned. THAT\u0027s the one that would break Edge deployments!\n\n[1] https://review.opendev.org/c/openstack/puppet-tripleo/+/866100\n\nI still think my typo needs to be addressed, but it (and another suggestion) can easily be handled in your THT patch. I\u0027m going to park my patch for now, and will likely abandon it later. Meanwhile, I\u0027ll change my -2 to -1 on your patch, and add a couple more thoughts.\n\nSorry again for the confusion!","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":6796,"name":"Giulio Fidente","email":"gfidente@redhat.com","username":"gfidente"},"change_message_id":"13aa8ae71fc12ac4dd9d498cccd2c0c3cd4bffc3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ea04e681_b4710a74","in_reply_to":"2eb664df_c36cd9cd","updated":"2022-12-07 13:59:25.000000000","message":"hi Rabi, thanks for looking!\n\nand sorry I did not mean removing GlanceApiInternalNetwork just from the default map but to remove the GlanceApiInternalNetwork definition entirely; it\u0027s just not a configurable parameter ... it should go on the same network where GlanceApi is\n\nif I understand correctly the overrides will ignore any value set for it right? I think I have seen something similar in haproxy.pp [1] for Ceph Prometheus; point is to make the haproxy Prometheus backend configured on the same network where Ceph Grafana is; we don\u0027t have any definition of a Prometheus network\n\nin other words it seems a parameter we don\u0027t really want to (and can) configure; eventually I would remove it entirely instead of overriding it\n\n1. https://github.com/openstack/puppet-tripleo/blob/master/manifests/haproxy.pp#L1024-L1028","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"6e54bac55cec7bb018ea17928d159241ef73b7a7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"06549bdf_ef07c681","in_reply_to":"377b21e0_eb436b5f","updated":"2022-12-02 08:00:03.000000000","message":"thanks for adding more info here Rabi.\n\nThis is in the gate now and I\u0027d rather we let it go through to address the blocker.\n\nCan we anyway add your overrides to address the bug you outlined above? It really points to an issue/limitation/bug with ServiceNetMap and GlanceApiInternal and probably we should have a bug about that with a \u0027poper\u0027 fix... whether that is actually making sure GlanceApiInternal service uses ServiceNetMap or adding the overrides as in your patch.\n\nSo please don\u0027t abandon your patch, perhaps we can go with it anyway to avoid what you described here.","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"0b06e6a87ebf5372758276523a04ad226d11ebbe","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0f0fb402_a12a582f","in_reply_to":"7541cf8f_fe0f4b13","updated":"2022-11-30 18:02:16.000000000","message":"\u003e \u003e The ServiceNetMap defines .....\n\u003e  \n\u003e I surely know all that and how *node_ips heirdata is generated.\n\nSorry, but I was merely trying to provide full context for the rest of my explanation.\n\n\u003e \n\u003e \u003e In the case of DCN/Edge, we want the glance haproxy to include only Edge nodes (glance_api_edge_node_ips)\n\u003e \n\u003e They why do you set \u0027glance_api_internal_node_ips\u0027 \u003d \u0027glance_api_edge_node_ips\u0027 in HaProxyEdge service?\n\nBecause tripleo::haproxy uses \u0027glance_api_internal_node_ips\u0027 to set up the proxy, and in an edge environment that would include the edge nodes plus the controlplane nodes. We don\u0027t want that. At edge sites, the HAproxyEdge tht ensures proxies at the edge only include the edge nodes, and not any in the controlplane.\n\n\u003e \n\u003e With your patch:\n\u003e \n\u003e For edge haproxy: glance_api_internal_node_ips \u003d glance_api_edge_node_ips\n\u003e For others: glance_api_internal_node_ips (separate network) !\u003d glance_api_node_ips\n\nNot quite. For non-edge (including DCN controlplane), glance_api_internal_node_ips \u003d\u003d glance_api_node_ips because the ServiceNetMap specifies they both use the internal_api network.\n\n\u003e \n\u003e Having said that, what I can tell from THT construct pov, both this patch and the one I proposed would work (check the results for that https://review.rdoproject.org/zuul/build/48d0aa0b58674e78854b7dcb3edf18e6 and I think it did not deserve a -2).\n\nThe root cause of the problem is the typo in my original patch, and this patch fixes that typoe. Your patch does yield positive results, but it doesn\u0027t fix the root problem (my typo). The -2 is because our patches are in different projects (I\u0027d have gone with -1 if yours was a tht patch).\n\nFurthermore, your patch works for non-edge deployments, but would break things for Edge. It would result in an haproxy configuration at the edge that includes nodes in the controlplane, which is incorrect.","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"6229ce70f683ad66f973fe39fe08d5e0af393523","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7541cf8f_fe0f4b13","in_reply_to":"7809f5cc_e0debdb7","updated":"2022-11-30 16:19:49.000000000","message":"\u003e The ServiceNetMap defines .....\n \nI surely know all that and how *node_ips heirdata is generated.\n\n\n\u003e In the case of DCN/Edge, we want the glance haproxy to include only Edge nodes (glance_api_edge_node_ips)\n\nThey why do you set \u0027glance_api_internal_node_ips\u0027 \u003d \u0027glance_api_edge_node_ips\u0027 in HaProxyEdge service?\n\nWith your patch:\n\nFor edge haproxy: glance_api_internal_node_ips \u003d glance_api_edge_node_ips\nFor others: glance_api_internal_node_ips (separate network) !\u003d glance_api_node_ips\n\n\nHaving said that, what I can tell from THT construct pov, both this patch and the one I proposed would work (check the results for that https://review.rdoproject.org/zuul/build/48d0aa0b58674e78854b7dcb3edf18e6 and I think it did not deserve a -2).\n\nBut anyway, if you guys want to go with this, please go ahead.","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"0e8d41da5a29a3692c8c331a48de657d71d27416","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f09d40cd_02e0a2e0","in_reply_to":"d03839f6_6ed63105","updated":"2022-11-30 14:17:42.000000000","message":"I don\u0027t understand your concern. Your link [1] pertains to configuring the haproxy for the internal g-api at a DCN/Edge site. The code directs merely directs puppet to use the glance_api_edge_node_ips (the Edge addresses) rather than the glance_api_internal_node_ips (which will be the ones in the controlplane).\n\nIn case this information helps, I did extensive testing of a DCN/Edge deployment (using RHOSP 17).","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"643ce4714fa6355835fd12400f0e2bfc1aad71a3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7809f5cc_e0debdb7","in_reply_to":"d39abfcb_e4a29c34","updated":"2022-11-30 15:14:18.000000000","message":"The ServiceNetMap defines \u003cservice\u003eNetwork settings for every service, and most of them specify the internal_api network (or fall back to ctlplane). All of the glance services (GlanceApi, GlanceApiEdge and GlanceApiInternal) want to use the internal_api network.\n\nThe reason for the LP bug is I messed up the ServiceNetMap entry. \"GlanceApiEdgeInternalNetwork\" is wrong, because it doesn\u0027t follow the \u003cservice\u003eNetwork pattern. This causes tripleo-ansible to set the glance_api_internal_node_ips to ctlplane addresses. See [2] and note it wants to use the \u003cservice\u003e_network, and falls base to default(\u0027ctlplane\u0027)\n\n[2] https://github.com/openstack/tripleo-ansible/blob/master/tripleo_ansible/roles/tripleo_hieradata/templates/all_nodes.j2#L8\n\nIn the case of DCN/Edge, we want the glance haproxy to include only Edge nodes (glance_api_edge_node_ips) and *not* every glance instance on the internal_api network. All of the glance services are on the internal_api network, but the intent is that Edge sites only use their local instances (the ones at that site).","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"c15e8a1c0baa7063b086167a903d6f3325c3a823","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"377b21e0_eb436b5f","in_reply_to":"e154756c_e5909ad7","updated":"2022-12-01 21:48:02.000000000","message":"Alan/John/Steve, though we\u0027ve decided to go with this patch, I think there is a big issue with it (which I probably failed to make clearly earlier).\n\nWith the current way it\u0027s implemented, GlanceAPIInternal service uses config_settings[1] from GlanceApi and hence the bind_host ip[2] for it would be from GlanceApiNetwork. If someone provides a different network in ServiceNetMap for GlanceApiInternal, it would break the deployment as the service would be listening on a different network(GlanceApiNetwork) than what HaProxy backend is configured for(GlanceApiInternalNetwork).\n\nSo the other patch was more appropriate unless you want to re-implement GlanceAPIInternal service after this patch.\n\nAs for the redundant ServiceNetMap entries are concerned, they won\u0027t have any impact and can be cleaned up later.\n\nMy -1 is still valid here and we still have time to change the decision;)\n\n[1] https://github.com/openstack/tripleo-heat-templates/blob/master/deployment/glance/glance-api-internal-container-puppet.yaml#L103\n\n[2] https://github.com/openstack/tripleo-heat-templates/blob/master/deployment/glance/glance-api-container-puppet.yaml#L576","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"5079a83203a75d21992668fa81263b7e43b0a27a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"5f074b83_5f0a7191","in_reply_to":"ea04e681_b4710a74","updated":"2022-12-12 03:01:50.000000000","message":"Hey Guilio,\n\nNot sure why you mean by GlanceApiInternalNetwork definition. AFAIK there is no THT parameter for it. It can only be defined in ServiceNetMap. \n\nIf you mean dropping the puppet hiera for it (instead of overriding it in services), we can possibly do it in puppet-glance but I think it would break edge deployments.","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"8deade83adfaafab015971a130cfecc77d0346c3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"d39abfcb_e4a29c34","in_reply_to":"f09d40cd_02e0a2e0","updated":"2022-11-30 14:37:28.000000000","message":"I think I read GlanceApiInternalEdgeNetwork in ServiceNetMap is used in the link I pasted, which is incorrect. So, Ignore that part\n\nHowever, for edge deployments you set[1] glance_api_internal_node_ips from the GlanceApiEdgeNetwork. However you have a separate network when it\u0027s not edge. Why do you even need to have a separate network defined for GlanceApiInternal? In my patch I\u0027ve set it the same way you do for edge (i.e set the ips from GlanceApiNetwork).\n\nDo you expect GlanceApiInternal and GlanceApi using separate networks (separate ips). I thought it\u0027s the same API network but different ports.\n\n\n[1]\nglance_api_internal_node_ips: \"%{alias(\u0027glance_api_edge_node_ips\u0027)}\"\nglance_api_internal_node_names: \"%{alias(\u0027glance_api_edge_node_names\u0027)}\"","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"},{"author":{"_account_id":8833,"name":"Rabi Mishra","email":"ramishra@redhat.com","username":"rabi"},"change_message_id":"e39ca88e6cc7c40fc46d7e374ba0544e36481af7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"2eb664df_c36cd9cd","in_reply_to":"f96e9e36_7e106529","updated":"2022-12-07 08:45:22.000000000","message":"Removing the entry for GlanceApiInternalNetwork in default resource registry ServiceNetMap is not by any means a fix as users can override ServiceNetMap in their custom environment that includes it. So overriding is  the only fix if making the two services work with different networks would be challenging. Thats make me wonder why we are still blocking the patch I had proposed.","commit_id":"d24d6121c01514d89c5bef73b6ed5ed969a85661"}]}