WIP: Validating config in docs #11394

dmitri-d · 2020-06-01T21:52:52Z

No description provided.

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

Really exciting to see this coming along!

tools/config_validation/validate_fragment.py

docs/_ext/validating_code_block.py

htuch · 2020-06-02T18:16:36Z

docs/_ext/validating_code_block.py

+        process = subprocess.Popen(['bazel', 'run', '//tools/config_validation:validate_fragment', '--', self.options.get('type-name'), "-s", "\n".join(self.content)],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE)
+        stdout, stderr = process.communicate()


This looks really nice and clean. You mention on Slack that this is pretty slow (around 10s), which won't scale. You mentioned a "customer builder", curious to learn more. If you want to continue with your existing PR, I'd suggest that you build a .par with all dependencies in docs/build.sh before invoking Sphinx. Then you can just reference the path of this .par here. That will allow you to skip all the Bazel overhead on each YAML processing.

That might still end up being too slow, e.g. if it takes ~1s and you have 100, that's over a minute and a half. When I would do then is add an env var to control validation. In CI we would always enable, but for local iterations on docs builds, we could disable.

You mentioned a "customer builder", curious to learn more

My thinking is that we'd use a dedicated pass to verify example configs by invoking a dedicated builder. All it would do is to validate configs and generate a report. When a "normal" builder is used, examples would still be rendered, but without validation.

Basically we'd be doing config validation in one batch and it would be explicitly invoked. Current implementation combines doc generation and validation, and I'm not sure I can/should continue on validation errors: I think putting a global state into a directive (which is what ValidatingCodeBlock is) is a way to go.

+1 to failing hard on validation errors.

How about this for an idea..

We have a Sphinx plugin that just writes out the (YAML fragment, type) tuples to some directory and build all the docs.

We then run them all through the config_validator in a single bazel run at the end?

