WIP: implement initial set of MCP enhancements by ayj · Pull Request #10635 · istio/istio

ayj · 2018-12-22T01:57:16Z

This introduces the initial set of MCP enhancements as described by the MCP enhancements design proposal. The existing client/server implementation is retained, though several internal interfaces have been refactored so both implementations can coexist during migration.

istio-testing · 2018-12-22T01:57:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayj
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rshriram

If they are not already assigned, you can assign the PR to them by writing /assign @rshriram in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ayj · 2018-12-22T01:58:42Z

cc @Nino-K @jeffmendoza @ozevren

This PR includes istio/api#741 and #10628. The relevant commit to this PR is ae865f3

golangcibot · 2018-12-22T01:59:08Z

pkg/mcp/source/source.go

nackLimitFreq is unused (from deadcode)

golangcibot · 2018-12-22T01:59:08Z

pkg/mcp/sink/sink_test.go

recvError is unused (from deadcode)

golangcibot · 2018-12-22T01:59:09Z

pkg/mcp/sink/server.go

connections is unused (from structcheck)

golangcibot · 2018-12-22T01:59:09Z

pkg/mcp/snapshot/snapshot.go

typeURL is unused (from structcheck)

golangcibot · 2018-12-22T01:59:09Z

pkg/mcp/source/server.go

connections is unused (from structcheck)

golangcibot · 2018-12-22T01:59:11Z

pkg/mcp/sink/sink_test.go

error var updateError should have name of the form errFoo (from golint)

golangcibot · 2018-12-22T01:59:11Z

pkg/mcp/source/source.go

type name will be used as source.SourceStream by other packages, and that stutters; consider calling this Stream (from golint)

golangcibot · 2018-12-22T01:59:11Z

pkg/mcp/source/source_test.go

error var sendError should have name of the form errFoo (from golint)

golangcibot · 2018-12-22T01:59:11Z

pkg/mcp/sink/journal_test.go

File is not goimports-ed (from goimports)

golangcibot · 2018-12-22T01:59:11Z

pkg/mcp/source/source_test.go

makeWatchResponse - typeURL always receives test.FakeType0TypeURL ("type.googleapis.com/istio.io.galley.pkg.mcp.server.FakeType0") (from unparam)

golangcibot · 2019-01-03T22:33:03Z

pkg/mcp/sink/sink_test.go

errRecv is unused (from varcheck)

golangcibot · 2019-01-03T22:33:03Z

pkg/mcp/source/server_source.go

connections is unused (from structcheck)

golangcibot · 2019-01-03T22:33:03Z

pkg/mcp/source/client_source.go

File is not goimports-ed (from goimports)

geeknoid

I'm surprised to see these abstractions with mutual exclusion logic. Are these really going to be used by multiple concurrent callers? When?

pkg/mcp/internal/env.go

geeknoid · 2019-01-04T00:42:36Z

pkg/mcp/sink/journal.go

Does this object init need to happen under lock or could it be moved out? Putting it under lock ensures that all the RecentRequestInfo instance are inserted in time order, but is that needed? At least, the call to proto.Clone could be done outside the lock.

Good catch. I moved it outside the lock. We can optimize this further and remove the proto clone if we treat the request proto as immutable. One less memory allocation in the client/sink path.

geeknoid · 2019-01-04T00:44:19Z

pkg/mcp/sink/journal.go

Is this really what you want here? I think this will end up allocating an ever-increasing array under the covers.

This was copy/pasted from the original MCP journal code. I think the same concern was raised during the review of that code, but @ozevren said the golang runtime would reclaim the memory. In any case, I rewrote to use a fixed slice without additional memory allocations.

geeknoid · 2019-01-04T00:45:09Z

pkg/mcp/sink/server_sink.go

failus -> failures
logs as a -> logged in a

geeknoid · 2019-01-04T00:48:44Z

pkg/mcp/sink/server_sink.go

I think it's bad form to have an otherwise reentrant library pull in state from the ambient environment. I would prefer if these values came in as arguments when initializing this package, and let the wrapping command read the environment and supply the values.

That is, environment variables should handled like command-line arguments and read by the top-level command code.

Fair enough. I've moved the envvar handling out of the common mcp packages and into the galley/configmap loading code. I've also added Option structs to the source/sink packages where callers can optionally set these sorts of overrides in addition to the other required parameters.

geeknoid · 2019-01-04T00:51:01Z

pkg/mcp/sink/server_sink.go

If ServerSink a thread-safe abstraction? I wasn't expecting it to be...

It's minimally thread-safe. The majority of state is per-connection/stream. The extra atomic check here prevents the case of multiple sources connecting, which isn't supported (yet). Eventually, Galley may status from multiple Pilot instances, in which case a single sink server may have multiple source connections.

geeknoid · 2019-01-04T00:52:49Z

pkg/mcp/sink/sink.go

Stream is for sending...

geeknoid · 2019-01-04T00:53:42Z

pkg/mcp/sink/sink.go

When is a sink used in a multithread environment?

The lock here protects the per-type resource version state. This state is used in the request path for fast-resume on re-connection. The caller can invoke ResetResourceVersions() to reset this version information and force a full state delivery. This function is invoked outside of the normal request path and must therefore protect access to the state with a lock.

Given this is a utility package that several components will use, it'd worth stating the multithreading expectations/support explicitly in a comment.

golangcibot · 2019-01-05T01:39:37Z

pkg/mcp/source/source_test.go

h can be Watcher (from interfacer)

golangcibot · 2019-01-09T21:30:18Z

galley/tools/gen-meta/main.go

struct field tag json:- not compatible with reflect.StructTag.Get: bad syntax for struct tag value (from govet)

golangcibot · 2019-01-09T21:30:18Z

galley/tools/gen-meta/main.go

struct field tag json:- not compatible with reflect.StructTag.Get: bad syntax for struct tag value (from govet)

golangcibot · 2019-01-09T21:30:19Z

pilot/pkg/config/coredatamodel/controller.go

ineffectual assignment to byNamespace (from ineffassign)

golangcibot · 2019-01-09T21:30:19Z

pkg/mcp/snapshot/snapshot_test.go

fakeResource2 is unused (from varcheck)

golangcibot · 2019-01-09T21:30:19Z

pkg/mcp/snapshot/snapshot_test.go

fakeResource1 is unused (from varcheck)

golangcibot · 2019-01-09T21:30:19Z

pkg/mcp/snapshot/snapshot_test.go

fakeResource0 is unused (from varcheck)

golangcibot · 2019-01-09T21:30:19Z

pkg/mcp/snapshot/inmemory_test.go

mustMarshalAny is unused (from deadcode)

golangcibot · 2019-01-09T21:30:20Z

pilot/pkg/config/coredatamodel/controller.go

addedTracking is unused (from deadcode)

golangcibot · 2019-01-09T21:30:20Z

pilot/pkg/config/coredatamodel/controller.go

obj is unused (from structcheck)

golangcibot · 2019-01-09T21:30:20Z

pilot/pkg/config/coredatamodel/controller.go

updated is unused (from structcheck)

Nino-K · 2019-01-10T18:54:07Z

pilot/pkg/config/coredatamodel/controller.go

@ayj Should I go ahead and close this #10446 since it's covered here?

I think so. This PR still has some TODO's around optimizing ServiceEntry updates and invoking ConfigUpdate() directly that we'll still need to address in a follow-up PR

Nino-K · 2019-01-10T18:59:56Z

pkg/mcp/configz/configz_test.go

is this still needed?

Nino-K · 2019-01-10T19:01:58Z

pkg/mcp/sink/sink.go

It would be nice to call this New so the caller can do sink.New as oppose to sink.NewSink, but very minor.

Nino-K · 2019-01-10T19:20:59Z

pkg/mcp/sink/client_sink.go

retryDelay?

Nino-K · 2019-01-10T19:31:00Z

pkg/mcp/sink/sink.go

Just curious why is this needed? if no update received for a resource within a time frame it is destined to be removed.

golangcibot · 2019-01-11T03:09:33Z

tests/e2e/tests/pilot/cloudfoundry/copilot_test.go

ineffectual assignment to err (from ineffassign)

…-mcpv2-rebased

golangcibot · 2019-01-12T02:04:37Z

mixer/pkg/config/mcp/backend.go

