fix the default value of imagePullPolicy

[ktalg]

Ⅰ. Describe what this PR does

Fixed an issue that caused sidecarset in-place update to not work in some scenarios

Ⅱ. Does this pull request fix one issue?

fix #1270

Ⅲ. Describe how to verify it

just see the unit test

Ⅳ. Special notes for reviews

The setting of default value of the container's imagePullPolicy in sidecarSet is affected by the image tag , which makes it possible to change the value of SidecarSetHashWithoutImage even if only the image tag is modified, thus making the in-place upgrade of the container ineffective.

[kruise-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zmberg for approval by writing /assign @zmberg in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kruise-bot]

Welcome @ktalg! It looks like this is your first PR to openkruise/kruise 🎉

[sonatype-lift]

goimports: File is not goimports-ed

---

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command	Usage
@sonatype-lift ignore	Leave out the above finding from this PR
@sonatype-lift ignoreall	Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool>	Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

[codecov-commenter]

Codecov Report

Merging #1275 (61251ce) into master (7f5046d) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
- Coverage   48.45%   48.43%   -0.02%     
==========================================
  Files         149      149              
  Lines       20627    20634       +7     
==========================================
  Hits         9994     9994              
- Misses       9537     9544       +7     
  Partials     1096     1096              

Flag	Coverage Δ
unittests	48.43% <0.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...arset/mutating/sidecarset_create_update_handler.go	22.00% <0.00%> (-3.59%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[zmberg]

@ktalg Thank you very much, I have tried to understand this issue. I feel that the core of this problem is that modifying the Image also changes the ImagePullPolicy field, which prevents in-place upgrades from continuing. This is indeed quite strange, but we cannot fix it in this way, because if the user has really made changes to the ImagePullPolicy field, we cannot simply reuse the old value.

If we don't use the SetDefaults_Container function, it's not very reasonable. My opinion right now is to explicitly declare the ImagePullPolicy field to avoid this issue.

[ktalg]

@ktalg Thank you very much, I have tried to understand this issue. I feel that the core of this problem is that modifying the Image also changes the ImagePullPolicy field, which prevents in-place upgrades from continuing. This is indeed quite strange, but we cannot fix it in this way, because if the user has really made changes to the ImagePullPolicy field, we cannot simply reuse the old value.

If we don't use the SetDefaults_Container function, it's not very reasonable. My opinion right now is to explicitly declare the ImagePullPolicy field to avoid this issue.

This pr does cause some logical changes, but I don't think it causes the problem you are talking about, because it is a setting of the default value of ImagePullPolicy, which only takes effect when it is empty (just like the setting of the default value of other fields, which only works on empty values).Of course, this behavior does not take effect for newly created objects

If the user does not explicitly set the ImagePullPolicy when updating (not creating) the object, then we do not treat the "not set" action of this field as "set to empty" but rather we need to set This field needs to be set to a reasonable value(again, just like any other field), I think it should be the old value and not the value determined by the image tag (which raises unexpected cases and is not reasonable)

(If I'm missing something, please point it out)

[zmberg]

@ktalg I'm just thinking, it seems a bit strange to me. Additionally, we have changed the default value for ImagePullPolicy in k8s, and I'm not sure if that could bring up new issues.

[ktalg]

@ktalg I'm just thinking, it seems a bit strange to me. Additionally, we have changed the default value for ImagePullPolicy in k8s, and I'm not sure if that could bring up new issues.

After confirming that this change is work, let me clarify the possible undesired side effects it can cause and the scenarios where it happens:

1. the user changes the container of the sidecarSet after the sidecar has been injected: changes the image tag (from latest to a specific version) and sets the ImagePullPolicy to empty, when this value does not change with the image tag.
2. After changing the sidecarSet in the above scenario and scale up the pod, the ImagePullPolicy in the new pod does not change with the image tag

The conditions for the above to happen are so harsh that it seems we can assume that the user would not expect ImagePullPolicy to be affected by the image tag - the above side effect should not have happened

[zmberg]

@ktalg You have considered things very comprehensively, but my concerns are as follows:

1. The imagePullPolicy actually affects the SidecarSet hash, and for existing SidecarSets, there may be unexpected changes. The SidecarSet hash is a very important field, not only for new additions but also for existing ones. I haven't fully thought through the implications for existing SidecarSets, and I'm concerned about the potential risks involved.
2. Is there really such a high frequency of scenarios where sidecar image switch from "latest" to a specific tag? Would the benefits and risks of doing this be proportional? If the likelihood of this happening is small, can users fix it by rebuilding Pods or changing the Pod annotations hash?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.