I think what you are suggesting is quite close to a dedicated builder, it would qork quite similar to what you have described. This is the way to go If we want to validate all config examples and then report the ones that failed (as opposed to stopping at the first failure, like it's currently implemented).

Yeah, I think the other advantage is pure speed. Right now, you are invoking Bazel and Python multiple times to be able to do the validation. I'm guessing only a small fraction of CPU cycles are spent actually in the validation (could be worth measuring).

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

Looks great! Can you provide timing for docs build before this change, with the skip and without it? Thanks.

docs/_ext/validating_code_block.py

htuch · 2020-06-04T00:31:42Z

docs/_ext/validating_code_block.py

+
+    if ValidatingCodeBlock.skip_validation.lower() != 'true':
+      args = [
+          arg for arg in ['bazel', 'run'] + ValidatingCodeBlock.bazel_build_options.split() + [


shlex.split()

The reason I'm jumping through the hoops here is that ValidatingCodeBlock.bazel_build_options is put in quotes otherwise (by subprocess.Popen()), which breaks bazel command line parser. Not sure how shlex.split() would help?

shlex.split() knows how to take a string of distinct CLI options and split them back into their args, which it looks like is what is going on here. It handles things like shell escapes.

docs/build.sh

dmitri-d · 2020-06-04T18:20:39Z

Can you provide timing for docs build before this change, with the skip and without it?

Processing a single validated-code-block directive
with SPHINX_SKIP_CONFIG_VALIDATION=true: ~.02 ms (there's little difference between validated-code-block and plain code-block directives in this case -- by ~1 micro second)
with validation turned on: ~3.5s

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch · 2020-06-04T23:04:36Z

@dmitri-d looking at CI, it seems docs only took ~4 mins to do the work in https://app.circleci.com/pipelines/github/envoyproxy/envoy/26065/workflows/3b37b45e-45cb-49ae-ab83-463248faeaf0/jobs/350645/steps, whereas in other recent jobs, it's almost ~4-6mins, e.g. https://app.circleci.com/pipelines/github/envoyproxy/envoy/26021/workflows/86f49169-f140-4296-b7c4-1f415c368d34/jobs/350461/steps.

I guess this is because you haven't turned this on for more than one example?

I count 197 possible places we could use this in ag ":: yaml" generated/rst | wc -l.

So, that would be a slowdown of 689s, or a build time increase from 4m to 15.5m on CI.

@lizan @mattklein123 is this going to be acceptable? Or should we keep trying to find ways to speed up the validation.

We definitely want this functionality, it's a question of how best to express it.

mattklein123 · 2020-06-05T03:26:29Z

I think it's OK if we have a slowdown given how long other CI takes and how important this is, though it would be nice to see if there is any low hanging fruit to speed it up. I wonder if it would be faster to shell out to a C++ binary?

htuch · 2020-06-05T17:38:30Z

Rather than rewriting in C++, I think #11394 (comment) provides a path to be able to speed things up by avoiding having to spin up tons of Python processes.

stale · 2020-06-12T17:52:30Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Dmitri Dolguikh <[email protected]>

repokitteh-read-only · 2020-06-17T20:51:43Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11394 was synchronize by dmitri-d.

see: more, trace.

dmitri-d · 2020-06-17T20:53:15Z

documented SPHINX_SKIP_CONFIG_VALIDATION env var

dmitri-d · 2020-06-17T20:53:28Z

ping @htuch

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

LGTM modulo two nits, thanks!
/wait

htuch · 2020-06-19T00:18:35Z

docs/_ext/validating_code_block.py

+          self.options.get('type-name'), '-s', '\n'.join(self.content)
+      ]
+      process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+      stdout, stderr = process.communicate()


Nit: is there a reason not to just use subprocess.check_call() here?

check_call/check_output seem to lose at least some of the output (and using PIPE with stderr isn't recommended). The process output is helpful as it points to the error in yaml.

Have you tried subprocess.run (new in Python 3) at https://docs.python.org/3/library/subprocess.html? We're not a Python 3 only shop.

tools/config_validation/validate_fragment.py

stale · 2020-06-26T03:50:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-07-03T05:24:19Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2020-07-06T20:34:20Z

switched to argparser in validate_fragment.py

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2020-07-07T19:25:16Z

ping @htuch

htuch

Thanks, just the nits remaining.
/wait

tools/config_validation/validate_fragment.py

htuch · 2020-07-08T15:13:02Z

docs/_ext/validating_code_block.py

+          self.options.get('type-name'), '-s', '\n'.join(self.content)
+      ]
+      process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+      stdout, stderr = process.communicate()


Have you tried subprocess.run (new in Python 3) at https://docs.python.org/3/library/subprocess.html? We're not a Python 3 only shop.

Signed-off-by: Dmitri Dolguikh <[email protected]>

dmitri-d · 2020-07-08T17:51:21Z

switched to subprocess.run()
renamed cli arguments to message_type and fragment_path

ping @htuch

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch

LGTM, thanks! Can't wait to see this in action when we upgrade the examples. Such a huge improvement in maintainability.

dmitri-d · 2020-07-09T17:45:35Z

Thanks for the reviews and feedback!

mattklein123 · 2020-07-13T21:05:02Z

Amazing!!!

Signed-off-by: Dmitri Dolguikh <[email protected]> Signed-off-by: scheler <[email protected]>

Dmitri Dolguikh added 2 commits June 1, 2020 14:51

Validation of configuration examples in docs

97e003b

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream' into validating-config-in-docs

4975c02

Signed-off-by: Dmitri Dolguikh <[email protected]>

mattklein123 assigned htuch Jun 2, 2020

htuch reviewed Jun 2, 2020

View reviewed changes

Dmitri Dolguikh added 3 commits June 2, 2020 16:01

Responded to feedback

906ff63

Signed-off-by: Dmitri Dolguikh <[email protected]>

Improved error reporting

6b92ccb

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed rendering of BAZEL_BUILD_OPTIONS during bazel command generation

204428d

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch reviewed Jun 4, 2020

View reviewed changes

Dmitri Dolguikh added 2 commits June 4, 2020 11:23

Responded to feedback

382c36d

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed formatting

c7bdb31

Signed-off-by: Dmitri Dolguikh <[email protected]>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 12, 2020

Dmitri Dolguikh added 2 commits June 17, 2020 13:50

Dcoumented SPHINX_SKIP_CONFIG_VALIDATION env var

881e654

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream' into validating-config-in-docs

2bbe15c

Signed-off-by: Dmitri Dolguikh <[email protected]>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 17, 2020

repokitteh-read-only bot added the api label Jun 17, 2020

Dmitri Dolguikh added 4 commits June 17, 2020 14:33

Fixed build failure

0f0446b

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed formatting issues

071de20

Signed-off-by: Dmitri Dolguikh <[email protected]>

Fixed format

ee0ef1d

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream' into validating-config-in-docs

3312681

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch suggested changes Jun 19, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 19, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 26, 2020

stale bot closed this Jul 3, 2020

Dmitri Dolguikh added 2 commits July 6, 2020 10:39

Merge remote-tracking branch 'upstream' into validating-config-in-docs

77b7b75

Signed-off-by: Dmitri Dolguikh <[email protected]>

Switched to argparse in validate_fragment.py

4f0aecd

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch reopened this Jul 6, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2020

repokitteh-read-only bot removed the waiting label Jul 6, 2020

Dmitri Dolguikh added 2 commits July 6, 2020 15:55

Fixed format

1fb3282

Signed-off-by: Dmitri Dolguikh <[email protected]>

Merge remote-tracking branch 'upstream' into validating-config-in-docs

8619b7a

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch suggested changes Jul 8, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 8, 2020

Responded to feedback

6c13087

Signed-off-by: Dmitri Dolguikh <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 8, 2020

Fixed build failure

a60d3ff

Signed-off-by: Dmitri Dolguikh <[email protected]>

htuch approved these changes Jul 8, 2020

View reviewed changes

htuch merged commit a1d3f4b into envoyproxy:master Jul 9, 2020

scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020

Validating config in docs (envoyproxy#11394)

bd7440f

Signed-off-by: Dmitri Dolguikh <[email protected]> Signed-off-by: scheler <[email protected]>

htuch mentioned this pull request Sep 21, 2020

docs: Fix REQ command operator usage example #13197

Merged

mattklein123 mentioned this pull request Sep 23, 2020

docs: verify config snippets are valid #8837

Open

htuch mentioned this pull request Sep 23, 2020

Make it easier to build docs #13229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Validating config in docs #11394

WIP: Validating config in docs #11394

dmitri-d commented Jun 1, 2020

htuch left a comment

htuch Jun 2, 2020

dmitri-d Jun 2, 2020

htuch Jun 2, 2020

dmitri-d Jun 2, 2020

htuch Jun 5, 2020

htuch left a comment

htuch Jun 4, 2020

dmitri-d Jun 4, 2020 •

edited

Loading

htuch Jun 4, 2020

dmitri-d commented Jun 4, 2020

htuch commented Jun 4, 2020

mattklein123 commented Jun 5, 2020

htuch commented Jun 5, 2020

stale bot commented Jun 12, 2020

repokitteh-read-only bot commented Jun 17, 2020

dmitri-d commented Jun 17, 2020

dmitri-d commented Jun 17, 2020

htuch left a comment

htuch Jun 19, 2020

dmitri-d Jul 6, 2020

htuch Jul 8, 2020

stale bot commented Jun 26, 2020

stale bot commented Jul 3, 2020

dmitri-d commented Jul 6, 2020

dmitri-d commented Jul 7, 2020

htuch left a comment

htuch Jul 8, 2020

dmitri-d commented Jul 8, 2020

htuch left a comment

dmitri-d commented Jul 9, 2020

mattklein123 commented Jul 13, 2020

WIP: Validating config in docs #11394

WIP: Validating config in docs #11394

Conversation

dmitri-d commented Jun 1, 2020

htuch 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

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jun 4, 2020

htuch commented Jun 4, 2020

mattklein123 commented Jun 5, 2020

htuch commented Jun 5, 2020

stale bot commented Jun 12, 2020

repokitteh-read-only bot commented Jun 17, 2020

dmitri-d commented Jun 17, 2020

dmitri-d commented Jun 17, 2020

htuch 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

stale bot commented Jun 26, 2020

stale bot commented Jul 3, 2020

dmitri-d commented Jul 6, 2020

dmitri-d commented Jul 7, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jul 8, 2020

htuch left a comment

Choose a reason for hiding this comment

dmitri-d commented Jul 9, 2020

mattklein123 commented Jul 13, 2020

dmitri-d Jun 4, 2020 •

edited

Loading