Change imagePullPolicy to Always

[ozhuraki]

These patches change imagePullPolicy to Always in base deployments
and demos as discussed in #400.

In case a specific imagePullPolicy is required, it can be provided
through the relevant kustomization.yaml.

Closes #400.

[mythi]

@ozhuraki the operator code needs changes too: pkg/controllers/*/controller.go

I'd also want the policy documented somewhere because it has implications to users especially in today's environment with Docker's image pull request limit.

Perhaps add a sub-section to top-level README under Plugins:

Pre-built plugin images
Pre-built images of the plugins are available on the Docker hub. These images are automatically built and uploaded to the hub from the latest master branch of this repository.

Release tagged images of the components are also available on the Docker hub, tagged with their release version numbers in the format x.y.z, corresponding to the branches and releases in this repository.

**Note:** the default deployment files and are operator are configured with [imagePullPolicy](https://kubernetes.io/docs/concepts/containers/images/#updating-images) Always`

[ozhuraki]

@mythi

Thanks a lot for the help, done.

[codecov-io]

Codecov Report

Merging #504 (1c86a58) into master (09d213d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #504   +/-   ##
=======================================
  Coverage   56.70%   56.70%           
=======================================
  Files          30       30           
  Lines        2028     2028           
=======================================
  Hits         1150     1150           
  Misses        807      807           
  Partials       71       71           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09d213d...084bff8. Read the comment docs.

[mythi]

LGTM

[bart0sh]

It would be nice to have instructions for resetting policy to IfNotPresent for development purposes.
I'm using my locally built images quite often to debug FPGA plugin. This change would make it harder to do as far as I understood.

[mythi]

It would be nice to have instructions for resetting policy to IfNotPresent for development purposes.

From what I'm reading, kustomize is hard for this. We could have a developer script similar to scripts/set-version.sh and a make target OR we turn this the other way around and make it IfNotPresent by default and document how to use, e.g., AlwaysPullImages admission controller

[ozhuraki]

@mythi @bart0sh

[...] we turn this the other way around and make it IfNotPresent by default and document how to use, e.g., AlwaysPullImages admission controller

It looks that this might be more suitable.

[rojkov]

From what I'm reading, kustomize is hard for this.

I thought kustomize is designed exactly for this kind of task. What's wrong with it? And the controllers can be parameterized to use different policy when instantiating DaemonSets.

[mythi]

I thought kustomize is designed exactly for this kind of task. What's wrong with it?

It's hard if all the images need the overlay maintained. If we are fine just documenting how to add the overlay (and leave all the debugging to the end user), it's probably easier.

[bart0sh]

+1 for leaving IfNotPresent default.

Implicit upgrades caused by "Always" can be surprising for users in a negative sense I guess.

[rojkov]

IMHO for release images it's safer to use Always from security POV. They are not supposed to have drastic changes in API by definition.

For the master branch I'd prefer IfNotPresent as a developer. Looks like I've just made the situation a bit more complicated :)
Perhaps extending set-version.sh to update the policy for a release would suffice.

[mythi]

Perhaps extending set-version.sh to update the policy for a release would suffice.

For the controllers too?

[bart0sh]

IMHO for release images it's safer to use Always from security POV.

I have mixed feelings about it. From one side you're right, it's more secure. However, I'd prefer to avoid implicit things and let cluster admins decide when then want to upgrade their software.

[bart0sh]

At least we seem to agree to use IfNotPresent policy for master branch. This is good for development and testing.

[pohly]

IMHO for release images it's safer to use Always from security POV. They are not supposed to have drastic changes in API by definition.

With "release images", do you mean tagged images? Those should be immutable, so Always just adds unnecessary overhead and requests to Docker Hub...

My two cents: use imagePullPolicy: IfNoPresent and then make it easy for developers to get something deployed with imagePullPolicy: Always. That's what we do in PMEM-CSI when deploying: https://github.com/intel/pmem-csi/blob/5c1f2b8ef2f6a8367b562a46315b290644ee2abe/test/setup-deployment.sh#L118

It's using sed here for historic reasons, but kustomize would also work.

[bart0sh]

@pohly when do you use image policy Always in PMEM-CSI?

[rojkov]

Alright, if release images are immutable that simplifies everything.

[ozhuraki]

@mythi @bart0sh @rojkov @pohly

OK, left our present imagePullPolicy intact, added a script for changing it and updated the doc.

Please take a look.

[pohly]

when do you use image policy Always in PMEM-CSI?

It's used when developers deploy PMEM-CSI via the test script. Normal users deploy either via YAML or operator and both use the default "IfNotPresent".

[mythi]

I'm OK with the script approach and the default setting.

Just one comment: IfNotPresent was causing problems for some users who had an old :devel locally and it did not match with they were deploying from yamls. Also debugging #461 was problematic due to this too.

To get new users avoid any of these problems, perhaps we should more clearly point them to use the release tagged images.

[mythi]

@ozhuraki some .yamls are missing the imagePullPolicy setting:

deployments/operator/manager/manager.yaml
deployments/gpu_plugin/base/intel-gpu-plugin.yaml (initContainer, check from gpu controller too)

[pohly]

Just one comment: IfNotPresent was causing problems for some users who had an old :devel locally and it did not match with they were deploying from yamls. Also debugging #461 was problematic due to this too.

I understand the motivation, but I think that changing the image policy to address this particular problem is just whacking one mole. Normal users should never use "devel" artifacts, whether it's images or YAML files.

[ozhuraki]

@mythi

some .yamls are missing the imagePullPolicy

OK, thanks, added.

[...] check from gpu controller too

Looks that in the controllers' code the ImagePullPolicy is present everywhere (except in patchermanager_test, which is probably the intent there):

pkg/fpgacontroller/patcher/patchermanager_test.go
74:					Image: "test-image",
94:					Image: "test-image",

pkg/controllers/gpu/controller.go
119:							Image:           devicePlugin.Spec.Image,
120:							ImagePullPolicy: "IfNotPresent",
184:			Image:           imageName,
185:			ImagePullPolicy: "IfNotPresent",

pkg/controllers/fpga/controller.go
106:							Image:           devicePlugin.Spec.Image,
107:							ImagePullPolicy: "IfNotPresent",
133:							Image:           devicePlugin.Spec.InitImage,
134:							ImagePullPolicy: "IfNotPresent",

pkg/controllers/qat/controller.go
96:							Image:           devicePlugin.Spec.Image,
97:							ImagePullPolicy: "IfNotPresent",

pkg/controllers/sgx/controller.go
108:							Image:           devicePlugin.Spec.InitImage,
109:							ImagePullPolicy: "IfNotPresent",
126:							Image:           devicePlugin.Spec.Image,
127:							ImagePullPolicy: "IfNotPresent",

[eero-t]

Kubernetes default for tags is "IfNotPresent", "Always" is used by default for "latest" and when there's no tag: https://kubernetes.io/docs/concepts/containers/images/#updating-images

Only moving tags (like "devel") need explicit "Always" policy, but why device plugins would use such things instead of "latest" which already implies has that?

Immutable (version) tags are fine with the default policy, it doesn't need to be explicitly set.

[mythi]

but why device plugins would use such things instead of "latest" which already implies has that?

we don't have :latest tag on purpose: to give clear indication that there are only released versions and a development version