From c4f207dd90b125cdaea7d48bb0da5caad9fa84d2 Mon Sep 17 00:00:00 2001
From: Vincent Untz <vuntz@suse.com>
Date: Tue, 19 Aug 2014 17:51:30 +0200
Subject: [PATCH 1/3] Use local variables in functions

This will avoid undesired side-effects.
---
 lib/common                   | 24 +++++++++++++-----------
 lib/helper-neutron-migration |  4 ++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/common b/lib/common
index 6afbbe1..94eda97 100644
--- a/lib/common
+++ b/lib/common
@@ -130,7 +130,8 @@ ask_yes_no_default_no() {
 json_edit=/opt/dell/bin/json-edit
 parse_node_data=/opt/dell/barclamps/provisioner/updates/parse_node_data
 json_read () {
-    file="$1" attr="$2"
+    local file="$1"
+    local attr="$2"
     if [ "$file" = '-' ]; then
         $parse_node_data -a "$attr"
     elif [ -f "$file" ]; then
@@ -139,8 +140,9 @@ json_read () {
 }
 
 get_repo_url_from() {
-  repo="$1" file="$2"
-  attr_repo="${repo//./\\.}"
+  local repo="$1"
+  local file="$2"
+  local attr_repo="${repo//./\\.}"
 
   json_read "$file" attributes.provisioner.suse.autoyast.repos.${attr_repo}.url
 }
@@ -154,15 +156,15 @@ get_allocated_nodes() {
     die "Internal error: get_allocated_nodes requires at least one argument."
   fi
 
-  allocated_nodes=
-  admin_fqdn=$(hostname -f)
+  local allocated_nodes=
+  local admin_fqdn=$(hostname -f)
 
   for node in $(crowbar machines list); do
-    node_data=$(crowbar machines show "$node" | /opt/dell/barclamps/provisioner/updates/parse_node_data -a crowbar.allocated -a fqdn -a target_platform --)
+    local node_data=$(crowbar machines show "$node" | /opt/dell/barclamps/provisioner/updates/parse_node_data -a crowbar.allocated -a fqdn -a target_platform --)
 
-    node_allocated=$(echo "$node_data" | grep ^crowbar.allocated= | cut -d = -f 2-)
-    node_fqdn=$(echo "$node_data" | grep ^fqdn= | cut -d = -f 2-)
-    node_target_platform=$(echo "$node_data" | grep ^target_platform= | cut -d = -f 2-)
+    local node_allocated=$(echo "$node_data" | grep ^crowbar.allocated= | cut -d = -f 2-)
+    local node_fqdn=$(echo "$node_data" | grep ^fqdn= | cut -d = -f 2-)
+    local node_target_platform=$(echo "$node_data" | grep ^target_platform= | cut -d = -f 2-)
 
     # ignore non-allocated node
     if test "x$node_allocated" == "xfalse"; then
@@ -187,10 +189,10 @@ get_allocated_nodes() {
 
   # Filter nodes for specific roles
   if [[ $2 != ""  ]]; then
-    nodes_with_role=$(knife search node "roles:$2" -a name | grep ^name: | cut -d : -f 2)
+    local nodes_with_role=$(knife search node "roles:$2" -a name | grep ^name: | cut -d : -f 2)
 
     # We cannot change allocated_nodes yet, as we need it for string comparison
-    found_nodes=
+    local found_nodes=
     for node in $nodes_with_role; do
       if [[ $allocated_nodes == *$node* ]]; then
         found_nodes="$found_nodes $node"
diff --git a/lib/helper-neutron-migration b/lib/helper-neutron-migration
index 2d64f50..2a52d03 100644
--- a/lib/helper-neutron-migration
+++ b/lib/helper-neutron-migration
@@ -21,7 +21,7 @@ UPGRADE_DATA_DIR=/var/lib/crowbar/upgrade/suse-cloud-3-to-4
 prepare_neutron_migration() {
 
   # Get nodes that have the neutron_server role
-  neutron_servers=$(cat ${UPGRADE_DATA_DIR}/neutron-server.nodes)
+  local neutron_servers=$(cat ${UPGRADE_DATA_DIR}/neutron-server.nodes)
 
   for neutron_server in $neutron_servers; do
     ssh "$neutron_server" 'neutron-db-manage --config-file /etc/neutron/neutron.conf \
@@ -39,7 +39,7 @@ prepare_neutron_migration() {
 finish_neutron_migration() {
 
   # Get nodes that have the neutron_server role
-  neutron_servers=$(cat ${UPGRADE_DATA_DIR}/neutron-server.nodes)
+  local neutron_servers=$(cat ${UPGRADE_DATA_DIR}/neutron-server.nodes)
 
   for neutron_server in $neutron_servers; do
     ssh "$neutron_server" 'neutron-db-manage --config-file /etc/neutron/neutron.conf \
-- 
2.0.4


From 9a86999ea910910c403cf921cea9be29db02cf4a Mon Sep 17 00:00:00 2001
From: Vincent Untz <vuntz@suse.com>
Date: Tue, 19 Aug 2014 17:59:18 +0200
Subject: [PATCH 2/3] Make get_allocated_nodes always set
 get_allocated_nodes_retval as retval

The old code was broken because the name of the variable passed to
get_allocated_nodes to save the result should not be used inside the
function; but it was...
---
 lib/common                        | 18 +++++++++---------
 lib/reboot-nodes                  |  4 ++--
 lib/suse-cloud-upgrade-3-to-4-pre |  7 ++++---
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/lib/common b/lib/common
index 94eda97..0569b9e 100644
--- a/lib/common
+++ b/lib/common
@@ -148,13 +148,13 @@ get_repo_url_from() {
 }
 
 get_allocated_nodes() {
+  # Note: We can't simple echo the result and capture stdout, because of the
+  # logging. Therefore, the return value of this function lives in the
+  # $get_allocated_nodes_retval variable.
+  #
+  # $1 allows to filter for special roles
+
   echo_if_log "Finding allocated nodes..."
-  # Note: $1 is the name of the variable that we will set with the result
-  # We can't simple echo the result and capture stdout, because of the logging.
-  # $2 allows to filter for special roles
-  if test $# -lt 1; then
-    die "Internal error: get_allocated_nodes requires at least one argument."
-  fi
 
   local allocated_nodes=
   local admin_fqdn=$(hostname -f)
@@ -188,8 +188,8 @@ get_allocated_nodes() {
   done
 
   # Filter nodes for specific roles
-  if [[ $2 != ""  ]]; then
-    local nodes_with_role=$(knife search node "roles:$2" -a name | grep ^name: | cut -d : -f 2)
+  if [[ $1 != ""  ]]; then
+    local nodes_with_role=$(knife search node "roles:$1" -a name | grep ^name: | cut -d : -f 2)
 
     # We cannot change allocated_nodes yet, as we need it for string comparison
     local found_nodes=
@@ -201,5 +201,5 @@ get_allocated_nodes() {
     allocated_nodes=$found_nodes
   fi
 
-  eval "$1=\"$allocated_nodes\""
+  get_allocated_nodes_retval=$allocated_nodes
 }
diff --git a/lib/reboot-nodes b/lib/reboot-nodes
index c64cc90..cd7bc0a 100755
--- a/lib/reboot-nodes
+++ b/lib/reboot-nodes
@@ -35,8 +35,8 @@ finish_neutron_migration
 ### Preliminary checks
 echo_summary "Performing sanity checks..."
 
-allocated_nodes=
-get_allocated_nodes allocated_nodes
+get_allocated_nodes
+allocated_nodes=$get_allocated_nodes_retval
 
 for node in $allocated_nodes; do
   if ! ssh "$node" 'rpm -q suse-cloud-release &> /dev/null && test "$(rpm -q --qf "%{version}" suse-cloud-release)" == 4'; then
diff --git a/lib/suse-cloud-upgrade-3-to-4-pre b/lib/suse-cloud-upgrade-3-to-4-pre
index 491242e..a0d32b0 100755
--- a/lib/suse-cloud-upgrade-3-to-4-pre
+++ b/lib/suse-cloud-upgrade-3-to-4-pre
@@ -47,8 +47,8 @@ fi
 
 
 ### Check that all nodes are powered on
-allocated_nodes=
-get_allocated_nodes allocated_nodes
+get_allocated_nodes
+allocated_nodes=$get_allocated_nodes_retval
 
 for node in $allocated_nodes; do
   if ! ssh "$node" true 2> /dev/null; then
@@ -116,7 +116,8 @@ fi
 
 
 ### Collect neutron-server nodes and write them to a file
-get_allocated_nodes neutron_nodes "neutron-server"
+get_allocated_nodes "neutron-server"
+neutron_nodes=$get_allocated_nodes_retval
 echo $neutron_nodes > ${UPGRADE_DATA_DIR}/neutron-server.nodes
 
 
-- 
2.0.4


From 7119426ce4039d595fd2466c136f4640b4e6406c Mon Sep 17 00:00:00 2001
From: Vincent Untz <vuntz@suse.com>
Date: Wed, 20 Aug 2014 12:14:26 +0200
Subject: [PATCH 3/3] Fix log in get_allocated_nodes to mention role

---
 lib/common | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/common b/lib/common
index 0569b9e..987b384 100644
--- a/lib/common
+++ b/lib/common
@@ -154,7 +154,12 @@ get_allocated_nodes() {
   #
   # $1 allows to filter for special roles
 
-  echo_if_log "Finding allocated nodes..."
+  local role=$1
+  if test -z "$role"; then
+    echo_if_log "Finding allocated nodes..."
+  else
+    echo_if_log "Finding allocated nodes with role ${role}..."
+  fi
 
   local allocated_nodes=
   local admin_fqdn=$(hostname -f)
@@ -188,8 +193,8 @@ get_allocated_nodes() {
   done
 
   # Filter nodes for specific roles
-  if [[ $1 != ""  ]]; then
-    local nodes_with_role=$(knife search node "roles:$1" -a name | grep ^name: | cut -d : -f 2)
+  if test -n "$role"; then
+    local nodes_with_role=$(knife search node "roles:$role" -a name | grep ^name: | cut -d : -f 2)
 
     # We cannot change allocated_nodes yet, as we need it for string comparison
     local found_nodes=
-- 
2.0.4

