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.

Sign up for GitHub

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

Jump to bottom

feat(#29): set imagePullPolicy: Always #30

Merged
merged 5 commits into from
Nov 4, 2024
Merged

feat(#29): set imagePullPolicy: Always #30

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24fb19b
set imagePullPolicy: Always
dianabarsan Oct 31, 2024
c96fc6d
Code review feedback
dianabarsan Nov 1, 2024
5e4c6ea
adds values variable
dianabarsan Nov 1, 2024
013ec66
Merge remote-tracking branch 'origin/29-always-pull-images' into 29-a…
dianabarsan Nov 1, 2024
182026a
Bumps chart version.
dianabarsan Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/api-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ spec:
- name: API_PORT
value: '5988'
image: {{ .Values.upstream_servers.docker_registry }}/cht-api:{{ .Values.cht_image_tag }}
imagePullPolicy: Always
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deterministic deployments

When would the images be pulled, in normal operations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the policy is set to Always, that could happen if the pod restarts for whatever reason. Not just at the time of install.

name: cht-api
ports:
- containerPort: 5988
Expand Down
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/couchdb-n-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ spec:
containers:
- name: cht-couchdb-{{ $nodeNumber }}
image: {{ $root.Values.upstream_servers.docker_registry }}/cht-couchdb:{{ $root.Values.cht_image_tag }}
imagePullPolicy: Always
ports:
- containerPort: 5984
env:
Expand Down
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/couchdb-single-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ spec:
- name: SVC_NAME
value: couchdb.{{ .Values.namespace }}.svc.cluster.local
image: {{ .Values.upstream_servers.docker_registry }}/cht-couchdb:{{ .Values.cht_image_tag }}
imagePullPolicy: Always
name: cht-couchdb
ports:
- containerPort: 5984
Expand Down
3 changes: 2 additions & 1 deletion charts/cht-chart-4x/templates/haproxy-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ spec:
value: "5984"
- name: HEALTHCHECK_ADDR
value: healthcheck.{{ .Values.namespace }}.svc.cluster.local
image: {{ .Values.upstream_servers.docker_registry }}/cht-haproxy:{{ .Values.cht_image_tag }}
image: {{ .Values.upstream_servers.docker_registry }}/cht-haproxy:{{ .Values.cht_image_tag }}\
imagePullPolicy: Always
dianabarsan marked this conversation as resolved.
Show resolved Hide resolved
name: cht-haproxy
ports:
- containerPort: 5984
Expand Down
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/healthcheck-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ spec:
name: cht-couchdb-credentials
key: COUCHDB_USER
image: {{ .Values.upstream_servers.docker_registry }}/cht-haproxy-healthcheck:{{ .Values.cht_image_tag }}
imagePullPolicy: Always
name: cht-haproxy-healthcheck
resources: {}
ports:
Expand Down
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/sentinel-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ spec:
- name: API_PORT
value: '5988'
image: {{ .Values.upstream_servers.docker_registry }}/cht-sentinel:{{ .Values.cht_image_tag }}
imagePullPolicy: Always
name: cht-sentinel
resources: {}
restartPolicy: Always
Expand Down
1 change: 1 addition & 0 deletions charts/cht-chart-4x/templates/upgrade-service-deployment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ spec:
serviceAccountName: cht-upgrade-service-user
containers:
- image: medicmobile/upgrade-service:{{ .Values.upgrade_service.tag }}
imagePullPolicy: Always
name: upgrade-service
resources: {}
env:
Expand Down