Change in vdsm[master]: check-merged: split functions by logic and few nits

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has uploaded a new change for review.

Change subject: check-merged: split functions by logic and few nits
......................................................................

check-merged: split functions by logic and few nits

adding local, var renames

Change-Id: I74f9326289b972abaef4507ab4d2582a8d641778
Signed-off-by: Yaniv Bronhaim 
---
M automation/check-merged.sh
1 file changed, 27 insertions(+), 18 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/70825/1

diff --git a/automation/check-merged.sh b/automation/check-merged.sh
index ef81436..e6933b9 100755
--- a/automation/check-merged.sh
+++ b/automation/check-merged.sh
@@ -6,17 +6,25 @@
 # direct backend, but without KVM(much slower).
 ! [[ -c "/dev/kvm" ]] && mknod /dev/kvm c 10 232

+# Fix when running in an el* chroot in fc2* host
+[[ -e /usr/bin/qemu-kvm ]] \
+|| ln -s /usr/libexec/qemu-kvm /usr/bin/qemu-kvm

+# ENV vars
 DISTRO='el7'
 VM_NAME="vdsm_functional_tests_host-${DISTRO}"
 AUTOMATION="$PWD"/automation
 PREFIX="$AUTOMATION"/vdsm_functional
 EXPORTS="$PWD"/exported-artifacts

