Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idea: consider 0 as valid value for EventListener replicas #892

Closed
Fabian-K opened this issue Jan 11, 2021 · 5 comments
Closed

Idea: consider 0 as valid value for EventListener replicas #892

Fabian-K opened this issue Jan 11, 2021 · 5 comments
Labels
kind/question Issues or PRs that are questions around the project or a particular feature

Comments

@Fabian-K
Copy link
Contributor

Hi,

I´m trying to replace the deployments created by EventListener with Knative Services to enable scale to zero. I´m aware of https://github.com/tektoncd/community/blob/master/teps/0008-support-knative-service-for-triggers-eventlistener-pod.md and think this is a great long term solution. Until this is fully implemented: what are your thoughts of considering 0 as a valid value for the field replicas of EventListener? With this I could create EventListeners with replicas=0 and therefore prevent the pod from running all the time and instead manually create an additional Knative Service to cover the functionality instead.

Thanks, Fabian

@dibyom
Copy link
Member

dibyom commented Jan 13, 2021

Until this is fully implemented: what are your thoughts of considering 0 as a valid value for the field replicas of EventListener? With this I could create EventListeners with replicas=0 and therefore prevent the pod from running all the time and instead manually create an additional Knative Service to cover the functionality instead.

Hmm interesting idea...Are you manually creating a Knative Service using the EventListener sink image?

I'm guessing we only need the EventListener because we fetch the EventListener to get the list of triggers to process...I wonder if there is a way we could refactor that code so that you could just pass it a list of triggers.

also /cc @savitaashture for timeline on Knative EventListener implementation.

@dibyom dibyom added the kind/question Issues or PRs that are questions around the project or a particular feature label Jan 13, 2021
@Fabian-K
Copy link
Contributor Author

Yes, exactly! I only did a quick test so far but the following service definition seems to work (almost a 1:1 copy of the deployment)

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: ${name}-ksvc
  namespace: ${namespace}
spec:
  template:
    spec:
      containers:
        - args:
            - --el-name=${name}
            - --el-namespace=${namespace}
            - --port=8080
            - --readtimeout=5
            - --writetimeout=40
            - --idletimeout=120
            - --timeouthandler=30
            - --is-multi-ns=false
            - --tls-cert=
            - --tls-key=
          env:
            - name: SYSTEM_NAMESPACE
              value: ${namespace}
          image: gcr.io/tekton-releases/github.com/tektoncd/triggers/cmd/eventlistenersink:v0.10.2@sha256:1a91b57e5007ad6f524c2157cf83bdef97743bcc06a50c73ace8f48d9055fc7c
          imagePullPolicy: IfNotPresent
          volumeMounts:
            - mountPath: /etc/config-logging
              name: config-logging
      serviceAccountName: tekton-triggers-listener
      volumes:
        - configMap:
            defaultMode: 420
            name: config-logging-triggers
          name: config-logging

Fabian-K added a commit to Fabian-K/triggers that referenced this issue Jan 25, 2021
…dditional work, replicas=0 results in a broken setup.

This change is intended as a workaround until a proper Knative EventListener implementation is available (see tektoncd#892)
Fabian-K added a commit to Fabian-K/triggers that referenced this issue Jan 25, 2021
This change is intended as a workaround until a proper Knative EventListener implementation is available (see tektoncd#892). Note: without additional work, replicas=0 results in a broken setup.
@Fabian-K
Copy link
Contributor Author

The required changes to enable this seems to be small. I opened a PR in case you consider this change :)

@dibyom
Copy link
Member

dibyom commented Mar 24, 2021

I think with the Knative EL, this is no longer needed. Feel free to re-open if that's not the case.
/close

@tekton-robot
Copy link

@dibyom: Closing this issue.

In response to this:

I think with the Knative EL, this is no longer needed. Feel free to re-open if that's not the case.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/question Issues or PRs that are questions around the project or a particular feature
Projects
None yet
Development

No branches or pull requests

3 participants