Update Envoy to 124c160 (March 8th 2021)

[eric846]

• Sync .bazelrc
• Update Docker image version in .circleci/config.yml
• New way to declare Bazel pip imports in WORKSPACE (the requirement("...") side in test/integration/BUILD is unchanged)
• ci/run_envoy_docker.sh was unchanged
• Code changes
  • Relax some byte count checks in integration tests
    • The counters started to come out lower after updating Envoy, e.g. 1450 decreased to 900. This was because the underlying Envoy infrastructure in the Nighthawk client stopped sending content-length: 0 in the requests.
    • Updating the test to only check if the count is >=500, rather than checking for a specific value
      • This should still catch failure modes where Nighthawk fails after 1 request or less
      • test_http_h2 already has such relaxed checks
  • Pass through new TimeSource arg in allocateConnPool()
  • factoryForGrpcService() changed the skip_cluster_check boolean to an enum Envoy::Grpc::AsyncClientFactoryClusterChecks::Skip
  • Empty dumpState() override added to StreamDecoder
  • UUID generator for tracing was moved
  • Ignore warning "Unable to use runtime singleton for feature envoy.reloadable_features.header_map_correctly_coalesce_cookies"

[eric846]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: integration_test_coverage (failed build)
🔨 rebuilding ci/circleci: test (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #641 (comment) was created by @eric846.

see: more, trace.

[eric846]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #641 (comment) was created by @eric846.

see: more, trace.

[eric846]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: integration_test_coverage (failed build)

🐱

Caused by: a #641 (comment) was created by @eric846.

see: more, trace.

[mum4k]

Thank you for putting this together @eric846.

[mum4k]

We already have a way of skipping some warnings:

nighthawk/test/integration/nighthawk_test_server.py

Lines 63 to 69 in e2f5265

	# A list of _TestCaseWarnErrorIgnoreList instances, message pieces that should
	# be ignored even if logged by the test server at a WARNING or an ERROR
	# severity.
	#
	# If multiple test_case_regexp entries match the current test case name, all the
	# corresponding ignore lists will be used.
	_TEST_SERVER_WARN_ERROR_IGNORE_LIST = frozenset([

Can we utilize this one?

[eric846]

Done.

[oschaaf]

Re: "The counters started to come out lower after updating Envoy, e.g. 1450 decreased to 900. Is this expected?"
It might be worth comparing headers to have a look-see at what precisely changed. This seems significant enough to make an actual real difference?

[oschaaf]

Yeah, thinking about it more, the strict byte count testing may seem overly strict, but it might just be worth it as it will prompt investigation in cases like this. In headers-only tests we will transfer a significant % less bytes, possibly impacting packet counts required for transmitting a single request; so not requesting changes in this PR right away but I do feel it is worth discussing.

[eric846]

Re: "The counters started to come out lower after updating Envoy, e.g. 1450 decreased to 900. Is this expected?"
It might be worth comparing headers to have a look-see at what precisely changed. This seems significant enough to make an actual real difference?

Bingo: The Envoy infrastructure inside the Nighthawk client seems to have stopped sending content-length: 0 in the request.

This PR

$ nc -l 127.0.0.1 12345
GET / HTTP/1.1
host: 127.0.0.1:12345

$ bazel-bin/nighthawk_client --rps 1 --duration 5 http://127.0.0.1:12345
...
upstream_cx_tx_bytes_total              123         24.60
...

Main branch

$ nc -l 127.0.0.1 12345
GET / HTTP/1.1
host: 127.0.0.1:12345
content-length: 0

$ bazel-bin/nighthawk_client --rps 1 --duration 5 http://127.0.0.1:12345
...
upstream_cx_tx_bytes_total              180         36.00
...

[eric846]

Yeah, thinking about it more, the strict byte count testing may seem overly strict, but it might just be worth it as it will prompt investigation in cases like this. In headers-only tests we will transfer a significant % less bytes, possibly impacting packet counts required for transmitting a single request; so not requesting changes in this PR right away but I do feel it is worth discussing.

Agreed, opened #644 to discuss. Depending on the schedule, we might want to move forward with these relaxed assumptions for now, rather than adjust the strict counts. Or on the other hand, we might want to make the counts in the h2 test more strict to match. There's a chance Envoy could change its header behavior again.

[oschaaf]

Approving; while it seems odd to have a content-length header appear in a GET request, apparently it causes no trouble as all integration tests past — modulo the strict byte count check.
We may want to file an issue upstream though?