✨ Refresh cache entries on cache hit in SSA caches#13459
✨ Refresh cache entries on cache hit in SSA caches#13459k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
This PR is currently missing an area label, which is used to identify the modified component when generating release notes. Area labels can be added by org members by writing Please see the labels list for possible areas. DetailsInstructions 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. |
k8s-ci-robot
added
the
size/XS
sbueringer
added
area/util
k8s-ci-robot
added
the
cncf-cla: yes
|
/test pull-cluster-api-e2e-blocking-main |
sbueringer
force-pushed
the
improve-ssa-caching
branch
from
eb7cf59 to
b119620
Compare
k8s-ci-robot
added
the
size/XS
sbueringer
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/test pull-cluster-api-e2e-blocking-main |
internal/util/ssa/cache.go
Outdated
| const ( | ||
| // ttl is the duration for which we keep the keys in the cache. | ||
| ttl = 10 * time.Minute | ||
| // The ttl is a multiple of the syncPeriod to ensure we have a few chances |
There was a problem hiding this comment.
| // The ttl is a multiple of the syncPeriod to ensure we have a few chances | |
| // The ttl is a multiple of the default syncPeriod to ensure we have a few chances |
May be as a follow up / future work we might try to introduce a direct link between the two values, but not required at this stage (I'm not aware of people changing default syncPeriod nor with any issue with cache ttl)
There was a problem hiding this comment.
Yeah, it's not ideal. On the other side I also didn't want to allow users to fine-tune this cache TTL (either directly with a flag or indirectly via sync period)
There was a problem hiding this comment.
(added to the umbrella issue)
| // Note: We only add an identifier to the cache if the result of the dry run was no diff. | ||
| if exists := dryRunCtx.ssaCache.Has(requestIdentifier, dryRunCtx.originalUnstructured.GetKind()); exists { | ||
| // Refresh the cache entry so we don't have to execute the dry-runs again after the cache TTL. | ||
| dryRunCtx.ssaCache.Add(requestIdentifier) |
There was a problem hiding this comment.
(Q: not directly related to this change, but related on how we use ssaCache)
We when there is no item in cache, and we detect there is a diff after the dry run call, we are not adding an item in cache.
I assume this because at this stage (after a dry run call) we don't know what the target resource version will be after the diff is applied, so we have to wait for another dry run call before adding the new version to the item to the cache.
Now, the question is: Can we avoid the second dry call by adding items to cache after Patch?
There was a problem hiding this comment.
I assume this because at this stage (after a dry run call) we don't know what the target resource version will be after the diff is applied, so we have to go a second dry run call before adding to the cache.
Three's also no guarantee that the Patch call succeeds.
Now, the question is: Can we avoid the second dry call by adding items to cache after Patch?
I don't know if the assumption that after a Patch the next Patch wouldn't change anything is safe.
(because I don't know if the fact that we did a patch is equivalent to all the dry-run logic determining that another patch wouldn't lead to a diff)
There was a problem hiding this comment.
But I assume it's not really worth further optimizing, the additional dry-runs that we could safe should be negligible compared to that we are not going to do any further applies once the object is stable.
For all the cases where further applies are necessary we wouldn't save anything. Only for the cases where everything becomes stable we save some calls
There was a problem hiding this comment.
On top of that, consider:
- object resourceVersion: 1
- Cluster topology reconcile starts
- someone else modifies the object resourceVersion: 2
- Cluster topology controller patches, resourceVersion: 3
If we would now cache based on RV 3 we would miss whatever might have changed in RV 2
(Maybe that's not a problem, but I think the potential benefit is not worth the risk. Considering that we now cache ~ forever we should really not make any mistakes when adding a cache entry)
There was a problem hiding this comment.
Ok, may be let's keep track of the idea in the umbrella issue
There was a problem hiding this comment.
Added to the umbrella issue
Signed-off-by: Stefan Büringer buringerst@vmware.com
sbueringer
force-pushed
the
improve-ssa-caching
branch
from
b119620 to
4348956
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: c2e380d7dba1063bab241abab00a3f60b1e3fe43 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
k8s-ci-robot
added
the
approved
3 participants
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #13305