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

[Release-1.5] Add option to enable v8 runtime for telemetry v2 #21846

Merged
merged 3 commits into from Mar 8, 2020

Conversation

@bianpengyuan
Copy link
Contributor

bianpengyuan commented Mar 5, 2020

Manual cherry-pick of #21776 and #21801. This adds a preview profile and several new options for Wasm filters, as well as a test for it.

bianpengyuan and others added 2 commits Mar 5, 2020
* add a test for wasm based stats filter

* fix comments

* typo
@bianpengyuan bianpengyuan requested a review from istio/release-managers-1-5 as a code owner Mar 5, 2020
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 5, 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.

@googlebot googlebot added the cla: no label Mar 5, 2020
@istio-testing istio-testing added the size/L label Mar 5, 2020
@bianpengyuan bianpengyuan added cla: yes and removed cla: no labels Mar 5, 2020
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 5, 2020

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

dgn left a comment

While I think this is a great feature to get in, sneaking this in as part of a z-stream release doesn't feel right. @fpesce @johnma14 wdyt?

@dgn

This comment has been minimized.

Copy link
Contributor

dgn commented Mar 5, 2020

/retest

@fpesce

This comment has been minimized.

Copy link
Contributor

fpesce commented Mar 5, 2020

@dgn I think as an option it's ok to add.

@duderino

This comment has been minimized.

Copy link
Contributor

duderino commented Mar 5, 2020

I think this is safe enough. My only concern is the time it will take to rebuild 1.5.0

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Mar 5, 2020

My 2c (maybe 1c) is to merge it into 1.5.1 (or 1.6)

@duderino duderino changed the title [release-1.5] Add option to enable v8 runtime for telemetry v2 [Release-1.5] Add option to enable v8 runtime for telemetry v2 Mar 5, 2020
@duderino

This comment has been minimized.

Copy link
Contributor

duderino commented Mar 5, 2020

Definitely too late for 1.5.0. We could put this in 1.5.1, but we also don't need to since @bianpengyuan is going to document how to enable this with envoyfilter (and in 1.6.0 we can do it with the preview profile)

@bianpengyuan

This comment has been minimized.

Copy link
Contributor Author

bianpengyuan commented Mar 5, 2020

Now I think about it, strictly speaking this is not a new feature. Feature is already there in 1.5, and will be documented in istio.io 1.5. What this PR does is just low-risk improvement for the enablement of an experiemental feature. So I don't see why we not ship it in 1.5. If preview profile does not feel like something go into 1.5, I am fine with removing it and just leaving istioctl manifest update, which is still a big plus to the user experience.

@duderino

This comment has been minimized.

Copy link
Contributor

duderino commented Mar 6, 2020

@bianpengyuan the issue is that the 1.5.0 release has already been built and takes a lot of time to rebuild. But otherwise I agree with you.

And based on your argument, I agree that it could be added to a patch release.

@bianpengyuan

This comment has been minimized.

Copy link
Contributor Author

bianpengyuan commented Mar 6, 2020

Yeah, I never expect it to be in 1.5.0, I was arguing for 1.5.1. :)

@fpesce @dgn Let me know if this sounds right to you, and I could remove preview profile if that is too much. Thanks!

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 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.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 6, 2020
@bianpengyuan bianpengyuan added cla: yes and removed cla: no labels Mar 6, 2020
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Mar 6, 2020

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@bianpengyuan

This comment has been minimized.

Copy link
Contributor Author

bianpengyuan commented Mar 6, 2020

I removed preview profile and this PR only includes options to enable wasm runtime using istioctl. ptal. thanks!

Copy link
Contributor

mandarjog left a comment

Let’s get this in for 1.5.1

@mandarjog

This comment has been minimized.

Copy link
Contributor

mandarjog commented Mar 7, 2020

@fpesce ptal

@fpesce
fpesce approved these changes Mar 8, 2020
Copy link
Contributor

fpesce left a comment

As an option I think it's safe enough to ship in 1.5.1

@istio-testing istio-testing merged commit 5649ada into istio:release-1.5 Mar 8, 2020
23 checks passed
23 checks passed
cla/google CLAs have been manually verified by Googler who has set the 'cla: yes' label
e2e-bookInfoTests-envoyv2-v1alpha3_istio_release-1.5 Job succeeded.
Details
e2e-dashboard_istio_release-1.5 Job succeeded.
Details
e2e-mixer-no_auth_istio_release-1.5 Job succeeded.
Details
gencheck_istio_release-1.5 Job succeeded.
Details
integ-conformance-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-conformance-local-tests_istio_release-1.5 Job succeeded.
Details
integ-distroless-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-galley-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-galley-local-tests_istio_release-1.5 Job succeeded.
Details
integ-istioio-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-mixer-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-pilot-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-pilot-local-tests_istio_release-1.5 Job succeeded.
Details
integ-security-k8s-tests_istio_release-1.5 Job succeeded.
Details
integ-security-local-tests_istio_release-1.5 Job succeeded.
Details
integ-telemetry-k8s-tests_istio_release-1.5 Job succeeded.
Details
lint_istio_release-1.5 Job succeeded.
Details
pilot-e2e-envoyv2-v1alpha3_istio_release-1.5 Job succeeded.
Details
pilot-multicluster-e2e_istio_release-1.5 Job succeeded.
Details
release-test_istio_release-1.5 Job succeeded.
Details
tide In merge pool.
Details
unit-tests_istio_release-1.5 Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.