[3.4] fix: enable strict mode for CI #15504

fuweid · 2023-03-18T03:29:12Z

By default, the pipefail is disabled. For instance, the commands should
return 1 as exit code, but we get zero. After pipefail enabled, it's
back to normal.

~~And the test.log is used to grep the error from a ton of messages. So~~
~~that we should set +e and set it back after egrep.~~

fixes: #15487

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

~~1. If failed to build: https://github.com/etcd-io/etcd/actions/runs/4453530207/jobs/7822105485~~
~~2. If panic in the test: https://github.com/etcd-io/etcd/actions/runs/4453540829/jobs/7822123516~~

@ahrtr @serathius @jmhbnz

fuweid · 2023-03-18T05:00:25Z

.github/workflows/tests.yaml

@@ -58,8 +58,12 @@ jobs:
            GO_BUILD_FLAGS='-v' GOARCH=s390x ./build
            ;;
          linux-amd64-grpcproxy)
+            set -o pipefail; set +e;


if it looks good, I will file a fix pr for main branch.

jmhbnz

LGTM

To test I checked out this pr, pushed as a new branch with a bogus edit to a test file.
Opening a PR against 3.4 the workflow now correctly failed, refer: https://github.com/etcd-io/etcd/actions/runs/4454519493/jobs/7823765743?pr=15506

Thanks @fuweid 🙏🏻

ahrtr

Good catch.

Thank you @fuweid

Usually we fix the main branch firstly, then backport to 3.5 & 3.4. But I won't insist on it for such a small change.

ahrtr · 2023-03-19T23:26:13Z

.github/workflows/tests.yaml

            PASSES='build grpcproxy' CPU='4' RACE='true' ./test 2>&1 | tee test.log
-            ! egrep "(--- FAIL:|DATA RACE|panic: test timed out|appears to have leaked)" -B50 -A10 test.log
+            ecode=$?
+            egrep "(--- FAIL:|DATA RACE|panic: test timed out|appears to have leaked)" -B50 -A10 test.log


Why do you remove the ! ? We may need to keep it. If we find any (--- FAIL:|DATA RACE|panic: test timed out|appears to have leaked), we may still consider the test failed.

Sorry for the late reply.

I was thinking if we can delete the egrep command in this patch.

I didn't see any comment about egrep introduced in #8655.
It seems that the egrep is used to fix the unexpected exit code, since | tee won't return expected exit code because of pipefail.

As the tee already writes the log in the stdout, we won't lost any details and the command will return expected exit code. Maybe we can go with the following script:

set -o pipefail PASSES='build grpcproxy' CPU='4' RACE='true' ./test 2>&1 | tee test.log

Does it make senses?

NOTE: In my opinion, if the test.log is not in the CI artifacts, we don't need the log file.

It should be OK, because the following case will never happen:
The test script/cases exits with 0, but somehow there are some (--- FAIL:|DATA RACE|panic: test timed out|appears to have leaked) errors.

Yes. So I believe that egrep is to fix the pipefail issue. I will update the pr later and send new pr to main branch as well.

One of the things that first caught my eye in etcd scripting is fact that it doesn't run in strict mode and it even depends on some parts failing for correctness. By strict mode I mean set -euo pipefail as defined in http://redsymbol.net/articles/unofficial-bash-strict-mode/.

