Skip to content

✨ Introduce & use wait for cache utils #12957

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 6, 2025
Merged

✨ Introduce & use wait for cache utils #12957

merged 2 commits into from
Nov 6, 2025
+754 −216

Conversation

@sbueringer
Copy link
Member

@sbueringer sbueringer commented Nov 5, 2025

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 #12291

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2025
@sbueringer sbueringer force-pushed the pr-wait-for-cache-utils branch from 0395f91 to f95110e Compare November 5, 2025 14:54
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2025
@sbueringer sbueringer added the area/misc Issues or PRs not related to any other area label Nov 5, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Nov 5, 2025
@sbueringer sbueringer force-pushed the pr-wait-for-cache-utils branch 3 times, most recently from f94f7dd to fd6def0 Compare November 6, 2025 05:55
@sbueringer sbueringer changed the title [WIP] 🌱 Introduce wait for cache utils 🌱 Introduce & use wait for cache utils Nov 6, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2025
@sbueringer sbueringer changed the title 🌱 Introduce & use wait for cache utils ✨ Introduce & use wait for cache utils Nov 6, 2025
@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

/assign @fabriziopandini @chrischdi

@sbueringer sbueringer force-pushed the pr-wait-for-cache-utils branch from fd6def0 to 9c4e58b Compare November 6, 2025 06:16
@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

/test pull-cluster-api-e2e-main-gke

alexander-demicev
alexander-demicev reviewed Nov 6, 2025
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2025

LGTM label has been added.

Git tree hash: 2b7c1e59dd5aa9241ac7a5651d97e04000be3f52

fabriziopandini
fabriziopandini reviewed Nov 6, 2025
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have this util!
Just a few nits from my side

internal/util/client/client.go
// WaitForCacheToBeUpToDate waits until the cache is up-to-date in the sense of that the cache contains
// all passed in objects with at least the passed in resourceVersion.
// This is done by retrieving objects from the cache via the client and then comparing resourceVersions.
// Note: This func will update the passed in objects while polling.
Copy link
Member

@fabriziopandini fabriziopandini Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason for this behaviour?
I'm a little bit worried that this could bring in some unexpected change that could invalidate assumptions on the object state that the following code relies on

(I also saw in some places you are deep copying the object before calling this method)

Copy link
Member Author

@sbueringer sbueringer Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use DeepCopy only in once place and that is in the topology controller for cluster as it is extremely hard on pre-existing code like that to figure out what changes if I change that.

In general I think this is the right thing to do. Usually the code is used like this

  • obj in state 1
  • ssa.Patch(obj)
  • afterwards => obj is already updated to state 2 (the response from the apiserver)
  • WaitForCacheToBeUpToDate
  • afterwards => obj is updated to state 3 which has resourceVersion >= of state 2

If I don't do this any subsequent code might be running with a stale object and I don't think that's a good idea.

If the feature will be embedded in the CR client we would get the same behavior as there is no reason to knowingly keep using a stale object

tl;dr this matches the behavior of any client write call that we do immediately before calling this func

Copy link
Member

@fabriziopandini fabriziopandini Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see the point in aligning to the future CR behaviour, we can always deep copy when calling the func

Copy link
Member Author

@sbueringer sbueringer Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I think the one case where we deepcopy will also remain the exception. Going forward we will just not deepcopy.

It's just hard to figure out what effects it might have if I change it considering all the code that is running after we update the cluster

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

Thx everyone, PTAL :)

@stmcginnis
Copy link
Contributor

stmcginnis commented Nov 6, 2025

Looks good!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2025

LGTM label has been added.

Git tree hash: a2087241fcfdc6c08c024d290c300e90cdf6e633

@fabriziopandini
Copy link
Member

fabriziopandini commented Nov 6, 2025

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2025

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2025
@sbueringer sbueringer force-pushed the pr-wait-for-cache-utils branch from 15e510f to e379253 Compare November 6, 2025 15:27
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

/test pull-cluster-api-e2e-main-gke

@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

Had to push another very small change, forgot to adjust the expected error message in one unit test

fabriziopandini
fabriziopandini reviewed Nov 6, 2025
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2025

LGTM label has been added.

Git tree hash: d059807b84ee471f52f24833dc2f41ebfe99e841

@stmcginnis
Copy link
Contributor

stmcginnis commented Nov 6, 2025

Oops. :)

/lgtm

@sbueringer sbueringer force-pushed the pr-wait-for-cache-utils branch from e379253 to f897573 Compare November 6, 2025 15:46
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@sbueringer
Copy link
Member Author

sbueringer commented Nov 6, 2025

And of course a merge conflict, rebased :)

/test pull-cluster-api-e2e-main-gke

@fabriziopandini
Copy link
Member

fabriziopandini commented Nov 6, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2025

LGTM label has been added.

Git tree hash: 3ad356412a4a183f8e2f3694ef4fc3761a0279af

@stmcginnis
Copy link
Contributor

stmcginnis commented Nov 6, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit f3be4e2 into kubernetes-sigs:main Nov 6, 2025
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Nov 6, 2025
@sbueringer sbueringer deleted the pr-wait-for-cache-utils branch November 6, 2025 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardcase richardcase Awaiting requested review from richardcase

@sivchari sivchari Awaiting requested review from sivchari

@alexander-demicev alexander-demicev Awaiting requested review from alexander-demicev

@chrischdi chrischdi Awaiting requested review from chrischdi

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@chrischdi chrischdi

@stmcginnis stmcginnis

@fabriziopandini fabriziopandini

@alexander-demicev alexander-demicev

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/misc Issues or PRs not related to any other area cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

v1.12

Development

Successfully merging this pull request may close these issues.

6 participants

@sbueringer @k8s-ci-robot @stmcginnis @fabriziopandini @alexander-demicev @chrischdi