-function prepare {
+function setup_env {
+    # TODO: ask lago to create this folder
+    mkdir "$EXPORTS"/lago-logs
+
     # Creates RPMS
     "$AUTOMATION"/build-artifacts.sh

+    # TODO: jenkins mock env should take care of that
     if [[ -d "$PREFIX" ]]; then
         pushd "$PREFIX"
         echo 'cleaning old lago env'
@@ -25,15 +33,14 @@
         rm -rf "$PREFIX"
     fi

-    # Fix when running in an el* chroot in fc2* host
-    [[ -e /usr/bin/qemu-kvm ]] \
-    || ln -s /usr/libexec/qemu-kvm /usr/bin/qemu-kvm

lago init \
         "$PREFIX" \
         "$AUTOMATION"/lago-env.yml

cd "$PREFIX"
+
+    # TODO: remove --reposync-yum-config /dev/null if not needed
     lago ovirt reposetup \
         --reposync-yum-config /dev/null \
         --custom-source "dir:$EXPORTS"
@@ -84,9 +91,6 @@
 }

function run {
-    mkdir "$EXPORTS"/lago-logs
-    failed=0
-
     lago start "$VM_NAME"

prepare_and_copy_yum_conf
@@ -95,28 +99,33 @@
     # otherwise
     lago ovirt deploy

+    # TODO: ask lago to have cooler way to keep the localrepo up
     lago ovirt serve &
-    PID=$!
+    local httpd_pid=$!

-    fake_ksm_in_vm
+    fake_ksm_in_vm && run_all_tests
+    local failed=$?

+    kill $httpd_pid
+
+    return $failed
+}
+
+function run_all_tests {
     run_infra_tests | tee "$EXPORTS/functional_tests_stdout.$DISTRO.log"
     failed="${PIPESTATUS[0]}"

run_network_tests | tee -a "$EXPORTS/functional_tests_stdout.$DISTRO.log"
     res="${PIPESTATUS[0]}"
     [ "$res" -ne 0 ] && failed="$res"
+}

-    kill $PID
-
-    lago copy-from-vm \
-    "$VM_NAME" \
-    "/tmp/nosetests-${DISTRO}.xml" \
-    "$EXPORTS/nosetests-${DISTRO}.xml" || :
+function collect_logs {
+    lago copy-from-vm "$VM_NAME" \
+        "/tmp/nosetests-${DISTRO}.xml" \
+        "$EXPORTS/nosetests-${DISTRO}.xml" || :
     lago collect --output "$EXPORTS"/lago-logs
-
     cp "$PREFIX"/current/logs/*.log "$EXPORTS"/lago-logs
-    return $failed
 }

function cleanup {
@@ -124,5 +133,5 @@
     lago cleanup
 }

-prepare && run && cleanup
+setup_env && run && collect_logs && cleanup
 exit $?

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 1:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1091/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 1:

something went around with first run, trying again - http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1093/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 1: Verified+1

works great

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 2:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1096/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 3:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1097/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 3: Code-Review-1

Thanks for the review Nadav! will fix.

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 4:

(1 comment)

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1112/

https://gerrit.ovirt.org/#/c/70825/3/automation/check-merged.sh
File automation/check-merged.sh:

Line 116: 
Line 117:     run_network_tests | tee -a "$EXPORTS/functional_tests_stdout.$DISTRO.log"
Line 118:     res="${PIPESTATUS[0]}"
Line 119:     [ "$res" -ne 0 ] && failed="$res"
Line 120: }
> Lago supports configuring directories to collect logs from in the init file

this will be done separately to this patch
Line 121: 
Line 122: function collect_logs {
Line 123:     lago copy-from-vm "$VM_NAME" \
Line 124:         "/tmp/nosetests-${DISTRO}.xml" \

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 6:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1118/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 7:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1119/

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 9: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/70825/9/automation/check-merged.sh
File automation/check-merged.sh:

PS9, Line 9: Fix when running in an el* chroot in fc2* host
I don't understand this comment.

is this a fix? or should we fix this?

Line 125:     lago stop "$VM_NAME"
Line 126:     lago cleanup
Line 127: }
Line 128: 
Line 129: function quit {
it feels like what you need is

trap collect_and_clean EXIT
Line 130:     collect_logs
Line 131:     cleanup
Line 132:     exit $0
Line 133: }

PS9, Line 136: if [ $? -ne 0 ]; then
             :     echo 'check-merged.sh: ERROR during setup_env'
             :     quit 1
             : fi
does this work? the script is running with -e, so the script would explode on the very first non-zero return value, and would never get to here.

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/70825/9/automation/check-merged.sh
File automation/check-merged.sh:

PS9, Line 9: Fix when running in an el* chroot in fc2* host
> I don't understand this comment.

:) good question. the next 2 lines are the fix which is required when the script is running over fedora>20 hosts with el* chroot

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/70825/9/automation/check-merged.sh
File automation/check-merged.sh:

PS9, Line 136: if [ $? -ne 0 ]; then
             :     echo 'check-merged.sh: ERROR during setup_env'
             :     quit 1
             : fi
> does this work? the script is running with -e, so the script would explode

so it supports also without -e run .. why is it bad?

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 11: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/70825/11/automation/check-merged.sh
File automation/check-merged.sh:

Line 5: # ensure /dev/kvm exists, otherwise it will still use
Line 6: # direct backend, but without KVM(much slower).
Line 7: ! [[ -c "/dev/kvm" ]] && mknod /dev/kvm c 10 232
Line 8: 
Line 9: # Fix when running in an el* chroot in fc2* host
comment still not readable
Line 10: [[ -e /usr/bin/qemu-kvm ]] \
Line 11: || ln -s /usr/libexec/qemu-kvm /usr/bin/qemu-kvm
Line 12: 
Line 13: # ENV vars

PS11, Line 96: ret
other functions call it res

PS11, Line 110: failed
why not stay with "res"?

PS11, Line 113: res
always declare as local

PS11, Line 138: if [ $? -ne 0 ]; then
              :     echo 'check-merged.sh: ERROR during setup_env'
              :     exit 1
              : fi
again, with -e, this could would never be reached.

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 12: Verified+1

please review - the patch is ready

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1373/consol...

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 12: Code-Review-1

please see comments to ps11. they apply to ps12 too.

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 11:

(2 comments)

https://gerrit.ovirt.org/#/c/70825/11/automation/check-merged.sh
File automation/check-merged.sh:

Line 5: # ensure /dev/kvm exists, otherwise it will still use
Line 6: # direct backend, but without KVM(much slower).
Line 7: ! [[ -c "/dev/kvm" ]] && mknod /dev/kvm c 10 232
Line 8: 
Line 9: # Fix when running in an el* chroot in fc2* host
> comment still not readable

its not related to the patch though...
Line 10: [[ -e /usr/bin/qemu-kvm ]] \
Line 11: || ln -s /usr/libexec/qemu-kvm /usr/bin/qemu-kvm
Line 12: 
Line 13: # ENV vars

PS11, Line 138: if [ $? -ne 0 ]; then
              :     echo 'check-merged.sh: ERROR during setup_env'
              :     exit 1
              : fi
> again, with -e, this could would never be reached.

I removed the -e

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 13:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1378/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 14: Verified+1

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 14: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/70825/14/automation/check-merged.sh
File automation/check-merged.sh:

Line 1: #!/bin/bash -x
why? running a bash script with no -e is a bad practice.
it means that we would never notice a failure to run a command - processing would just go on.
Line 2: 
Line 3: export LIBGUESTFS_BACKEND=direct
Line 4: 
Line 5: # ensure /dev/kvm exists, otherwise it will still use

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/70825/14/automation/check-merged.sh
File automation/check-merged.sh:

Line 1: #!/bin/bash -x
> why? running a bash script with no -e is a bad practice.

I know, that's how I prefer to run this script, otherwise it exits and its hard to understand why so for me its easier to flow my echos
Line 2: 
Line 3: export LIBGUESTFS_BACKEND=direct
Line 4: 
Line 5: # ensure /dev/kvm exists, otherwise it will still use

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/70825/14/automation/check-merged.sh
File automation/check-merged.sh:

Line 1: #!/bin/bash -x
> I know, that's how I prefer to run this script, otherwise it exits and its

Running bash without -e is just like writing Python with

try:
  statement
except:
  pass

around each statement. It's a great way to quickly generate code with undetected bugs.

non-trivial code must not use it.
Line 2: 
Line 3: export LIBGUESTFS_BACKEND=direct
Line 4: 
Line 5: # ensure /dev/kvm exists, otherwise it will still use

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 15:

http://jenkins.ovirt.org/job/vdsm_master_check-merged-el7-x86_64/1394/

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 15: Code-Review+1

[Code Review]

From Yaniv Bronhaim :

Yaniv Bronhaim has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 15: Verified+1

please review again.. I think its ready

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: check-merged: split functions by logic and few nits
......................................................................

Patch Set 15: Code-Review+2

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has submitted this change and it was merged.

Change subject: check-merged: split functions by logic and few nits
......................................................................

check-merged: split functions by logic and few nits

Change-Id: I74f9326289b972abaef4507ab4d2582a8d641778
Signed-off-by: Yaniv Bronhaim 
---
M automation/check-merged.sh
1 file changed, 48 insertions(+), 35 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, but someone else must approve
  Yaniv Bronhaim: Verified; Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Leon Goldberg: Looks good to me, but someone else must approve