Tests: Some fixes for macOS by yangbodong22011 · Pull Request #7757 · redis/redis

yangbodong22011 · 2020-09-08T03:17:51Z

1) cur_test: when restart_server, "no such variable" error occurs

➜  redis git:(unstable) ✗ ./runtest --single integration/rdb    
Cleanup: may take some time... OK
Starting test server at port 11111
[ready]: 67092
Testing integration/rdb
[ready]: 67093
[ready]: 67091
[ready]: 67094
[ready]: 67095
[ready]: 67097
[ready]: 67099
[ready]: 67104
[ready]: 67096
[ready]: 67100
[ready]: 67098
[ready]: 67105
[ready]: 67106
[ok]: RDB encoding loading test
[ok]: Check for memory leaks (pid 67109)
[ok]: Server started empty with non-existing RDB file
[ready]: 67101
[ready]: 67102
[ready]: 67103
[ok]: Check for memory leaks (pid 67133)
[ok]: Server started empty with empty RDB file
[ok]: Check for memory leaks (pid 67153)
[ok]: Test RDB stream encoding
[ok]: Check for memory leaks (pid 67173)
[ok]: Server should not start if RDB file can't be open
[ok]: Server should not start if RDB is corrupted
[ok]: Test FLUSHALL aborts bgsave
[ok]: bgsave resets the change counter
[ok]: Check for memory leaks (pid 67207)
[ok]: Check for memory leaks (pid 67231)
[ok]: Check for memory leaks (pid 67282)
[ok]: client freed during loading
[exception]: Executing test client: can't unset "::cur_test": no such variable.
can't unset "::cur_test": no such variable
    while executing
"unset ::cur_test"
    (procedure "test" line 92)
    invoked from within
"test {client freed during loading} {
    start_server [list overrides [list key-load-delay 10 rdbcompression no]] {
        # create a big rdb that wi..."
    (file "tests/integration/rdb.tcl" line 151)
    invoked from within
"source $path"
    (procedure "execute_test_file" line 4)
    invoked from within
"execute_test_file $data"
    (procedure "test_client_main" line 10)
    invoked from within
"test_client_main $::test_server_port "
Killing still running Redis server 67193
Killing still running Redis server 67200

The calling process is as follows：

test {client freed during loading}
      SET ::cur_test
      restart_server
        kill_server
          test "Check for memory leaks (pid $pid)"
          SET ::cur_test
          UNSET ::cur_test
      UNSET ::cur_test // This global variable has been unset, so "no such variable" error occurs

fixd way:

I think unset can be cancelled, because every test will actively set at the beginning of the run.

@oranagra Do you have any suggestions?

2) ps --ppid

ps --ppid not available on macOS platform, can be replaced with pgrep -P pid.

oranagra · 2020-09-08T05:38:19Z

@yangbodong22011 i didn't think of nested tests, which in theory can happen not only at termination:
like:

test{test 1} {
	start_server {
		test{test 1.1 - master only} {
		}
		test{test 1.2 - with replication} {
			start_server {
			}
		}
	}
}

to support that properly your fix is not enough (when test 1.1 exits cur_test will be undefined).
i think the right solution is to back it up on the stack and restore it when test exists.

regarding the use of pgrep instead of ps i wonder if that's standard enough?
i don't wanna fix MacOS only to fall on a problem with another OS or distro (see b61b663).
@yossigo what do you think?

yossigo · 2020-09-08T05:55:48Z

@oranagra there's no generic solution to this, I'd stick with ps for Linux and just use pgrep for MacOS.

oranagra · 2020-09-08T05:57:39Z

so we need to try one and fallback to the other? or detect the distro?
how about doing it manually by looking at /proc/<pid>/stat?

yossigo · 2020-09-08T06:02:15Z

AFAIR there's no /proc in MacOS. I think just check the OS we're running on and decide.

yangbodong22011 · 2020-09-08T06:10:48Z

@oranagra there's no generic solution to this, I'd stick with ps for Linux and just use pgrep for MacOS.

On Linux, is it possible that pgrep is not supported?

yossigo · 2020-09-08T06:21:11Z

@yangbodong22011 Looking now, I see that pgrep has been around procps longer than I thought so it may make sense to use it for Linux as well. There's no guarantee we won't bump into a distro where that breaks of course.

oranagra · 2020-09-08T06:27:41Z

question is if procps is always installed by default or requires a manual install?

1) cur_test: when restart_server, "no such variable" error occurs ./runtest --single integration/rdb test {client freed during loading} SET ::cur_test restart_server kill_server test "Check for memory leaks (pid $pid)" SET ::cur_test UNSET ::cur_test UNSET ::cur_test // This global variable has been unset. 2) `ps --ppid` not available on macOS platform, can be replaced with `pgrep -P pid`.

yangbodong22011 · 2020-09-08T06:35:16Z

I made compatibility changes. Another change way is:

index b9a65358f..d354a23b7 100644
--- a/tests/support/util.tcl
+++ b/tests/support/util.tcl
@@ -510,11 +510,10 @@ proc get_child_pid {idx} {
     set pid [srv $idx pid]
     if {[string match {*Darwin*} [exec uname -a]]} {
         set fd [open "|pgrep -P $pid" "r"]
-        set child_pid [string trim [lindex [split [read $fd] \n] 0]]
     } else {
-        set fd [open "|ps --ppid $pid -o pid" "r"]
-        set child_pid [string trim [lindex [split [read $fd] \n] 1]]
+        set fd [open "|ps --ppid $pid -o pid -h" "r"]
     }
+    set child_pid [string trim [lindex [split [read $fd] \n] 0]]
     close $fd

Do you have any suggestions?

oranagra · 2020-09-08T06:48:08Z

@yangbodong22011 why are you re-introducing -h?
don't forget to backup and restore ::cur_test

yangbodong22011 · 2020-09-08T07:07:06Z

@yangbodong22011 why are you re-introducing -h?

for set child_pid [string trim [lindex [split [read $fd] \n] 0]], only this line is needed, if you think it is not necessary, just follow the latest code now.

don't forget to backup and restore ::cur_test

Okay, I will finish it later.

yangbodong22011 · 2020-09-08T08:01:55Z

@oranagra Can it be modified like this, when cur_test exists, call unset.

diff --git a/tests/support/test.tcl b/tests/support/test.tcl
index 54d323fa2..9555e63d0 100644
--- a/tests/support/test.tcl
+++ b/tests/support/test.tcl
@@ -190,4 +190,8 @@ proc test {name code {okpattern undefined} {options undefined}} {
             send_data_packet $::test_server_fd err "Detected a memory leak in test '$name': $output"
         }
     }
+
+    if {[info exists ::cur_test]} {
+        unset ::cur_test
+    }
 }

oranagra · 2020-09-08T08:10:58Z

@yangbodong22011 the problem is not only that unset crashes if already unset.
but also that if we have nested tests, after the inner test exists, the framework doesn't know that it's inside a test (the outer one).

i think the right thing to do is back up the old value when entering test, and restore it when exiting.

oranagra · 2020-09-08T08:13:51Z

@yangbodong22011 i didn't understand your post:

for set child_pid [string trim [lindex [split [read $fd] \n] 0]], only this line is needed, if you think it is not necessary, just follow the latest code now.

the -h argument was trimmed in:
b61b663
why do you wish to re-introduce it?

yangbodong22011 · 2020-09-08T08:17:46Z

@yangbodong22011 i didn't understand your post:

for set child_pid [string trim [lindex [split [read $fd] \n] 0]], only this line is needed, if you think it is not necessary, just follow the latest code now.

the -h argument was trimmed in:
b61b663
why do you wish to re-introduce it?

