🌱 Add items to cache immediately after apply #12877
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XS
lentzi90
force-pushed
the
lentzi90/ssa-cache-after-apply
branch
from
5e9c698 to
83a129c
Compare
k8s-ci-robot
added
needs-rebase
lentzi90
force-pushed
the
lentzi90/ssa-cache-after-apply
branch
from
83a129c to
3f0d275
Compare
k8s-ci-robot
removed
the
needs-rebase
lentzi90
changed the title
internal/util/ssa/patch.go
Outdated
| options.Cache.Add(requestIdentifier) | ||
| // If the object changed, we need to recompute the request identifier before caching. | ||
| if options.Original.GetResourceVersion() != modifiedUnstructured.GetResourceVersion() { | ||
| requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), modifiedUnstructuredBeforeApply, modifiedUnstructured) |
There was a problem hiding this comment.
| requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), modifiedUnstructuredBeforeApply, modifiedUnstructured) | |
| requestIdentifier, err = ComputeRequestIdentifier(c.Scheme(), modifiedUnstructured, modifiedUnstructuredBeforeApply) |
We have to use the resourceVersion from modifiedUnstructured (it contains at this point the response from the server after Apply including the latest resourceVersion) and the hash from modifiedUnstructuredBeforeApply because this is what the next reconcile will pass into the func. modifiedUnstructured will contain all fields of the resource, not just the ones that our client wants to set
There was a problem hiding this comment.
Please check why the unit test didn't detect that this didn't cache correctly and see if that can be improved that it would have detected it
(I'll skip review of the unit test for this review as I"m not sure how much it will change)
There was a problem hiding this comment.
Good catch, I'll check!
There was a problem hiding this comment.
Fixed. I got confused because of the argument names in ComputeRequestIdentifier. It takes in original and modified, so I wanted to put the "before apply" as original. 😵 .
Anyway, this makes me wonder how useful the test really is. I just did the same there to calculate the request identifier. At least the test will catch if the code is changed...
There was a problem hiding this comment.
Yup, let's improve as suggested in the Slack thread.
The names are indeed not great. Not sure if I have ideas for just better names, I would have some for also changing the type of the first parameter but that might have some larger impact on other usages of this util
(so probably let's keep the ComputeRequestIdentifier func as is for now)
This improves the SSA helper by avoding a second no-op patch just to add items to the cache. Instead we calculate the new request identifier and add to the cache directly. Signed-off-by: Lennart Jern <[email protected]>
lentzi90
force-pushed
the
lentzi90/ssa-cache-after-apply
branch
from
3f0d275 to
2355648
Compare
3 participants
What this PR does / why we need it:
Add items to cache immediately after apply to avoid extra no-op patch just for caching.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Part of #12291