)]}'
{"README.md":[{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e64e89c30743ef8a4496757a7a2296af423bdf6a","unresolved":false,"context_lines":[{"line_number":84,"context_line":"The operation expects:"},{"line_number":85,"context_line":" 1. `juju run-action hacluster/N pause --wait`. This will make sure no Pacemaker"},{"line_number":86,"context_line":" resources run on the unit"},{"line_number":87,"context_line":" 1. `juju config hacluster maintenance-mode\u003dtrue`. This will set all Pacemaker"},{"line_number":88,"context_line":" resources unmanaged. It is expected that the units holding VIP resources"},{"line_number":89,"context_line":" continue running during the operation."},{"line_number":90,"context_line":" 1. `juju remove-unit principal-unit/N`. Iterate through this step as many times"},{"line_number":91,"context_line":" as units want to be removed (ie. to scale back from 6 to 3 units)."},{"line_number":92,"context_line":" 1. `juju run-action hacluster/leader update-ring i-really-mean-it\u003dtrue --wait`."}],"source_content_type":"text/x-gfm","patch_set":9,"id":"bf51134e_84e355a4","line":89,"range":{"start_line":87,"start_character":0,"end_line":89,"end_character":39},"updated":"2020-07-24 12:08:03.000000000","message":"I guess we no longer need this given it is not included in the functional test https://github.com/openstack-charmers/zaza-openstack-tests/pull/369/files#diff-a4b8da2c71d905a5c912d727f9ce482cR74","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e64e89c30743ef8a4496757a7a2296af423bdf6a","unresolved":false,"context_lines":[{"line_number":87,"context_line":" 1. `juju config hacluster maintenance-mode\u003dtrue`. This will set all Pacemaker"},{"line_number":88,"context_line":" resources unmanaged. It is expected that the units holding VIP resources"},{"line_number":89,"context_line":" continue running during the operation."},{"line_number":90,"context_line":" 1. `juju remove-unit principal-unit/N`. Iterate through this step as many times"},{"line_number":91,"context_line":" as units want to be removed (ie. to scale back from 6 to 3 units)."},{"line_number":92,"context_line":" 1. `juju run-action hacluster/leader update-ring i-really-mean-it\u003dtrue --wait`."},{"line_number":93,"context_line":" This step will remove corosync nodes from the ring and update corosync.conf to"}],"source_content_type":"text/x-gfm","patch_set":9,"id":"bf51134e_c461cd13","line":90,"range":{"start_line":90,"start_character":1,"end_line":90,"end_character":2},"updated":"2020-07-24 12:08:03.000000000","message":"\"1.\" everywhere doesn\u0027t look right.\n\nHowever, you could use automatic numbering like this:\n\nhttps://docutils.sourceforge.io/docs/user/rst/quickref.html#enumerated-lists\n\n#. A\n#. B\n#. C","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e64e89c30743ef8a4496757a7a2296af423bdf6a","unresolved":false,"context_lines":[{"line_number":92,"context_line":" 1. `juju run-action hacluster/leader update-ring i-really-mean-it\u003dtrue --wait`."},{"line_number":93,"context_line":" This step will remove corosync nodes from the ring and update corosync.conf to"},{"line_number":94,"context_line":" list an updated number of nodes (min_quorum is recalculated)."},{"line_number":95,"context_line":" 1. `juju config hacluster maintenance-mode\u003dfalse`. Pacemaker will manage its"},{"line_number":96,"context_line":" resources from this moment onwards."},{"line_number":97,"context_line":""},{"line_number":98,"context_line":" In case a unit goes into lost state (ie. caused by hardware failure), the"},{"line_number":99,"context_line":" initial step (pause a unit) can be skipped. Unit removal may also be replaced"}],"source_content_type":"text/x-gfm","patch_set":9,"id":"bf51134e_049e2506","line":96,"range":{"start_line":95,"start_character":0,"end_line":96,"end_character":36},"updated":"2020-07-24 12:08:03.000000000","message":"Ditto, not sure we need the maintenance mode reference anymore.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"}],"charmhelpers/core/hookenv.py":[{"author":{"_account_id":8992,"name":"Billy Olsen","email":"billy.olsen@canonical.com","username":"billy-olsen"},"change_message_id":"8b156c13bca5e5df6cf2e1798c93989bbb3b2bda","unresolved":true,"context_lines":[{"line_number":226,"context_line":"        raise ValueError(\u0027Must specify neither or both of relation_name and service_or_unit\u0027)"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"def departing_unit():"},{"line_number":230,"context_line":"    \"\"\"The departing unit for the current relation hook."},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    Available since juju 2.8\"\"\""}],"source_content_type":"text/x-python","patch_set":22,"id":"02f875e5_dbd1b2aa","line":229,"updated":"2021-01-28 20:46:47.000000000","message":"note this depends on https://github.com/juju/charm-helpers/pull/563 landing first, then this updating to reflect that via a c-h sync","commit_id":"3446a42085f87edc2a90bd9beefa6f62856ab688"},{"author":{"_account_id":2424,"name":"Felipe Reyes","email":"felipe.reyes@canonical.com","username":"freyes"},"change_message_id":"45113ac41210e3e99071e360187cc0e3c54ce9d7","unresolved":false,"context_lines":[{"line_number":226,"context_line":"        raise ValueError(\u0027Must specify neither or both of relation_name and service_or_unit\u0027)"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"def departing_unit():"},{"line_number":230,"context_line":"    \"\"\"The departing unit for the current relation hook."},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    Available since juju 2.8\"\"\""}],"source_content_type":"text/x-python","patch_set":22,"id":"c2e05ce0_90fa7f76","line":229,"in_reply_to":"02f875e5_dbd1b2aa","updated":"2021-02-17 14:10:07.000000000","message":"Done","commit_id":"3446a42085f87edc2a90bd9beefa6f62856ab688"}],"hooks/hooks.py":[{"author":{"_account_id":935,"name":"James Page","email":"ringo.page@gmail.com","username":"james-page"},"change_message_id":"bd30842926c63cc897645700a8a4747a523486b9","unresolved":false,"context_lines":[{"line_number":276,"context_line":""},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"@hooks.hook(\u0027hanode-relation-departed\u0027,"},{"line_number":279,"context_line":"            \u0027hanode-relation-broken\u0027)"},{"line_number":280,"context_line":"def hanode_relation_departed(relid\u003dNone):"},{"line_number":281,"context_line":"    if config(\u0027maintenance-mode\u0027):"},{"line_number":282,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_2cba7de0","line":279,"updated":"2020-07-21 09:49:12.000000000","message":"-broken seems surplus as this only happens when the relation is being torn down as the last hook execution.","commit_id":"ed191a344902e44c91c8d5d7150a2f1c97f48332"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e64e89c30743ef8a4496757a7a2296af423bdf6a","unresolved":false,"context_lines":[{"line_number":277,"context_line":"        ha_relation_changed()"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":""},{"line_number":280,"context_line":"@hooks.hook(\u0027hanode-relation-departed\u0027)"},{"line_number":281,"context_line":"def hanode_relation_departed(relid\u003dNone):"},{"line_number":282,"context_line":"    if config(\u0027maintenance-mode\u0027):"},{"line_number":283,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"},{"line_number":284,"context_line":"        return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    # The departee unit will remove itself"},{"line_number":287,"context_line":"    if remote_unit() \u003d\u003d local_unit():"}],"source_content_type":"text/x-python","patch_set":9,"id":"bf51134e_6401e106","line":284,"range":{"start_line":280,"start_character":0,"end_line":284,"end_character":14},"updated":"2020-07-24 12:08:03.000000000","message":"I think it would be good to document that node removals will not be processed while the cluster is in the maintenance mode.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"99f1744bd0ed373836ce1e2a189671fcb11d9b66","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"},{"line_number":284,"context_line":"        return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    # The departee unit will remove itself"},{"line_number":287,"context_line":"    if remote_unit() \u003d\u003d local_unit():"},{"line_number":288,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":289,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":290,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name))"},{"line_number":291,"context_line":"            return"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    # Note(aluria): all units will update corosync.conf list of nodes"},{"line_number":294,"context_line":"    # in the aim of having up to date stored configurations. However,"}],"source_content_type":"text/x-python","patch_set":9,"id":"bf51134e_3fced282","line":291,"range":{"start_line":286,"start_character":0,"end_line":291,"end_character":18},"updated":"2020-07-24 12:25:19.000000000","message":"I am not sure about this. Is there ever a case where JUJU_REMOTE_UNIT \u003d\u003d JUJU_UNIT_NAME?\n\nBased on what I see in the newly added functionality (as of Juju 2.8), there is a new environment variable exposed called JUJU_DEPARTING_UNIT which can be equal to the local unit name (JUJU_UNIT_NAME) but it doesn\u0027t look like it affects JUJU_REMOTE_UNIT.\n\nhttps://github.com/juju/juju/blob/juju-2.8.0/worker/uniter/relation/resolver.go#L173-L185\nhttps://github.com/juju/juju/blob/juju-2.8.0/worker/uniter/runner/context/context.go#L1009-L1013\nhttps://github.com/juju/juju/commit/5a978061e1c031f6b4f38475696b5ed352fc39b0\nhttps://github.com/juju/juju/commit/23e9e6270a7","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":2424,"name":"Felipe Reyes","email":"felipe.reyes@canonical.com","username":"freyes"},"change_message_id":"ddbbd3bc89e6a5a441825819139bcd3497101a17","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"},{"line_number":284,"context_line":"        return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    # The departee unit will remove itself"},{"line_number":287,"context_line":"    if remote_unit() \u003d\u003d local_unit():"},{"line_number":288,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":289,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":290,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name))"},{"line_number":291,"context_line":"            return"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    # Note(aluria): all units will update corosync.conf list of nodes"},{"line_number":294,"context_line":"    # in the aim of having up to date stored configurations. However,"}],"source_content_type":"text/x-python","patch_set":9,"id":"d8c9d644_f252f2d6","line":291,"range":{"start_line":286,"start_character":0,"end_line":291,"end_character":18},"in_reply_to":"70bcab6d_b161e950","updated":"2021-01-28 19:53:22.000000000","message":"The latest version of the patch uses the previously described approach.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":2424,"name":"Felipe Reyes","email":"felipe.reyes@canonical.com","username":"freyes"},"change_message_id":"933ae11798cef6ae8ede466ba1fb3662cc359691","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"},{"line_number":284,"context_line":"        return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    # The departee unit will remove itself"},{"line_number":287,"context_line":"    if remote_unit() \u003d\u003d local_unit():"},{"line_number":288,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":289,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":290,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name))"},{"line_number":291,"context_line":"            return"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    # Note(aluria): all units will update corosync.conf list of nodes"},{"line_number":294,"context_line":"    # in the aim of having up to date stored configurations. However,"}],"source_content_type":"text/x-python","patch_set":9,"id":"70bcab6d_b161e950","line":291,"range":{"start_line":286,"start_character":0,"end_line":291,"end_character":18},"in_reply_to":"842c0c1d_d255464b","updated":"2021-01-28 13:00:50.000000000","message":"When running \"juju remove-unit\" for a principal related with hacluster this is how the relevante JUJU_* environment variable look:\n\nroot@juju-60ffb3-2:/var/lib/juju/agents/unit-hacluster-1/charm# env | grep JUJU_\nJUJU_UNIT_NAME\u003dhacluster/1\nJUJU_RELATION_ID\u003dhanode:1\nJUJU_DEPARTING_UNIT\u003dhacluster/1\nJUJU_VERSION\u003d2.8.5\nJUJU_PRINCIPAL_UNIT\u003dpercona-cluster/2\nJUJU_REMOTE_UNIT\u003dhacluster/0\nJUJU_HOOK_NAME\u003dhanode-relation-departed\nJUJU_RELATION\u003dhanode\nJUJU_REMOTE_APP\u003dhacluster\nJUJU_DISPATCH_PATH\u003dhooks/hanode-relation-departed\n\nSo it is correct that JUJU_REMOTE_UNIT \u003d\u003d JUJU_UNIT_NAME won\u0027t happen during the execution of hanode-relation-departed hook.\n\nWe could make this patch work for juju\u003e\u003d2.8 taking advantage of JUJU_DEPARTING_UNIT and users running juju\u003c\u003d2.7 would need to run the \u0027update-ring\u0027 action, I believe this is a lot better than the current situation is.\n\nThoughts?","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"02c75ac8c2ed60e34eb662fc6c576c634bf86743","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        log(\u0027pcmk is in maintenance mode - skip any action\u0027, DEBUG)"},{"line_number":284,"context_line":"        return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    # The departee unit will remove itself"},{"line_number":287,"context_line":"    if remote_unit() \u003d\u003d local_unit():"},{"line_number":288,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":289,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":290,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name))"},{"line_number":291,"context_line":"            return"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"    # Note(aluria): all units will update corosync.conf list of nodes"},{"line_number":294,"context_line":"    # in the aim of having up to date stored configurations. However,"}],"source_content_type":"text/x-python","patch_set":9,"id":"842c0c1d_d255464b","line":291,"range":{"start_line":286,"start_character":0,"end_line":291,"end_character":18},"in_reply_to":"bf51134e_3fced282","updated":"2020-11-27 14:02:42.000000000","message":"This comment is still worth addressing/answering. Indeed looking at the log, this code-path is never executed.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e20e7e4a5caf6eea5a30fc30462f8a412789ec6f","unresolved":true,"context_lines":[{"line_number":348,"context_line":"        # the corosync.conf list of nodes. If it\u0027s the case, no other"},{"line_number":349,"context_line":"        # action will be run (a future hook re: ready\u003dTrue may trigger"},{"line_number":350,"context_line":"        # other logic)"},{"line_number":351,"context_line":"        if (remote_unit() !\u003d principal_unit() and"},{"line_number":352,"context_line":"            trigger_corosync_update_from_leader("},{"line_number":353,"context_line":"                remote_unit(), relid_hanode[0]"},{"line_number":354,"context_line":"        )):"}],"source_content_type":"text/x-python","patch_set":24,"id":"d13ee724_3958a48d","line":351,"updated":"2021-02-23 15:44:56.000000000","message":"This `remote_unit() !\u003d principal_unit()` logic in a relation hook probably calls for future refactoring since it does not make sense outside of the broader context. What it means is that a relation hook for the container-scoped relationship (primary to subordinate) can be called from a different relation hook (in our case, the hanode-relation-changed hook) since normally for a container-scoped relationship it is always the case that JUJU_REMOTE_UNIT \u003d\u003d JUJU_PRINCIPAL_UNIT.","commit_id":"fdfb865eed41dea0fafd8b27099d144abb6da510"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"edda8cea2fe3cced1fd92c3ff441ecb0adb3d870","unresolved":true,"context_lines":[{"line_number":312,"context_line":"        # won\u0027t enter here. The operator is then expected to run the"},{"line_number":313,"context_line":"        # `update-ring` action."},{"line_number":314,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":315,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":316,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name), level\u003dINFO)"},{"line_number":317,"context_line":"            return"},{"line_number":318,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"90a8f867_bac5fae4","line":315,"updated":"2021-02-24 12:13:31.000000000","message":"I found that for a departing unit itself, there are as many \u003crelname\u003e-relation-departed hooks executed where JUJU_DEPARTING_UNIT \u003d\u003d JUJU_UNIT_NAME as there are remote units (there\u0027s also a Juju bug about those not being represented via multiple entries in the status log https://bugs.launchpad.net/juju/+bug/1916746).\n\nhttps://review.opendev.org/c/openstack/charm-hacluster/+/741592/25/hooks/utils.py#1622\n\nThe reason it doesn\u0027t fail during functional testing is probably due to the fact that pcmk.commit(cmd) is called with a default argument value for failure_is_fatal (\u003d\u003dFalse):\n\nhttps://review.opendev.org/plugins/gitiles/openstack/charm-hacluster/+/3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3/hooks/pcmk.py#58\n\nWe should probably make this properly idempotent without silently ignoring non-zero exit codes.","commit_id":"3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"a219b758c9d15ecf9e4e465a6321f9fe46023250","unresolved":true,"context_lines":[{"line_number":312,"context_line":"        # won\u0027t enter here. The operator is then expected to run the"},{"line_number":313,"context_line":"        # `update-ring` action."},{"line_number":314,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":315,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":316,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name), level\u003dINFO)"},{"line_number":317,"context_line":"            return"},{"line_number":318,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"d06ffd4f_d481ed85","line":315,"in_reply_to":"90a8f867_bac5fae4","updated":"2021-02-25 20:29:09.000000000","message":"After another debugging session, here is what I discovered:\n\n1. remove_corosync_node()\u0027s command \u0027crm_node --force -R \u003cnode-name\u003e\u0027 always fails (prints \u0027Connection to crmd failed: Connection refused (111)\u0027 and exits 201) because we stop corosync just before. Even a simple `sudo crm status` fails in the same way at this point.\n2. If I don\u0027t stop corosync just before that command, it succeeds and seems to be idempotent, unfortunately it has no real effect: the corosync ring remains wrong if we don\u0027t run the `update-ring` action.\n\nSo I\u0027m going to ditch this part as I think this review is complex enough. It will then always be necessary to run the `update-ring` action, which was the original intent of this review anyway. This can be revisited in a follow-up review if need be.","commit_id":"3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"1d1ccb232f0fc684d373287f2be7a1ec8485686f","unresolved":false,"context_lines":[{"line_number":312,"context_line":"        # won\u0027t enter here. The operator is then expected to run the"},{"line_number":313,"context_line":"        # `update-ring` action."},{"line_number":314,"context_line":"        node_name \u003d socket.gethostname()"},{"line_number":315,"context_line":"        if remove_corosync_node(node_name):"},{"line_number":316,"context_line":"            log(\u0027{} corosync node removed\u0027.format(node_name), level\u003dINFO)"},{"line_number":317,"context_line":"            return"},{"line_number":318,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"ce0114ed_9b8bef02","line":315,"in_reply_to":"d06ffd4f_d481ed85","updated":"2021-03-12 13:29:50.000000000","message":"Done","commit_id":"3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3"}],"hooks/utils.py":[{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"e64e89c30743ef8a4496757a7a2296af423bdf6a","unresolved":false,"context_lines":[{"line_number":1481,"context_line":""},{"line_number":1482,"context_line":"    service_stop(\u0027corosync\u0027)"},{"line_number":1483,"context_line":"    pcmk.commit(\u0027crm_node -R {}\u0027.format(node_id))"},{"line_number":1484,"context_line":"    pcmk.commit("},{"line_number":1485,"context_line":"        \"cibadmin --delete --obj_type nodes\""},{"line_number":1486,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1487,"context_line":"        \"\".format(node_name)"},{"line_number":1488,"context_line":"    )"},{"line_number":1489,"context_line":"    pcmk.commit("},{"line_number":1490,"context_line":"        \"cibadmin --delete --obj_type status\""},{"line_number":1491,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode_state uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1492,"context_line":"        \"\".format(node_name)"},{"line_number":1493,"context_line":"    )"},{"line_number":1494,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":9,"id":"bf51134e_49e5d809","line":1493,"range":{"start_line":1484,"start_character":0,"end_line":1493,"end_character":5},"updated":"2020-07-24 12:08:03.000000000","message":"Although this page suggests that this is needed\n\nhttps://clusterlabs.org/pacemaker/doc/en-US/Pacemaker/1.1/html/Pacemaker_Explained/_removing_a_cluster_node.html\n\nit looks like this was needed at some point for AIS-based clusters.\n\nhttps://github.com/ClusterLabs/pacemaker/commit/e8f37ea83740a1e38e72007bb3f30bf3424d0b19\n\nA corosync-specific page for 1.1 says:\n\nhttps://clusterlabs.org/pacemaker/doc/en-US/Pacemaker/1.1/html/Pacemaker_Explained/_removing_a_corosync_node.html\n\"From one of the remaining active cluster nodes, tell Pacemaker to forget about the removed host, which ***will also delete the node from the CIB***:\n\n# crm_node -R pcmk-1\"\n\n\nSo there is no need to do the additional operations via cibadmin it seems.\n\nFor 2.0 the same content is true:\n\nhttps://clusterlabs.org/pacemaker/doc/en-US/Pacemaker/2.0/html/Pacemaker_Administration/_removing_a_corosync_node.html\n\nI can see that the cache cleanup code is triggered by using `crm_node -R` so I think we should be good without manual cibadmin invocations.\n\n2.0:\nhttps://github.com/ClusterLabs/pacemaker/blob/2deceaa3ae1fbadd844f5c5b47fd33129fa2c227/tools/crm_node.c#L646-L648\nhttps://github.com/ClusterLabs/pacemaker/blob/2deceaa3ae1fbadd844f5c5b47fd33129fa2c227/tools/crm_node.c#L462-L486\nhttps://github.com/ClusterLabs/pacemaker/blob/2deceaa3ae1fbadd844f5c5b47fd33129fa2c227/tools/crm_node.c#L394-L450\n\n\n1.1\nhttps://github.com/ClusterLabs/pacemaker/blob/28dd98fada1e39a9219df0c115d978a0f20352a8/tools/crm_node.c#L496-L507\nhttps://github.com/ClusterLabs/pacemaker/blob/28dd98fada1e39a9219df0c115d978a0f20352a8/tools/crm_node.c#L330-L401","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":2424,"name":"Felipe Reyes","email":"felipe.reyes@canonical.com","username":"freyes"},"change_message_id":"ddbbd3bc89e6a5a441825819139bcd3497101a17","unresolved":false,"context_lines":[{"line_number":1481,"context_line":""},{"line_number":1482,"context_line":"    service_stop(\u0027corosync\u0027)"},{"line_number":1483,"context_line":"    pcmk.commit(\u0027crm_node -R {}\u0027.format(node_id))"},{"line_number":1484,"context_line":"    pcmk.commit("},{"line_number":1485,"context_line":"        \"cibadmin --delete --obj_type nodes\""},{"line_number":1486,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1487,"context_line":"        \"\".format(node_name)"},{"line_number":1488,"context_line":"    )"},{"line_number":1489,"context_line":"    pcmk.commit("},{"line_number":1490,"context_line":"        \"cibadmin --delete --obj_type status\""},{"line_number":1491,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode_state uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1492,"context_line":"        \"\".format(node_name)"},{"line_number":1493,"context_line":"    )"},{"line_number":1494,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":9,"id":"f68f0ebc_ee35e583","line":1493,"range":{"start_line":1484,"start_character":0,"end_line":1493,"end_character":5},"in_reply_to":"59442c6f_7cd86ba4","updated":"2021-01-28 19:53:22.000000000","message":"I got to the same conclusion from the docs and when testing the changes I saw no issue with just calling \"crm_node -R\", I also simplified it a bit using the node_name instead of getting the node_id since crm_node is happy receiving the id or the name.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"02c75ac8c2ed60e34eb662fc6c576c634bf86743","unresolved":true,"context_lines":[{"line_number":1481,"context_line":""},{"line_number":1482,"context_line":"    service_stop(\u0027corosync\u0027)"},{"line_number":1483,"context_line":"    pcmk.commit(\u0027crm_node -R {}\u0027.format(node_id))"},{"line_number":1484,"context_line":"    pcmk.commit("},{"line_number":1485,"context_line":"        \"cibadmin --delete --obj_type nodes\""},{"line_number":1486,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1487,"context_line":"        \"\".format(node_name)"},{"line_number":1488,"context_line":"    )"},{"line_number":1489,"context_line":"    pcmk.commit("},{"line_number":1490,"context_line":"        \"cibadmin --delete --obj_type status\""},{"line_number":1491,"context_line":"        \"\"\" --crm_xml \u0027\u003cnode_state uname\u003d\"_{}_\"/\u003e\u0027\"\"\""},{"line_number":1492,"context_line":"        \"\".format(node_name)"},{"line_number":1493,"context_line":"    )"},{"line_number":1494,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":9,"id":"59442c6f_7cd86ba4","line":1493,"range":{"start_line":1484,"start_character":0,"end_line":1493,"end_character":5},"in_reply_to":"bf51134e_49e5d809","updated":"2020-11-27 14:02:42.000000000","message":"This comment is still worth addressing/answering.","commit_id":"c5ca8bca64daba9aa7246b7f5ef770616c7b2644"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"f816cf80dda66765593835c7ccd636cbd3ea6f76","unresolved":true,"context_lines":[{"line_number":1588,"context_line":"            log(\"update_node_list: run action [{}] against old node [{}]\""},{"line_number":1589,"context_line":"                \"\".format(action, old_node))"},{"line_number":1590,"context_line":"            cmd \u003d CMD_TMPL.format(action, old_node)"},{"line_number":1591,"context_line":"            pcmk.commit(cmd)"},{"line_number":1592,"context_line":""},{"line_number":1593,"context_line":"    return diff_nodes"},{"line_number":1594,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"282c89c5_741086b2","line":1591,"updated":"2021-02-24 12:37:34.000000000","message":"I think we should error out here if the commit operation fails. The default used in this function is failure_is_fatal\u003dFalse and we ignore the returned exit code here.\n\nhttps://review.opendev.org/plugins/gitiles/openstack/charm-hacluster/+/3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3/hooks/pcmk.py#58\n\nThis might lead to situations where a node was not deleted from the ring while the charm code thinks it was.","commit_id":"3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"b99b03adc9005f82b7b17c337cb17baec71c011a","unresolved":false,"context_lines":[{"line_number":1588,"context_line":"            log(\"update_node_list: run action [{}] against old node [{}]\""},{"line_number":1589,"context_line":"                \"\".format(action, old_node))"},{"line_number":1590,"context_line":"            cmd \u003d CMD_TMPL.format(action, old_node)"},{"line_number":1591,"context_line":"            pcmk.commit(cmd)"},{"line_number":1592,"context_line":""},{"line_number":1593,"context_line":"    return diff_nodes"},{"line_number":1594,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"339d625f_540a789e","line":1591,"in_reply_to":"282c89c5_741086b2","updated":"2021-02-25 20:35:40.000000000","message":"Done","commit_id":"3ee12babc1a8d9168d73ec6b68ccabf631ed4bc3"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"07a9de4f5bfa50688e515513e548dc73ab7507c7","unresolved":true,"context_lines":[{"line_number":538,"context_line":"        enable_stonith()"},{"line_number":539,"context_line":"        set_stonith_configured(True)"},{"line_number":540,"context_line":"    else:"},{"line_number":541,"context_line":"        disable_stonith()"},{"line_number":542,"context_line":""},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"def configure_legacy_stonith():"}],"source_content_type":"text/x-python","patch_set":29,"id":"9338fc70_47f389f6","line":541,"updated":"2021-03-04 09:21:50.000000000","message":"This used to be non-fatal before and having made it fatal now seems to be an issue, as it sometimes fails. configure_pacemaker_remote_stonith_resource() is always falsy because we\u0027re not running on MAAS, so we end up unconditionally (re-)disabling stonith 8 times during a test run. So I\u0027ll make it non-fatal again and add some inline comments.","commit_id":"e554b531930c29741df479020165f560a4104409"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"07a9de4f5bfa50688e515513e548dc73ab7507c7","unresolved":true,"context_lines":[{"line_number":541,"context_line":"        disable_stonith()"},{"line_number":542,"context_line":""},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"def configure_legacy_stonith():"},{"line_number":545,"context_line":"    if config(\u0027stonith_enabled\u0027) not in [\u0027true\u0027, \u0027True\u0027, True]:"},{"line_number":546,"context_line":"        if configure_pacemaker_remote_stonith_resource():"},{"line_number":547,"context_line":"            log(\u0027Not disabling STONITH as pacemaker remotes are present\u0027,"}],"source_content_type":"text/x-python","patch_set":29,"id":"ff15b502_ade76765","line":544,"updated":"2021-03-04 09:21:50.000000000","message":"This entire function is actually dead code and can be removed. The `stonith_enabled` config option has been deprecated.","commit_id":"e554b531930c29741df479020165f560a4104409"},{"author":{"_account_id":31289,"name":"Aurelien Lourot","email":"aurelien.lourot@gmail.com","username":"lourot"},"change_message_id":"657bdcbb922255e44deb92e21dc29b481dfdbdce","unresolved":true,"context_lines":[{"line_number":541,"context_line":"        disable_stonith()"},{"line_number":542,"context_line":""},{"line_number":543,"context_line":""},{"line_number":544,"context_line":"def configure_legacy_stonith():"},{"line_number":545,"context_line":"    if config(\u0027stonith_enabled\u0027) not in [\u0027true\u0027, \u0027True\u0027, True]:"},{"line_number":546,"context_line":"        if configure_pacemaker_remote_stonith_resource():"},{"line_number":547,"context_line":"            log(\u0027Not disabling STONITH as pacemaker remotes are present\u0027,"}],"source_content_type":"text/x-python","patch_set":29,"id":"f5cdb192_979008ad","line":544,"in_reply_to":"ff15b502_ade76765","updated":"2021-03-04 09:55:02.000000000","message":"Doing in a separate review: https://review.opendev.org/c/openstack/charm-hacluster/+/778626","commit_id":"e554b531930c29741df479020165f560a4104409"}]}