makefile, test and scripts updates to use go toolchain instead of bazel. #2220

costinm · 2017-12-18T03:14:49Z

Multiple changes to improve build and testing with the go toolchain.

presubmit changes to use go tools.
stop using the k8s config linked from the user home to some dir that gets picked by bazel.
It is pretty dangerous and flaky - unit tests are actually run against whatever cluster the
developer has set, may affect a live cluster. It also pollutes the source tree and is not standard
go. Refactored the files using config to use a common method.
script equivalent with the circleci local unit test env, consisting of etcd+apiserver.
checkin few missing files, pending Dougz full move to make/shell generation for automation.
Current circleci build simply excludes the tests.

- istio builds using go toolchain instead of bazel.

Moved the local k8s apiserver testing env from circleci scripts.

For go build/test, use the local k8s for unit tests - not only avoids poluting the source tree, but it was dangerous to run unit test against a user real k8s config. Envoy is also tested from the output directory instead of source tree.

The circleci scripts were excluding those tests. Doug's script will automate generation - this PR doesn't include the bazel generate and copy since it's going away.

Doug's change for standalone generation should generate all files, this PR doesn't add back the bazel gen.

The GOPATH is mapped on the same location in the container, and the build runs as the current user.

The bazel part (with the user's real cluster linked and used by unit tests) will go away when we stop using bazel.

First use KUBECONFIG, then the kube/platform/config which might be a safe config. Last use the users' .kube/config - with a warning.

codecov · 2017-12-18T03:39:56Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2220      +/-   ##
==========================================
- Coverage   78.24%   78.05%   -0.19%     
==========================================
  Files         205      206       +1     
  Lines       18342    18251      -91     
==========================================
- Hits        14351    14246     -105     
- Misses       3242     3258      +16     
+ Partials      749      747       -2

Flag	Coverage Δ
#broker	`47.27% <ø> (+1.17%)`	⬆️
#mixer	`77.77% <ø> (-0.3%)`	⬇️
#pilot	`79.15% <ø> (-0.06%)`	⬇️
#security	`87.24% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/cmd/pilot-discovery/server/monitoring.go	`64% <0%> (-16%)`	⬇️
mixer/adapter/prometheus/server.go	`92% <0%> (-3.84%)`	⬇️
mixer/adapter/denier/denier.go	`88.88% <0%> (-3.22%)`	⬇️
mixer/adapter/servicecontrol/testhelper.go	`68.75% <0%> (-2.31%)`	⬇️
mixer/adapter/list/list.go	`94.21% <0%> (-1%)`	⬇️
mixer/adapter/list/stringList.go	`100% <0%> (ø)`	⬆️
mixer/adapter/servicecontrol/client.go	`0% <0%> (ø)`	⬆️
mixer/adapter/memquota/keys.go	`100% <0%> (ø)`	⬆️
broker/pkg/version/version.go	`100% <0%> (ø)`
mixer/adapter/stackdriver/metric/bufferedClient.go	`93.54% <0%> (+3.22%)`	⬆️

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 4c05f70...9eece2a. Read the comment docs.

This is only needed for the transition.

Bazel is failing again for me with an obscure message - the build files are written blindly, it appears prow still works.

kyessenov · 2017-12-18T21:23:09Z

bin/testEnvLocalK8S.sh

+export OUT=${TOP}/out
+
+# components used in the test (starting with circleci for consistency, eventually ci will use this)
+export K8S_VER=v1.7.4


This is very old. 1.9 just got released.

Using same version that circleci is using, and I believe it's the min. version of k8s we support.

We can run test envs with 1.9 as well.

kyessenov · 2017-12-18T21:24:02Z

broker/pkg/platform/kube/crd/client_test.go

-	}
-
-	return f
+	return k8s.Kubeconfig("/../../../../../pilot/platform/kube/config")


Where's the file being symlinked?

In the old bazel - I'm not changing anything, I assume scripts will do it.

For go/make: it is no longer used. Once bazel is out I'll also remove this line, and only use
KUBECONFIG and the localhost/standalone apiserver conf for unit tests, and whatever the new
testing framework uses for integration ( most tests can be run in multiple environments ).

kyessenov · 2017-12-18T21:24:18Z

mixer/tools/codegen/pkg/interfacegen/testdata/apa_tmpl.pb.go

@@ -1,11 +1,11 @@
 // Code generated by protoc-gen-gogo. DO NOT EDIT.
