Add valgrind to CI (#507)

* wip

* [fix] Fixed missing suppressions file ( we might need to double check supressions file )
* added supressions

Co-authored-by: filipecosta90 <filipecosta.90@gmail.com>
Co-authored-by: Dvir Dukhan <dvirdukhan@pop-os.localdomain>
Co-authored-by: rafie <rafi@redislabs.com>
Co-authored-by: Luca Antiga <luca.antiga@orobix.com>

Author: 5 people

.circleci/config.yml

@@ -12,6 +12,14 @@ commands:
       - setup_remote_docker:
           docker_layer_caching: true
 
+  setup-automation:
+    steps:
+      - run:
+          name: Setup automation
+          command: |
+            PIP=1 ./opt/readies/bin/getpy3
+            ./opt/system-setup.py
+
   build-steps:
     parameters:
       platform:
@@ -25,11 +33,10 @@ commands:
           keys:
           - v1-dependencies-{{ checksum "get_deps.sh" }}
           # If no exact match is found will get dependencies from source
+      - setup-automation
       - run:
           name: Install dependencies
           command: |
-            ./opt/readies/bin/getpy3
-            BREW_NO_UPDATE=1 ./opt/system-setup.py
             ./opt/readies/bin/getredis -v 6 --force
             ./get_deps.sh cpu
       - save_cache:
@@ -60,8 +67,6 @@ commands:
             - artifacts/*.zip
             - artifacts/*.tgz
             - artifacts/*.tar
-            # - artifacts/shapshots/*.zip
-            # - artifacts/shapshots/*.tgz
       - store_artifacts:
           path: tests/logs
 
@@ -98,8 +103,6 @@ commands:
             - artifacts/*.zip
             - artifacts/*.tgz
             - artifacts/*.tar
-            # - artifacts/shapshots/*.zip
-            # - artifacts/shapshots/*.tgz
       - store_artifacts:
           path: test/logs
 
@@ -156,11 +159,10 @@ jobs:
           keys:
           - build-dependencies-{{ checksum "get_deps.sh" }}
           # If no exact match is found will get dependencies from source
+      - setup-automation
       - run:
           name: Install dependencies
           command: |
-            ./opt/readies/bin/getpy3
-            ./opt/system-setup.py
             ./opt/readies/bin/getredis -v 6 --valgrind --force
             ./get_deps.sh cpu
       - run:
@@ -174,6 +176,34 @@ jobs:
             make -C opt cov-upload
           no_output_timeout: 30m
 
+  valgrind:
+    docker:
+      - image: redisfab/rmbuilder:6.0.5-x64-buster
+    steps:
+      - checkout
+      - run:
+          name: Submodule checkout
+          command: git submodule update --init --recursive 
+      - restore_cache:
+          keys:
+          - build-dependencies-{{ checksum "get_deps.sh" }}
+          # If no exact match is found will get dependencies from source
+      - setup-automation
+      - run:
+          name: Install dependencies
+          command: |
+            ./opt/readies/bin/getredis -v 6 --valgrind --force
+            ./get_deps.sh cpu
+      - run:
+          name: Build for valgrind
+          command: |
+            make -C opt all VALGRIND=1 SHOW=1
+      - run:
+          name: Test with valgrind
+          command: |
+            make -C opt test VALGRIND=1
+          no_output_timeout: 120m
+
   build-macos:
     macos:
       xcode: 11.3.0
@@ -322,7 +352,6 @@ on-master-and-version-tags: &on-master-and-version-tags
     tags:
       only: /^v[0-9].*/
 
-
 platform-build-defs: &platform-build-defs
   requires:
     - build-debian
@@ -362,6 +391,9 @@ workflows:
       - coverage:
           <<: *on-any-branch
           <<: *after-linter
+      - valgrind:
+          <<: *on-master
+          <<: *after-linter
       - build-and-test-gpu:
           <<: *on-any-branch
           <<: *after-linter

opt/Makefile

@@ -231,15 +231,29 @@ export SLAVES ?= 1
 export AOF ?= 1
 export CLUSTER ?= 1
 
+define UNIT_TESTS
+$(SHOW)echo Running unit tests ...
+$(SHOW)\
+BINDIR=$(realpath $(BINDIR)) \
+$(ROOT)/tests/unit/tests.sh
+endef
+
+define FLOW_TESTS
+$(SHOW)echo Running flow tests ...
+$(SHOW)\
+DEVICE=$(DEVICE) \
+MODULE=$(realpath $(INSTALLED_TARGET)) \
+CLUSTER=$(CLUSTER) \
+GEN=$(GEN) AOF=$(AOF) SLAVES=$(SLAVES) \
+VALGRIND=$(VALGRIND) \
+$(ROOT)/tests/flow/tests.sh
+endef
+
 unit_tests: build
-	@echo "$(BINDIR)"
 	$(COVERAGE_RESET)
-	$(SHOW)\
-		BINDIR=$(realpath $(BINDIR)) \
-		$(ROOT)/tests/unit/tests.sh
+	$(UNIT_TESTS)
 	$(COVERAGE_COLLECT_REPORT)
 
-
 flow_tests: build
 	$(COVERAGE_RESET)
 	$(SHOW)\
@@ -250,10 +264,12 @@ flow_tests: build
 		VALGRIND=$(VALGRIND) \
 		REDIS=$(REDIS) \
 		$(ROOT)/tests/flow/tests.sh
+
 	$(COVERAGE_COLLECT_REPORT)
 
 test: build
 	$(COVERAGE_RESET)
+	$(UNIT_TESTS)
 	$(SHOW)\
 		BINDIR=$(realpath $(BINDIR)) \
 		$(ROOT)/tests/unit/tests.sh
@@ -267,6 +283,8 @@ test: build
 		$(ROOT)/tests/flow/tests.sh
 	$(COVERAGE_COLLECT_REPORT)
 
+#----------------------------------------------------------------------------------------------
+
 valgrind:
 	$(SHOW)$(ROOT)/tests/flow/valgrind.sh $(realpath $(INSTALLED_TARGET))
 

opt/redis_valgrind.sup

@@ -43,3 +43,28 @@
    Memcheck:Value8
    fun:lzf_compress
 }
