As @lavalamp suggested in #23104, instead of hash the whole container spec, can we maintain a list of container fields of different releases that should be taken into consideration during hash, and identify the hash values with release prefix?
The field list may looks like:

release1: {field1, field2...fieldN}
release2: {field1, field2...fieldN, fieldN+1}

and imagine the kubelet has been upgraded to release2 (new container field fieldN+1 added), when walking through the pods, if it found a container has a hash with a prefix of "release1", then it knows the container should be hashed with the fields of release1, thus, the result hash would not change.

And if the users have an update on a pod or its deployment, then the pod is expected to be recreated, thus, the fields of new release begin to take effect on its containers.

The only downside I can think of is that we might need to maintain the field list manually (e.g deprecate the field of old releases).

can we maintain a list of container fields of different releases that should be taken into consideration during hash

That means we have to hard code this. I don't like this way.

As I mentioned in #57741, how about adding a new label, like containerSerializedSpecLabel , with backwards compatibility supported as well.

As I mentioned in #57741, how about adding a new label, like containerSerializedSpecLabel, with backwards compatibility supported as well.

We don't have field level versioning for kubernetes api today, so it won't help (kubelet itself has no idea which version a field is introduced or changed).

If we have to go with the field label approach, adding a version indicator might help:
For example

type Container struct {
    foo string `containerSerializedSpecLabel:"since v1.9"`
    bar string `containerSerializedSpecLabel:"since v1.10"`
}

Then, foo will be included for checking container hash like "v1.9-xxxxxxxx"
foo and bar will be included for checking container hash like "v1.10-xxxxxxxx"

TBO, I don't see many benefits than having a container fields list const some where in pkg/kubelet,(e.g. in pkg/kubelet/container/helpers.go)

Another problem with comparing hashes across versions is changes to the hash function.

Similar issues have come up in workload controllers, where they are used for collision avoidance only, for these types of reasons.

Persisted hashes shouldn't be used as the sole determination of equality, since there are multiple reasons why they may change across releases.

If a hash comparison fails, one needs to fall back to actually comparing properties that matter.

cc @janetkuo @kow3ns

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

/remove-lifecycle stale

In order to fix this issue, I suppose PR #57741 help to omit nil/empty filed from now on, even new filed was added in the future. I will prevent restarting container continues. we need to make decision to close this kind of issue since 1.8.x -> 1.9.x, 1.11.x -> 1.12.x and so on.
It does not help for migration immediately to omit nil/empty filed, but if it go into 1.16.x from now on, it will help on future upgrade from 1.16.x -> 1.17.x and so on.

Is this a proper time to re-consider this issue, find a one-time fix?

We still struggle from container restart when trying to upgrade our 1.15.x cluster, backport #57741 won't help since itself would bring a hash change.
And #57741 can't deal with new added filed with default value(and maybe delete desperated field in the furture). Nil default proposed in kubernetes/community#4571 may help fix this issue forever with #57741, but it seems discussion moved to another approach(use generation or server side apply), which can't apply in this situation.

containerSerializedSpecLabel approach @liggitt @dixudx proposed in #57741 (comment) make sense to me. Backwards compatibility can be done with serialize the current container spec and deserialize into a legacy container spec which we previously stored for compatibility. This will guarantee a smooth upgrade.

cc @thockin who may help with some input.

Changing the kubelet mechanism on a minor version boundary is acceptable, because pods are expected to be drained on kubelet minor version upgrade.

Updated the title and description to clarify the impact of the current logic and why it should be changed

xref discussion in #95587 (comment)

my naive recommendation would be to limit the container hash to specific fields that are known to be immutable or are expected to be able to change (maybe just name and image?)

limit the container hash to specific fields that are known to be immutable or are expected to be able to change

✅ 💯

If kubelet doesn't know how to react to a field change, it must not be included in the hash.

In-place upgrades add some restrictions:

• The hash must be versioned so that a future kubelet which knows how to react to additional field changes doesn't do an unnecessary restart. (it has to be able to reproduce the hashes of the old kubelet.)
• A new hash version must not be used until there's no chance of rolling back to an old kubelet version which doesn't understand it.

(the above goes for controllers that perform this operation, too)

maybe just name and image

And resource limits, if those would require a restart.

In-place upgrades add some restrictions:

• The hash must be versioned so that a future kubelet which knows how to react to additional field changes doesn't do an unnecessary restart. (it has to be able to reproduce the hashes of the old kubelet.)
• A new hash version must not be used until there's no chance of rolling back to an old kubelet version which doesn't understand it.

Specifically for kubelets, if the change is made on a minor version boundary, I don't think we need those protections, given sig-node's position that in-place (minor version) kubelet upgrades without draining is not supported.

(the above goes for controllers that perform this operation, too)

Yes, for controllers, which have to deal with persisted ControllerRevisions and annotations on API objects made by earlier versions of the controller, they definitely have to consider the approach taken by previous versions of the controller. I plan to spawn similar issues to track resolving this for controllers

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

it looks like the in-place resource change expanded the surface area of this issue in f2bd94a by adding a second hash that is tracked now - HashWithoutResources

I had hoped handling mutable resources would prompt fixing this issue :-/

cc @smarterclayton @vinaykul for visibility to the problems with detecting container changes using hashes of API content

Revisiting this as we look at more and more Pod fields which cannot add default values. This forces us to treat nil as the default value, which means all code that consumes these fields has to know about this. It's frankly awful.

This is not unique to kubelet, it's just particularly acute.

We could convert this to a list of specific fields to hash, but that still feels brittle. Shouldn't the API server have a way to help with this? Either managedFields should have a way to say "this was defaulted" or we should have a way to request the object without defaulted fields or we should have a way to know what the default values are and strip them out or something.

@jpbetz @deads2k

/sig api-machinery
/triage accepted

We could convert this to a list of specific fields to hash, but that still feels brittle

The kubelet has to do stuff to set up containers correctly, it has to know which fields could change and how much it has to do to correctly handle the changes. Hashing the immutable fields that assert identity (name,namespace,uid,containerName) and the effective values of the fields that it handles mutations to seems correct to me.

Sure, but it's just another place that needs to be touched if any other field becomes mutable, which could be forgotten. I'm not saying not to fix this with urgency, just that I want to consider paths towards less open-coded lists of things to hash.

After PR #124220 is merged, I think this issue should be closed. If there are still any problems, we can reopen this issue.
/close

@HirazawaUi: Closing this issue.

After PR #124220 is merged, I think this issue should be closed. If there are still any problems, we can reopen this issue. /close

@HirazawaUi If you have bandwidth, please run a manual upgrade test if this is not covered in CI somewhere. This change is important but also feels a bit risky.

After PR #124220 is merged, I think this issue should be closed. If there are still any problems, we can reopen this issue. /close

@HirazawaUi If you have bandwidth, please run a manual upgrade test if this is not covered in CI somewhere. This change is important but also feels a bit risky.

I'll try to do this in my spare time :)