HTTP2: Add DumpState support for HTTP2 by KBaichoo · Pull Request #14923 · envoyproxy/envoy

KBaichoo · 2021-02-03T03:34:21Z

Signed-off-by: Kevin Baichoo [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: HTTP2: Add DumpState support for HTTP2
Additional Description: While dispatching, the HTTP2 Connection object registers itself as a scopetrackedobject. If we crash during parsing pipeline, we'll dump information such as the current streams, current buffer, etc.
Risk Level: medium
Testing:unit tests
Docs Changes: N/A
Release Notes: Included
Platform Specific Features: N/A
Related Issue #14249

This is a follow up to PR #14812.

Originally got some feedback on this in KBaichoo#2.

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-02-03T03:34:33Z

/assign @antoniovicente

antoniovicente

I think it looks good otherwise.

antoniovicente · 2021-02-04T00:46:10Z

source/common/common/utility.cc

    case '\t':
      os << "\\t";
      break;
+    case '\v':


Please also escape '\0'

test/common/http/http2/codec_impl_test.cc

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-02-04T19:20:54Z

/retest

repokitteh-read-only · 2021-02-04T19:20:58Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14923 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo

Thanks for the review @antoniovicente !

KBaichoo · 2021-02-05T13:31:46Z

source/common/common/utility.cc

    case '\t':
      os << "\\t";
      break;
+    case '\v':


test/common/http/http2/codec_impl_test.cc

Signed-off-by: Kevin Baichoo <[email protected]>

antoniovicente · 2021-02-06T02:08:36Z

source/common/http/http2/codec_impl.cc

+
+void ConnectionImpl::StreamImpl::dumpState(std::ostream& os, int indent_level) const {
+  const char* spaces = spacesForLevel(indent_level);
+  os << "ConnectionImpl::StreamImpl " << this << DUMP_MEMBER(stream_id_)


nit: I think you're missing spaces in this line

os << spaces << "ConnectionImpl::StreamImpl "

antoniovicente · 2021-02-06T02:19:23Z

test/common/http/http2/codec_impl_test.cc

+  OutputBufferStream ostream{buffer.data(), buffer.size()};
+
+  Stats::TestUtil::MemoryTest memory_test;
+  server_->dumpState(ostream, 0);


nit: passing in a non-zero number of spaces here may make testing easier/more comprehensive. suggestion: set to 2 instead of 0.

Good idea. It now generates 2 spaces for all of these tests. Note: the spacesForLevel generates min(12, 2*i) spaces for i in [0, inf).

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-02-08T15:11:27Z

@antoniovicente since you're the maintainer on-call can you assign someone from @envoyproxy/senior-maintainers who is a non-googler? Thanks!

KBaichoo · 2021-02-09T02:53:53Z

/retest

repokitteh-read-only · 2021-02-09T02:53:56Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14923 (comment) was created by @KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-02-09T18:55:23Z

/retest

repokitteh-read-only · 2021-02-09T18:55:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14923 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-02-09T21:12:16Z

PTAL @snowp . @antoniovicente suggested you'd be a good senior-maintainer to look at this.

Thanks!

/assign @snowp

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-02-16T21:45:49Z

/retest

Windows seems like a flake

repokitteh-read-only · 2021-02-16T21:45:52Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14923 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-02-17T00:52:02Z

/retest

repokitteh-read-only · 2021-02-17T00:52:07Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14923 (comment) was created by @KBaichoo.

see: more, trace.

moderation · 2021-02-17T18:25:01Z

I'm getting a compilation error on RHEL 8.3 with Clang 10

no member named 'for_each_n' in namespace 'std'

Is this a Clang feature issue or something else @KBaichoo ?

Edit: this line is the only one in the codebase that uses for_each_n. May be a C++17 or greater feature

KBaichoo · 2021-02-17T18:28:39Z

It is a C++17 feature: https://en.cppreference.com/w/cpp/algorithm/for_each_n

Edit: What's the procedure then? I'm fine with just replacing it.

moderation · 2021-02-17T18:39:39Z

This appears to be the first introduction of a C++17 feature and it breaks Clang 10. The current build requirements are outlined at https://github.com/envoyproxy/envoy/blob/main/docs/root/start/building.rst. Will likely need the @envoyproxy/senior-maintainers to chime in

jrajahalme · 2021-02-17T23:48:11Z

for_each_n also breaks build on GCC 7. Apparently it is not implemented in libstdc++, maybe libc++ works. Build docs state that both are supported, though. Maybe code the loop without using for_each_n?

snowp · 2021-02-17T23:50:33Z

Yeah I think the simplest thing to do here is to just avoid the use of std::for_each_n since it seems to be poorly supported. @lizan do you have any thoughts here? Is this just due to std lib implementations poorly supporting C++17 features?

antoniovicente · 2021-02-17T23:59:15Z

IIRC there are other places where the Envoy codebase depends on C++ 17. Is there some compiler flag that can be used to disallow for_each_n in c++ 17 if we're not ready to support it yet?

KBaichoo · 2021-02-18T00:06:37Z

We use structured bindings in

envoy/source/common/upstream/subset_lb.cc

Line 137 in de8c505

auto [iterator, did_insert] =

which is c++17

snowp · 2021-02-18T00:16:02Z

Language/library support in compilers tend to be on a per feature basis, so I can totally believe that some of the older standard library versions don't support this feature while still supporting structured bindings. I think if we want to maintain support for GCC-7 we would probably want to avoid using this, similar to how we avoid using std::filesystem due to spotty support on some platforms (e.g. mobile).

Is there some compiler flag that can be used to disallow for_each_n in c++ 17 if we're not ready to support it yet?
I doubt it, probably our best bet would be a lint check

KBaichoo · 2021-02-18T01:56:29Z

See: #15093 for a lint of this as you suggested.

lizan · 2021-02-18T02:15:13Z

It must be due to older libstdc++, libstdc++-9 has support for that. @moderation can you try with gcc-toolset-9?

moderation · 2021-02-18T16:47:50Z

@lizan I have gcc-toolset-10 in my RHEL 8 environment and will try that but I recall moving to the clang 10 for some reason. Will try and report back

moderation · 2021-02-18T18:14:00Z

@lizan @KBaichoo With no config flag and therefore gcc 10 + libstdc++ and a very limited set of extensions enabled the first failure was in the quic code (I forgot to comment that out).

I commented that extension out and the next problem I hit was

So with the addition of the single C++17 feature it doesn't look like there is a way to build on RHEL 8.3 with either GCC 10 or Clang 10

lizan · 2021-02-18T19:26:36Z

You can use libstdc++ >= 9 with clang, no need use gcc, but that gcc 10 issue is a one issue that we should address.

What's your output of clang -x c++ -v -c /dev/null? If you see it picks libstdc++-8, you might need symlink ln -s /opt/rh/gcc-toolset-9/ /usr/lib/gcc/x86_64-redhat-linux/9 to let clang to pick a newer libstc++.

rgs1 · 2021-02-22T18:59:52Z

This broke for me too in ubuntu 18 fwiw. Are we planning to revert or document/fix fwd?

rgs1 · 2021-02-22T19:00:44Z

Ah nvm #15093 is the fix, i'll wait for that (or add it to our repo in the meanwhile).

moderation · 2021-02-22T19:36:23Z

Looking forward to #15093 too as we are still blocked with Clang 10 on RHEL8.3 (and GCC is broken too but we'd rather stick with Clang)

antoniovicente · 2021-02-23T01:49:48Z

PR #15093 seems stuck in review. It would be good to unblock folks using ubuntu 18 and RHEL8.3. What is the recommended path forward?

This reverts commit 3f740bc. Because it breaks builds on RHEL 8 and Ubuntu 18. See: envoyproxy#15093 Signed-off-by: Raul Gutierrez Segales <[email protected]>

rgs1 · 2021-02-23T15:41:51Z

I'd kindly suggest we revert (PR here #15153) for now, so we can unblock people who need to track master closely.

antoniovicente · 2021-02-23T17:41:12Z

PR #15093 was merged, ubuntu 18 and RHEL8.3 builds should work again.

Implement H2 Dump.

5668414

Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only bot assigned antoniovicente Feb 3, 2021

antoniovicente reviewed Feb 4, 2021

View reviewed changes

test/common/http/http2/codec_impl_test.cc Outdated Show resolved Hide resolved

antoniovicente reviewed Feb 4, 2021

View reviewed changes

test/common/http/http2/codec_impl_test.cc Outdated Show resolved Hide resolved

KBaichoo added 2 commits February 4, 2021 14:41

Use EndsWith instead of HasSubstr; try to appease msvc compiler.

930ab22

Signed-off-by: Kevin Baichoo <[email protected]>

Fix cross platform address issue.

6e92b59

Signed-off-by: Kevin Baichoo <[email protected]>

antoniovicente added the waiting label Feb 5, 2021

KBaichoo commented Feb 5, 2021

View reviewed changes

Escape null char.

337f40a

Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only bot removed the waiting label Feb 5, 2021

antoniovicente previously approved these changes Feb 6, 2021

View reviewed changes

Formatting output dump.

53068d9

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo dismissed antoniovicente’s stale review via 53068d9 February 8, 2021 15:05

Merge remote-tracking branch 'upstream/main' into h2-rebase

1db1ac2

Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only bot assigned snowp Feb 9, 2021

antoniovicente previously approved these changes Feb 10, 2021

View reviewed changes

KBaichoo mentioned this pull request Feb 11, 2021

HTTP: Associate Response crashes with the corresponding Request. #15029

Merged

KBaichoo added 2 commits February 16, 2021 18:51

Merge remote-tracking branch 'upstream/main' into h2-rebase

f3b0502

Signed-off-by: Kevin Baichoo <[email protected]>

Update test from protocol_constraints change from merge.

fd6ea43

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo dismissed antoniovicente’s stale review via fd6ea43 February 16, 2021 19:01

snowp merged commit 3f740bc into envoyproxy:main Feb 17, 2021

KBaichoo mentioned this pull request Feb 18, 2021

Lint: Lint for std::for_each_n #15093

Merged

KBaichoo mentioned this pull request Feb 23, 2021

Replace for-each-n loop. #15154

Closed

moderation mentioned this pull request Apr 7, 2021

Initial partial support for C++20 in Envoy #15874

Open

Conversation

KBaichoo commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo commented Feb 3, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovicente Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

KBaichoo Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KBaichoo commented Feb 4, 2021

Uh oh!

repokitteh-read-only bot commented Feb 4, 2021

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

KBaichoo Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

KBaichoo Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

KBaichoo Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

KBaichoo commented Feb 8, 2021

Uh oh!

KBaichoo commented Feb 9, 2021

Uh oh!

repokitteh-read-only bot commented Feb 9, 2021

Uh oh!

KBaichoo commented Feb 9, 2021

Uh oh!

repokitteh-read-only bot commented Feb 9, 2021

Uh oh!

KBaichoo commented Feb 9, 2021

Uh oh!

KBaichoo commented Feb 16, 2021

Uh oh!

repokitteh-read-only bot commented Feb 16, 2021

Uh oh!

KBaichoo commented Feb 17, 2021

Uh oh!

repokitteh-read-only bot commented Feb 17, 2021

Uh oh!

moderation commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moderation commented Feb 17, 2021

Uh oh!

jrajahalme commented Feb 17, 2021

Uh oh!

snowp commented Feb 17, 2021

Uh oh!

antoniovicente commented Feb 17, 2021

Uh oh!

KBaichoo commented Feb 18, 2021

Uh oh!

snowp commented Feb 18, 2021

Uh oh!

KBaichoo commented Feb 18, 2021

Uh oh!

KBaichoo commented Feb 3, 2021 •

edited

Loading

moderation commented Feb 17, 2021 •

edited

Loading

KBaichoo commented Feb 17, 2021 •

edited

Loading