+
+{
+   <dlopen>
+   Memcheck:Leak
+   fun:malloc
+   ...
+   fun:dlopen@@GLIBC_2.2.5
+   ...
+   fun:RAI_LoadBackend
+}
+
+{
+   <tf-operator new>
+   Memcheck:Leak
+   ...
+   fun:clone
+}
+
+{
+   <malloc>
+   Memcheck:Leak
+   fun:malloc
+   ...
+   fun:clone
+}

opt/system-setup.py

@@ -66,6 +66,9 @@ def fedora(self):
         self.install("clang")
         self.install_git_lfs_on_linux()
 
+    def linux_last(self):
+        self.install("valgrind")
+
     def macos(self):
         self.install_gnu_utils()
         self.install("git-lfs")

tests/flow/tests.sh

@@ -12,9 +12,10 @@ error() {
 [[ -z $_Dbg_DEBUGGER_LEVEL ]] && trap 'error $LINENO' ERR
 
 HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"
-. $HERE/../../opt/readies/shibumi/functions
+export ROOT=$(cd $HERE/../..; pwd)
+. $ROOT/opt/readies/shibumi/functions
 
-export ROOT=$(realpath $HERE/../..)
+cd $HERE
 
 #----------------------------------------------------------------------------------------------
 
@@ -34,6 +35,7 @@ help() {
 		SLAVES=0|1      Tests with --test-slaves
 		
 		TEST=test        Run specific test (e.g. test.py:test_name)
+		LOG=0|1          Write to log
 		VALGRIND|VGD=1   Run with Valgrind
 		CALLGRIND|CGD=1  Run with Callgrind
 
@@ -69,13 +71,25 @@ valgrind_config() {
 	VALGRIND_SUPRESSIONS=$ROOT/opt/redis_valgrind.sup
 
 	RLTEST_ARGS+="\
-		--no-output-catch \
 		--use-valgrind \
 		--vg-no-fail-on-errors \
-		--vg-verbose \
 		--vg-suppressions $VALGRIND_SUPRESSIONS"
 }
 
+valgrind_summary() {
+	# Collect name of each flow log that contains leaks
+	FILES_WITH_LEAKS=$(grep -l "definitely lost" logs/*.valgrind.log)
+	if [[ ! -z $FILES_WITH_LEAKS ]]; then
+		echo "Memory leaks introduced in flow tests."
+		echo $FILES_WITH_LEAKS
+		# Print the full Valgrind output for each leaking file
+		echo $FILES_WITH_LEAKS | xargs cat
+		exit 1
+	else
+		echo Valgrind test ok
+	fi
+}
+
 #----------------------------------------------------------------------------------------------
 
 run_tests() {
@@ -107,8 +121,11 @@ MODULE=${MODULE:-$1}
 [[ $VALGRIND == 1 || $VGD == 1 ]] && valgrind_config
 
 if [[ ! -z $TEST ]]; then
-	RLTEST_ARGS+=" --test $TEST -s"
-	export PYDEBUG=${PYDEBUG:-1}
+	RLTEST_ARGS+=" --test $TEST"
+	if [[ $LOG != 1 ]]; then
+		RLTEST_ARGS+=" -s"
+		export PYDEBUG=${PYDEBUG:-1}
+	fi
 fi
 
 [[ $VERBOSE == 1 ]] && RLTEST_ARGS+=" -v"
@@ -124,5 +141,7 @@ check_redis_server
 [[ ! -z $REDIS ]] && RL_TEST_ARGS+=" --env exiting-env --existing-env-addr $REDIS" run_tests "redis-server: $REDIS"
 [[ $GEN == 1 ]]    && run_tests
 [[ $CLUSTER == 1 ]] && RLTEST_ARGS+=" --env oss-cluster --shards-count 1" run_tests "--env oss-cluster"
-[[ $SLAVES == 1 ]] && RLTEST_ARGS+=" --use-slaves" run_tests "--use-slaves"
-[[ $AOF == 1 ]]    && RLTEST_ARGS+=" --use-aof" run_tests "--use-aof"
+[[ $VALGRIND != 1 && $SLAVES == 1 ]] && RLTEST_ARGS+=" --use-slaves" run_tests "--use-slaves"
+[[ $AOF == 1 ]] && RLTEST_ARGS+=" --use-aof" run_tests "--use-aof"
+[[ $VALGRIND == 1 ]] && valgrind_summary
+exit 0

tests/flow/tests_dag.py

@@ -141,6 +141,9 @@ def test_dagro_common_errors(env):
 def test_dagrun_ro_modelrun_scriptrun_resnet(env):
     if (not TEST_TF or not TEST_PT):
         return
+    if(VALGRIND):
+        env.debugPrint("skipping {} since it's hanging CI".format(sys._getframe().f_code.co_name), force=True)
+        env.skip()
     con = env.getConnection()
     model_name = 'imagenet_model{{1}}'
     script_name = 'imagenet_script{{1}}'
@@ -188,6 +191,9 @@ def test_dagrun_ro_modelrun_scriptrun_resnet(env):
 def test_dagrun_modelrun_scriptrun_resnet(env):
     if (not TEST_TF or not TEST_PT):
         return
+    if(VALGRIND):
+        env.debugPrint("skipping {} since it's hanging CI".format(sys._getframe().f_code.co_name), force=True)
+        env.skip()
     con = env.getConnection()
     model_name = 'imagenet_model:{{1}}'
     script_name = 'imagenet_script:{{1}}'

tests/flow/valgrind.sh

@@ -11,9 +11,10 @@ error() {
 # trap error ERR
 
 HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"
-. $HERE/../opt/readies/shibumi/functions
+export ROOT=$(cd $HERE/../..; pwd)
+. $ROOT/opt/readies/shibumi/functions
 
-export ROOT=$(realpath $HERE/..)
+cd $HERE
 
 #----------------------------------------------------------------------------------------------
 
@@ -24,10 +25,10 @@ help() {
 		[ARGVARS...] valgrind.sh [--help|help] [<module-so-path>]
 		
 		Argument variables:
-		VERBOSE=1     Print commands
-		IGNERR=1      Do not abort on error
+		VERBOSE=1         Print commands
+		IGNERR=1          Do not abort on error
 		
-		CALLGRIND|CGD=1  Run with Callgrind
+		CALLGRIND|CGD=1   Run with Callgrind
 
 	END
 }
@@ -59,6 +60,7 @@ valgrind_config() {
 	else
 		VALGRIND_OPTIONS+="\
 			-q \
+			--suppressions=$VALGRIND_SUPRESSIONS \
 			--leak-check=full \
 			--show-reachable=no \
 			--show-possibly-lost=no \
@@ -83,7 +85,5 @@ valgrind_config
 
 #----------------------------------------------------------------------------------------------
 
-cd $ROOT/test
-
 check_redis_server
 $OP valgrind $(echo "$VALGRIND_OPTIONS") redis-server --protected-mode no --save '' --appendonly no --loadmodule $MODULE