feat(k8s): Update pods with correct probes and remove unnecessary dependencies #2362
Conversation
|
Tagging folks who actively use helm charts to make sure the change looks good on your sides. @pedro93 @bernardino @frsann @shakti-garg-saxo |
|
We're rolling our own kustomize-targeted manifests, only loosely based on these charts, so no need to take us into consideration 😅 Thanks for the ping, though! |
| @@ -46,26 +46,38 @@ spec: | |||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | |||
| ports: | |||
| - name: http | |||
| containerPort: 9001 | |||
| containerPort: 9002 | |||
There was a problem hiding this comment.
| containerPort: 9002 | |
| containerPort: {{ .Values.service.port }} |
There was a problem hiding this comment.
This is an internal port. I think it should be hardcoded. service.port field is used in the service.yml to define external port. WDYT?
| initialDelaySeconds: 60 | ||
| periodSeconds: 30 | ||
| failureThreshold: 4 |
There was a problem hiding this comment.
I would probably make these come from values
There was a problem hiding this comment.
Should I make the whole readiness and liveness probe come from values?
There was a problem hiding this comment.
i have a suggestion here to only move below properties to values.yaml. The httpGet path can stay hardcoded as this the behavior of app which is not going to change on user wish.
initialDelaySeconds: 60
periodSeconds: 30
failureThreshold: 8
| # This is the version number of the application being deployed. This version number should be | ||
| # incremented each time you make changes to the application. | ||
| appVersion: latest #0.3.1 | ||
| dependencies: |
There was a problem hiding this comment.
Though these sub-chart dependencies are local, I still believe listing dependencies is good for two reasons:
- Good for documentation and chart navigation
- one of sub-chart(component) can be toggled off in special deployment scenarios. It is enabled by "condition" property like
datahub-mae-consumer.enabled. In our case, we don't need to deploy datahub-ingestion-cron(as we have our own ingestion service), so we just provide parameter "datahub-ingestion.cron.enabled=false"
There was a problem hiding this comment.
You are right. I initially removed this because helm dep update doesn't work for datahub since all the subcharts are in the charts directory. Reverting. Going to keep the version update since multiple setup jobs were added.
There was a problem hiding this comment.
Updated version to 1.1.2 since the change is minor
contrib/kubernetes/datahub/README.md
Outdated
| @@ -4,16 +4,6 @@ A Helm chart for LinkedIn DataHub | |||
|
|
|||
| Current chart version is `0.1.1` | |||
There was a problem hiding this comment.
Version needs to be upgraded as per chart.
List of changes:
Checklist