v1.9.0 has already introduced a new field attribute VolumeDevices for Container, which is causing the hash value get changed.

/cc @yujuhong

This wouldn't help much. If you read the old issue #23104 completely, you'd realize that if you introduce a new field in the container spec with a non-nil default value, the hash would still be different.

I do think this behavior (container restarts) should be call out in the release note.

I don't understand how the serialization/deserialization helps the issue

I don't understand how the serialization/deserialization helps the issue

@liggitt By serialization/deserialization, the new obj removes nil ptr and empty slice from the struct, which will keep the to-be-hashed obj unchanged even with new ptr and array type added.

Currently the container hash will get changed if struct Container is appended/updated with a new field. This is making the container restarted when upgrading to a new version, where a new field is added to Container struct.

This is why we should omit nil or empty field when calculating container hash value.

you'd realize that if you introduce a new field in the container spec with a non-nil default value, the hash would still be different.

@yujuhong Yes, right. IMO, if we change the default behavior, we should let the container get restarted. That means the hash got changed is desired.

What this PR is going to do is to omit nil or empty new/old fields when calculating container hash. For new-added non-nil or non-empty filed, it is inevitable to let the hash changed.

Currently the container hash will get changed if struct Container is appended/updated with a new field. This is making the container restarted when upgrading to a new version, where a new field is added to Container struct.
This is why we should omit nil or empty field when calculating container hash value.

I still don't understand. All the pods the kubelet deals with should be obtained from deserializing API responses or manifests from disk. How does reserializing and deserializing again help change the container the kubelet deals with?

All the pods the kubelet deals with should be obtained from deserializing API responses or manifests from disk.

Right.

How does reserializing and deserializing again help change the container the kubelet deals with?

@liggitt Everytime kubelet gets restarted, the container hash is calculated to identify whether it needs restarted. What this PR is trying to fix is to keep the hash value consistent as far as possible to avoid un-needed restarts. This PR won't change the pod spec and container spec.

By reserializing and deserializing again to a new obj (map[string]interface{}), nil or empty field is omitted when calculating container hash value. This will let the hash value unchanged even new field with nil/empty value is appended to the Container struct.

Currently v1.9.0 introduces a new field attribute VolumeDevices for Container, which is causing the hash value get changed when upgrading cluster < v1.9.0.

By reserializing and deserializing again to a new obj, nil or empty field is omitted when calculating container hash value.

Can you describe how the kubelet loaded this v1 container into memory from an api response or a manifest in a way that would result in different content than if it were roundtripped back to json and then back into memory?

it seems like the issue is less with nil fields and more with the way the hash is being computed. This change will not prevent a new defaulted field from changing the hash, and seems like it would guarantee that hashes would change between 1.9 and 1.10, which isn't great.

Do we not save the serialized container spec we had when we launched the container? If we did, we could load it, apply defaults, and do a semantic deepequal to see if there was a diff that required restarting. Working to detect actual differences seems better than trying to improve the hashing approach which still leaves many unnecessary restart scenarios in place.

This change will not prevent a new defaulted field from changing the hash

Right.

Do we not save the serialized container spec we had when we launched the container?

We do save the hash value in container labels, but not the serialized container spec.

If we did, we could load it, apply defaults, and do a semantic deepequal to see if there was a diff that required restarting.
Working to detect actual differences seems better than trying to improve the hashing approach which still leaves many unnecessary restart scenarios in place.

@liggitt Makes sense. But this will not gonna be a small change.

1. Marks current hash annotation deprecated; May takes 2 releases to remove it?

2. Adds new label, such as containerSerializedSpecLabel, to save serialized container spec; Load and apply the defaults for deep equal.

3. Backwards compatible. For an older container without containerSerializedSpecLabel in labels, how can we identify whether it should be restarted.

   • Still follow the old behavior, finding old containerHashLabel? If hash value matches, no need to restart. Currently the container labels cannot be updated (Proposal: Update labels  moby/moby#21721) without a restart. This means the new label containerSerializedSpecLabel won't be enforced.
   • Enforce label containerSerializedSpecLabel directly. If this label is missing, simply restart it and append this new annotation.

I prefer the second option. @liggitt Any suggestions?

ping @kubernetes/sig-node-api-reviews Needs your opinions. Thanks.

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

/remove-lifecycle stale

/assign @yujuhong

As the PR is tagged for 1.16, is it still planned for this release?

@xmudrii Yes, still targeted to v1.16.

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Ping @liggitt for review. Thanks.

the way I'd probably test this is to take your new HashContainer method, and compute a hash for a stubbed v1.Container in 1.15, then compare that to the same stubbed container in 1.16. The prior implementation would differ because new fields were added to the struct. The new implementation should remain the same since the json serializations would not have changed

rather than committing the copies of types.go files, I'd just record the computed hash for a given pod spec in 1.14 and 1.15 and test against the same HEAD pod spec

@liggitt Please take another review. Thanks.

@dixudx: The following tests failed, say /retest to rerun them all:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, liggitt

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

The pull request process is described here