-		// We can do this, because there is no error that can be raised from now on.
-		b.state.items[kind] = newTypeState
+	for key, val := range added {
+		collection, found := b.state.items[key.Kind]


ineffectual assignment to collection (from ineffassign)

istio-testing · 2019-01-15T01:26:25Z

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

Test name	Commit	Details	Rerun command
prow/istio-integ-local-tests.sh	`d8aac25`	link	`/test istio-integ-local-tests`
prow/istio-integ-k8s-tests.sh	`d8aac25`	link	`/test istio-integ-k8s-tests`

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.

istio-testing · 2019-01-15T23:58:44Z

@ayj: PR needs rebase.

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.

andraxylia · 2019-01-26T01:02:54Z

galley/pkg/runtime/resource/schema.go

-func (b *SchemaBuilder) Register(typeURL string) Info {
-	if _, found := b.schema.byURL[typeURL]; found {
-		panic(fmt.Sprintf("schema.Register: Proto type is registered multiple times: %q", typeURL))
+func (b *SchemaBuilder) Register(rawCollection, typeURL string) Info {


I thought we replace typeURL with collection, why do we keep them both?

A collection has a corresponding schema (e.g. typeURL) defined by the proto. This mapping is defined in galley/tools/gen-meta/metadata.yaml and is registered at runtime here. The typeURL is used elsewhere to allocate new protobuf instances of the correct type.

ayj requested a review from ozevren December 22, 2018 01:57

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Dec 22, 2018

istio-testing requested review from frankbu and kyessenov December 22, 2018 01:57

googlebot added the cla: yes label Dec 22, 2018

ayj removed request for frankbu and kyessenov December 22, 2018 01:57

golangcibot reviewed Dec 22, 2018

View reviewed changes

ayj force-pushed the release-1.1-mcpv2-rebased branch from ae865f3 to 6e619bf Compare December 22, 2018 02:03

golangcibot reviewed Jan 3, 2019

View reviewed changes

ayj requested a review from geeknoid January 3, 2019 23:07

geeknoid reviewed Jan 4, 2019

View reviewed changes

golangcibot reviewed Jan 5, 2019

View reviewed changes

pkg/mcp/source/source_test.go Outdated

Copy link
Copy Markdown

Contributor

golangcibot Jan 5, 2019

Choose a reason for hiding this comment

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

h can be Watcher (from interfacer)

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 9, 2019

golangcibot reviewed Jan 9, 2019

View reviewed changes

This was referenced Jan 10, 2019

Galley: Add support for outgoing connections. #10785

Closed

Pilot support for incremental MCP and ServiceEntries #10497

Closed

Nino-K reviewed Jan 10, 2019

View reviewed changes

pkg/mcp/configz/configz_test.go Outdated

Copy link
Copy Markdown

Member

Nino-K Jan 10, 2019

Choose a reason for hiding this comment

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

is this still needed?

Nino-K reviewed Jan 10, 2019

View reviewed changes

pkg/mcp/sink/client_sink.go Outdated

Copy link
Copy Markdown

Member

Nino-K Jan 10, 2019

Choose a reason for hiding this comment

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

retryDelay?

Nino-K reviewed Jan 10, 2019

View reviewed changes

Nino-K mentioned this pull request Jan 10, 2019

Only compute diff for ServiceEntry #10446

Merged

ayj force-pushed the release-1.1-mcpv2-rebased branch from 048179d to 19063a5 Compare January 10, 2019 23:50

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 10, 2019

ayj force-pushed the release-1.1-mcpv2-rebased branch from 19063a5 to 81497ac Compare January 10, 2019 23:52

mcp v2 changes

c43172b

ayj force-pushed the release-1.1-mcpv2-rebased branch from 81497ac to c43172b Compare January 11, 2019 00:00

fix linter errors

cf9d3c8

golangcibot reviewed Jan 11, 2019

View reviewed changes

tests/e2e/tests/pilot/cloudfoundry/copilot_test.go Outdated

Copy link
Copy Markdown

Contributor

golangcibot Jan 11, 2019

Choose a reason for hiding this comment

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

ineffectual assignment to err (from ineffassign)

ayj added 3 commits January 11, 2019 08:54

fix copilot tests

d4e865e

add missing mixer types

55b7517

Merge branch 'release-1.1' of github.com:istio/istio into release-1.1…

b54ae47

…-mcpv2-rebased

ayj force-pushed the release-1.1-mcpv2-rebased branch from 4b99dbb to b54ae47 Compare January 11, 2019 18:12

ayj added 4 commits January 11, 2019 11:20

don't clear pending response for stale ack/nacks

bb5773a

fix unit test flakes

f018e27

increase code coverage

e1e7e46

remove meshpolicy hack in pilot mcp client

2c857d5

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 11, 2019

100% coverage in resource_test.go

3176601

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 12, 2019

Merge branch 'release-1.1' of github.com:istio/istio into release-1.1…

c40901c

…-mcpv2-rebased

ayj force-pushed the release-1.1-mcpv2-rebased branch from 3933efa to c40901c Compare January 12, 2019 00:51

ayj added 2 commits January 11, 2019 17:02

fix cloudfoundry copilot test

226ef99

fix mixer backend mcp integration

c9e1d71

golangcibot reviewed Jan 12, 2019

View reviewed changes

ayj added 3 commits January 11, 2019 18:34

fix linter and mcp_test.go

95746a5

use types.MarshalAny in mcp_test.go

1011d2b

update galley testdata for new test framework

d8aac25

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 15, 2019

andraxylia reviewed Jan 26, 2019

View reviewed changes

ayj closed this Jan 26, 2019

ayj deleted the release-1.1-mcpv2-rebased branch January 26, 2019 01:07

Conversation

ayj commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Dec 22, 2018

Uh oh!

ayj commented Dec 22, 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!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

ayj commented Dec 22, 2018 •

edited

Loading

Nino-K Jan 10, 2019 •

edited

Loading