[FLINK-21346][build system] Adding timeout to all tasks #15546

dawidwys · 2021-04-09T08:45:36Z

What is the purpose of the change

I took over #14834

When a test gets stuck, it's usually killed after 10 min of inactivity. #13260 excluded all tests that still show log activity from this timeout to allow test runs that take longer than 10 min.

However, some stuck tests are not deadlocked but rather caught in some kind of livelock. These tests now timeout with AZP timeout (4h) without any logs. This commit also adds a timeout to all build tasks in the same way as e2e tests are timeboxed. On reaching the timeout, the task logs are uploaded together with a stack trace.

Verifying this change

Manually verified.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

dawidwys · 2021-04-09T08:46:23Z

Hey, @AHeise @rmetzger Do you mind having a look when you have time? Thanks!

flinkbot · 2021-04-09T08:48:38Z

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 6213fbc (Fri May 28 09:08:20 UTC 2021)

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 · 2021-04-09T09:05:36Z

CI report:

6213fbc Azure: SUCCESS

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

dawidwys · 2021-04-09T13:05:33Z

The Azure failure is expected to show the layout of uploaded files. Once the change is reviewed I will remove the temp commits and rerun azure.

dawidwys · 2021-04-13T09:27:21Z

Regarding manual compression:

I think it is safe to remove the manual compression as it is done automatically by azure:
without manual compression:

Content upload statistics:
Total Content: 116.6 MB
Physical Content Uploaded: 7.0 MB
Logical Content Uploaded: 58.3 MB
Compression Saved: 51.3 MB
Deduplication Saved: 58.3 MB
Number of Chunks Uploaded: 593

with manual compression:

Content upload statistics:
Total Content: 14.0 MB
Physical Content Uploaded: 6.6 MB
Logical Content Uploaded: 7.0 MB
Compression Saved: 0.4 MB
Deduplication Saved: 7.0 MB
Number of Chunks Uploaded: 89

Stats from: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=16278&view=logs&j=d44f43ce-542c-597d-bf94-b0718c71e5e8&t=800a4f47-e103-5bd8-7e1e-053f424b0a53

AHeise

Thank you for picking that PR up!

Downloaded misc and there is a superfluous file in there.
Downloaded e2e and all looks good.
I like that we got rid of the double compression. Makes it easier to check.

AHeise · 2021-04-13T09:33:15Z

tools/azure-pipelines/jobs-template.yml

@@ -81,7 +81,7 @@ jobs:
 condition: and(succeeded(), not(eq(variables['MODE'], 'e2e')))
 pool: ${{parameters.test_pool_definition}}
 container: ${{parameters.container}}
- timeoutInMinutes: 240
+ timeoutInMinutes: 60


For the final PR this should be set to 2h or so (lower than the 4h we used before).

Why should we lower it? We did not make the tests run faster...

The test didn't need 4h and they run quicker into a potential timeout this way. Just check the build times on recent builds (btw 4h is the default).

tools/azure-pipelines/uploading_watchdog.sh

dawidwys · 2021-04-13T09:49:50Z

Which superfluous file do you have in mind? Is it mvn-${sys:mvn.forkNumber}.log? It's an unrelated issue of some tests in that profile. Current setup also produces this file. See e.g. https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=16410&view=results

rmetzger · 2021-04-13T11:14:38Z

tools/azure-pipelines/jobs-template.yml

 displayName: Upload Logs
 inputs:
- targetPath: $(ARTIFACT_DIR)
- artifact: logs-${{parameters.stage_name}}-e2e


