)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"b5a8b60b5eccfd46ce70a57e01656afc1d41e068","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e35eeffe_634bca04","updated":"2022-03-11 02:59:28.000000000","message":"btw, the update script was independently tests prior to me submitting the patch.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"e7a459df80ef7399b686f4cf44bec2b4ac38f038","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"bcd26dbe_d3497fa4","updated":"2022-03-18 12:54:36.000000000","message":"sorry, restoring review, I see the question has been answered","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":7144,"name":"James Slagle","email":"jslagle@redhat.com","username":"slagle"},"change_message_id":"167cce7c2b5d33840ec29d42803431c6ce948872","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"37c09177_a5493b23","updated":"2022-03-17 21:21:50.000000000","message":"what about scale down, scale up? would the parameter also need to be set to existing in that case?","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"eed7ca82813b79e246798a891325901f04b17d4e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f6cb92ae_613fe61f","in_reply_to":"37c09177_a5493b23","updated":"2022-03-17 22:31:20.000000000","message":"No, it doesn\u0027t need to be set. We mandate 3 nodes for A/A, so scale down doesn\u0027t really apply. Furthermore, the parameter really only comes into play when a new etcd node is *added* to the cluster (if you just remove a node, it might still be considered a member that is simply no longer available, but that won\u0027t affect the other members). And scaling up is handled using the DistributedComputeScaleOut.yaml or DistributedComputeHCIScaleOut.yaml roles, which don\u0027t include the etcd service. In short, there should always be 3 nodes running etcd.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"}],"deployment/etcd/etcd-container-puppet.yaml":[{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"bde7cb1034eccb9cbd45dfeaa451c6c50f554d73","unresolved":true,"context_lines":[{"line_number":44,"context_line":"  EtcdInitialClusterState:"},{"line_number":45,"context_line":"    description: Initial cluster state (\"new\" or \"existing\"). The default value \"new\""},{"line_number":46,"context_line":"                 needs to be overridden only when an overcloud node is replaced, at"},{"line_number":47,"context_line":"                 which time the value should be set to \"existing\"."},{"line_number":48,"context_line":"    type: string"},{"line_number":49,"context_line":"    default: \u0027new\u0027"},{"line_number":50,"context_line":"    constraints:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"a1f56b2c_d9fcd8c1","line":47,"range":{"start_line":47,"start_character":28,"end_line":47,"end_character":65},"updated":"2022-03-11 15:05:57.000000000","message":"I think it should default to existing, to provide idempotent behavior upon minor updates and not cause any turbulence to the nodes in cluster. After etcd cluster is assembled, initial state of the cluster should be `existing`. It could be auto-detected as \"new\", if \n\netcdctl --peers\u003d\u003call peers\u003e cluster-health\n\ndoes not return \u0027cluster is healthy\u0027 for the queried node being placed into the all peers list.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":18002,"name":"John Fulton","email":"fulton@redhat.com","username":"fultonj"},"change_message_id":"670d18822a579a518b97c8fc36000e24f7daa301","unresolved":true,"context_lines":[{"line_number":44,"context_line":"  EtcdInitialClusterState:"},{"line_number":45,"context_line":"    description: Initial cluster state (\"new\" or \"existing\"). The default value \"new\""},{"line_number":46,"context_line":"                 needs to be overridden only when an overcloud node is replaced, at"},{"line_number":47,"context_line":"                 which time the value should be set to \"existing\"."},{"line_number":48,"context_line":"    type: string"},{"line_number":49,"context_line":"    default: \u0027new\u0027"},{"line_number":50,"context_line":"    constraints:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"6e89a544_89a3a15a","line":47,"range":{"start_line":47,"start_character":28,"end_line":47,"end_character":65},"in_reply_to":"5250907c_7be6ce13","updated":"2022-03-18 13:31:48.000000000","message":"The logic makes sense to me:\n\n- either bootstrap idempotently (the impotence makes stack updates not bootstrap)\n- or add new members because it already exists (existing is only set on scale up)","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"d54307a04d202c6cc92be6e08c62b4a94e8fd63f","unresolved":true,"context_lines":[{"line_number":44,"context_line":"  EtcdInitialClusterState:"},{"line_number":45,"context_line":"    description: Initial cluster state (\"new\" or \"existing\"). The default value \"new\""},{"line_number":46,"context_line":"                 needs to be overridden only when an overcloud node is replaced, at"},{"line_number":47,"context_line":"                 which time the value should be set to \"existing\"."},{"line_number":48,"context_line":"    type: string"},{"line_number":49,"context_line":"    default: \u0027new\u0027"},{"line_number":50,"context_line":"    constraints:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"5250907c_7be6ce13","line":47,"range":{"start_line":47,"start_character":28,"end_line":47,"end_character":65},"in_reply_to":"a1f56b2c_d9fcd8c1","updated":"2022-03-11 15:19:42.000000000","message":"I don\u0027t believe it\u0027s possible to default the initial_cluster_state to \"existing\" because that would preclude being able to bootstrap the cluster. Only existing members are able to add new members to the cluster (it\u0027s a chicken/egg situation). And you can\u0027t use \"cluster-health\" because only a cluster member is able to respond, and there are no members until the cluster is bootstrapped.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":18002,"name":"John Fulton","email":"fulton@redhat.com","username":"fultonj"},"change_message_id":"670d18822a579a518b97c8fc36000e24f7daa301","unresolved":true,"context_lines":[{"line_number":214,"context_line":"                  exit 0"},{"line_number":215,"context_line":"                fi"},{"line_number":216,"context_line":""},{"line_number":217,"context_line":"                # Remove old members. These are nodes in the current list of members"},{"line_number":218,"context_line":"                # that are *not* in the ETCD_INITIAL_CLUSTER."},{"line_number":219,"context_line":"                while read id status name peers clients; do \\"},{"line_number":220,"context_line":"                  if [[ \"${ETCD_INITIAL_CLUSTER}\" !\u003d *\"${name}\u003d${peers}\"* ]]; then"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"358de967_ec8742a6","line":217,"range":{"start_line":217,"start_character":18,"end_line":217,"end_character":37},"updated":"2022-03-18 13:31:48.000000000","message":"If a scale down is run to remove node foo which is a member, then I assume foo is not logically removed from the etcd cluster until this is run. I assume the intention is to ensure any old member is logically removed before adding the new node. I assume also that this portion of code is run during a stack update which is likely during scale up. Is that correct?","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"af3ece9adf88b38baf7d72ba5e001af38a993902","unresolved":true,"context_lines":[{"line_number":214,"context_line":"                  exit 0"},{"line_number":215,"context_line":"                fi"},{"line_number":216,"context_line":""},{"line_number":217,"context_line":"                # Remove old members. These are nodes in the current list of members"},{"line_number":218,"context_line":"                # that are *not* in the ETCD_INITIAL_CLUSTER."},{"line_number":219,"context_line":"                while read id status name peers clients; do \\"},{"line_number":220,"context_line":"                  if [[ \"${ETCD_INITIAL_CLUSTER}\" !\u003d *\"${name}\u003d${peers}\"* ]]; then"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"c5946ff4_00e53a15","line":217,"range":{"start_line":217,"start_character":18,"end_line":217,"end_character":37},"in_reply_to":"358de967_ec8742a6","updated":"2022-03-18 13:44:37.000000000","message":"Your assumptions are correct. The script is executed as a deploy_steps_task (L283), assuming the condition at L293 is met.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"ca24d73ca9c4adb976f31f6b940050ee2284c33c","unresolved":true,"context_lines":[{"line_number":285,"context_line":"                initial_cluster_state: {get_param: EtcdInitialClusterState}"},{"line_number":286,"context_line":"              shell: |"},{"line_number":287,"context_line":"                \"{{ container_cli }}\" exec -ti -u root etcd /etcd_update_members.sh 2\u003e\u00261 | \\"},{"line_number":288,"context_line":"                  tee -a /var/log/containers/stdouts/etcd_update_members.log"},{"line_number":289,"context_line":"              become: true"},{"line_number":290,"context_line":"              failed_when: false"},{"line_number":291,"context_line":"              when:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"ac388f8f_41203bfd","line":288,"range":{"start_line":288,"start_character":45,"end_line":288,"end_character":52},"updated":"2022-03-11 14:56:18.000000000","message":"I\u0027m not sure this dir is not CI only? Maybe use /var/log/containers/etcd/ for that?","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"d54307a04d202c6cc92be6e08c62b4a94e8fd63f","unresolved":true,"context_lines":[{"line_number":285,"context_line":"                initial_cluster_state: {get_param: EtcdInitialClusterState}"},{"line_number":286,"context_line":"              shell: |"},{"line_number":287,"context_line":"                \"{{ container_cli }}\" exec -ti -u root etcd /etcd_update_members.sh 2\u003e\u00261 | \\"},{"line_number":288,"context_line":"                  tee -a /var/log/containers/stdouts/etcd_update_members.log"},{"line_number":289,"context_line":"              become: true"},{"line_number":290,"context_line":"              failed_when: false"},{"line_number":291,"context_line":"              when:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"bef8d675_228930ae","line":288,"range":{"start_line":288,"start_character":45,"end_line":288,"end_character":52},"in_reply_to":"ac388f8f_41203bfd","updated":"2022-03-11 15:19:42.000000000","message":"I don\u0027t believe stdouts is CI only, and I would be disappointed if it were because I believe that\u0027s where puppet errors are reported. Looking around in my own (non-CI) deployment, I see similar log files from other services, e.g.:\n\nhorizon_fix_perms.log\nnova_api_ensure_default_cell.log\nplacement_wait_for_service.log","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"4d9ac444fda9ef6a548770f613c4646a786462c8","unresolved":true,"context_lines":[{"line_number":287,"context_line":"                \"{{ container_cli }}\" exec -ti -u root etcd /etcd_update_members.sh 2\u003e\u00261 | \\"},{"line_number":288,"context_line":"                  tee -a /var/log/containers/stdouts/etcd_update_members.log"},{"line_number":289,"context_line":"              become: true"},{"line_number":290,"context_line":"              failed_when: false"},{"line_number":291,"context_line":"              when:"},{"line_number":292,"context_line":"                - step|int \u003d\u003d 3"},{"line_number":293,"context_line":"                - initial_cluster_state \u003d\u003d \"existing\""}],"source_content_type":"text/x-yaml","patch_set":1,"id":"7af869d5_db109311","line":290,"range":{"start_line":290,"start_character":14,"end_line":290,"end_character":32},"updated":"2022-03-11 14:57:28.000000000","message":"why do you think it should never fail?","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"d54307a04d202c6cc92be6e08c62b4a94e8fd63f","unresolved":true,"context_lines":[{"line_number":287,"context_line":"                \"{{ container_cli }}\" exec -ti -u root etcd /etcd_update_members.sh 2\u003e\u00261 | \\"},{"line_number":288,"context_line":"                  tee -a /var/log/containers/stdouts/etcd_update_members.log"},{"line_number":289,"context_line":"              become: true"},{"line_number":290,"context_line":"              failed_when: false"},{"line_number":291,"context_line":"              when:"},{"line_number":292,"context_line":"                - step|int \u003d\u003d 3"},{"line_number":293,"context_line":"                - initial_cluster_state \u003d\u003d \"existing\""}],"source_content_type":"text/x-yaml","patch_set":1,"id":"b37b039d_90e1baa9","line":290,"range":{"start_line":290,"start_character":14,"end_line":290,"end_character":32},"in_reply_to":"7af869d5_db109311","updated":"2022-03-11 15:19:42.000000000","message":"I pondered this quite a lot. In then end, I decided that any issues related to etcd cluster membership (which only happen when replacing a node) should fail the deployment. From experiences working with someone who encountered the membership issue in a lab PoC, it was relatively straightforward to fix up the etcd cluster after the deployment.\n\nThat said, I am increasingly confident that the script will not throw an unexpected error, and would be OK removing the failed_when if that remains your recommendation.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"78ad210e8f989fb39af35dbfa75ed115ed74bb2e","unresolved":true,"context_lines":[{"line_number":287,"context_line":"                \"{{ container_cli }}\" exec -ti -u root etcd /etcd_update_members.sh 2\u003e\u00261 | \\"},{"line_number":288,"context_line":"                  tee -a /var/log/containers/stdouts/etcd_update_members.log"},{"line_number":289,"context_line":"              become: true"},{"line_number":290,"context_line":"              failed_when: false"},{"line_number":291,"context_line":"              when:"},{"line_number":292,"context_line":"                - step|int \u003d\u003d 3"},{"line_number":293,"context_line":"                - initial_cluster_state \u003d\u003d \"existing\""}],"source_content_type":"text/x-yaml","patch_set":1,"id":"b5259125_2f118df1","line":290,"range":{"start_line":290,"start_character":14,"end_line":290,"end_character":32},"in_reply_to":"b37b039d_90e1baa9","updated":"2022-03-11 15:24:10.000000000","message":"Edit:\n\n\"any issues related to etcd cluster membership (which only happen when replacing a node) should _NOT_ fail the deployment.\"","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":18002,"name":"John Fulton","email":"fulton@redhat.com","username":"fultonj"},"change_message_id":"670d18822a579a518b97c8fc36000e24f7daa301","unresolved":true,"context_lines":[{"line_number":293,"context_line":"                - initial_cluster_state \u003d\u003d \"existing\""},{"line_number":294,"context_line":"          - if:"},{"line_number":295,"context_line":"            - internal_tls_enabled"},{"line_number":296,"context_line":"            - - name: Certificate generation"},{"line_number":297,"context_line":"                when: step|int \u003d\u003d 1"},{"line_number":298,"context_line":"                block:"},{"line_number":299,"context_line":"                  - include_role:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"ba62e6ca_125775fd","line":296,"range":{"start_line":296,"start_character":22,"end_line":296,"end_character":44},"updated":"2022-03-18 13:31:48.000000000","message":"So the Certificate code is essentially unchanged, you\u0027re just indenting it further so it can be list_concat\u0027d with the new cluster membership task. Is that right?","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"af3ece9adf88b38baf7d72ba5e001af38a993902","unresolved":true,"context_lines":[{"line_number":293,"context_line":"                - initial_cluster_state \u003d\u003d \"existing\""},{"line_number":294,"context_line":"          - if:"},{"line_number":295,"context_line":"            - internal_tls_enabled"},{"line_number":296,"context_line":"            - - name: Certificate generation"},{"line_number":297,"context_line":"                when: step|int \u003d\u003d 1"},{"line_number":298,"context_line":"                block:"},{"line_number":299,"context_line":"                  - include_role:"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"d5ca4bb9_72bfba3b","line":296,"range":{"start_line":296,"start_character":22,"end_line":296,"end_character":44},"in_reply_to":"ba62e6ca_125775fd","updated":"2022-03-18 13:44:37.000000000","message":"Correct. It shifts to accommodate the list_concat at L282.","commit_id":"d3d20bb4423a66ff0e547b487041716f78ff83df"}]}