Skip to content
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

Propagate HasSynced properly #113985

Merged
merged 1 commit into from Dec 15, 2022
Merged

Propagate HasSynced properly #113985

merged 1 commit into from Dec 15, 2022

Conversation

lavalamp
Copy link
Member

@lavalamp lavalamp commented Nov 18, 2022 •
edited

Fix #113763 by:

  • Fix informers to track HasSynced properly
  • propagate HasSynced to each Handle individually
  • Add AsyncTracker to help multi-worker controllers track whether they've synced or not

/kind bug

What this PR does / why we need it:

Plumbs the fact of whether an item was in the initial list or not to controllers, where they can choose to track whether they have processed it or not.

Shared informers now correctly propagate whether they are synced or not. Individual informer handlers may now check if they are synced or not (new HasSynced method). Library support is added to assist controllers in tracking whether their own work is completed for items in the initial list (AsyncTracker).

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Nov 18, 2022
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.26 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.26.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Nov 17 21:25:15 UTC 2022.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 18, 2022
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 18, 2022
@lavalamp lavalamp mentioned this pull request Nov 18, 2022
Closed
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 18, 2022
@lavalamp
Copy link
Member Author

OK shockingly this is now a local change that doesn't break the world!

I'd like feedback on the names before I fix all the comments?

@k8s-ci-robot k8s-ci-robot added area/apiserver and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 19, 2022
@lavalamp
Copy link
Member Author

I'm gonna be out next week, so this will be on hold for a bit unless I feel especially inspired

@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 19, 2022
@lavalamp
Copy link
Member Author

(I was feeling inspired, all this needs now is more tests and a use of the AsyncTracker. And to be squashed.)

howardjohn
howardjohn reviewed Nov 21, 2022
View reviewed changes
Copy link
Contributor

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Tested this out with some of the rests from the original issues, passes with handler.HasSync checked 👍 👍

@lavalamp
Copy link
Member Author

Great news, then! We can probably get this ready to go next week when I'm back.

@eahydra
Copy link

eahydra commented Nov 22, 2022 •
edited

Wow, looking forward to this PR being merged. If possible, consider cherry-picking client-go v1.22 and later. 🎉🎉🎉 👍🏻👍🏻👍🏻

@lavalamp lavalamp changed the title [WIP] Show one way to improve HasSynced Propagate HasSynced properly Nov 28, 2022
@deads2k
Copy link
Contributor

deads2k commented Dec 14, 2022

The interface lgtm now, the logic looks fine. I'm concerned about how we'll know if we've done something incorrect with the sync tracker without any error and without a panic. This code is used to ensure to synchronization of resources used for server safety and as it stands now, if we had a significant bug, we could improperly report "synchronized==true" without any reporting or failure. I think that panicking is a better outcome in that case than silently being unsafe in a way that we have no detection for.

@deads2k
Copy link
Contributor

deads2k commented Dec 14, 2022

/lgtm
/hold

holding for other reviewers.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 14, 2022
@lavalamp
Enable propagration of HasSynced
8100efc 
* Add tracker types and tests
* Modify ResourceEventHandler interface's OnAdd member
* Add additional ResourceEventHandlerDetailedFuncs struct
* Fix SharedInformer to let users track HasSynced for their handlers
* Fix in-tree controllers which weren't computing HasSynced correctly
* Deprecate the cache.Pop function
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2022
@lavalamp
Copy link
Member Author

squashed/rebased

@liggitt
Copy link
Member

liggitt commented Dec 14, 2022

/approve
for vendor/modules.txt change

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, lavalamp, liggitt

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 Dec 14, 2022
@lavalamp lavalamp mentioned this pull request Dec 14, 2022
Closed
@lavalamp
Copy link
Member Author

/retest

1 similar comment
@lavalamp
Copy link
Member Author

/retest

@lavalamp
Copy link
Member Author

This is a concerning amount of flakes, but they don't appear related to this PR.

/retest

@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2022
@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2022

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit 843b40a into kubernetes:master Dec 15, 2022
SIG Node CI/Test Board automation moved this from Archive-it to Done Dec 15, 2022
SIG Node PR Triage automation moved this from Triage to Done Dec 15, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Dec 15, 2022
@alexzielenski alexzielenski mentioned this pull request Dec 16, 2022
Merged
@lavalamp lavalamp mentioned this pull request Jan 4, 2023
Merged
@liggitt liggitt mentioned this pull request Feb 1, 2023
Open
@twz123 twz123 mentioned this pull request Mar 10, 2023
Merged
@Huang-Wei Huang-Wei mentioned this pull request Nov 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexzielenski alexzielenski alexzielenski approved these changes

@howardjohn howardjohn howardjohn left review comments

@eahydra eahydra eahydra left review comments

@liggitt liggitt liggitt left review comments

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@deads2k deads2k deads2k left review comments

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@liggitt liggitt

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Done
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

informers: provide a way to know when a resource handler has "synced"
10 participants
@lavalamp @k8s-ci-robot @eahydra @fedebongio @dims @deads2k @liggitt @alexzielenski @howardjohn @stevekuznetsov