justinmchase commented 3 years ago

Is your feature request related to a problem? Please describe. I installed argo-events through helm but it just doesn't work. I setup my kafka event source and it never connects giving an error about a missing EventBus.

When I manually run kubectl apply -n argo-events -f https://raw.githubusercontent.com/argoproj/argo-events/stable/examples/eventbus/native.yaml then the bus appears and I see it starts connecting.

It really seems like the helm chart should create the EventBus when you install it and just have configuration options to configure it. But the event system seems to not function without it so it probably should be in the chart.

Describe the solution you'd like I'd like to just be able to helm install argo-events and boom it has everything to function, at least simply, by default.

Describe alternatives you've considered I can just run more stuff and manually create resources but it is confusing and takes a lot of effort just to even try argo out at all.

Additional context Additionally, I hate that argo goes into one namespace and argo-events goes into another... Please just put everything under the 1 argo namespace.

osherlevi commented 3 years ago

Same for me, saw this error: 2021-08-11T10:35:29.296Z ERROR argo-events.eventsource-controller eventsource/controller.go:53 reconcile error {"namespace": "argo-events", "eventSource": "webhook", "error": "eventbus default not found", "errorVerbose": "eventbus default not found\ngithub.com/argoproj/argo-events/controllers/eventsource.Reconcile\n\t/home/runner/work/argo-events/argo-events/controllers/eventsource/resource.go:48\ngithub.com/argoproj/argo-events/controllers/eventsource.(reconciler).reconcile\n\t/home/runner/work/argo-events/argo-events/controllers/eventsource/controller.go:93\ngithub.com/argoproj/argo-events/controllers/eventsource.(reconciler).Reconcile\n\t/home/runner/work/argo-events/argo-events/controllers/eventsource/controller.go:51\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(Controller).reconcileHandler\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.7.0/pkg/internal/controller/controller.go:263\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(Controller).processNextWorkItem\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.7.0/pkg/internal/controller/controller.go:235\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1.1\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.7.0/pkg/internal/controller/controller.go:198\nk8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext.func1\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:185\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:185\nk8s.io/apimachinery/pkg/util/wait.UntilWithContext\n\t/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.19.7-rc.0/pkg/util/wait/wait.go:99\nruntime.goexit\n\t/opt/hostedtoolcache/go/1.14.15/x64/src/runtime/asm_amd64.s:1373"}

I guess that the issue is that the name of the eventbus is different from default, how can we adjust the event source controller?

kurtbomya commented 3 years ago

I am also surprised to learn that the required CRDs are not generated with something like helm template argo/argo-events since the README.md states that I need to put --skip-crds to prevent the creation of CRDs. But here I am, not putting that flag, and still not getting the CRDs included.

$ helm template argo/argo-events --skip-crds | wc -l
280
$ helm template argo/argo-events | wc -l
280

mkilchhofer commented 3 years ago

I am also surprised to learn that the required CRDs are not generated with something like helm template argo/argo-events since the README.md states that I need to put --skip-crds to prevent the creation of CRDs. But here I am, not putting that flag, and still not getting the CRDs included.
$ helm template argo/argo-events --skip-crds | wc -l
280
$ helm template argo/argo-events | wc -l
280

The helm template subcommand acts different from helm installor helm upgrade. To include the CRDs in your helm template output, you need to use --include-crds

reinvantveer commented 3 years ago

This behavior is, I agree, unexpected. Just started using this helm chart for event processing, but to my surprise it needs an additional event bus resource for each namespace that is home to event sources and event bindings. Which is not supplied by the helm chart. Is this correct @mkilchhofer ?

reinvantveer commented 2 years ago

@mkilchhofer maybe this escaped your attention. This issue was still waiting for a response.

reinvantveer commented 2 years ago

Could you also re-open the issue? I'm fine submitting a pull request, but what's in it rather depends on the intended strategy for the chart.

mkilchhofer commented 2 years ago

Oh yeah, this should not have been closed.

But as I don't use argo-events, I cannot help much with this issue. I don't understand what this EventBus.argoproj.io object does and why it's not included in the default manifests.

reinvantveer commented 2 years ago

Thanks for the action @mkilchhofer. I'll see if I can open a PR for this soon. Argo Events is useless without a EventBus resource. It creates the bus itself where event messages are written to and from.

mkilchhofer commented 2 years ago

Hmm I read through the documentation. The architecture picture made me thinking about this issue again.

architecture The chart seems to be designed to just deploy the controllers.

Besides the EventBus the users need to deploy (depending on their use-case):

an event source

apiVersion: argoproj.io/v1alpha1
kind: EventSource
metadata:
name: webhook
spec:
service:
  ports:
    - port: 12000
      targetPort: 12000
