EDS tests and refactoring by costinm · Pull Request #3949 · istio/istio

costinm · 2018-03-05T17:57:14Z

added a unit/integration local test checking the grpc streaming directly
refactored the testing code to allow better starting pilot and envoy for the local tests
refactored the grpc server initialization, now part of pilot/pkg/bootstrap
moved edsv2 code in the v2 directory.
fixed the streaming of endpoints

This PR uses EDSv2 by default, depending on testing results we may chose a different
value.

istio-merge-robot · 2018-03-05T17:57:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: kyessenov

Assign the PR to them by writing /assign @kyessenov in a comment when ready.

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

Details

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

OWNERS

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

costinm · 2018-03-05T18:00:34Z

Note that while testing this I found that EDSv1 was not working well with the envoy in front of pilot - by default the TCP cluster has max 1024 connections, and the sidecars make one TCP connection per cluster, resulting in cases where EDS1 was failing ( I test on a cluster with ~500 services, to make sure we don't break the memory fixes ).

So temporarily I've moved back the 'insecure' pilot to 8080 - and will try to have better settings for the
grpc port, which doesn't yet have certificates.

xiaolanz · 2018-03-05T19:57:51Z

Makefile


 istio.yaml:
 	helm template --set global.tag=${TAG} \
+		  --namespace=istio-system \


mis-alignment with 1 blank space.

xiaolanz · 2018-03-05T19:58:32Z

pilot/pkg/bootstrap/server.go

 	"time"

+	"google.golang.org/grpc"
+


imports follow one style, run/fmt.sh

Well - run/fmt.sh seems to be broken, doesn't match linter...

xiaolanz · 2018-03-05T19:58:58Z

pilot/pkg/bootstrap/server.go


 // AdmissionArgs provides configuration options for the admission controller. This is a partial duplicate of
-// admit.ControllerOptions (other fields are filled out before constructing the admission controller). Only
+// admito.ControllerOptions (other fields are filled out before constructing the admission controller). Only


typo admit.

xiaolanz · 2018-03-05T20:01:10Z

pilot/pkg/bootstrap/server.go

 type Server struct {
 	mesh              *meshconfig.MeshConfig
-	serviceController *aggregate.Controller
+	ServiceController *aggregate.Controller


Justify for making these public.

doc comments are needed for every public variables.

Are used from other packages - for some reasons all exposed fields in this class don't have
doc comments. Since it's not an API we would expose outside pilot, and most of this is
scheduled for deletion/deprecation - I don't think it's a huge priority.

There are no exposed fields on the base version. I'm surprised we don't have a linter forcing some comments on public fields.
But if this is really soon to be deleted code... ok (though why do we need to add and expose new soon to be deleted code?)

Having a struct exposing both private and public fields is an antipattern. Please add TODO to remove this code later.

bootstrap/server.go is full of public fields - MeshArgs, ConsuleArgs, etc.
We are planning to replace the v1 registries with new data structures and event-based code, and almost all the http-based envoy discovery will also go away once v2 is done ( target is 0.8)

xiaolanz · 2018-03-05T20:04:00Z

pilot/pkg/bootstrap/server.go

+	GRPCListeningAddr net.Addr
 	clusterStore      *clusterregistry.ClusterStore
+
+	EnvoyXdsServer   *envoyv2.DiscoveryServer


package private variable should declare in lowercase: such as grpcServer.

It's intended to be public, used in the other packages.

… pointers in creating the lb

rshriram · 2018-03-05T23:51:32Z

Can you file a bug for the Envoy in front of pilot thing ? Assign to Krishna since he implemented the istio on istio. IMO we should do tls directly from pilot instead of adding yet another Envoy.

costinm · 2018-03-06T00:10:17Z

Yes, I generally agree on doing mtls in pilot in future. There are many problems around this...

For envoy fixes - I'll probably do this work in a separate PR for the grpc port and switching to v2 bootstrap.

costinm · 2018-03-06T04:44:11Z

PTAL - tests are passing ! There are few more improvements to make before deciding if we leave it
enabled by default, one of the main reasons to move ahead is that it reduces the load on pilot, this
has been a big problem with v1.

ldemailly · 2018-03-06T06:12:39Z

pilot/pkg/bootstrap/server.go

 type Server struct {
 	mesh              *meshconfig.MeshConfig
-	serviceController *aggregate.Controller
+	ServiceController *aggregate.Controller


There are no exposed fields on the base version. I'm surprised we don't have a linter forcing some comments on public fields.
But if this is really soon to be deleted code... ok (though why do we need to add and expose new soon to be deleted code?)

ldemailly · 2018-03-06T06:16:49Z

pilot/pkg/bootstrap/server.go

+	addr := s.HTTPServer.Addr
+	listener, err := net.Listen("tcp", addr)
+	if err != nil {
+		return err


I couldn't find in the call chain where those errors get logged.
It would probably help troubleshooting to log something near/where the error occured?

Code moved from discovery without changes :-)

(DiscoveryService.Start)

Agree with the comment - but additional improvements can be made in separate PRs, not trying to fix all the problems in one.

istio-testing · 2018-03-06T19:08:28Z

@costinm: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-simpleTests.sh	`419f530`	link	`/test e2e-simple`
prow/e2e-bookInfoTests.sh	`419f530`	link	`/test e2e-bookInfo`
prow/istio-pilot-e2e.sh	`419f530`	link	`/test istio-pilot-e2e`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sebastienvas · 2018-03-07T21:11:03Z

this basically prevent the code package check. There is a codecov.skip where you can put the thing that are do not want to be covered instead of disabling the check for all packages.

EDS tests and refactoring

9201d2a

costinm requested a review from ldemailly as a code owner March 5, 2018 17:57

costinm requested review from a team March 5, 2018 17:57

googlebot added the cla: yes label Mar 5, 2018

costinm requested review from ZackButcher, gnirodi, nmittler and rshriram March 5, 2018 17:57

costinm added 5 commits March 5, 2018 10:02

Add missing copyright/licence

a57692c

Lint fixes

7bad223

More lint

57e3d8d

Use 8080 until envoy config is fixed

c9ca4fb

More lint

a942c3e

xiaolanz suggested changes Mar 5, 2018

View reviewed changes

costinm added 6 commits March 5, 2018 12:10

Add port for grpc

632ae3f

Add port for pilot e2e

6cc6596

make fmt

53f91de

Lint

7bd1079

Merge branch 'master' of github.com:istio/istio into costin-eds

6672394

Few more debug statements, but I think the bug was in using values vs…

0614599

… pointers in creating the lb

More logging around resource list update, fix bug

6925562

costinm added 4 commits March 5, 2018 16:12

Format and checkstyle fix attempt

bcd4942

Format and checkstyle fix attempt

72b4257

Cleanup TestEnvoyRun, was testing v1 bootstrap no longer used

ac11864

Blind attempt to fix codecov. The file is tested indirectly

5895f09

ldemailly reviewed Mar 6, 2018

View reviewed changes

costinm added 2 commits March 6, 2018 09:51

Skip coverage

7672b0a

Skip coverage - add rationale

b2110c7

costinm requested a review from a team March 6, 2018 17:56

Fix the skip code

419f530

xiaolanz approved these changes Mar 6, 2018

View reviewed changes

costinm merged commit 80e65b5 into master Mar 6, 2018

costinm deleted the costin-eds branch March 7, 2018 23:48

sakshigoel12 added the kind/fixit label Mar 13, 2018

Conversation

costinm commented Mar 5, 2018

Uh oh!

istio-merge-robot commented Mar 5, 2018

Uh oh!

costinm commented Mar 5, 2018

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!

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!

rshriram commented Mar 5, 2018

Uh oh!

costinm commented Mar 6, 2018

Uh oh!

costinm commented Mar 6, 2018

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!

istio-testing commented Mar 6, 2018

Uh oh!

sebastienvas commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants