Add NATS Streaming provisioner · Pull Request #624 · knative/eventing

ghost · 2018-11-16T10:24:47Z

Proposed Changes

Add NATS Streaming provisioner

matzew · 2018-11-19T11:29:39Z

@@ -0,0 +1,101 @@
+apiVersion: v1


Can you add a README at the root of the natss folder, similar like the in-memory and kafka channel provisioners have it, e.g.:
https://github.com/knative/eventing/tree/master/config/provisioners/kafka

feel free to include links to setup for nats(s) as well

Harwayne · 2018-11-20T18:05:22Z

+
+NATS Streaming is deployed as a StatefulSet. 
+For tuning NATS Streaming, see:
+https://github.com/nats-io/nats-streaming-server#configuring


Also state the name of the ConfigMap that contains the configuration.

Added in README.
Done

Harwayne · 2018-11-20T18:07:00Z

+		r.logger.Info("Not reconciling Channel, it is not controlled by this Controller", zap.Any("ref", c.Spec))
+		return reconcile.Result{}, nil
+	}
+	//r.logger.Info("Reconciling Channel:", zap.Any("channel", c))


Either uncomment or remove.

Harwayne · 2018-11-20T18:09:06Z

+
+	// Modify a copy, not the original.
+	c = c.DeepCopy()
+	err = r.reconcile(ctx, c)


I think this follows a pattern in other reoncilers, but I changed my preference recently and think that this should be in a new variable reconcileErr.

My reasoning is that err tends to be checked and handled immediately, without the intention of using it again in the future. As this piece of code wants to use err again later, I think it is useful to give it a different name.

Harwayne · 2018-11-20T18:09:57Z

+
+	if c.DeletionTimestamp != nil {
+		// K8s garbage collection will delete the K8s service and VirtualService for this channel.
+		// We use a finalizer to ensure the channel config has been synced.


It doesn't look like there is any config that is synced. Do we need a finalizer at all?

Right, don't need it.
Done

Harwayne · 2018-11-20T18:11:49Z

+	// Modify a copy of this object, rather than the original.
+	ccp = ccp.DeepCopy()
+
+	err = r.reconcile(ctx, ccp)


reconcileErr, using the same reasoning as above.

Harwayne · 2018-11-20T18:15:38Z

+	// Modify a copy, not the original.
+	c = c.DeepCopy()
+
+	err = r.reconcile(ctx, c)


reconcileErr, same reasoning as above.

Harwayne · 2018-11-20T19:15:22Z

+		err = errors.New("can't close empty stan connection")
+		return
+	}
+	err = (*sc).Close()


Is the dereference needed? Does sc.Close() work?

Here "sc" is a pointer to an interface, see it in the function signature: "sc *stan.Conn"

Harwayne · 2018-11-20T19:15:55Z