webhook:
  # event-source can run multiple HTTP servers. Simply define a unique port to start a new HTTP server
  example:
    # port to run HTTP server on
    port: "12000"
    # endpoint to listen to
    endpoint: /example
    # HTTP request method to allow. In this case, only POST requests are accepted
    method: POST

a sensor:

apiVersion: argoproj.io/v1alpha1
kind: Sensor
metadata:
name: webhook
spec:
template:
  serviceAccountName: operate-workflow-sa
dependencies:
  - name: test-dep
    eventSourceName: webhook
    eventName: example
triggers:
  - template:
    # (..)

--> While we could extend the chart to provide this functionality, we must not install anything of EventSource, EventBus Sensor by default.

WDYT @jbehling and @VaibhavPage?

reinvantveer commented 2 years ago

@mkilchhofer I believe resources like EventSource and Sensor are more or less like Workflow resources in the sense that they are fully user-defined and use case dependent: I agree that there is no place for them in the chart. However, the case is different for the EventBus I think.

The event bus is a dependency for the controller deployments. By default, controllers try to connect to an event bus with the unfortunate default name of default (which I believe should be marked as bad practice) and fails to start if not found (see error message by @osherlevi). As is, the events helm chart is an incomplete release in that it does not fully deploy.

Furthermore, the event bus is a dependency for deploying any EventSource (see its spec first field) or Sensor: they get an eventBusName that should match an event bus present in the namespace.

HTH

mkilchhofer commented 2 years ago

No, also the EventBus CRD is a resource like EventSource and Sensor. Only the eventbus controller talks to this CRD as it is responsible for the lifecycle of the eventbus (upgrade, downgrade, scaling, configuring, etc.). The dependency you mentioned is exactly this. It watches for these kind of objects and then would create a StatefulSet and other resources to implement the CRD with real (NATS) pods.

From the picture above it looks clear to me that the controllers do not talk with the eventbus.

reinvantveer commented 2 years ago

Aha I see, thanks for the clarification. So, it seems there is a bit of a contradiction in the docs. The architecture picture can be read such that EventBus is a resource like EventSource and Sensor. On the other hand, the install guide marked in yellow in https://github.com/argoproj/argo-helm/issues/840#issuecomment-1014267141 indicates that the EventBus is part of the installation procedure. From the latter, it would not be unreasonable to imply that the argo-events helm chart comes with step three of the install guide included.

Which brings me to this: it would be helpful if the helm chart either

offers better documentation on why the event bus is not included,
comes with the option to include the event bus, or
just comes with the event bus.

Either way is fine, I think, but in its current state causes confusion, hence this issue. I leave it up to the maintainers to decide on this, but I guess for people starting out with this helm chart, it could be a tremendous time saver to have either option 1 or 2.

Thanks again for your thoughts.

reinvantveer commented 2 years ago

@mkilchhofer can we agree on option 2?

mkilchhofer commented 2 years ago

Yes, sure. There is already a PR open from the community: https://github.com/argoproj/argo-helm/pull/1109

reinvantveer commented 2 years ago

Ah someone beat me to it! Excellent!

mgfreshour commented 2 years ago

Since that PR was closed without merging 7 days ago, is there any workaround for this?

phoenix1480 commented 2 years ago

Hi there, we use ArgoCD, Argo Workflows, and are in process of deploying Argo Events. For Argo Events we are facing same issue as other community members here.

Does the helm chart include the event bus or just a controller to interface with the event bus? It isn't clear if we should be installing the EventBus separately.
If the EventBus could be included in the helm-chart that would be incredibly valuable. I could see it not being added to keep the EventBus fully decoupled. If that is the case, perhaps modifying the documentation to specify that Argo Events requires the EventBus (NATS/JetStream) be installed first.

2 is okay by us for our needs and a better design practice. That said, getting up and running is made more difficult by this design choice for newcomers like ourselves.

If someone could please confirm that we MUST install the EventBus separately, that would be much appreciated. I admit, our team is stuck atm ;)

pdrastil commented 2 years ago

@phoenix1480 Hi, I've reimplemented the recent version of argo-events and the reason why I haven't added the EventBus even when I saw this issue was because of managing upgrades in multi-tenant environments that can contain multiple EventBus resources.

Example:

apiVersion: argorpoj.io/v1alpha1
kind: EventBus
metadata:
  name: default
spec:
  jetstream:
    version: my-version
    replicas: 3

As you can see in example above the version is tied to configs section in Helm chart below. If there would be a version bump it might upgrade the default bus created / managed by chart but also break other additionally created EventBuses.

