tofieldset: Add tests to show it already allow duplicates #249
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| _P("list", _KBF("key", "a", "id", 1), "key"), | ||
| _P("list", _KBF("key", "a", "id", 1), "id"), | ||
| _P("list", _KBF("key", "a", "id", 1), "nv"), | ||
| _P("list", _KBF("key", "a", "id", 2)), |
There was a problem hiding this comment.
cc @jpbetz
This code is mostly irrelevant because we only use ToFieldSet on objects that are applied, and we should never allow apply to have duplicates (now, that being said, I'm curious if this whole change even makes sense?). I still wanted to illustrate that when someone will update with duplicates items, we will just give them ownership of the associative key and that's it, as a way to indicate that they own that key multiple times. WDYT?
There was a problem hiding this comment.
One drawback of this, but I don't know if we can solve it anyway, is that if someone applies one item, and then it gets updated into two items, that item won't be pruned when it's removed from the applied configuration, meaning it can no longer be removed with server-side apply. I think that's reasonable.
There was a problem hiding this comment.
This code is mostly irrelevant because we only use ToFieldSet on objects that are applied, and we should never allow apply to have duplicates (now, that being said, I'm curious if this whole change even makes sense?)
If apply can never have duplicates why are we making SMD ToFieldSet tolerate duplicates? How do Updates get the list of fields to own if not via ToFieldSet?
There was a problem hiding this comment.
If apply can never have duplicates why are we making SMD ToFieldSet tolerate duplicates?
Context, we're trying to solve #234
For built-ins, the listType=map is used a little loosely, and we don't have a great way to address that, except for accepting some duplicates. We still can't apply duplicates, but right now you can't apply an objects that has these duplicates.
How do Updates get the list of fields to own if not via ToFieldSet?
By using "Compare", they own the fields that they changed, since we don't know exactly what they intended to own.
There was a problem hiding this comment.
We still can't apply duplicates, but right now you can't apply an objects that has these duplicates.
Is ToFieldSet invoked on the final merged object, or the partial patch? If it is invoked on the partial patch aren't there no duplicates and this PR has no effect?
There was a problem hiding this comment.
Correct, that's more or less what I said in the PR body:
It's not super relevant because no one should ever be able to apply duplicates
But:
- This is for consistency, if we agree that this is how ownership of things that are duplicates, we should probably apply it everywhere, even if it's not relevant here.
- This is the first time we use that new pattern in the code so I want to make sure we're fine with it.
|
/hold I'm trying a new test with pruning fields that are removed by appliers and this doesn't work, looking into this. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel Thought about this further, I think it's fine, unless I forgot the use-case I had in mind ... |
k8s-ci-robot
removed
the
do-not-merge/hold
| _P("list", _KBF("key", "a", "id", 1), "key"), | ||
| _P("list", _KBF("key", "a", "id", 1), "id"), | ||
| _P("list", _KBF("key", "a", "id", 1), "nv"), | ||
| _P("list", _KBF("key", "a", "id", 2)), |
There was a problem hiding this comment.
This code is mostly irrelevant because we only use ToFieldSet on objects that are applied, and we should never allow apply to have duplicates (now, that being said, I'm curious if this whole change even makes sense?)
If apply can never have duplicates why are we making SMD ToFieldSet tolerate duplicates? How do Updates get the list of fields to own if not via ToFieldSet?
| _P("list", _KBF("key", "a", "id", 1), "key"), | ||
| _P("list", _KBF("key", "a", "id", 1), "id"), | ||
| _P("list", _KBF("key", "a", "id", 1), "nv"), | ||
| _P("list", _KBF("key", "a", "id", 2)), |
There was a problem hiding this comment.
if someone applies one item, and then it gets updated into two items, that item won't be pruned when it's removed from the applied configuration, meaning it can no longer be removed with server-side apply.
Why wouldn't it be pruned?
There was a problem hiding this comment.
Because when you update to multiple items, it steals the ownership of the whole item and we don't prune things that you don't have ownership over.
There was a problem hiding this comment.
it steals the ownership of the whole item and we don't prune things that you don't have ownership over.
It sounds like you do own it if it steals ownership of the whole item? Do you only have ownership of one copy? Can you share example of the behavior for this. I would expect that you have ownerhsip of the whole duplicated field, and when you submit an UPDATE or PATCH that does not include it, then no one has ownership, and it is pruned.
There was a problem hiding this comment.
Using sets, since it's a little simpler:
Apply, "applier": [1]
Update, "updater": [1, 1] # Updater steals ownership of 1
Apply, "applier": [] # 1 is not pruned because it's owned by updater.
Final object: [1, 1]
There was a problem hiding this comment.
That seems like correct behavior to me. What if it was a map typed list with duplicate keys but unequal objects?
Apply, "applier": [ObjWithKeyA]
Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] # Should be three copies of 'A'?
Apply, "applier": [] # is A pruned?
Whats the final object
There was a problem hiding this comment.
Very good point.
So first of all, Update replaces the whole list, so if you do
Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] # Should be three copies of 'A'?
you end-up with that exact object.
Second, the way this is meant to work, is that if you have duplicate, you own the key and the key only, so here's what happens in your scenario:
Apply, "applier": [ObjWithKeyA] -> applier owns ObjWithKeyA
Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] -> duplicates, updater owns just "KeyA" (none of the fields)
Apply, "applier": [] # is A pruned? no
Whats the final object
[ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] with KeyA owned by updater.
There was a problem hiding this comment.
i think thats reasonable
|
LGTM from me. I think you mentioned you wanted @jpbetz input so will defer tag |
|
@alexzielenski: Closed this PR. In 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/test-infra repository. |
|
@alexzielenski: Reopened this PR. In 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/test-infra repository. |
|
whoops. brain misfire /lgtm |
k8s-ci-robot
added
the
lgtm
Continuation of #247.
This uses the pattern that I've generally used to solve the problem, which is to not try to be too smart about duplicate entries, and just record the path element for the whole thing, rather than try to track all the fields within. It's not super relevant because no one should ever be able to apply duplicates, so they wouldn't really have conflicts with sub-fields anyway, we've decided that they should have a conflict with the idea of "duplicates" (de-duplicating is a conflict).
/assign @alexzielenski