feat: jetstream eventbus controller implementation #1705

whynowy · 2022-03-03T19:58:00Z

Signed-off-by: Derek Wang [email protected]

This PR is part of the effort of JetStream EventBus implementation.

A JetStream EventBus spec looks like below, where version is a required field in the spec.

apiVersion: argoproj.io/v1alpha1
kind: EventBus
metadata:
  name: default
spec:
  jetstream:
    version: 2.7.3

TLS enabling is not done in this PR, it should be implemented before we do an official release.

Current Nats Streaming image configuration is also moved into a configmap, together with JetStream image configs. This configmap is now only used by eventbus-controller. As one of the follow-up tasks, all of the 3 controllers will be merged to one #1564.

Checklist:

My organization is added to USERS.md.

Signed-off-by: Derek Wang <[email protected]>

whynowy · 2022-03-03T20:08:04Z

common/hash.go

@@ -0,0 +1,21 @@
+package common


We should refactor all those util functions under several common across the project.

juliev0 · 2022-03-03T23:07:59Z

pkg/apis/eventbus/v1alpha1/jetstream_eventbus.go

+	// See https://docs.nats.io/running-a-nats-service/configuration#jetstream.
+	// Only configure "max_memory_store" or "max_file_store", do not set "store_dir" as it has been hardcoded.
+	// +optional
+	Settings *string `json:"settings,omitempty" protobuf:"bytes,15,opt,name=settings"`


This is nice and generic. I'll update my design document to reflect this.

juliev0 · 2022-03-03T23:19:06Z

pkg/apis/eventbus/v1alpha1/jetstream_eventbus.go

+	if j.Replicas == nil {
+		return 3
+	}
+	if *j.Replicas < 3 {


oh, interesting. We don't let them have less than 3. Ideally maybe we'd warn them (log message or whatever) but not critical.

Cluster setting has a different configuration from single node setting, it's not worth supporting that for us.

For a cluster setting, it needs at least 2 nodes to make it work. JetStream uses a RAFT algorithm for clustering, which does not make sense to use an even number. So a reasonable cluster node number is 3 or 5.

In order to ensure data consistency across complete restarts, a quorum of servers is required. A quorum is ½ cluster size + 1. This is the minimum number of nodes to ensure at least one node has the most recent data and state after a catastrophic failure. So for a cluster size of 3, you’ll need at least two JetStream enabled NATS servers available to store new messages. For a cluster size of 5, you’ll need at least 3 NATS servers, and so forth.

https://docs.nats.io/running-a-nats-service/configuration/clustering/jetstream_clustering#the-quorum

Okay. Just wondering if we might want to log something so it's not a silent change. But I'm good with whatever - probably rare that somebody would set replicas to 1 or 2.

Adding a log sounds good.

It seems to be awkward to add a log in the model, skip it for now.

juliev0

Nice. Left a few comments but didn't really see any issues.

daniel-codefresh

Overall looks great! left few small comments/questions

daniel-codefresh · 2022-03-04T17:50:03Z

controllers/config.go

+			return &r, nil
+		}
+	}
+	return nil, fmt.Errorf("no nats streaming configuration found for %q", version)


Maybe it would be better to let the user know which versions are currently supported so the user would have a better indication on how to fix this error, WDYT?

Same goes for GetJetStreamVersion

Good point!

daniel-codefresh · 2022-03-04T17:55:30Z

controllers/eventbus/installer/jetstream.go

