-
Notifications
You must be signed in to change notification settings - Fork 77
Add proposal for selective pod metadata patching #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add proposal for selective pod metadata patching #196
Conversation
f9fb7b4 to
1bbe02c
Compare
This proposal introduces support for in-place patching of Pod metadata (labels and annotations) without requiring rolling restarts. Key changes proposed: - Add PodDiff utility class to categorize changes as NONE, METADATA_ONLY, or REQUIRES_RESTART - Add patchPodMetadata() helper method to StrimziPodSetController - Modify maybeCreateOrPatchPod() to use PodDiff for detecting changes - Use Strategic Merge Patch for metadata-only changes This addresses the TODO at StrimziPodSetController.java line 466 and improves operational efficiency by avoiding unnecessary Pod restarts when only labels or annotations change. Signed-off-by: Said BOUDJELDA <[email protected]>
1bbe02c to
54dc7aa
Compare
|
@scholzj This is the proposal for selective metadata, a first version of the implementation is on strimzi/strimzi-kafka-operator#12386 |
scholzj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this references the PR, I will share what I shared there as this needs to be addressed:
- Doesn't this remove annotations and labels added directly on the Pods by the user (or other components in the user's cluster)? I think we cannot afford it. So how do you figure out how to update the metadata? I think this is a very important aspect.
- The change to the Pod revision includes changes to the Metadata and you do not seem to have changed how the revision is generated. So I'm not sure I understand how you figure out whether the spec changed or whether only metadata changed.
- How do you prevent race conditions between the rolling update mechanism and the dynamic updates?
- In general, the Strimzi code would do the diffing differently through JSON/YAML. Should this mechanism from this PR be used, I think it should use the same logic as other parts of the code. However, we moved away from diffing Pods because it does not work well with all kinds of injected and default values. We started using the revisions instead. The main reason for it is the first point here.
- I think this implementation would not be compatible with #194 (that said, that proposal might suggest the way to do the dynamic metadata updates, assuming we figure out the first point here)
|
|
||
| 2. **Improved user experience**: Users often want to add custom labels or annotations for various operational purposes (e.g., cost tracking, monitoring integration). These changes should be seamless. | ||
|
|
||
| 3. **Better alignment with Kubernetes practices**: Kubernetes itself allows in-place patching of Pod metadata. Strimzi should leverage this capability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it does not. It allows you to add labels or annotations. But have you tried to edit deployment and see what happens? Rolling update.
|
|
||
| This means that adding a new annotation to a Pod (e.g., for monitoring or tracing purposes) will cause an unnecessary Pod restart and potential service disruption. | ||
|
|
||
| ## Motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is really a motivation or a use-case. That proper motivation or use-case would by why do you need to change the metadata so often that maintaining the in-place updates makes sense. That is a start. Without a convincing use-case, the proposal is not very interesting.
This proposal introduces support for in-place patching of Pod metadata (labels and annotations) without requiring rolling restarts. Key changes proposed: