raystack / charts Goto Github PK

Summary
Siren has its own template. We can reuse app/templates and remove templates in siren.

Proposed solution
Reusing app/templates for siren will make sure that every updates in app/templates can be utilized siren and making a single source of truth for templates. This is also to avoid unnecessary duplication in the charts.

We add helm hook annotations inside Job.spec.template.metadata. The correct approach is to add the annotations inside Job.metadata as mentioned in helm docs.

We need to move annotations from Job.spec.template.metadata to Job.metadata.

Summary
Should the service or ingress annotations be part of the charts, since the current values are based on our internal infra, and users can have their own different controllers?

Example:

• Service annotation

  service:
    annotations:
      projectcontour.io/upstream-protocol.h2c: tcp

https://github.com/odpf/charts/blob/main/stable/guardian/values.yaml#L33

• Ingress annotation:

  ingress:
    enabled: true
    annotations:
      kubernetes.io/ingress.class: contour

https://github.com/odpf/charts/blob/main/stable/guardian/values.yaml#L66

Related discussion - #66 (comment)

In siren chart, we have 2 workers and require to use livenessProbe and readinessProbe with exec.

    livenessProbe:
      exec:
        command:
        - /bin/sh
        - -c
        - ps -ef | grep siren | grep -v grep
      httpGet: null
      initialDelaySeconds: 10
      periodSeconds: 10
    readinessProbe:
      exec:
        command:
        - /bin/sh
        - -c
        - ps -ef | grep siren | grep -v grep
      httpGet: null
      initialDelaySeconds: 10
      periodSeconds: 10

And there is already a default httpGet value in both probes in charts/app. I expect setting this as httpGet=null would unset the httpGet value. But turns out both probes exec and httpGet are rendered and this caused an error when doing helm upgrade

Error: failed to create resource: Deployment.apps "xxx" is invalid: [spec.template.spec.containers[0].livenessProbe.httpGet: Forbidden: may not specify more than 1 handler type, spec.template.spec.containers[0].readinessProbe.httpGet: Forbidden: may not specify more than 1 handler type]

This is a long pending issue since helm 3. Refer here.

Solution

In this PR, I add if-else checking in deployment template. If exec probe exists, use it only, if not use whatever the value in the livenessProbe or readinessProbe.

Description
cron job names from app chart have a length of more than 52 characters, causing the deployment to fail.

Line - https://github.com/odpf/charts/blob/main/stable/app/templates/cron.yaml#L9

Need to reduce the length to <= 52 characters.
Like - name: "{{ $fullName | trunc 24 }}-{{ $i }}-{{ $job.name | trunc 24}}"

Currently, the app chart does not allow to pass custom annotation to the service object

Since the app chart is a generic chart and if the implementing chart has GRPC endpoint exposed. To make it accessible via ingress custom annotations need to be added to the service object.

In case if we are using contour ingress the annotation is
projectcontour.io/upstream-protocol.h2c: "<port-number>"
if plain HTTP/2 is used, for grpc without tls

As of now charts are getting published directly from PRs instead of them getting published after being merged in the main branch.
We need to change the workflows to avoid this from happening.

app chart should allow opening multiple ports from the container

https://github.com/odpf/charts/blob/88a1df58dc290d2e9f001d9853ded5c618959cb3/stable/app/values.yaml#L76

Right now .Values.container.entryPoint is being used which is confusing.

https://github.com/odpf/charts/blob/ef9aecae88ba3fae4be16144e0d28fa917eecdc6/stable/app/templates/deployment.yaml#L46

Add support for raccoon helm chart

We should run linter on all commits and pull requests and release only from odpf:main branch.
As of now any pull request from main branch of the user will have the release step triggered.

The current migration-job template does not clean up post the job completion. During app redeployments, this poses an issue, and one has to manually delete the completed migration job.

The error that is being generated during redeployment currently -

Error: cannot patch "columbus-app-migration-job" with kind Job: Job.batch "columbus-app-migration-job" is invalid:
spec.template: Invalid value: core.PodTemplateSpec{ .... }:  field is immutable

Skip generating YAML manifest from template if .Values.config is empty
https://github.com/odpf/charts/blob/main/stable/app/templates/configmap.yaml#L1

Summary
In the current meteor chart, even if values.secretConfig are empty, k8s secrets are created.

(⎈ |core-sourcing:meteor)➜  ~ kgsec core-meteor-kafka-secret -oyaml
apiVersion: v1
kind: Secret
metadata:
  annotations:
    meta.helm.sh/release-name: core-meteor-kafka
    meta.helm.sh/release-namespace: meteor
  creationTimestamp: "2022-06-06T15:46:08Z"
  labels:
    app.kubernetes.io/managed-by: Helm
  name: core-meteor-kafka-secret
  namespace: meteor
  resourceVersion: "297849918"
  uid: cbabakea-126b-4bf8-be37-2efeghifd084
type: Opaque
(⎈ |core-sourcing:meteor)➜  ~

Proposed solution
Create k8s secret only when there are secrets present

Description
With helm version v3.5.3, used by terraform helm provider v2.1.2.

Helm hook annotation are added in configmap and secret in this commit. During helm upgrade, the migration job pods are terminated along with the deletion of configmap and secret resources.

However, running helm upgrade multiple times cause different final states of upgrade.

Behaviour:
1st upgrade
Initial state:

• Config exist
• App pod exist

Hook running:

• Config removed
• Config added
• Migration pod running
• Migration terminated
• Config removed

Final state:

• Config not exist
• Old pods are running
• Some new pods are created with CreateContainerConfigError

2nd upgrade
Initial state:

• Config not exist
• Old app pods are running, New app pods are created with CreateContainerConfigError

Hook running:

• Config removed
• Config added
• Migration pod running
• Migration terminated
• Config removed

Final state:

• Config not exist
• Old pods are terminated
• Some new app pods are running, the other new app pods are created with CreateContainerConfigError

3rd upgrade
Initial state:

• Config not exist
• Some new app pods are running, some new app pods are created with CreateContainerConfigError

Hook running:

• Config removed
• Config added
• Migration pod running
• Migration terminated
• Config removed

Final state:

• Config not exist
• All new app pods are running

4th upgrade
Initial state:

• Config not exist
• All new app pods are running

Hook running:

• Config removed
• Config added
• Migration pod running
• Migration terminated
• Config removed

Final state:

• Config exist
• All new app pods are running (no-op)

To Reproduce
Run helm upgrade with odpf app chart version v0.4.0

Expected behavior
Regardless failed or success, the migration jobs pods and configmap should not be removed when deployment is finished

Usage and setup instructions missing for guardian chart

Is your feature request related to a problem? Please describe.
I want to use app chart for deploying my service. My service happens to have a db migration command that needs to be run before running my service. Right now app chart does not support a direct way to do that.

Describe the solution you'd like
Have a migration config and values that we can toggle and fill to create a k8s job for running db migration.
e.g.

# values.yaml
...
migration:
  enabled: false
  command: ~
  args:
    - migrate