+
+// Close must be the last call to close the connection
+func Close(sc *stan.Conn, logger *zap.SugaredLogger) (err error) {
+	defer func() {


Add a comment about why this might panic, so we defer a recover().

Harwayne · 2018-11-20T19:16:31Z

+
+// Publish a message to a subject
+func Publish(sc *stan.Conn, subj string, msg *[]byte, logger *zap.SugaredLogger) (err error) {
+	defer func() {


Add a comment about why this might panic, so we defer a recover().

Harwayne · 2018-11-20T19:16:43Z

+		err = errors.New("cant'publish on empty stan connection")
+		return
+	}
+	err = (*sc).Publish(subj, *msg)


Is the dereference needed? Does sc.Close() work?

"sc" is a pointer to an interface

Harwayne · 2018-11-20T19:17:37Z

+)
+
+var (
+	natsConnMux sync.Mutex


Add a comment about when this lock needs to be held.

Refactored.
Done

matzew · 2018-11-20T23:35:43Z

+kind: Service
+metadata:
+  name: nats-streaming
+  namespace: knative-eventing


not sure if the nats-streaming bit should be in the knative-eventing ns

NATSS is running now in its own namespace: natss
Done

matzew · 2018-11-20T23:37:36Z

+1. Setup [Knative Eventing](../../../DEVELOPMENT.md).
+1. Apply the 'natss' ClusterChannelProvisioner, Controller, and Dispatcher.
+     ```shell
+     ko apply -f config/provisioners/natss/


The above would provision natss, and the cpp - right ?

I think I'd prefer some separation here (e.g see Kafka). and that the actual provisioning of the natss bits would be more a sample - as well as showing how to use existing/external natss.

and than for the ccp, have just something like:

ko apply -f config/provisioners/natss/natss.yaml

Harwayne · 2018-12-04T16:11:16Z

+      - list
+      - watch
+      - create
+


- update

The common lib was changed to update the VirtualService if it has an unexpected spec.

Harwayne · 2018-12-04T16:12:29Z

+        sidecar.istio.io/inject: "true"
+      labels: *labels
+    spec:
+      serviceAccountName: natss-controller


Ideally this would use a different service account (as it needs fewer permissions than the controller). Doesn't need to be this PR, but please add a TODO to use a different service account.

The dispatcher uses now "natss-dispatcher" as ServiceAccount
Done

Harwayne · 2018-12-04T16:14:28Z

+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestInjectClient(t *testing.T) {


Should this be in reconcile_test.go?

Harwayne · 2018-12-04T16:23:09Z

+	// The Channel may have been deleted since it was added to the workqueue.
+	if errors.IsNotFound(err) {
+		r.logger.Info("Could not find Channel", zap.Error(err))
+		// unsubscribe all active subscriptions for this channel


I'm not sure we can rely on this to notice Channel deletion. I would expect to use a finalizer if you want to notice the channel is being deleted.

Channel finalizer is used now to cleanup the NATSS active subscriptions.
Done

… namespace

…ssage

- add natss-dispatcher ServiceAccount

knative-metrics-robot · 2018-12-05T15:10:02Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/provisioners/natss/controller/channel/controller.go	Do not exist	0.0%
pkg/provisioners/natss/controller/channel/reconcile.go	Do not exist	74.1%
pkg/provisioners/natss/controller/clusterchannelprovisioner/controller.go	Do not exist	0.0%
pkg/provisioners/natss/controller/clusterchannelprovisioner/reconcile.go	Do not exist	74.4%
pkg/provisioners/natss/controller/main.go	Do not exist	0.0%
pkg/provisioners/natss/dispatcher/channel/controller.go	Do not exist	0.0%
pkg/provisioners/natss/dispatcher/channel/reconcile.go	Do not exist	0.0%
pkg/provisioners/natss/dispatcher/dispatcher/dispatcher.go	Do not exist	54.5%
pkg/provisioners/natss/dispatcher/dispatcher/types.go	Do not exist	100.0%
pkg/provisioners/natss/dispatcher/main.go	Do not exist	0.0%
pkg/provisioners/natss/stanutil/stanutil.go	Do not exist	60.0%

Harwayne

/lgtm

Harwayne · 2018-12-05T16:54:59Z

+		s.subscriptions[cRef] = chMap
+	}
+	for _, sub := range subscriptions {
+		// check if the subscribtion already exist and do nothing in this case


subscribtion -> subscription

Harwayne · 2018-12-05T16:56:16Z

+	}
+	// Unsubscribe for deleted subscriptions
+	for sub := range chMap {
+		if ok := activeSubs[sub]; !ok {


Because this is a set, we probably shouldn't rely on the value being true

if _, ok := activeSubs[sub]; !ok {

evankanderson

/approve

/hold for comments

evankanderson · 2018-12-05T17:56:28Z

@@ -0,0 +1,44 @@
+# NATS Streaming Channels
+


It might be worth providing a quick overview here of the benefits of NATS Streaming:

Approx throughput?
Durability / chance of data loss?
Resource requirements?
Ordering guarantees?

evankanderson · 2018-12-05T17:57:30Z

+    app: nats-streaming
+spec:
+  serviceName: nats-streaming
+  replicas: 1


Does 1 replica have durability implications? Can this number be changed on the fly?

evankanderson · 2018-12-05T18:26:58Z

+	return reconcile.Result{}, reconcileErr
+}
+
+// IsControlled determines if the in-memory Channel Controller should control (and therefore


s/in-memory/nats/, correct?

or s/in-memory Channel/this/

evankanderson · 2018-12-05T19:06:31Z

+	for i := 0; i < 60; i++ {
+		if sc, err = stan.Connect(clusterId, clientId, stan.NatsURL(natsUrl)); err != nil {
+			logger.Warnf("Connect(): create new connection failed: %v", err)
+			time.Sleep(1 * time.Second)


Given the 1-60 loop above, I don't know that changing this sleep alone will help much.

knative-prow-robot · 2018-12-05T19:07:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, radufa

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~~ [evankanderson]

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

evankanderson · 2018-12-05T19:29:07Z

Feel free to do a followup PR for the comments; I wanted to strike while Adam's review was hot.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 16, 2018

knative-prow-robot requested review from grantr and inlined November 16, 2018 10:24

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 16, 2018

ghost changed the title ~~Add natss provisioner~~ WIP: Add natss provisioner Nov 16, 2018

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2018

ghost changed the title ~~WIP: Add natss provisioner~~ [WIP] Add natss provisioner Nov 16, 2018

ghost changed the title ~~[WIP] Add natss provisioner~~ [WIP] Add NATSS provisioner Nov 16, 2018

ghost changed the title ~~[WIP] Add NATSS provisioner~~ [WIP] Add NATS Streaming provisioner Nov 16, 2018

matzew reviewed Nov 19, 2018

View reviewed changes

matzew mentioned this pull request Nov 19, 2018

[WIP] Add NATS Streaming as a new bus implementation #491

Closed

Harwayne reviewed Nov 20, 2018

View reviewed changes

matzew reviewed Nov 20, 2018

View reviewed changes

abbi-gaurav mentioned this pull request Dec 3, 2018

Knative contribution - Remove WIP label from the PR kyma-project/kyma#1894

Closed

ghost changed the title ~~[WIP] Add NATS Streaming provisioner~~ Add NATS Streaming provisioner Dec 4, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2018

Harwayne reviewed Dec 4, 2018

View reviewed changes

ghost changed the title ~~Add NATS Streaming provisioner~~ [WIP] Add NATS Streaming provisioner Dec 5, 2018

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2018

Radu Fantaziu added 9 commits December 5, 2018 09:06

Add natss provisioner: initial commit

8408d06

Cleanup code, add more log messages

050d484

Remove unused code related to the configmap

2259b1e

Update vendor and gopkg.toml

2b156f9

Remove sample applicatoin

d9ea8c4

Update Gopkg.lock

61efb16

Update VENDOR-LICENSE

767d39e

Update vendor

634becd

Add README.md

7012ca8

Radu Fantaziu added 16 commits December 5, 2018 09:06

Update README.md with NATSS configuration link

806a843

Add first unit tests for controller/channel

fd017f1

Update after rebase

e41a19b

Fix missed import in test

da50050

Solve some of requested changes

809615f

Unsubscribe NATSS subscriptions if the related channel is simply deleted

a38e62b

Update unit tests to not use finalizers

7df51d4

Run NATS Streaming in "natss" namespace instead of "knative-eventing"…

6d4428c

… namespace

Add more unit tests

1ac2018

Update after rebase

c9a70e2

Update based on running update-deps.sh

98807bf

Add test "Update Subscriptions" for dispatcher

aff8faa

Update after running update-deps.sh

961082b

Move NATS Streaming installation in a subdirectory.

ad8b23c

Add subscription's namespace to DispatchDefault at calling DispatchMe…

6dc52bc

…ssage

Use channel's finalizer to cleanup NATSS subscriptions

f188a52

- add natss-dispatcher ServiceAccount

ghost changed the title ~~[WIP] Add NATS Streaming provisioner~~ Add NATS Streaming provisioner Dec 5, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2018

Harwayne reviewed Dec 5, 2018

View reviewed changes

knative-prow-robot assigned Harwayne Dec 5, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2018

evankanderson reviewed Dec 5, 2018

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2018

knative-prow-robot merged commit b25850b into knative:master Dec 5, 2018

ghost mentioned this pull request Dec 6, 2018

Knative contribution - make NATSS provisioner production ready kyma-project/kyma#1940

Closed

Harwayne mentioned this pull request Feb 7, 2019

Add Harwayne as an approver. #782

Merged

pierDipi added a commit to pierDipi/eventing that referenced this pull request Jun 12, 2024

Add JobSink Certificate to TLS artifact (knative#624)

8b63860

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Conversation

ghost commented Nov 16, 2018

Proposed Changes

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!

ghost Nov 29, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

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!

ghost Nov 29, 2018 •

edited by ghost

Loading