To my understanding !grep (--- FAIL:|... was a workaround for not properly written scripts. I think long term it would be better to fix all scripts and switch on strict mode. What do you think? @ahrtr @fuweid

By strict mode I mean set -euo pipefail as defined in

Makes sense to me.

serathius

It came to me while reviewing #15519.
We cannot just set set -o pipefail; set +e; in run as bash doesn't pass the flags we set to scripts it calls. This will only affect run and not ./test.

So this change doesn't work if ./test runs go test that writes --- FAIL: to stdout and doesn't exit with non-zero status code.

fixes: etcd-io#15487 Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid · 2023-03-22T10:13:16Z

test

@@ -588,6 +596,8 @@ function receiver_name_pass {
 }

 function commit_title_pass {
+	# TODO: git rev-parse --abbrev-ref doesn't work in pull-request and it


https://github.com/etcd-io/etcd/actions/runs/4488556590/jobs/7893308908?pr=15504#step:5:135

fuweid · 2023-03-22T10:15:25Z

ping @serathius @ahrtr @jmhbnz please take a look. Thanks

serathius · 2023-03-23T10:14:08Z

Can you prepare similar PR for v3.5?

fuweid · 2023-03-23T11:22:29Z

Can you prepare similar PR for v3.5?

yeah. Still working on it. Will file it when it is ready.

fuweid force-pushed the fix-15487 branch 5 times, most recently from 3e2d228 to 9d7354f Compare March 18, 2023 04:00

fuweid changed the title ~~fix: use pipefail to show the error~~ [3.4] fix: enable pipefail mode for grpcproxy test Mar 18, 2023

fuweid force-pushed the fix-15487 branch from 9d7354f to 91a101a Compare March 18, 2023 04:07

fuweid marked this pull request as ready for review March 18, 2023 04:55

fuweid commented Mar 18, 2023

View reviewed changes

jmhbnz approved these changes Mar 18, 2023

View reviewed changes

ahrtr approved these changes Mar 19, 2023

View reviewed changes

ahrtr reviewed Mar 19, 2023

View reviewed changes

serathius mentioned this pull request Mar 20, 2023

Rewrite etcd scripts in strict mode #15514

Closed

serathius requested changes Mar 20, 2023

View reviewed changes

fuweid marked this pull request as draft March 22, 2023 06:08

fuweid force-pushed the fix-15487 branch from 91a101a to 130ab5b Compare March 22, 2023 09:30

fix: enable strict mode for CI

3fc5fbe

fixes: etcd-io#15487 Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid force-pushed the fix-15487 branch from 130ab5b to 3fc5fbe Compare March 22, 2023 09:57

fuweid commented Mar 22, 2023

View reviewed changes

fuweid changed the title ~~[3.4] fix: enable pipefail mode for grpcproxy test~~ [3.4] fix: enable strict mode for CI Mar 22, 2023

fuweid marked this pull request as ready for review March 22, 2023 10:14

serathius approved these changes Mar 23, 2023

View reviewed changes

serathius merged commit e0fcb9e into etcd-io:release-3.4 Mar 23, 2023

fuweid mentioned this pull request May 14, 2023

Actions workflow for 3.4 passed despite failures within tests #15487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.4] fix: enable strict mode for CI #15504

[3.4] fix: enable strict mode for CI #15504

fuweid commented Mar 18, 2023 •

edited

Loading

fuweid Mar 18, 2023

jmhbnz left a comment

ahrtr left a comment

ahrtr Mar 19, 2023

fuweid Mar 20, 2023 •

edited

Loading

ahrtr Mar 20, 2023

fuweid Mar 20, 2023

serathius Mar 20, 2023

ahrtr Mar 20, 2023

serathius left a comment •

edited

Loading

fuweid Mar 22, 2023

fuweid commented Mar 22, 2023

serathius commented Mar 23, 2023

fuweid commented Mar 23, 2023

[3.4] fix: enable strict mode for CI #15504

[3.4] fix: enable strict mode for CI #15504

Conversation

fuweid commented Mar 18, 2023 • edited Loading

fuweid Mar 18, 2023

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr Mar 19, 2023

Choose a reason for hiding this comment

fuweid Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

ahrtr Mar 20, 2023

Choose a reason for hiding this comment

fuweid Mar 20, 2023

Choose a reason for hiding this comment

serathius Mar 20, 2023

Choose a reason for hiding this comment

ahrtr Mar 20, 2023

Choose a reason for hiding this comment

serathius left a comment • edited Loading

Choose a reason for hiding this comment

fuweid Mar 22, 2023

Choose a reason for hiding this comment

fuweid commented Mar 22, 2023

serathius commented Mar 23, 2023

fuweid commented Mar 23, 2023

fuweid commented Mar 18, 2023 •

edited

Loading

fuweid Mar 20, 2023 •

edited

Loading

serathius left a comment •

edited

Loading