+	return nil
+}
+
+func (r *jetStreamInstaller) buildStatefulSetSpec(jeVersion *controllers.JetStreamVersion) appv1.StatefulSetSpec {


can we have a default version here (like we have in the NATS installer) to make the Version field optional for the user?

Having a default version will cause future upgrade issue - if version is not specified, we use a default version, that will prevent us from updating the default version later, otherwise if the default one is changed, those installation with default versions will be automatically updated. This is a problem for current nats streaming implementation.

Got it, thanks for the clarification

daniel-codefresh · 2022-03-04T18:00:44Z

manifests/base/eventbus-controller/controller-config.yaml

+        - version: 2.7.3
+          natsImage: nats:2.7.3
+          metricsExporterImage: natsio/prometheus-nats-exporter:0.9.1
+          startCommand: /nats-server


shouldn't it be nats-server (without the /) ?

This image is built from scratch, the binary is put under /, so we have to use /nats-server.

Signed-off-by: Derek Wang <[email protected]>

sarabala1979 · 2022-03-08T02:15:07Z

controllers/eventbus/validate.go

+	}
+	if x := eb.Spec.JetStream; x != nil {
+		if x.Version == "" {
+			return fmt.Errorf("invalid spec: a version for jetstream needs to be specified")


Can we add replicas validation here?

sounds good, done.

Signed-off-by: Derek Wang <[email protected]>

sarabala1979

LGTM

Signed-off-by: Derek Wang <[email protected]>

feat: jetstream eventbus controller implementation

cc14681

Signed-off-by: Derek Wang <[email protected]>

whynowy requested a review from alexec March 3, 2022 20:02

whynowy added 2 commits March 3, 2022 12:06

lint

a8e8e90

Signed-off-by: Derek Wang <[email protected]>

Merge remote-tracking branch 'upstream/master' into jsbus

585d74b

whynowy commented Mar 3, 2022

View reviewed changes

whynowy requested a review from daniel-codefresh March 3, 2022 20:14

juliev0 reviewed Mar 3, 2022

View reviewed changes

juliev0 approved these changes Mar 3, 2022

View reviewed changes

whynowy requested a review from sarabala1979 March 4, 2022 06:51

daniel-codefresh approved these changes Mar 4, 2022

View reviewed changes

whynowy added 2 commits March 4, 2022 11:54

.

ae683dc

Signed-off-by: Derek Wang <[email protected]>

fix preStop and adding config reloader container

f98237f

Signed-off-by: Derek Wang <[email protected]>

sarabala1979 reviewed Mar 8, 2022

View reviewed changes

.

8995117

Signed-off-by: Derek Wang <[email protected]>

sarabala1979 approved these changes Mar 8, 2022

View reviewed changes

.

bc701c8

Signed-off-by: Derek Wang <[email protected]>

whynowy merged commit 475baac into argoproj:master Mar 8, 2022

whynowy deleted the jsbus branch March 8, 2022 07:35

juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022

feat: jetstream eventbus controller implementation (argoproj#1705)

de87b73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: jetstream eventbus controller implementation #1705

feat: jetstream eventbus controller implementation #1705

whynowy commented Mar 3, 2022 •

edited

Loading

whynowy Mar 3, 2022 •

edited

Loading

juliev0 Mar 3, 2022

juliev0 Mar 3, 2022 •

edited

Loading

whynowy Mar 4, 2022

juliev0 Mar 4, 2022

whynowy Mar 4, 2022

whynowy Mar 4, 2022

juliev0 left a comment

daniel-codefresh left a comment

daniel-codefresh Mar 4, 2022

daniel-codefresh Mar 4, 2022

whynowy Mar 4, 2022

daniel-codefresh Mar 4, 2022

whynowy Mar 4, 2022

daniel-codefresh Mar 4, 2022

daniel-codefresh Mar 4, 2022

whynowy Mar 4, 2022

sarabala1979 Mar 8, 2022

whynowy Mar 8, 2022

sarabala1979 left a comment

feat: jetstream eventbus controller implementation #1705

feat: jetstream eventbus controller implementation #1705

Conversation

whynowy commented Mar 3, 2022 • edited Loading

whynowy Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliev0 Mar 3, 2022 • edited Loading

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

juliev0 left a comment

Choose a reason for hiding this comment

daniel-codefresh left a 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

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

sarabala1979 left a comment

Choose a reason for hiding this comment

whynowy commented Mar 3, 2022 •

edited

Loading

whynowy Mar 3, 2022 •

edited

Loading

juliev0 Mar 3, 2022 •

edited

Loading