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

refactor policy admission Validator to be lock free #114527

Merged
merged 2 commits into from Jan 17, 2023
Merged

refactor policy admission Validator to be lock free #114527

merged 2 commits into from Jan 17, 2023

Conversation

alexzielenski
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

for cel admission controller introduced as alpha in 1.26 we have possible contention on a RWLock between calls to Validate (called for every operation on a resource), and the controller reconcilers.

This refactor adjusts the design so that there is no data sharing between the Validate function and the controller loop, and that Validate takes no locks in its implementation - it now relies on a pre-baked list of policy definitions provided to it by a worker thread via atomic.Value.

This PR accomplishes this by first separating all the reconciliation logic out into a separate policyController. Then we can spawn a worker thread which polls the policyController up to every 1s for a reconstructed list of definitions, if it has changed.

/sig api-machinery
/cc @cici37 @jpbetz

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@alexzielenski
defer Done call
517df8f 
safer in case of panic
@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 16, 2022
@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Dec 16, 2022
@alexzielenski
Copy link
Contributor Author

/triage accepted
/hold

@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. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 16, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Dec 16, 2022
@alexzielenski alexzielenski force-pushed the apiserver/policy/lockfree-refactor branch from 3ac8349 to ded7f2f Compare December 16, 2022 23:41
@alexzielenski alexzielenski changed the title [WIP] refactor policy admission Validator to be lock free refactor policy admission Validator to be lock free Dec 17, 2022
@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 Dec 17, 2022
@alexzielenski alexzielenski force-pushed the apiserver/policy/lockfree-refactor branch from ded7f2f to 536fb5a Compare December 17, 2022 01:56
alexzielenski
alexzielenski commented Jan 12, 2023
View reviewed changes
Copy link
Contributor Author

@alexzielenski alexzielenski left a comment •
edited

Choose a reason for hiding this comment

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

/hold for cool synctrack.Lazy to merge which will remove polling from this

EDIT: on second thought, not sure this is a good application of that tool.

sync track.Lazy would have Get synchronize if the value was invalidated. We instead want the latest payload to be "delivered" ready to be used to the threads performing validation, and avoid validation threads from taking a lock and waiting on each other.

I think the current solution is good as-is

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go Show resolved Hide resolved
jpbetz
jpbetz reviewed Jan 12, 2023
View reviewed changes
Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

Concurrency looks right to me. cachedPolicies are set and invalidated behind a lock but are safely copied (1s interval) to an atomic for lock free access during validation.

The introduction of policyController cleans things up substantially.

.../src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go Outdated Show resolved Hide resolved
.../src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go Outdated Show resolved Hide resolved
@alexzielenski
Copy link
Contributor Author

/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 Jan 13, 2023
@alexzielenski
refactor admission controller to avoid contention
5f59f44 
refresh admission policies up to once per second based upon last known good data
@alexzielenski alexzielenski force-pushed the apiserver/policy/lockfree-refactor branch from 8233fd1 to 5f59f44 Compare January 13, 2023 22:52
@jpbetz
Copy link
Contributor

jpbetz commented Jan 17, 2023

/lgtm

Thanks for fixing this up @alexzielenski!

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

LGTM label has been added.

Git tree hash: e4f07d06e9e7ff437aa0d3cb841dfe5c0d70620c

@k8s-ci-robot k8s-ci-robot merged commit 7e09238 into kubernetes:master Jan 17, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Jan 17, 2023
@alexzielenski alexzielenski mentioned this pull request Jul 20, 2023
Merged
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz left review comments

@maxsmythe maxsmythe maxsmythe left review comments

@cici37 cici37 Awaiting requested review from cici37

Assignees

@jpbetz jpbetz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alexzielenski @k8s-ci-robot @jpbetz @maxsmythe