Added default ObjectMetadata and maps #4160
Conversation
|
SonarCloud Quality Gate failed. |
|
It's not easy to review on the GH interface, I'll try to check later on in the IDE. My main concerns are about how this might affect certain PATCH requests (depending on the strategy), since submitting empty collection or map values will wipe the field in the server. This can be mitigated with the serialization configuration, but that will also affect users who use our serializer to dump their yaml files. 🙂 🙃 You provide a partial explanation for this in your description, but I'm not grasping it completely. |
|
So the GH interface finally loaded....
This is what I mentioned, it might cause issues for existing users who serialize their model instances. |
Try looking at the first and last commit in isolation. The second commit I just checked every modification in - but that was a lot more than I was expecting.
With these changes empty maps or default ObjectMetadata are omitted from the serialized form. Just like with arrays that means our logic won't distinguish null or an empty map either. The ramification for this in terms of patching are just a couple of cases - a default ObjectMetadata or an explicitly empty map. On the former I can't think of a case where you need to something like metadata: {} - that will always be invalid. On the latter since our merge and strategic merge with an item are additive only there's no difference between labels: {} and having labels omitted. A JSON patch via a diff will understand an empty map to mean full removal - I don't believe that will be a problem as that is generally the behavior of the api server. If you for example set labels to an empty map, the object that comes back from the server has labels removed - not with an empty map. There was an older issue with the opposite problem - that even though you set a map to null, an empty map was serialized, which caused an issue with CRDs because the api server didn't consider the case of an empty map. So the only time there will be a problem is if some resource, potentially custom, that is purposefully trying to distinguish between null and empty maps (if there were such a thing we'd likely have hit it already with our array handling). |
|
I managed to check the changes, I think you missed my comment #4160 (comment) As I comment here, we have 2 big breaking changes:
From my point of view, these changes only benefit a certain group users. There are other alternatives to be considered: For example users can customize the global ObjectMapper so that collections are always deserialized as empty |
No that is addressed with my last comment.
That is noted in the migration doc.
Please see my previous comment, that is not how it functions. If you do something like: It will add / modify the annotation. If you use an empty map: No modification is made - it does not remove the annotations. That's the case as well with strategic merge.
The motivation for this change isn't the serialization behavior, but rather not having to do pervasive null checks. |
|
For the YAML dumping, if it's documented, I think it's good enough.
🤦 My bad, for some reason (probably heat wave 🥵) I was considering labels and annotations as JSON arrays. This would be the operation to clear the annotations, which wasn't possible before either: Just to make yet another remark 😅, I'm not sure how this change might affect Quarkus and its performance. |
Yes we mentioned in the past that maybe a specialized builder could help there - new Pod().withNewMetadata().withFinalizers(null).endMetadata().asPatch() - that could explicitly track if null was set. Otherwise we need everything to be held by some kind Value object that can differentiate between default null and explicit null...
You mean because of the map creation? I'd be fine if we defaulted the arrays and maps to a smaller size, even 0. However once a builder is involved, it will use default sized maps / lists. For serialization performance there will be a hit for objectmetadata - it will need to compare to a new instance. |
Yes. Not because of the map size allocation, but more in general, by the number of allocated objects. I'm not sure if that might affect applications which deal with huge loads of Kubernetes Resources (you're the expert in this area).
This too. Again, I think you have the experience dealing with these kind of applications, I trust your opinion and expertise on this. |
I hadn't checked in a while, but setting the capacity to 0 doesn't help that much - the underlying collection storage won't be initialized until the first modification. So other than the hit on the heap for the primitives and object points, you are not greatly increasing the total object count. If you modify the object using a builder or using the default collection that we're creating then you'll end up with something that has the default capacity - so that's not much different than most usage today. More typically the references you hold in memory will be the ones that are deserialized from the server, those derserialized collections will already have their capacity = size.
Per serialization this will be an additional allocation of ObjectMeta and then the invocation of the equals method. I'm okay with that overhead to not have to do null checks on the metadata. The only way to improve upon this is to do more jackson overrides - related to what was being proposed in some of the conversion of the serialization logic to non-static - you can customize the HandlerInstantiator to use a cached ObjectMeta. But given how heavy weight a serialization already is, I'm not too worried about a little bit of additional garbage. |
It's not a great workaround, but I realized we do have an approach to this in object form: You could imagine instead updating the setter generation so that explicit nulls were also additional properties (and non-null values would remove the explicit null in the additional properties). |
| @@ -56,7 +57,8 @@ public class GenericKubernetesResource implements HasMetadata { | |||
| @JsonProperty("kind") | |||
| private String kind; | |||
| @JsonProperty("metadata") | |||
| private ObjectMeta metadata; | |||
| @JsonInclude(value = Include.CUSTOM, valueFilter = ObjectMeta.class) | |||
There was a problem hiding this comment.
Is this supposed to ignore this field when the ObjectMeta is "blank"-> metadata.equals(new ObjectMeta()) ?
|
@shawkins : I have a minor question. Why have we committed |
|
@rohanKanojia "The second commit was all the modifications after running the generation" - by that I mean running kubernetes-model-generator/generateModel.sh and checking in all of the changes. I did not try to determine if anything could / should be excluded under the assumption that it would already have an ignore. |
|
FYI these changes break completely Eclipse JKube fragment merging capabilities. JKube relies on deserialized fragments metadata to be null to add default processing and so on. There might be some other issues when checking if a Map is null instead of empty (these ones should be easier to fix). I think we can sort of fix the merging problem for JKube (without being very hacky). But I'm not sure if this might negatively affect other users too. I think this might mess up some of Dekorate/Quarkus visitors/decorators too @iocanel |
|
@manusa do you want to back out the change to the ObjectMeta? I'd like to keep the map change as that is consistent with arrays and is the main focus of the original issues. |
The Map change is not problematic (or shouldn't be). Our problem has more to do with Fragments (incomplete resource definitions which usually lack the metadata field), and how those are deserialized and merged with whatever opinionated resources we create. This merge process might or might not happen depending on the metadata field (e.g. a user might want to define more than one Service, Volume, and so on; so one of the fragments might be eligible for merging and not the other) Ioannis said that most probably they wouldn't be affected. It definitely causes some pain for us. I'm not sure if other users might be affected by this too. But as you can see, something that in appearance seems completely harmless, is in fact a main breaking point for us. My idea to mitigate it is to convert any Do you think the added benefits of no null-checking the metadata field are worth it? |
It's marginal and at this point a one-off as it's the only required child object that is getting defaulted, so I'm fine if we don't pursue it. There was some existing inconsistency wrt metadata prior to this change - CustomResource was already defaulted with an ObjectMetadata. |
Let's remove it then. We needed to create the follow-up issue either way to see if it was interesting to default the required fields with empty instances. We can add ObjectMeta to the mix and seek feedback from users to see if we move forward.
I think CustomResource had/has more inconsistencies |
Description
addresses #3625 more generally at the generator level.
The first commit has just the generation changes. The second commit was all the modifications after running the generation - there seems to be more there than there used to be.
Maps will now be initialized (just like arrays) and omit when empty.
I'm not sure if everyone will agree with this, but ObjectMetadata is treated as a special case. It will be defaulted to an actual instance. It will be omitted if it remains as the default - I don't think this is required for correctness, but it will ensure things like partial patches look as minimal as possible - they won't container metadata: {}
Custom and Generic resource metadata were similarly updated.
Being more general, we could apply the same handling to any KubernetesResource which is known to be required - but I didn't see that was captured in the metadata yet.
Type of change
test, version modification, documentation, etc.)
Checklist