Skip to content

fix(reporter):added idempotency gate to prevent API server flooding#263

Open
LightCreator1007 wants to merge 4 commits into
kubernetes-sigs:mainfrom
LightCreator1007:fix/has-taint-by-spec-value
Open

fix(reporter):added idempotency gate to prevent API server flooding#263
LightCreator1007 wants to merge 4 commits into
kubernetes-sigs:mainfrom
LightCreator1007:fix/has-taint-by-spec-value

Conversation

@LightCreator1007
Copy link
Copy Markdown

Description

Adds an idempotency gate to updateNodeCondition in the readiness-condition-reporter.
UpdateStatus is now bypassed if the Status, Reason, and Message are unchanged, preventing unnecessary API server flooding and etcd write amplification on every tick.

Related Issue

NONE

Type of Change

/kind bug

Testing

  • Modified main_test.go locally to assert against the fake.Clientset's tracked actions, confirming that the UpdateStatus call is definitively bypassed when the condition state is unchanged.

Checklist

  • make test passes
  • make lint passes

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 20, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented May 20, 2026

Deploy Preview for node-readiness-controller canceled.

Name Link
🔨 Latest commit ea637a9
🔍 Latest deploy log https://app.netlify.com/projects/node-readiness-controller/deploys/6a0edb147d99f70008e9451a

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LightCreator1007
Once this PR has been reviewed and has the lgtm label, please assign dchen1107 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 20, 2026
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

Hi @LightCreator1007. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 20, 2026
@sahitya-chandra
Copy link
Copy Markdown
Contributor

sahitya-chandra commented May 20, 2026

Hey @LightCreator1007, PR looks good but the diff only touches main.go, so the skip path isn't covered. Could you add a test where the node already has the matching condition and assert no update is sent?

Also, a comment noting that skipping the write stops refreshing the heartbeat (so it no longer signals the reporter is alive), would be good

Comment thread cmd/readiness-condition-reporter/main.go
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 21, 2026
@LightCreator1007
Copy link
Copy Markdown
Author

skip path isn't covered. Could you add a test where the node already has the matching condition and assert no update is sent

Yes!
I have added the same in the newer commits to cover both the idempotency and the 5 min refresh cases :)

Thanks for the help!!

@Priyankasaggu11929
Copy link
Copy Markdown
Member

I ran the reporter locally with a 10s check interval, to track how many writes actually went to the node.

from my testing, it's one write, per tick (10s interval), per node.

For a component that exists to periodically report node health, that's exactly what you would expect.

So, in my opinion, it is not a problem to optimise away with 1 write/tick. Is there a specific scale scenario motivating this?

@LightCreator1007
Copy link
Copy Markdown
Author

I ran the reporter locally with a 10s check interval, to track how many writes actually went to the node.

from my testing, it's one write, per tick (10s interval), per node.

For a component that exists to periodically report node health, that's exactly what you would expect.

So, in my opinion, it is not a problem to optimise away with 1 write/tick. Is there a specific scale scenario motivating this?

Hi @Priyankasaggu11929 , thanks for testing this!

While 1 update per 10 seconds is fine locally, the project targets a scale of say 5,000 nodes.

At 5,000 nodes, a 10-second interval would create 500 requests per second hitting the server just for health reporting. Processing this constant stream of "no change" updates may quickly overwhelm the API server.

To prevent this, the reporter should cache its state locally and only send an update to the API server when its health status actually changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants