hubble: implement peer service, enable it locally #10969

rolinh · 2020-04-14T13:55:26Z

Please, see individual commits for details.

coveralls · 2020-04-14T15:14:25Z

Coverage increased (+0.1%) to 44.74% when pulling 0c5a9e9 on pr/rolinh/hubble-peer into 531e61f on master.

michi-covalent

the change looks good, but it wasn't clear to me how i can test this in my cluster. i guess nobody is actually calling rpc Notify(NotifyRequest) yet?

pkg/hubble/peer/service.go

pkg/hubble/peer/peer.go

rolinh · 2020-04-15T06:09:15Z

the change looks good, but it wasn't clear to me how i can test this in my cluster.

You can use a gRPC client. For example, with grpcurl, you can invoke the service like this from the root of the git repository:

$ grpcurl -import-path api/v1/peer -proto peer.proto -unix -plaintext /var/run/cilium/hubble.sock peer.Peer.Notify
{
  "name": "runtime1",
  "address": "10.0.2.15",
  "type": "PEER_ADDED"
}

Note that this call "blocks". So one way to test this is to invoke this command then add/remove nodes from your cluster. Of course, the cilium-agent needs the --enable-hubble flag.

i guess nobody is actually calling rpc Notify(NotifyRequest) yet?

Not yet. I have an in-development version of ~~hubble-proxy~~ hubble-relay that is using this service but this will be for a follow-up PR once this one is merged.

pkg/hubble/peer/peer.go

pkg/hubble/peer/service.go

aanm · 2020-04-15T07:40:02Z

pkg/hubble/peer/service.go