I added the stage name to the artifact name to avoid duplicate file issues in the nightly CI profiles. It seems that the stage name is done again based on timestamps, which has the risk of artifacts not being uploaded, because they exist already (IIRC it has happened that files had exactly the same timestamp (since builds are triggered at the same time)

I reintroduced the stage name.

rmetzger · 2021-04-13T11:17:29Z

I think it is safe to remove the manual compression as it is done automatically by azure:

I wonder if the compression is only done for the transfer, or if the files are stored as archives. As an authenticated user, it looks like files are stored without compression:

I'm not saying that we need to change it. The only concern I have is that we might run into a resource limitation, if Azure enables them at some point.

rmetzger · 2021-04-13T11:17:57Z

Besides these two comments, I didn't spot anything in the changes.

dawidwys · 2021-04-14T07:27:04Z

As for the compression, let's try to remove it for now as it makes it a bit easier to work with the files. It lets you also download a single file from a build. If we hit a problem with a resource limitation we can easily reintroduce the compression.

…stacktraces. The test/e2e task is killed before timeout and artifacts are published in the regular way. If killed jps traces and watchdog output are additionally attached to mvn.logs.

AHeise

LGTM % squash fixups. Thank you for your contributions.

tillrohrmann

I think this change breaks the local e2e test setup on my machine (MacOS). The problem is that $FLINK_DIR/logs does not exist.

tillrohrmann · 2021-08-25T10:26:06Z

flink-end-to-end-tests/run-nightly-tests.sh

@@ -32,30 +32,24 @@ if [ -z "$FLINK_DIR" ] ; then
 exit 1
 fi

+if [ -z "$FLINK_LOG_DIR" ] ; then
+ export FLINK_LOG_DIR="$FLINK_DIR/logs"


I think this change breaks the e2e setup on my machine (MacOS) because the directory does not exist.

tillrohrmann · 2021-08-25T10:26:12Z

flink-end-to-end-tests/run-single-test.sh

@@ -46,6 +46,10 @@ if [ -z "$FLINK_DIR" ] ; then
 exit 1
 fi

+if [ -z "$FLINK_LOG_DIR" ] ; then
+ export FLINK_LOG_DIR="$FLINK_DIR/logs"


tillrohrmann · 2021-08-25T10:26:16Z

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

@@ -26,6 +26,10 @@ if [[ -z $FLINK_DIR ]]; then
 exit 1
 fi

+if [ -z "$FLINK_LOG_DIR" ] ; then
+ export FLINK_LOG_DIR="$FLINK_DIR/logs"


AHeise · 2021-08-25T11:29:25Z

I think this change breaks the local e2e test setup on my machine (MacOS). The problem is that $FLINK_DIR/logs does not exist.

Can you please fix it? Dawid is MIA afaik.
The main question is where are your logs? They should be there somewhere after your run even if there is a failure in job submission.
I see a log in my flink-dist but this feels wrong somehow. Maybe that is the actual bug.

Or maybe we should have a mkdirs $FLINK_LOG_DIR before starting stuff.

tillrohrmann · 2021-08-25T12:01:10Z

Maybe, I will just fix the problem by setting FLINK_LOG_DIR=FLINK_DIR/log. Changing this to something can be done in a follow-up ticket.

dawidwys requested review from rmetzger and AHeise April 9, 2021 08:45

rmetzger added the review=description? label Apr 9, 2021

rmetzger added the component=BuildSystem/CI label Apr 9, 2021

AHeise mentioned this pull request Apr 13, 2021

[FLINK-21346][build system] Adding timeout to all tasks #14834

Closed

AHeise reviewed Apr 13, 2021

View reviewed changes

rmetzger reviewed Apr 13, 2021

View reviewed changes

dawidwys mentioned this pull request Apr 13, 2021

[Flink-22248][Flink-22249] Harden JobMasterStopWithSavepointITCase #15589

Merged

Arvid Heise added 2 commits April 14, 2021 09:41

[FLINK-21346][build system] Add uploading watchdog to all tasks with …

9e2a32c

…stacktraces. The test/e2e task is killed before timeout and artifacts are published in the regular way. If killed jps traces and watchdog output are additionally attached to mvn.logs.

[FLINK-21346][build system] Using FLINK_LOG_DIR in e2e.

b5a02e5

dawidwys force-pushed the FLINK-21234 branch from ebf3fcd to e241994 Compare April 14, 2021 07:48

dawidwys added 2 commits April 14, 2021 10:25

fixup! Guard against negative sleep

335a48d

fixup! Reintroduce stage name to artifacts names

4ac0133

dawidwys force-pushed the FLINK-21234 branch from 2518825 to 4ac0133 Compare April 14, 2021 08:25

fixup! Fix logs dir for mesos tests in docker

e16ddc3

dawidwys force-pushed the FLINK-21234 branch 3 times, most recently from a9291f1 to 5141fb3 Compare April 15, 2021 11:54

fixup! Fix e2e logging dir

6213fbc

dawidwys force-pushed the FLINK-21234 branch from 5141fb3 to 6213fbc Compare April 15, 2021 17:51

AHeise approved these changes Apr 16, 2021

View reviewed changes

dawidwys closed this in 6b430e6 Apr 16, 2021

tillrohrmann reviewed Aug 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-21346][build system] Adding timeout to all tasks #15546

[FLINK-21346][build system] Adding timeout to all tasks #15546

dawidwys commented Apr 9, 2021

dawidwys commented Apr 9, 2021

flinkbot commented Apr 9, 2021 •

edited

Loading

flinkbot commented Apr 9, 2021 •

edited

Loading

dawidwys commented Apr 9, 2021

dawidwys commented Apr 13, 2021

AHeise left a comment

AHeise Apr 13, 2021

dawidwys Apr 13, 2021

AHeise Apr 16, 2021

dawidwys commented Apr 13, 2021

rmetzger Apr 13, 2021

dawidwys Apr 14, 2021

rmetzger commented Apr 13, 2021

rmetzger commented Apr 13, 2021

dawidwys commented Apr 14, 2021

AHeise left a comment

tillrohrmann left a comment

tillrohrmann Aug 25, 2021

tillrohrmann Aug 25, 2021

tillrohrmann Aug 25, 2021

AHeise commented Aug 25, 2021

tillrohrmann commented Aug 25, 2021

[FLINK-21346][build system] Adding timeout to all tasks #15546

[FLINK-21346][build system] Adding timeout to all tasks #15546

Conversation

dawidwys commented Apr 9, 2021

What is the purpose of the change

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

dawidwys commented Apr 9, 2021

flinkbot commented Apr 9, 2021 • edited Loading

Automated Checks

Review Progress

flinkbot commented Apr 9, 2021 • edited Loading

CI report:

dawidwys commented Apr 9, 2021

dawidwys commented Apr 13, 2021

AHeise 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

dawidwys commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetzger commented Apr 13, 2021

rmetzger commented Apr 13, 2021

dawidwys commented Apr 14, 2021

AHeise left a comment

Choose a reason for hiding this comment

tillrohrmann 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

AHeise commented Aug 25, 2021

tillrohrmann commented Aug 25, 2021

flinkbot commented Apr 9, 2021 •

edited

Loading

flinkbot commented Apr 9, 2021 •

edited

Loading