Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy-Wasm mega backport #284

Merged
merged 16 commits into from
Nov 20, 2020
Merged

Conversation

PiotrSikora
Copy link

This backport includes Proxy-Wasm C++ Host and SDK updates,
including Emscripten, as well as build and related reliability fixes.

@istio-testing
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@google-cla
Copy link

google-cla bot commented Nov 6, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Nov 6, 2020
@PiotrSikora
Copy link
Author

PiotrSikora commented Nov 6, 2020

@kyessenov @bianpengyuan I'm still waiting for envoyproxy#13753, envoyproxy#13836 and envoyproxy#13840, but I'd like to get those fixes merged into 1.8 release.

@kyessenov
Copy link

kyessenov commented Nov 6, 2020

I am not happy about changes in istio/envoy that are not in upstream. I'm not convinced they solve the problem (e.g. the leak is still there AFAICT) and the configuration model change will have unpredictable impact on stats module.

Have you validated that they do not destabilize istio/proxy?

@bianpengyuan
Copy link

@kyessenov Those two unmerged changes are not included in this PR, and yeah agree the change has to be in upstream first.

@kyessenov
Copy link

@bianpengyuan Yes, I didn't read "those" correctly at first. Still many of the changes included here are a build-up to the fixes. They are not needed unless we get the actual fixes.

@PiotrSikora
Copy link
Author

@kyessenov there are no changes here that are not already merged upstream. I tested with the existing tests in istio/proxy.

@google-cla
Copy link

google-cla bot commented Nov 13, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@PiotrSikora
Copy link
Author

The only outstanding issue is the memory leak (fixed with either envoyproxy#13836 or envoyproxy#13941), everything else is backported here.

@bianpengyuan @kyessenov let me know if you want to merge this now, so that we can run tests with this over the weekend, or if you want to wait for the memory leak to be merged first.

@google-cla
Copy link

google-cla bot commented Nov 19, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@PiotrSikora
Copy link
Author

Cherry-picked envoyproxy#13836, so this is now ready to go.

@bianpengyuan @kyessenov the question is whether we want to merge envoyproxy#13753 and do extended testing before 1.8.1, or should I remove it from this backport? I originally said that we can drop it, but I'm having second thoughts now.

@bianpengyuan
Copy link

After some testing, I am worried that wasm extensions that works with 1.8.0 could break with 1.8.1. Is it possible that we can avoid this massive update, especially tool chain and dependencies, and only cherry-pick code fixes?

@bianpengyuan
Copy link

Is envoyproxy#13753 the reason why we need to make this cherry-pick so bloated? If that is the case I'd rather push that to 1.9 because I don't believe there is user who heavily reconfigures the plugin.

keith and others added 3 commits November 19, 2020 23:47
If you build without sandboxing for performance, the output files from
this custom build genrule contained timestamps which caused it to
rebuild every single build.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@google-cla
Copy link

google-cla bot commented Nov 19, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 19, 2020
@PiotrSikora
Copy link
Author

(removed Emscripten update from the backports)

Copy link

@bianpengyuan bianpengyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine checking this in as long as we test

  1. Reconfiguration extensively
  2. Plugin works with 1.8.0 could still run with 1.8.1.

I can take both work items since I am already working on some example plugins with 1.8.

@google-cla
Copy link

google-cla bot commented Nov 20, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@PiotrSikora
Copy link
Author

I dropped a few changes from here (including reconfiguration) now that we have istio-release/v1.8 branch in Proxy-Wasm repos. PTAL.

Copy link

@bianpengyuan bianpengyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. thanks!

@bianpengyuan
Copy link

Test failure seems to be real.

PiotrSikora and others added 2 commits November 20, 2020 02:20
Signed-off-by: Kuat Yessenov <kuat@google.com>
@google-cla
Copy link

google-cla bot commented Nov 20, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 20, 2020
@PiotrSikora
Copy link
Author

Test failure seems to be real.

I forgot to revert one of the new tests to the old-style setup with reconfiguration. Fixed now.

@istio-testing istio-testing merged commit 5892439 into istio:release-1.8 Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants