add condition and event info for not upgradable pods when update sidecarset (#1272)

[MarkLux]

Ⅰ. Describe what this PR does

Currently the sidecar container in-place upgrade can only support changing of image field, if user changed other fields, the injected pods will not execute sidecar container upgrade.

However, this rule is not explicit for users, they can not get any hint or info in this situation. This PR add an event for pods, when the injected sidecar changed but cannot be upgrade. If user changed SidecarSet but the pods are not upgraded, they can use kubectl describe sidecarset <sidecarset-name> or kubectl describe pod <pod-name> to see the event and figure out the reason.

Ⅱ. Does this pull request fix one issue?

fixes #1272

Ⅲ. Describe how to verify it

1. create a sidecar set

apiVersion: apps.kruise.io/v1alpha1
kind: SidecarSet
metadata:
  name: demo-sidecarset
spec:
  selector:
    matchLabels:
      app: demo-server
  updateStrategy:
    type: RollingUpdate
  containers:
  - name: sidecar
    image: nginx

2. create some pods with label (here I using a CloneSet), the pods will be injected with sidecar container immediately

apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  labels:
    app: demo-server
  name: demo-server
spec:
  updateStrategy:
    type: InPlaceIfPossible
  replicas: 3
  selector:
    matchLabels:
      app: demo-server
  template:
    metadata:
      labels:
        app: demo-server
    spec:
      containers:
      - name: demo-server
        image: tomcat
        ports:
        - containerPort: 8080

3. update the SidecarSet , change some fields more than image (here I change the image and add a command field)

apiVersion: apps.kruise.io/v1alpha1
kind: SidecarSet
metadata:
  name: demo-sidecarset
spec:
  selector:
    matchLabels:
      app: demo-server
  updateStrategy:
    type: RollingUpdate
  containers:
  - name: sidecar
    image: busybox
    command: [ "/bin/sh", "-c", "touch a.txt && tail -f a.txt"]

4. apply the changed sidecarset, the pods will not be upgraded, using kubectl get pod , we can see the condition info:

- lastProbeTime: null
    lastTransitionTime: "2023-06-19T05:41:13Z"
    message: Pod's sidecar set is not upgradable due to changes out of image field
    reason: UpdateImmutableField
    status: "False"
    type: SidecarSetUpgradable

5. and the SidecarSet got an event for not upgradable pods

Events:
  Type    Reason             Age                From                   Message
  ----    ------             ----               ----                   -------
  Normal  NotUpgradablePods  5s (x5 over 161m)  sidecarset-controller  SidecarSet in-place update detected 2 not upgradable pod(s) in this round, will skip them.

Ⅳ. Special notes for reviews

[kruise-bot]

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

[veophi]

@MarkLux How about considering a condition for such Pod instead of an event? (too many events will oppress ETCD）

[MarkLux]

@MarkLux How about a condition for such Pod instead of an event? (too many events will oppress ETCD）

@veophi

do you mean this?

Conditions:
  Type                 Status
  KruisePodReady       True
  InPlaceUpdateReady   True
  Initialized          True
  Ready                True
  ContainersReady      True
  PodScheduled         True
  SidecarUpgradble       False

But the Condition Field may be too short to show the reason, and it's not obvious. I think after user updated SidecarSet, they need a feedback.

If too many events can make an extra stress to etcd, how about add event for SidecarSet instead of Pods? (thus we only got an single event for each update)

btw, currently when the sidecar upgrade succeeded, an event will be added to all the pods(by kubectl), will this also generate too many events?

[veophi]

If too many events can make an extra stress to etcd, how about add event for SidecarSet instead of Pods? (thus we only got an single event for each update)

btw, if the sidecar upgrade succeeded, an event will be added to all the pods(by kubectl), I wonder if these events can will oppress etcd too?

@MarkLux Successful Update event will be sent only when Pod update successfully. But Failed Update events may be added at Each Reconciling.

How about this condition?

conditions:
- type: SidecarSetUpgradable
   status: False
   reason: UpdateImmutableField
   message: Pod env and volume fields cannot be modified

[MarkLux]

If too many events can make an extra stress to etcd, how about add event for SidecarSet instead of Pods? (thus we only got an single event for each update)
btw, if the sidecar upgrade succeeded, an event will be added to all the pods(by kubectl), I wonder if these events can will oppress etcd too?

@MarkLux Successful Update event will be sent only when Pod update successfully. But Failed Update events may be added at Each Reconciling.

How about this condition?

conditions:
- type: SidecarSetUpgradable
   status: False
   reason: UpdateImmutableField
   message: Pod env and volume fields cannot be modified

got it, I'll change the codes, thanks for your advice

[codecov-commenter]

Codecov Report

Merging #1309 (ffa34ca) into master (6d25366) will increase coverage by 0.24%.
The diff coverage is 63.88%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1309      +/-   ##
==========================================
+ Coverage   48.37%   48.62%   +0.24%     
==========================================
  Files         151      152       +1     
  Lines       21178    21341     +163     
==========================================
+ Hits        10245    10377     +132     
- Misses       9808     9824      +16     
- Partials     1125     1140      +15     

Flag	Coverage Δ
unittests	48.62% <63.88%> (+0.24%)	⬆️

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

Impacted Files	Coverage Δ
pkg/control/sidecarcontrol/util.go	53.48% <ø> (ø)
pkg/controller/sidecarset/sidecarset_processor.go	66.66% <58.51%> (-2.01%)	⬇️
pkg/controller/sidecarset/sidecarset_strategy.go	92.70% <100.00%> (+2.85%)	⬆️

... and 12 files with indirect coverage changes

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

[MarkLux]

@veophi made a draft based on adding condition, could you have a brief view on it? some problems are commented

[veophi]

/lgtm

[veophi]

/remove lgtm

[veophi]

@MarkLux If there are more than one SidecarSets that all select one Pod, you must refresh this condition of the Pod to true only if all the SidecarSets can upgrade the Pod.

You can refer to [the following codes] to fix your logic (

kruise/pkg/util/podreadiness/pod_readiness_utils.go

Line 98 in ffe60f2

func addMessage(base string, msg Message) (bool, messageList) {

[MarkLux]

@MarkLux If there are more than one SidecarSets that all select one Pod, you must refresh this condition of the Pod to true only if all the SidecarSets can upgrade the Pod.

You can refer to [the following codes] to fix your logic (

kruise/pkg/util/podreadiness/pod_readiness_utils.go

Line 98 in ffe60f2

func addMessage(base string, msg Message) (bool, messageList) {

got it

[MarkLux]

@zmberg PTAL

[zmberg]

/lgtm

[zmberg]

/approve

[kruise-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [zmberg]

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