-// source: bazel-out/local-fastbuild/genfiles/mixer/tools/codegen/pkg/interfacegen/testdata/apa_tmpl.proto
+// source: bazel-out/k8-fastbuild/genfiles/mixer/tools/codegen/pkg/interfacegen/testdata/apa_tmpl.proto


Unnecessary change?

Automated scripts... Will try to remove it, but once Doug has his PR in I assume all those will
be replaced.

kyessenov · 2017-12-18T21:44:20Z

Looks reasonable. I'd like to keep a check on the bash scripting that will be happening with the Makefile approach. I think what you have here for spinning up local apiserver is OK, but if there's a third party project that can do that (even if it's minikube), I'd prefer that.

Also, I see a lot of go install. Should we be using go build -i instead? That way we don't mess with other projects in the same $GOROOT (which I'm sure many developers have).

costinm · 2017-12-18T23:18:20Z

On the shell script: it's replicating what circleci has been doing for now, but eventually this will be migrated into the new test framework, as one of the environments.

For 'unit test' having the etcd/apiserver is a reasonable option, but the goal is to be able to run
one test in multiple environments - however this PR is only focusing on the short term
migration to go and consistency with the ci.

costinm · 2017-12-18T23:24:20Z

/retest

mandarjog · 2017-12-18T16:41:54Z

bin/testEnvLocalK8S.sh

+        --tls-cert-file ${CERTDIR}/apiserver.crt \
+        --tls-private-key-file ${CERTDIR}/apiserver.key \
+        --service-cluster-ip-range 10.99.0.0/16 \
+        --port 8080 -v 2 --insecure-bind-address 0.0.0.0 \


Define 8080 and ranges as constants before the command?

The port - yes, I'm planning to do this - current code is cut&pasted from circleci, but for running on
local machine it probably needs more customization. However not top priority, after it works we can
fine tune it... (and better yet - this can be moved into the new test framework !)

Need to keep bazel default until everythign else is in.

costinm · 2017-12-19T18:15:05Z

/retest

…rror message

douglas-reid

In general, this looks reasonable to me. Most important thing is to get it all passing, then we can come back and clean up.

douglas-reid · 2017-12-22T22:08:39Z

Makefile

+	go install istio.io/istio/pilot/cmd/sidecar-initializer
+
+.PHONY: mixs
+mixs: vendor


i'm not clear where this belongs, but at some point we need to add a generate target that can call go generate istio.io/istio/... before it does a full build.

douglas-reid · 2017-12-22T22:10:07Z

bin/pre-commit

-    echo "updating autogen files..."
-    bin/regenerate_files.py
+    #echo "updating autogen files..."
+    #bin/regenerate_files.py


as above, we need something to do proper generation somewhere. i don't have strong informed opinions on where, however.

I don't know if people are more likely to install the pre-submit hook (I forget sometimes), or to
run make / test.

But it's clear make will need to have the target - so maybe we can start with having it in the makefile.

douglas-reid · 2017-12-22T22:11:29Z

envsetup.sh