index b9a65358f..d354a23b7 100644
--- a/tests/support/util.tcl
+++ b/tests/support/util.tcl
@@ -510,11 +510,10 @@ proc get_child_pid {idx} {
     set pid [srv $idx pid]
     if {[string match {*Darwin*} [exec uname -a]]} {
         set fd [open "|pgrep -P $pid" "r"]
-        set child_pid [string trim [lindex [split [read $fd] \n] 0]]
     } else {
-        set fd [open "|ps --ppid $pid -o pid" "r"]
-        set child_pid [string trim [lindex [split [read $fd] \n] 1]]
+        set fd [open "|ps --ppid $pid -o pid -h" "r"]
     }
+    set child_pid [string trim [lindex [split [read $fd] \n] 0]]
     close $fd

Reduce one line of code, Move set child_pid xxx out of the braces.

oranagra · 2020-09-08T08:19:39Z

-        set fd [open "|ps --ppid $pid -o pid" "r"]
+        set fd [open "|ps --ppid $pid -o pid -h" "r"]

why did you add -h?

yangbodong22011 · 2020-09-08T08:22:24Z

-        set fd [open "|ps --ppid $pid -o pid" "r"]
+        set fd [open "|ps --ppid $pid -o pid -h" "r"]

why did you add -h?

$ps --ppid 1 -o pid -h | head
  1026
  7549
  7550
  9294
  9299
 12438
 14037
 14372
 18599
 18845

$ps --ppid 1 -o pid | head
   PID
  1026
  7549
  7550
  9294
  9299
 12438
 14037
 14372
 18599

-h will filter out the words PID, so that [lindex [split [read $fd] \n] 0] instead of [lindex [split [read $fd] \n] 1]

oranagra · 2020-09-08T08:25:12Z

but see b61b663
it's not supported on old CentOS

yangbodong22011 · 2020-09-08T08:28:09Z

but see b61b663
it's not supported on old CentOS

Oh, I understand, so I should clear why the -h was removed first, and then consider optimization. The current latest code has not been changed, please ignore my optimization.

if there are nested tests and nested servers, we need to restore the previous value of cur_test when a test exist. example: ``` test{test 1} { start_server { test{test 1.1 - master only} { } start_server { test{test 1.2 - with replication} { } } } } ``` when `test 1.1 - master only exists`, we're still inside `test 1`

* Tests: Some fixes for macOS 1) cur_test: when restart_server, "no such variable" error occurs ./runtest --single integration/rdb test {client freed during loading} SET ::cur_test restart_server kill_server test "Check for memory leaks (pid $pid)" SET ::cur_test UNSET ::cur_test UNSET ::cur_test // This global variable has been unset. 2) `ps --ppid` not available on macOS platform, can be replaced with `pgrep -P pid`. * handle cur_test for nested tests if there are nested tests and nested servers, we need to restore the previous value of cur_test when a test exist. example: ``` test{test 1} { start_server { test{test 1.1 - master only} { } start_server { test{test 1.2 - with replication} { } } } } ``` when `test 1.1 - master only exists`, we're still inside `test 1` Co-authored-by: Oran Agra <[email protected]>

yossigo closed this Sep 8, 2020

yossigo reopened this Sep 8, 2020

yangbodong22011 force-pushed the fix-tests-on-macOS branch from 46492e2 to f22fa95 Compare September 8, 2020 06:28

oranagra mentioned this pull request Sep 8, 2020

Add daily CI for MacOS #7759

Merged

oranagra approved these changes Sep 8, 2020

View reviewed changes

oranagra merged commit 340963b into redis:unstable Sep 8, 2020

yoav-steinberg mentioned this pull request Oct 19, 2021

Memory Leak Test Failure #2024

Closed

Conversation

yangbodong22011 commented Sep 8, 2020

1) cur_test: when restart_server, "no such variable" error occurs

The calling process is as follows：

fixd way:

2) ps --ppid

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yossigo commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yossigo commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020

Uh oh!

yossigo commented Sep 8, 2020

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangbodong22011 commented Sep 8, 2020

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 8, 2020

Uh oh!

yangbodong22011 commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yossigo commented Sep 8, 2020 •

edited

Loading

yangbodong22011 commented Sep 8, 2020 •

edited

Loading

yangbodong22011 commented Sep 8, 2020 •

edited

Loading

yangbodong22011 commented Sep 8, 2020 •

edited

Loading