feat(#29): set imagePullPolicy: Always #30
Conversation
There was a problem hiding this comment.
Cool initiative @dianabarsan. Thanks! Left a comment inline.
| @@ -33,6 +33,7 @@ spec: | |||
| - name: API_PORT | |||
| value: '5988' | |||
| image: {{ .Values.upstream_servers.docker_registry }}/cht-api:{{ .Values.cht_image_tag }} | |||
| imagePullPolicy: Always | |||
There was a problem hiding this comment.
Instead of setting the value here directly, we should add a variable in values and pull from there.
Something like adding to the values.yaml
imagePullPolicy: IfNotPresent # default value
The reason for this is that for production deployments images should have an immutable tag with the cluster pulling only if it's not present. Otherwise, it causes issues like increasing egress costs and network activity/load on the cluster. Always is okay for CI/CD and for dev or debugging deployments.
There was a problem hiding this comment.
I'm afraid we will run into a situation where we will need this in production too, where some prod instance is on a branch build because they needed some urgent hotpatch, and we can't upgrade them.
We don't do upgrades often enough to have frequent image pulls?
There was a problem hiding this comment.
I'm also reluctant to be so opinionated in the values file. Someone would need to have knowledge about what the consequences are for changing this value, and choose from either a preset (and no make any typos) or it would be a true/false that we would convert to the preset in the template.
I'm not convinced there is much gain from this.
We already know:
- we need this in dev
- we have production instances that are on branch builds when they need urgent hotpatches
There was a problem hiding this comment.
I'm also reluctant to be so opinionated in the values file. Someone would need to have knowledge about what the consequences are for changing this value
Folks could even leave out the key from the values.yaml and helm will apply the defaults you originally build it with in this PR.
I'm afraid we will run into a situation where we will need this in production too, where some prod instance is on a branch build because they needed some urgent hotpatch, and we can't upgrade them.
If we need to make an urgent patch to the production deployment, these should be rare and one-off and therefore editing the deployment object in k8s is the way to go.
We have things in the values.yml that we expect to change rarely. This setting could sit next to those. I think the main thing here though is that for production deployments we need to stick to the principle that images should have an immutable tag.
There was a problem hiding this comment.
Otherwise, we can't have deterministic deployments in production and we'd have to introduce image digests which I believe would be an overkill and very much like to avoid.
There was a problem hiding this comment.
deterministic deployments
When would the images be pulled, in normal operations?
There was a problem hiding this comment.
If the policy is set to Always, that could happen if the pod restarts for whatever reason. Not just at the time of install.
…lways-pull-images # Conflicts: # charts/cht-chart-4x/templates/haproxy-deployment.yaml
|
@dianabarsan we also need to set this version to be the default one to use - here. |
|
wow. ok... I think we should just remove this part, we shouldn't need a core release to get the correct helm charts. |
Fixes not being able to upgrade to a newer build of the same branch.
#29