+// interface so that it can be mocked for the purpose of testing.
+type Service struct {
+	stop chan (struct{})
+	mgr  *manager.Manager


Nit, consider using an interface instead of implementation.

Yup, this is required for testing, like mentioned in the struct comment just above. If it's fine with you, I'll add an interface in pkg/node/manager/manager.go.

The interface could be defined here, not in pkg/node/manager/manager.go, this allows you to not need to import pkg/node/manager in this package.

I've added the interface with this commit faae24149afbb9da49688945123c09a79716f903 already and implemented tests of the service.
I think that importing the interface from pkg/node/manager is not so bad because it's otherwise really hard to understand who implements it if you're not familiar enough with cilium's codebase.

@rolinh there are tools built for that purpose [0] so there's no point in keeping the interfaces in the same place as 1 of the implementations. What happens if in the future we will have a different node manager that implements the same interface? Where will the interface live?

[0] https://docs.google.com/document/d/1_Y9xCEMj5S-7rv2ooHpZNH15JgRT5iM742gJkw5LtmQ/edit#heading=h.f6hyappclfor

I don't mind moving the interface to pkg/hubble/peer if you have strong feelings about it; I don't think it matters much.

The interface could be defined here, not in pkg/node/manager/manager.go, this allows you to not need to import pkg/node/manager in this package.

BTW, by moving the interface to pkg/hubble/peer I could save on importing pkg/node/manager but then I'll have to import pkg/datapath as it's required for the interface definition. I'm not "saving" on import anyway.

gandro

Awesome! I do see an issue with blocking the Node Manager which I think must be addressed, but all my other comments are just comments and don't require any action.

pkg/hubble/server/serveroption/option.go

pkg/hubble/defaults/defaults.go

gandro · 2020-04-15T08:46:47Z

pkg/hubble/peer/service.go

+			case <-s.stop:
+				return nil
+			case cn := <-h.C:
+				if err := stream.Send(cn); err != nil {


stream.Send() can block (it does I/O). Which means the unbuffered h.C channel causes the NodeManager to be blocked as well in h.Node{Add,Update,Delete}.

Why isn't a context.Context part of the Send parameters?

Because the ServerStream interface doesn't use a context.Context. SendMsg() is used by Send(), as can be seen in the generated code in peer.pb.go:

type Peer_NotifyServer interface { Send(*ChangeNotification) error grpc.ServerStream } type peerNotifyServer struct { grpc.ServerStream } func (x *peerNotifyServer) Send(m *ChangeNotification) error { return x.ServerStream.SendMsg(m) }

The interface defines a context but it's global to ServerStream.

@gandro I have added a test that simulates a blocked send (TestService_NotifyWithBlockedSend) and it failed (which is good as it tests what we want).

To fix the problem and have the test pass, I've added small buffer (of size 10 by default but this is configurable through service options) that, when full, triggers the server to stop steaming to the client.
This eventually pushes the problem back to the client which will have to call Notify() again in such a case. I don't think it's a problem overall because upon calling Notify(), the client receives the list of all peers before receiving continuous updates so maintaining a local cache client-side is easy.

Please, have a look again at the Notify() implementation and tell me if you think this way of handling the problem is OK.

Awesome, yes, the code itself looks good! 🎉 We have to get a bit of experience with this, because I honestly have no clue how big the buffer size should be.

We have to get a bit of experience with this, because I honestly have no clue how big the buffer size should be.

Neither do I. I assumed that since the connection is over a local unix domain socket, a buffer of 10 should be sufficient. I also don't want to make it unnecessarily large because if the caller can't keep up then there's probably a problem elsewhere.

pkg/hubble/peer/service.go

michi-covalent · 2020-04-15T21:51:34Z

@rolinh ok it passed my rigorous testing of scaling nodepool size from 1 to 3 and then scaling it back down to 1:

# ./grpcurl -import-path api/v1/peer -proto peer.proto -unix -plaintext /var/run/cilium/hubble.sock peer.Peer.Notify
{
  "name": "gke-michi-default-pool-87b89101-ljbs",
  "address": "10.138.0.53",
  "type": "PEER_ADDED"
}
{
  "name": "gke-michi-default-pool-87b89101-4r14",
  "address": "10.138.0.55",
  "type": "PEER_ADDED"
}
{
  "name": "gke-michi-default-pool-87b89101-lwrc",
  "address": "10.138.0.56",
  "type": "PEER_ADDED"
}
{
  "name": "gke-michi-default-pool-87b89101-4r14",
  "address": "10.138.0.55",
  "type": "PEER_DELETED"
}
{
  "name": "gke-michi-default-pool-87b89101-lwrc",
  "address": "10.138.0.56",
  "type": "PEER_DELETED"
}

aanm · 2020-04-16T08:37:54Z

pkg/hubble/peer/service.go

+// interface so that it can be mocked for the purpose of testing.
+type Service struct {
+	stop chan (struct{})
+	mgr  *manager.Manager


The interface could be defined here, not in pkg/node/manager/manager.go, this allows you to not need to import pkg/node/manager in this package.

pkg/hubble/peer/handler.go

aanm · 2020-04-16T08:43:49Z

pkg/hubble/peer/service.go

+			case <-s.stop:
+				return nil
+			case cn := <-h.C:
+				if err := stream.Send(cn); err != nil {


Why isn't a context.Context part of the Send parameters?

rolinh · 2020-04-18T04:48:52Z

test-me-please

EDIT: test flake

rolinh · 2020-04-20T08:47:50Z

restart-ginkgo

EDIT: hit flake again

gandro · 2020-04-20T08:38:15Z

pkg/hubble/peer/service.go

+			case <-s.stop:
+				return nil
+			case cn := <-h.C:
+				if err := stream.Send(cn); err != nil {


Awesome, yes, the code itself looks good! 🎉 We have to get a bit of experience with this, because I honestly have no clue how big the buffer size should be.

rolinh · 2020-04-21T09:07:58Z

restart-ginkgo

EDIT: yet another flake :(
EDIT2: just rebased against latest master

rolinh · 2020-04-21T11:56:30Z

test-me-please

EDIT: update to adapt to changes from e99049f

rolinh · 2020-04-21T12:21:42Z

test-me-please

EDIT: rebased against master again to fix conflict introduced by f332fb5

rolinh · 2020-04-21T12:50:47Z

test-me-please

nebril · 2020-04-21T13:33:49Z

test-gke

nebril · 2020-04-21T16:23:22Z

test-gke

rolinh · 2020-04-22T06:47:06Z

Rebased against master to fix conflict with fef319a.

rolinh · 2020-04-22T06:47:19Z

test-me-please

nebril · 2020-04-22T10:41:53Z

test-gke

rolinh · 2020-04-22T12:48:36Z

Found a bug in the peer service implementation where the wrong context.Background() was used instead of the existing ctx.

rolinh · 2020-04-22T12:48:49Z

test-me-please

This commits removes the global variable DefaultOptions which contained no defaults but was just initializing the listeners map. The map initialization is done in the constructor instead in order to avoid problems with shallow copy of default options (a 2nd call to NewServer would use modified defaults). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

Make sure to only require the grpc interface definition for service options. The server is a simple grpc server which serves 1 or more services and it doesn't need to know about the internal of each service implementation. This change makes this explicit. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

This is useful for any package that needs to implement gRPC tests, thus move it to a more generic location. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

This interface will make it easier to implement tests, notably for the peer service that uses the node manager. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

This commit adds a new peer module for hubble that implements the peer gRPC server interface. This new gRPC service notifies of nodes being added, deleted or modified. This information is obtained by subscribing to the cilium node manager via a new datapath.NodeHandler interface implementation (peer.handler). This new service will be consumed by hubble proxy in order to connect to hubble peers. It is intended to only be served over a local unix domain socket. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

This commits enables the peer service for the hubble gRPC server that listens on the local unix domain socket. This will allow node discovery for the peer proxy. For all other hubble listen addresses, the peer service is not enabled. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

rolinh · 2020-04-22T13:44:32Z

rebased against master, fixed test failure uncovered by the context fix.

rolinh · 2020-04-22T14:11:06Z

test-me-please

nebril · 2020-04-22T14:51:39Z

test-gke

nebril · 2020-04-22T14:54:57Z

test-gke

Edit: looks like it timed out: https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/546/execution/node/112/log/

christarazi

LGTM!

christarazi · 2020-04-22T21:56:44Z

test-gke

rolinh · 2020-04-23T08:29:49Z

The GKE test was running for 10+ hours and stuck at this step (CI issue):

[Pipeline] Start of Pipeline
[Pipeline] node
Still waiting to schedule task
‘jenkins-node-gke-fixed0’ is offline

rolinh · 2020-04-23T09:11:55Z

test-gke

rolinh added kind/feature This introduces new functionality. release-note/major This PR introduces major new functionality to Cilium. sig/hubble Impacts hubble server or relay labels Apr 14, 2020

rolinh requested a review from a team April 14, 2020 13:55

rolinh requested a review from a team as a code owner April 14, 2020 13:55

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 14, 2020

michi-covalent reviewed Apr 14, 2020

View reviewed changes

pkg/hubble/peer/service.go Outdated Show resolved Hide resolved

pkg/hubble/peer/peer.go Show resolved Hide resolved

aanm reviewed Apr 15, 2020

View reviewed changes

rolinh force-pushed the pr/rolinh/hubble-peer branch from c5b9ce6 to 2650dd5 Compare April 15, 2020 08:10

gandro requested changes Apr 15, 2020

View reviewed changes

aanm reviewed Apr 16, 2020

View reviewed changes

rolinh force-pushed the pr/rolinh/hubble-peer branch from 2650dd5 to c586a3a Compare April 16, 2020 09:17

rolinh requested a review from a team as a code owner April 16, 2020 09:17

rolinh force-pushed the pr/rolinh/hubble-peer branch 2 times, most recently from c301be8 to 01611db Compare April 17, 2020 13:03

rolinh requested a review from gandro April 17, 2020 13:20

gandro approved these changes Apr 20, 2020

View reviewed changes

rolinh force-pushed the pr/rolinh/hubble-peer branch from 01611db to bfcc725 Compare April 21, 2020 11:56

rolinh force-pushed the pr/rolinh/hubble-peer branch from bfcc725 to c0137af Compare April 21, 2020 12:20

rolinh force-pushed the pr/rolinh/hubble-peer branch from c0137af to 85fd93d Compare April 21, 2020 12:49

rolinh force-pushed the pr/rolinh/hubble-peer branch from 85fd93d to c7cd0ad Compare April 22, 2020 06:46

rolinh force-pushed the pr/rolinh/hubble-peer branch from c7cd0ad to dfc21b9 Compare April 22, 2020 12:47

rolinh added 6 commits April 22, 2020 15:39

hubble: move FakeGRPCServerStream to testutils package

abbbf8e

This is useful for any package that needs to implement gRPC tests, thus move it to a more generic location. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

node/manager: add Notifier interface

bf31c7f

This interface will make it easier to implement tests, notably for the peer service that uses the node manager. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

rolinh force-pushed the pr/rolinh/hubble-peer branch from dfc21b9 to 0c5a9e9 Compare April 22, 2020 13:43

michi-covalent approved these changes Apr 22, 2020

View reviewed changes

christarazi approved these changes Apr 22, 2020

View reviewed changes

nebril merged commit 2d0ae5b into master Apr 23, 2020

1.8.0 automation moved this from In progress to Merged Apr 23, 2020

nebril deleted the pr/rolinh/hubble-peer branch April 23, 2020 12:07

tgraf mentioned this pull request May 6, 2020

hubble-relay: multi-node support #11361

Closed

7 tasks

hubble: implement peer service, enable it locally #10969

hubble: implement peer service, enable it locally #10969

Conversation

rolinh commented Apr 14, 2020

coveralls commented Apr 14, 2020 • edited

michi-covalent left a comment

Choose a reason for hiding this comment

rolinh commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gandro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolinh Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michi-covalent commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolinh commented Apr 18, 2020 • edited

rolinh commented Apr 20, 2020 • edited

Choose a reason for hiding this comment

rolinh commented Apr 21, 2020 • edited

rolinh commented Apr 21, 2020 • edited

rolinh commented Apr 21, 2020 • edited

rolinh commented Apr 21, 2020

nebril commented Apr 21, 2020

nebril commented Apr 21, 2020

rolinh commented Apr 22, 2020

rolinh commented Apr 22, 2020

nebril commented Apr 22, 2020

rolinh commented Apr 22, 2020

rolinh commented Apr 22, 2020

rolinh commented Apr 22, 2020

rolinh commented Apr 22, 2020

nebril commented Apr 22, 2020

nebril commented Apr 22, 2020 • edited by christarazi

christarazi left a comment

Choose a reason for hiding this comment

christarazi commented Apr 22, 2020

rolinh commented Apr 23, 2020 • edited

rolinh commented Apr 23, 2020

coveralls commented Apr 14, 2020 •

edited

rolinh Apr 16, 2020 •

edited

rolinh commented Apr 18, 2020 •

edited

rolinh commented Apr 20, 2020 •

edited

rolinh commented Apr 21, 2020 •

edited

rolinh commented Apr 21, 2020 •

edited

rolinh commented Apr 21, 2020 •

edited

nebril commented Apr 22, 2020 •

edited by christarazi

rolinh commented Apr 23, 2020 •

edited