search

temporalio / helm-charts

Temporal Helm charts
MIT License
299 stars 329 forks source link

Add support for ISTIO sidecar #354

Closed jeenadeepak closed 7 months ago

jeenadeepak commented 1 year ago

What was changed

Add support for ISTIO deployment

Why?

Istio run with each pod due to which one time job/pod are not successfully terminal, istio is not required here as db schema setup and update are one time job, hence skipping istio for jobs.

Checklist

  1. Closes

    https://github.com/temporalio/helm-charts/issues/353

  2. How was this tested:

    Testing on AWS EKS with istio.

  3. Any docs updates needed?

    no

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: jeenadeepak
:x: EC2 Default User

EC2 Default User seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

jeenadeepak commented 1 year ago

@mindaugasrukas : I have checked this example, it does not seems correct solution.

I am facing this issue with DB migration job due to istio, rest service are working fine with istio, so I do not want remove istio for entire application, but as per this example this will skip istio for other temporal services.

There are 2 problem I have identified with DB migration: 1: DB migration is one time job, hence db migration container quit but istio container is log running so it showing error, 2: DB migration started before istio networking hence it stop DB migration container to connect with SQL port.

mindaugasrukas commented 1 year ago

My point is that this change will add environment-specific fields to the generic template. We don't want to add every environment-specific setting to the template but have the ability to inject user-specific configs. So, I'd like to ask you to make the template change, allowing you or anyone else to inject their needs.

underrun commented 1 year ago

to be more specific - you can change this template here: https://github.com/temporalio/helm-charts/blob/master/templates/server-job.yaml#L15

to add something like

    {{- with .Values.serviceAccount.extraAnnotations }}
      {{- toYaml . | nindent 4 }}
    {{- end }}

but where you change serviceAccount to be for only jobs or something.

vishwa-trulioo commented 1 year ago

Any update on this?

jeenadeepak commented 1 year ago

@mindaugasrukas : I have updated template as per your suggestion.