/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandarjog

The full list of commands accepted by this bot can be found here.

Codecov Report

Merging #4292 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #4292    +/-   ##
=======================================
- Coverage      71%     71%   -<1%     
=======================================
  Files         315     315            
  Lines       28616   29094   +478     
=======================================
+ Hits        20116   20378   +262     
- Misses       7329    7491   +162     
- Partials     1171    1225    +54

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e114c0...c077ecf. Read the comment docs.

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @costinm @mandarjog

superseeds #4086 which does not pass tests either

yes, it's the only change that needs to be made to upgrade the proxy. There used to be another one in Docker.proxy, but that one got removed. Can I get a review approved?

circle-ci e2e pilot fails because of the known issue with routing rule v3.

@ZackButcher can you please approve? Need to review faster this kind of PRs.

merged with master, but still failing with v3, so that unrelated issue is not fixed.

I am sure I merged to master.

/retest all

/test istio-presubmit

@andraxylia it failed again in the exact same spot -- it's likely a real failure.

I0316 00:26:09.354] 2018-03-16T00:26:09.354142Z	info	RunBackground: /home/bootstrap/go/src/istio.io/proxy/src/envoy/http/mixer/start_envoy -e /home/bootstrap/go/out/linux_amd64/release/envoy > /tmp/mixer_envoy_env743812681/my_local_proxy.log 2>&1

try
./tests/integration/example/integration.sh
(or
go test -v ./tests/integration/example/tests/sample1 -envoy_binary /home/bootstrap/go/out/linux_amd64/release/envoy -envoy_start_script /home/bootstrap/go/src/istio.io/proxy/src/envoy/http/mixer/start_envoy -mixer_binary /home/bootstrap/go/out/linux_amd64/release/mixs -fortio_binary fortio
)
to run it locally
(@yutongz will improve the script so logs show up in artifacts)

This test is dangerous to run locally. It does a git reset --hard and some git clone.Luckily I did not have anything valuable in my ws.

Not sure what the value of this test is, and what it is supposed to test. Is it that envoy can be run as a process? It copies envoy back and forth. Until this test is written to a standard where people can understand what the problem is, it adds little value and should be disabled @kyessenov

I run integration.sh manually and debugged it, and this template file from proxy used by the start_envoy script is outdated and does not work with the latest envoy:

https://github.com/istio/proxy/blob/master/src/envoy/http/mixer/envoy.conf.template

See attached output and generated envoy.conf file.

@qiwzhang can help bring that conf file up to date. Meanwhile, the test should be disabled and this PR merged.

envoy.conf.txt
start_envoy_output.txt

Thanks for figuring it out, that's great

Meanwhile, the test should be disabled and this PR merged.

No I think given it works on master it should not be disabled but rather fixed
also we should maybe concerned about the incompatible change

ps: that template should be moved to istio/istio to avoid git cloning that repo just for that 1 file - that part maybe can wait

Meanwhile, we have to disable this test and remove the "git reset --hard" and all the other dangerous things, including the part where istio/proxy is re-downloaded. Hopefully people will not be tempted to run it locally and loose their changes. Tests should not alter the state of a workspace #4317

https://stackoverflow.com/questions/5788037/recover-from-git-reset-hard/5788069

@ldemailly this test is not ok, it should be disabled and removed until fixed properly.

That test should also do a make clean before make build, otherwise it uses old envoys from the /home/bootstrap directory. In a shared cluster, this can cause flakiness.

please do file the issues you found with the test, but in the meantime we need the presubmit to pass without dropping the test (imo)

This test is dangerous to run locally. It does a git reset --hard and some git clone.Luckily I did not have anything valuable in my ws.

there is always git reflog but the excerpt I mentioned above I took from the logs:

go test -v ./tests/integration/example/tests/sample1 -envoy_binary /home/bootstrap/go/out/linux_amd64/release/envoy -envoy_start_script /home/bootstrap/go/src/istio.io/proxy/src/envoy/http/mixer/start_envoy -mixer_binary /home/bootstrap/go/out/linux_amd64/release/mixs -fortio_binary fortio

isn't doing doing any of that, the ./tests/integration/example/integration.sh I guess indeed appears to be geared more toward machine than human runs / not to be ran without looking

That test should not be a pre-submit, there is nothing in it to justify it as a pre-submit.

@yutongz will work on removing the git reset part. I think we should wait until Wayne fixes the config to merge this. The point of this PR is to update Proxy, so if the current proxy is broken, it should not be merged.

@sebastienvas Wayne has fixed the config and I picked up the fix. I am waiting for the result. The failure of this test does not mean proxy is broken, it only means a file that only Wayne uses is out-of-date. So the test has little value and should not be in istio pre-submit, maybe it should be in istio/proxy presubmit. I suggest we continue the discussion about the usefulness and correctness of that test in #4317 , there are too many things about that test.

@qiwzhang Did you merge without waiting for all the proxy tests? It appears it did not upload the envoy with that SHA, it is likely the tests that upload it to storage.

@andraxylia SG

nvm, it seems there is a lag before Envoy is available

/retest all