Use envoy-wasm as upstream#2252
Conversation
|
/lgm
/approve
…On Fri, May 31, 2019 at 1:54 PM Pengyuan Bian ***@***.***> wrote:
@bianpengyuan <https://github.com/bianpengyuan> requested your review on:
#2252 <#2252> Use envoy-wasm as
upstream.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2252?email_source=notifications&email_token=ACIYRRRIHKRKXHOLJLQRHN3PYGGATA5CNFSM4HR55Y42YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGORX6E5EQ#event-2382122642>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACIYRRVKTFQ2RSMEMNCAINDPYGGATANCNFSM4HR55Y4Q>
.
|
|
/retest proxy-presubmit-tsan |
|
/test proxy-presubmit-tsan |
|
Per envoyproxy/envoy-wasm#19 (comment) sanitizer builds might be broken due to WAVM. |
|
@kyessenov the failure seems to be caused by flakiness in an existing test (lru_cache), and there is a typo in lgtm so could you do it again? |
|
As @kyessenov said, WAVM is leaking memory, but we don't exercise WASM tests in istio/proxy, and the local tests are passing... Perhaps we should be running upstream tests (all of them, not only WASM) as part of Q&A as well? Otherwise, we might be shipping broken build (e.g. due to different build options that upstream). cc @duderino |
|
/lgtm
…On Fri, May 31, 2019 at 3:57 PM Piotr Sikora ***@***.***> wrote:
As @kyessenov <https://github.com/kyessenov> said, WAVM is leaking
memory, but we don't exercise WASM tests in istio/proxy, and the local
tests are passing...
Perhaps we should be running upstream tests (all of them, not only WASM)
as part of Q&A as well? Otherwise, we might be shipping broken build (e.g.
due to different build options that upstream).
cc @duderino <https://github.com/duderino>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2252?email_source=notifications&email_token=ACIYRRX2PK5ETDLCYIW7DLDPYGUNJA5CNFSM4HR55Y42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWRYTA#issuecomment-497884236>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACIYRRTEKQCT6635KEFQEJDPYGUNJANCNFSM4HR55Y4Q>
.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bianpengyuan, kyessenov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@PiotrSikora This is on master branch, we are not trying to ship anything from this code yet, but just want to include the null sandbox API to unblock development. Are you worrying about daily build? Ideally envoy-wasm should be running all envoy test soon, right? |
|
@bianpengyuan no, what I'm saying that we have false sense of correctness, because we never run upstream tests (and I'm not talking only about envoy-wasm, but envoy in general) using the binaries that we build in istio/proxy. |
|
@PiotrSikora So it is a general question always existing with this repo. My understanding is that upstream should be responsible for testing its core functionality and filter interface, so that istio-proxy or any out of tree filter could focus on testing its own functionality, which is self-contained. Is this wrong and we should always run more upstream tests? Seems like quite a burden to me :) |
|
@bianpengyuan @PiotrSikora filed https://github.com/istio/proxy/issues/2256. We can break envoy tests if we change dependency versions, so we have to test it again here. |
|
Hey is there anything else now that I need to do to build Istio proxy? Running Revert this PR would allow |
|
@pitlv2109 what's the error output? |
|
It seems like it has something to do with me not having a |
|
Yeah looks like you need to install clang-7 and clang++-7. |
|
Thanks! I'll give it a try |
|
Looks like there is now some linking issue. Just curious, why did we switch to use envoy-wasm? |
|
It is to unblock development of mixer v2 because webassembly filter API is only available in envoy-wasm repo and has not been upstreamed. I assume your change should have nothing to do with webassembly filter, so feel free to change workspace back to use envoy upstream directly in your local environment. |
|
@bianpengyuan do you need WASM runtimes right now or only the null sandbox? |
|
@PiotrSikora We'd like a way to build only null sandbox for presubmit to avoid excessive build time, and build envoy with full WASM runtime at post-submit and run extension as a module in order to make sure the development is not breaking WASM integration. |
|
@bianpengyuan @kyessenov @mandarjog @PiotrSikora I'm a bit worried about this PR. It's making it harder for us to maintain the release-1.0, release-1.1, and release-1.2 branches because we are supposed to merge into master first, then cherry pick into one of those branches, but master is now very different from our release branches. Can you move your work to a different branch? CC: @crazyxy who is trying to get #2270 merged to unblock the 1.2 release and is running into problems on master |
|
Envoy in master is at exactly the same version as in release-1.2 (+ WASM changes, but they are self-contained), so I'm not sure how it affects #2270. |
|
Well for starters, I can't even build that PR which I was trying to do to debug some of the automated test failures on Yan's PR |
Backported from istio#2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Backported from istio#2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Backported from istio#2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Backported from istio#2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Backported from #2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
* Always use Bazel-0.25 for macOS on CircleCI (release-1.2). Backported from #2252. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: update Bazel for Linux on CircleCI. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
|
@bianpengyuan @kyessenov in retrospect, I'm more convinced that this should not have been merged to master and instead should have been done on a branch |
|
@duderino What is the downside of shipping with envoy-wasm? Sorry if this is an obvious question but I found it is hard for me to think of any... The extra code are all filter based and any feature shipped with envoy wasm filter will be considered experimental or very early alpha. They will not be enabled by default. Also we are not building istio proxy with v8 or wavm, but just a wrapper of envoy's filter API, so it seems to be not risky at all. |
|
Master is just another branch. We fully intend to ship the branch with envoy-wasm as the base, so it's not clear to me why the designation makes a difference. |
|
@duderino why? |
|
The immediate trigger for most people, I think, is that Istio has never carried such a large fork from upstream Envoy before. Yes, envoy-wasm will be upstreamed, but it will undergo changes during the code reviews which may break backwards compatibility. So the argument should be reversed - why do we want to ship envoy-wasm with Istio 1.3? Are there features we want to expose early? Or is it just easier for us to do all our development on master? |
|
envoy-wasm is a requirement for prometheus and stackdriver adapters in mixer v2 architecture. |
|
Regarding backward compatibility, do you mean wasm API's compatibility? I think we are not going to advertise it anywhere or publish it as feature on istio.io. That said it is not even an alpha feature, so I guess it is OK to make breaking changes when upstream? If people want to try out WASM based filter with 1.3, they should expect changes. |
|
@duderino this is not a fork, it's an up-to-date mirror of envoyproxy/envoy with extensions hosted there instead of here (i.e. istio/proxy) to ease the upstreaming process. There is zero difference between extensions hosted in envoyproxy/envoy-wasm and those hosted here, so people making an issue out of it just don't know what they're talking about. |
|
In any case, we're going to branch |
No description provided.