Document re-evaluating existing robustness test reports

[serathius]

cc @ahrtr @siyuanfoundation @MadhavJivrajani

[MadhavJivrajani]

Non-blocking comment but--
Feels a little odd to just have an empty testdata dir. Thoughts on using a ROBUSTNESS_TESTDATA env var instead so that folks don't have to bother with the copying? Or maybe both, defaulting the value of the env var to testdata, still feels odd but atleast we won't have the copying.

[serathius]

Heh cannot use https://github.com/orgs/community/discussions/16925 because of failures in static analysis @jmhbnz

'markdown_marker' started at Mon Apr  8 08:14:18 UTC 2024
% 'marker' '--skip-http' '--allow-absolute-paths' '--root' '/home/runner/work/etcd/etcd' '-e' './CHANGELOG' '-e' './etcdctl' '-e' 'etcdutl' '-e' './tools'
FAIL: (code:1):
  % 'marker' '--skip-http' '--allow-absolute-paths' '--root' '/home/runner/work/etcd/etcd' '-e' './CHANGELOG' '-e' './etcdctl' '-e' 'etcdutl' '-e' './tools'
Found broken reference (!NOTE -> ) at /home/runner/work/etcd/etcd/tests/robustness/README.md:41
Found broken reference (!NOTE -> ) at /home/runner/work/etcd/etcd/tests/robustness/README.md:55
FAIL: 'run marker --skip-http --allow-absolute-paths --root /home/runner/work/etcd/etcd -e ./CHANGELOG -e ./etcdctl -e etcdutl -e ./tools' checking failed (!=0 return code)
FAIL: 'markdown_marker' FAILED at Mon Apr  8 08:14:18 UTC 2024
make: *** [Makefile:153: verify-markdown-marker] Error 255

[serathius]

Non-blocking comment but-- Feels a little odd to just have an empty testdata dir.

The directory already exist, I just previously kept 1 report there for regression testing. Unfortunately it's hard maintain and use for testing performance (validation of report took locally 2 seconds on my local machine, and over 2 minutes on CI).

Thoughts on using a ROBUSTNESS_TESTDATA env var instead so that folks don't have to bother with the copying? Or maybe both, defaulting the value of the env var to testdata, still feels odd but atleast we won't have the copying.
pying.

Reproducing the CI failures that are main motivator for this documentation, not local tests. I just add them for completeness. I don't like idea of writing all reports to the testdata directory. It's normal to run tens or hundreds of runs to reproduce/validate some issue. We either always override the previous results (current approach) or create a separate report for each run which will overwhelm the directory. By default reports are stored in /tmp/ directory as they can be pretty large (100MB+) as they include both WAL and bbolt data files. We would quickly run out of disk space if not overriding previous reports.

[serathius]

@MadhavJivrajani Could you check if the documentation is detailed enough for re-evaluating results from github CI?

[ahrtr]

Can we just ignore the tests/robustness/testdata directory in the top level .gitignore?

[serathius]

I wanted to have the directory present, for that I need at least one file in it. Git doesn't stores information about directories, only about files. Proposed to have .gitignore.

[MadhavJivrajani]

Leaving a couple of comments based on what I noticed trying to follow the instructions.

[MadhavJivrajani]

I think this depends on setup to setup, but we should add in a toolchain directive in the go.mod under tests/: https://github.com/etcd-io/etcd/blob/main/tests/go.mod

Reason for this is similar to: golang/go#62278

And I had to add this directive to make it work since go will try to download the toolchain that is specified by the go directive of the go.mod in the directory you're running in. It will take the go directive if the toolchain is missing. So we probably need to do something similar to what was done in the go.mod at the root.

[serathius]

Don't understand what you are proposing. If you want to propose to add toolchain directive, please file an issue. Don't think we should block this PR on this.

[MadhavJivrajani]

What I mean is if I run the code here following the instructions this is what I get:

❯ make test-robustness-reports
cd ./tests && go test ./robustness/validate -v --run TestDataReports
go: downloading go1.22 (darwin/amd64)
go: download go1.22 for darwin/amd64: toolchain not available
make: *** [test-robustness-reports] Error 1

and the above linked issue explains why.

Created a PR here @serathius: #17734

[serathius]

Thank you, I didn't know that the command could not work on some golang setups.

[jmhbnz]

Would an alternative be to do something like:

test-robustness-reports:
-	cd ./tests && go test ./robustness/validate -v --run TestDataReports
+	cd ./tests && GOTOOLCHAIN=$(cat ../.go-version) go test ./robustness/validate -v --run TestDataReports

We put a lot of effort removing a bunch of duplicated hardcoded versions in the main repo so we only had one main file to update when we bump go. Keen to try and keep that simplicity if possible.

[serathius]

/retest

[MadhavJivrajani]

Thank you @serathius!

[siyuanfoundation]

Non-blocking for this PR, but I think it would be nice to save the cluster and failpoint configurations for a report along side the report.

[jmhbnz]

Heh cannot use https://github.com/orgs/community/discussions/16925 because of failures in static analysis @jmhbnz

Interesting find @serathius, this is the first time I have looked properly at the markdown_marker section of ./test.sh and we are actually using a rust crate here.

We might want to look at using a more golang native approach for how we do linting for markdown. We might also want to customize markdown linting rules to allow github flavored markdown as GitHub do things outside of the standard markdown spec.

[serathius]

Non-blocking for this PR, but I think it would be nice to save the cluster and failpoint configurations for a report along side the report.

Right, that definitely be useful, but I would wait until the API stabilizes more.