+# Predefined:
+# - minikube: will start a regular minikube in a VM
+#
+function lunch() {


was this written around midday 😜 ?

s/lunch/launch (here and in the comment).

No, 8 years of android ( envsetup and lunch are the first 2 commands android devs type every day).
https://source.android.com/setup/building

douglas-reid · 2017-12-22T22:12:02Z

mixer/adapter/opa/opa_integration_test.go

@@ -177,7 +177,8 @@ spec:
 `
 )

-func TestServer(t *testing.T) {
+//https://github.com/istio/istio/issues/2300
+func xTestServer(t *testing.T) {


there is an outstanding PR to fix this. Should we merge that first and then drop this change?

Either way - if it gets merged before I finish (likely), I'll drop.

costinm · 2017-12-22T23:42:17Z

And finally prow is happy !!!

Still missing (separate PRs):

integrate Doug's generate scripts
deb/rpm builds using .fpm
lots of cleanup and consistency improvements !

douglas-reid · 2017-12-22T23:43:57Z

/lgtm

istio-merge-robot · 2017-12-22T23:45:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [douglas-reid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-12-23T00:15:26Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-23T00:47:54Z

Automatic merge from submit-queue.

costinm added 11 commits December 17, 2017 09:02

Add go build/test targts to makefile.

10989ab

Moved the local k8s apiserver testing env from circleci scripts.

Add generated interfacegen, so mixer tests can pass.

8e8fb66

The circleci scripts were excluding those tests. Doug's script will automate generation - this PR doesn't include the bazel generate and copy since it's going away.

Add missing mixer genfiles.

d7c9288

Doug's change for standalone generation should generate all files, this PR doesn't add back the bazel gen.

Make sure the local k8s is running.

d95b211

Add option to run the build image locally.

d16e00f

The GOPATH is mapped on the same location in the container, and the build runs as the current user.

Merge branch 'master' of https://github.com/istio/istio into gobuild

267e4ee

Remove unused/old files

abb7d3c

Refactor the init method across mixer/pilot to a common one.

7458b8c

The bazel part (with the user's real cluster linked and used by unit tests) will go away when we stop using bazel.

Use the common method in broker as well.

f5d5e29

Fix the return.

0564bcb

istio-testing added the release-note-none label Dec 18, 2017

googlebot added the cla: yes label Dec 18, 2017

Fix kubeconfig loading.

069e134

First use KUBECONFIG, then the kube/platform/config which might be a safe config. Last use the users' .kube/config - with a warning.

costinm and others added 3 commits December 17, 2017 19:44

Update bazel files, manually, to use the refactored method.

2c1e2a0

This is only needed for the transition.

Fix target location.

f4b459f

Bazel is failing again for me with an obscure message - the build files are written blindly, it appears prow still works.

Merge branch 'gobuild' of https://github.com/istio/istio into gobuild

3724e39

mandarjog mentioned this pull request Dec 18, 2017

Implement pure go build + make system #2109

Closed

9 tasks

kyessenov reviewed Dec 18, 2017

View reviewed changes

mandarjog reviewed Dec 19, 2017

View reviewed changes

costinm added 2 commits December 18, 2017 22:18

gofmt the file

60cc59e

Start switching fmt.sh to non-bazel.

9102005

Need to keep bazel default until everythign else is in.

costinm added 3 commits December 19, 2017 12:23

Build test servers, add the new build to circleci

21cd331

Merge branch 'master' of https://github.com/istio/istio into gobuild

b29f225

Merge branch 'gobuild' of https://github.com/istio/istio into gobuild

73ab54a

costinm added 14 commits December 22, 2017 11:18

Explicitly set GOPATH

50b1b16

Set the flags explicitly

1fcaaaa

Add a small sleep to avoid flakiness in node test

3651a4c

Remove debug

630b67f

CI seems to run with -e, replace

ff05496

Found the bug - test was looking for error string, grpc changed the e…

c8bccac

…rror message

Testing on minikube showed that agent crashes if mixer is not found

9c9729d

Fix env for pilot auth

4cb82e0

Remove bazel from pilot e2e

4ea5e03

Comment (and disable) e2e test

0a046d9

Forgot ca/node_agent

1d2c5f8

Adjustments to the integration

80c81f3

Still wrong env

4c0bf41

Fix GOPATH and pwd (TODO: merge with the new make)

dbfb32e

douglas-reid reviewed Dec 22, 2017

View reviewed changes

costinm added 5 commits December 22, 2017 14:17

Another iteration, pilot integration base dir

9552b94

Remove push debian - will be fixed later

b84aaed

One iteration on lint, use real VM (mount didn't work)

9e2ee85

Lint iteration - ci root dir in vm

9f8ce3b

Not sure why go build is writing to goroot

e8eb2aa

istio-testing assigned douglas-reid Dec 22, 2017

istio-testing added the lgtm label Dec 22, 2017

istio-merge-robot added the approved label Dec 22, 2017

istio-merge-robot merged commit 2786139 into master Dec 23, 2017

chxchx mentioned this pull request Jan 9, 2018

Activate VM expansion test #2444

Merged

makefile, test and scripts updates to use go toolchain instead of bazel. #2220

makefile, test and scripts updates to use go toolchain instead of bazel. #2220

Uh oh!

Conversation

costinm commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Dec 18, 2017

Uh oh!

costinm commented Dec 18, 2017

Uh oh!

costinm commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm commented Dec 19, 2017

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm commented Dec 22, 2017

Uh oh!

douglas-reid commented Dec 22, 2017

Uh oh!

istio-merge-robot commented Dec 22, 2017

Uh oh!

istio-merge-robot commented Dec 23, 2017

Uh oh!

istio-merge-robot commented Dec 23, 2017

Uh oh!

Uh oh!

costinm commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading