[BUG] CloneSet creates new pods even if in-place update is possible

[amirakhmedov]

What happened:
Given a CloneSet with InPlaceIfPossible update strategy and maxSurge set. If you update the annotation in the pod template which should trigger in-place update for the running pods but this is not happening. It creates new pods configured in the maxSurge and in-place updates the remaining pods.

What you expected to happen:
I expect all pods to be in-place updated.

How to reproduce it (as minimally and precisely as possible):
Create a CloneSet with InPlaceIfPossible update strategy and maxSurge

Cloneset

apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  labels:
    app: sample
  name: sample
spec:
  replicas: 5
  selector:
    matchLabels:
      app: sample
  updateStrategy:
    type: InPlaceIfPossible
    maxSurge: 3
  template:
    metadata:
      labels:
        app: sample
      annotations:
        tracking-number: one
    spec:
      containers:
        - name: nginx
          image: nginx:alpine

Update the pod template to update the annotation

annotations:
  tracking-number: two

You can observe that 3 new pods are created, 3 old pods are deleted and the remaining 2 pods are in-place updated.

Anything else we need to know?:

Environment:

• Kruise version: v1.8.0
• Kubernetes version (use kubectl version): Client Version: v1.35.0, Server Version: v1.35.0
• Install details (e.g. helm install args): helm install kruise openkruise/kruise --version 1.8.0
• Others:

[pradhyum6144]

@FillZpp may i know are you working on this issue if not can i work on this.

[pradhyum6144]

/assign

[amirakhmedov]

Hi @FillZpp, I was looking at this and created a draft pull request which fixes the issue. If the described behavior in the issue is not expected then I would be happy to contribute the fix PR.

[Kunal1522]

u can link the pr to this issue @amirakhmedov .this would help in auto closing the pr

[initor]

👋 @amirakhmedov , thanks for identifying this bug and putting together #2370 👍

I took a slightly different approach in my PR #2377 for the same issue. The core fix is identical (gate surge on whether in-place update is possible), but instead of passing inplaceupdate.Interface and *apps.ControllerRevision into calculateDiffsWithExpectation, I pre-resolve a canInPlaceUpdate bool at the call sites and pass that in. The main motivation was to keep the diff calculation function pure, it's described in the codebase as "the most important algorithm in cloneset-controller," so I wanted to minimize the surface area of the change.

Happy to hear thoughts from maintainers on which direction they prefer. Either way, credit to you for surfacing the issue and the initial fix. 🙏