[FLINK-16423][e2e] Introduce timeouts for HA tests #11831

rmetzger · 2020-04-20T14:32:12Z

What is the purpose of the change

Before this change, HA tests were waiting indefinitely for certain conditions to be met.
This change introduces a timeout (10 minutes) in the test scripts that kills the test and prints the Flink logs for debugging.

Brief change log

Change the HA watchdogs to store their state in files rather than variables (so that different bash processes can access them)
launch HA test and a watchdog that sleeps for 10 minutes. If the test doesn't finish in 10, the watchdog will kill the test.

flinkbot · 2020-04-20T14:36:21Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 8c3475f (Mon Apr 20 14:36:20 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-04-20T15:12:12Z

CI report:

9632975 Travis: CANCELED Azure: SUCCESS
a791d09 UNKNOWN

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

rmetzger · 2020-04-24T13:15:12Z

Since this error can happen also on travis, we should probably set the timeout to 9 minutes (as Travis has a timeout of 10 minutes)

ifndef-SleePy

Hi, thanks for opening this PR. It would help a lot to find the failure cause of e2e test.
I'm not an expert of Bash programming. I tried to review the PR with the help of Google. I left a couple of comments. Correct me if I was wrong :)

ifndef-SleePy · 2020-04-25T07:24:42Z

flink-end-to-end-tests/test-scripts/common_ha.sh

@@ -23,10 +23,9 @@ source "${END_TO_END_DIR}"/test-scripts/common.sh
 # flag indicating if we have already cleared up things after a test
 CLEARED=0

-JM_WATCHDOG_PID=0


Just a curiosity, what's the reason of this change?

The JM_WATCHDOG_PID is used in the stop_watchdogs function, which is called from different bash-processes.
Because of that, if the variable is set in one process, it's not available in the other one.

That's why I decided to store the PID in a file.

ifndef-SleePy · 2020-04-25T07:37:27Z

flink-end-to-end-tests/test-scripts/test_ha_datastream.sh

+on_exit kill_test_watchdog
+
+( 
+    cmdpid=$BASHPID; 


BASHPID does not work on my Mac. It's introduced in Bash 4, however the version of Bash on my Mac is 3.2. Maybe we could use $$ when BASHPID is not supported? $$ is more general, however sometimes it does not work well as expected, like executing in background.

Yes, these changes require bash 4. Bash on mac is pretty outdated sadly.

I think $$ won't work, as it will return the PID of the bash-process, not of the subshell I'm launching there:

When you want to find out the PID of your current shell session, even from subshells, that’s when you use $$. Any subshell has its own PID as well, and you can access it via the $BASHPID variable.

https://bashwizard.com/shells-subshells-pids/

But I will add a warning to the script, that is requires at least bash 4

Ah, you are right. It's in another sub-process.

I come up with a work-around way. It probably works that executing the watchdog in original main process and executing the testing scripts in sub-process. We can get the sub-process pid in main process.

But I guess it's not a big deal, as MacOS is not a main scenario. I hope this PR could be merged asap. I would help a lot.

ifndef-SleePy · 2020-04-25T07:39:25Z

flink-end-to-end-tests/test-scripts/test_ha_per_job_cluster_datastream.sh

+    echo "Test (pid: $cmdpid) did not finish after $TEST_TIMEOUT_SECONDS seconds."
+    echo "Printing Flink logs and killing it:"
+    cat ${FLINK_DIR}/log/* 
+    kill "$cmdpid") & watchdog_pid=$!


Another curiosity, is there any magic of inline watchdog_pid=$!?

That's a good question. I don't know if this has to be in the same line or not. Probably not.
It is okay for you, I would not change this, because it would mean that I have to test the scripts again (I manually tested that they are behaving as expected in failure / timeout / success cases.)

That's OK for me :)

Before this change, HA tests were waiting indefinetly for certain conditions to be met. This change introduces a timeout in the test scripts that kills the test and prints the Flink logs for debugging.

rmetzger · 2020-04-27T06:46:17Z

Thanks a lot for your review. I rebased to current master and added a comment for the Bash 4+ requirement.

ifndef-SleePy

Thanks for response and updating. LGTM

ifndef-SleePy · 2020-04-27T07:30:40Z

flink-end-to-end-tests/test-scripts/test_ha_per_job_cluster_datastream.sh

+    echo "Test (pid: $cmdpid) did not finish after $TEST_TIMEOUT_SECONDS seconds."
+    echo "Printing Flink logs and killing it:"
+    cat ${FLINK_DIR}/log/* 
+    kill "$cmdpid") & watchdog_pid=$!


That's OK for me :)

ifndef-SleePy · 2020-04-27T07:40:39Z

flink-end-to-end-tests/test-scripts/test_ha_datastream.sh

+on_exit kill_test_watchdog
+
+( 
+    cmdpid=$BASHPID; 


Ah, you are right. It's in another sub-process.

I come up with a work-around way. It probably works that executing the watchdog in original main process and executing the testing scripts in sub-process. We can get the sub-process pid in main process.

But I guess it's not a big deal, as MacOS is not a main scenario. I hope this PR could be merged asap. I would help a lot.

rmetzger · 2020-04-27T07:53:01Z

Thanks for the review. Let's wait till Azure is green, then I'll merge it.

rmetzger added the review=description? label Apr 20, 2020

rmetzger added component=Runtime/Checkpointing component=Tests labels Apr 20, 2020

ifndef-SleePy reviewed Apr 25, 2020

View reviewed changes

rmetzger added 2 commits April 27, 2020 08:45

[FLINK-16423][e2e] Introduce timeouts for HA tests

855fde9

Before this change, HA tests were waiting indefinetly for certain conditions to be met. This change introduces a timeout in the test scripts that kills the test and prints the Flink logs for debugging.

address PR comments

9632975

rmetzger force-pushed the FLINK-16423-ha-e2e-timeout-pr branch from 8c3475f to 9632975 Compare April 27, 2020 06:45

Set timeout to 9 minutes

a791d09

ifndef-SleePy reviewed Apr 27, 2020

View reviewed changes

rmetzger closed this in 1855bf3 Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-16423][e2e] Introduce timeouts for HA tests #11831

[FLINK-16423][e2e] Introduce timeouts for HA tests #11831

rmetzger commented Apr 20, 2020 •

edited

flinkbot commented Apr 20, 2020

flinkbot commented Apr 20, 2020 •

edited

rmetzger commented Apr 24, 2020

ifndef-SleePy left a comment

ifndef-SleePy Apr 25, 2020

rmetzger Apr 27, 2020

ifndef-SleePy Apr 25, 2020

rmetzger Apr 27, 2020

ifndef-SleePy Apr 27, 2020

ifndef-SleePy Apr 25, 2020

rmetzger Apr 27, 2020

ifndef-SleePy Apr 27, 2020

rmetzger commented Apr 27, 2020

ifndef-SleePy left a comment

ifndef-SleePy Apr 27, 2020

ifndef-SleePy Apr 27, 2020

rmetzger commented Apr 27, 2020

[FLINK-16423][e2e] Introduce timeouts for HA tests #11831

[FLINK-16423][e2e] Introduce timeouts for HA tests #11831

Conversation

rmetzger commented Apr 20, 2020 • edited

What is the purpose of the change

Brief change log

flinkbot commented Apr 20, 2020

Automated Checks

Review Progress

flinkbot commented Apr 20, 2020 • edited

CI report:

rmetzger commented Apr 24, 2020

ifndef-SleePy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetzger commented Apr 27, 2020

ifndef-SleePy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetzger commented Apr 27, 2020

rmetzger commented Apr 20, 2020 •

edited

flinkbot commented Apr 20, 2020 •

edited