Shouldn't we just use "}" ? This might make the return value a bit weird. 🤔

I changed code, var msg string = "}" is compared with msg = fmt.Sprintf("Pod resize complete, {") + msg

I understand that this is not a correct way to initialize. Can we use like [] string to get msg and Join operation at the end instead of this string concatenation?

Thanks for your comments, I change the string definition.

Rather than tracking this here, let's just make ClearPodResizeInProgressCondition return whether a condition was cleared. Then the logic just becomes:

if kl.statusManager.ClearPodResizeInProgressCondition(pod.UID) {

@tallclair do you still want the event emitted when pending resize is reverted? If so, just checking that the pod ResizeInProgress condition is cleared is not enough (since pending resizes are tracked in the other condition). To handle that I think we'd need to move this line to the beginning of the function and unconditionally check if the entire resize status has been cleared at the end of the defer block (which is what I was suggesting in #130387 (comment))

(referencing your comment #127172 (comment))

@tallclair @natasha41575

Thanks for your comments.

1. I agree with natasha's comment, just checking pod ResizeInProgress condition is cleared is not enough, but this has nothing to do with "event emitted when pending resize is reverted". If only check if kl.statusManager.ClearPodResizeInProgressCondition(pod.UID)，after resize completed，resize complete event will be emitted continually every time when SyncPod is called. Because func handlePodResourcesResize defer block will check isPodResizeInProgress even after pod resize is completed, this is not what we want. We just want to emit complete event once resize is finished.

2. I think current code has satisfied this requirement, emit complete event when pending resize is reverted.
   Pending resize is reverted, first both InProgress and Pending condition are cleared，then set InProgress condition，after resize complete，clear InProgress condition( emit complete event). The detailed is as below.

• When pending
  ->SetPodResizePendingCondition
• pending reverted
  -> ClearPodResizePendingCondition/ClearPodResizeInProgressCondition(func handlePodResourcesResize)
  -> SetPodResizeInProgressCondition (func (kl *Kubelet) SyncPod)
  -> ClearPodResizeInProgressCondition(pod.UID) (defer func() in func handlePodResourcesResize )

Please correct me if there is any error.

@tallclair
Could you please give your comment？

FYI @ndixita

I'm torn on the type for this. The map[string]v1.ResourceRequriements is a more compact json representation, but it doesn't preserve container order. The alternative would be something like

InitContainers []ContainerResourceSummary
Containers []ContainerResourceSummary

// ...

type ContainerResourceSummary struct {
  Name string
  Resources v1.ResourceRequirements
}

I'm leaning toward the latter, since it would basically present as a filtered view of the pod spec, rather than a completely new type, if that makes sense.

I get your consideration, applied it to the new code

I disagree with this. The resize might not have been "actuated", but a waiting container can still be resized - it will use the newly allocated values when it starts up.

I delete this condition judgment in the new code, so that even the containerStatus is nil, but container resource in the completed message is displayed.

Why put this in annotations rather than the event message? If it's part of the message, we don't need to worry about the timestamp.

Alternatively, if it's too much information to dump in the message, maybe we could put the allocated generation in the message instead of the timestamp.

I put it to annotations to refer this comment, but annotations do not update if event message content is same, so added a time stamp to distinguish different times of resize completed event.
#127172 (comment)

But add a timestamp in the event message is indeed strange.
In the new code, I deleted timestamp and moved detailed information from annotations to the message.

+1 to this. I think it should have its own dedicated test.

adding this here just so it doesn't get lost but make sure you get an answer from @tallclair on #130387 (comment) on the desired behavior

We are holding off on merging #131157 for the time being, meaning I wouldn't expect the code that backfills the status manager conditions from there to be merged any time soon. If we want to move forward here, can you copy the condition backfilling into this PR? Basically this section

(This is a follow-up to my other comment: #130387 (comment))

#130387 (comment)

@natasha41575, for this comment #130387 (comment)
I merged backfills status manager conditions code to here, is my understanding correct？