configs:
  jetsream:
    version:
      -version: "2.0.2"
        natsImage: <nats-image>:2.0.2
     - version: "2.0.2-alpine"
        natsImage: <nats-image>:2.0.2-alpine
      -version: "2.0.1"
        natsImage: <nats-image>:2.0.2

Second problem would be in referencing customer resource definition that might not exists in the cluster yet. If you want to have preconfigured Argo Events with buses / triggers etc. I would recommend to create your own umbrella chart that will first deploy the controller with configuration as a dependency and then creates all these resources.

reinvantveer commented 2 years ago

If someone could please confirm that we MUST install the EventBus separately, that would be much appreciated. I admit, our team is stuck atm ;)

Yes, Argo Events will go absolutely nowhere without an event bus. The current solution is to create an umbrella chart @pdrastil suggests that includes a manifest like the one suggested in the getting started guide: https://github.com/argoproj/argo-events/blob/master/examples/eventbus/native.yaml

reinvantveer commented 2 years ago

@phoenix1480 If this helps: I did some tinkering here: https://github.com/Antfield-Creations/gke/tree/main/charts/antfield-argo-events Strongly advise to review this by the dev team, as it may not suit your needs

reinvantveer commented 2 years ago

@phoenix1480 Hi, I've reimplemented the recent version of argo-events and the reason why I haven't added the EventBus even when I saw this issue was because of managing upgrades in multi-tenant environments that can contain multiple EventBus resources.

This is good to take into consideration for designing updates to the current chart. It should not enable an event bus by default, but it should be made easy to opt into one

rakhbari commented 1 year ago

Sorry I'm a little late to the conversation about this, but I have to disagree with @reinvantveer 's point about "should not enable an event bus by default". Installing an EventBus named default in the argo-events namespace, aside from making things a lot easier on people new to Argo Events, does not prevent you in any way from adding other EventBuses with your own name in other namespaces.

At the very least the values.yaml of the chart should be modified to have a installDefaultEventBus flag (default: false) and the user can then supply true in their values.yaml to have the default EventBus installed in argo-events namespace. Or, if you really want to get fancy, values.yaml can have an array of eventBuses where name and namespace are fields of the array. Either way, IMHO leaving out the ability for creation of the EventBus from the chart is a real hinderance.

reinvantveer commented 1 year ago

@rakhbari thanks for sharing your point of view here. My main consideration was that an update to the chart should not break existing installations, let alone on an EventBus named default - an opt-in installDefaultEventBus flag or comparable should do fine I think. I was unable to find the time to make progress on this issue in last few months, and there has been https://github.com/argoproj/argo-helm/pull/1109 before but this hasn't been completed unfortunately. I still hope to be able to find the hours in January to follow this up.

ghost commented 1 year ago

Hi to all here - getting same on Helm Charts version 2.1.2

Can someone make some clarification of this case ? As i understand the temp solution of this it is to apply yaml of eventbus native.yaml

kubectl apply -n argo-events -f https://raw.githubusercontent.com/argoproj/argo-events/stable/examples/eventbus/native.yaml

this kubectl apply can make work helm chart as expected ?

reinvantveer commented 1 year ago

Hi @boris-shkarupelov that's right - you deploy an event bus. The "pretty" way of doing this would be to derive your own helm chart from the argo-events chart and put the contents of https://raw.githubusercontent.com/argoproj/argo-events/stable/examples/eventbus/native.yaml in there, so that you have a versioned release, including your event bus. This is exactly why it would be a good idea to include it into the argo-events chart in the first place.

I created something similar here: https://github.com/Antfield-Creations/gke/tree/main/charts/antfield-argo-events but this uses an old version of Argo Events, no guarantees on it working on newer versions.

joebowbeer commented 1 year ago

As things stand currently, the installation instructions should be edited to mention that the helm chart does not install an eventbus:

https://argoproj.github.io/argo-events/installation/#using-helm-chart

In addition, -n argo-events --create-namespace should be added to the helm install command line.

jmeridth commented 1 year ago

@joebowbeer would you like to create a PR for those doc change requests?

murand78 commented 1 year ago

apiVersion: argorpoj.io/v1alpha1

apiVersion: argorpoj.io/v1alpha1

Note the typo, "argorpoj", I noticed only after updating and recreated the crds a lot of times :-)

joebowbeer commented 1 year ago

@murand78 link to typo?

murand78 commented 1 year ago

sorry, the example in https://github.com/argoproj/argo-helm/issues/840#issuecomment-1222787247

argoproj / argo-helm

The argo-events chart doesn't include an EventBus #840

2 is okay by us for our needs and a better design practice. That said, getting up and running is made more difficult by this design choice for newcomers like ourselves.