Preserve deployment and its template labels and annotations #16199
Conversation
f079377 to
7632241
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16199 +/- ##
=======================================
Coverage 80.05% 80.06%
=======================================
Files 215 215
Lines 13322 13330 +8
=======================================
+ Hits 10665 10672 +7
+ Misses 2297 2296 -1
- Partials 360 362 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
LGTM, but I'd like @dprotaso to take a look as well. |
|
/remove-approve Was added automatically as I'm co-release lead. |
|
/ok-to-test |
|
/hold Will revisit after the release next Tuesday |
|
Hi @dprotaso , please take another look, thanks! |
pkg/reconciler/revision/cruds.go
Outdated
| desiredDeployment.Spec.Template.Labels = kmeta.UnionMaps(deployment.Spec.Template.Labels, have.Spec.Template.Labels) | ||
| desiredDeployment.Spec.Template.Annotations = kmeta.UnionMaps(deployment.Spec.Template.Annotations, have.Spec.Template.Annotations) |
There was a problem hiding this comment.
One thing I learned from working on - https://github.com/knative/serving/pull/16186/files
If we union the have map with the desired one then we end up losing the ability to delete things.
Thus we should filter out labels/annotations we control from the have map.
I think we can just remove *.knative.dev and we also have an app label (though I don't expect us to remove that)
Everything else should be preserved. See my other PR as a starting point - there we only care about specific autoscaling labels/annotations for the pod autoscaler.
There was a problem hiding this comment.
Thanks for the review!
I also thought about that but couldn't come up with a scenario where that would happen, as we (AFAIK) basically always create new deployments/revisions on changes.
It still makes sense though to be bit more defensive, I'll update the code.
7632241 to
1376cb1
Compare
|
I just rebased the PR, PTAL 🫶 |
|
/retest |
When running `kubectl rollout restart deployment`, Kubernetes adds the `kubectl.kubernetes.io/restartedAt` annotation to the deployment's pod template to trigger a rolling restart. However, Knative's reconciler was removing this annotation by completely replacing the deployment spec, causing new pods to be immediately terminated instead of completing the rollout. This change preserves externally-added metadata at both the deployment level and template level during reconciliation by filtering Knative-controlled keys (*.knative.dev and app label) from the current state before merging with the desired state. This ensures Knative maintains full control over its own metadata while preserving external additions.
1376cb1 to
dff5d18
Compare
|
/hold cancel |
|
/lgtm thanks for the change @linkvt 🎉 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Is this change released or planning to be released soon ? Asking since i cant see new releases in https://github.com/knative/operator/releases or https://knative.dev/docs/reference/relnotes/ |
|
Right now it is only included in main which means it would get released in knative 1.21. I'll trigger a cherry pick to the 1.20 release and let Dave decide whether it is OK to be included. /cherrypick 1.20 |
|
@linkvt: cannot checkout DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherrypick release-1.20 Thats the correct one :) |
|
@linkvt: new pull request created: #16303 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Hi,
when running
kubectl rollout restart deployment, Kubernetes adds thekubectl.kubernetes.io/restartedAtannotation to the deployment's pod template to trigger a rolling restart. However, Knative's reconciler was removing this annotation by completely replacing the deployment spec, causing new pods to be immediately terminated instead of completing the rollout.This change preserves externally-added metadata at both the deployment level and template level during reconciliation:
Fixes #14705
Proposed Changes
Release Note
/kind bug
Thanks for the review!