POC: using MutatingAdmissionPolicy to schedule CS pods #1777
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @sdowell, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a proof-of-concept (POC) for leveraging Kubernetes' MutatingAdmissionPolicy to control the node placement of Config Sync system pods. It includes the necessary infrastructure changes to enable this alpha feature in E2E tests, a new comprehensive test case to validate the policy's effect on pod scheduling, and an example policy with accompanying documentation to guide users on its application and limitations.
Highlights
- Enabling MutatingAdmissionPolicy in E2E: Configured Kind clusters in the E2E test suite to enable the alpha
MutatingAdmissionPolicyfeature gate and its corresponding API version (admissionregistration.k8s.io/v1alpha1), along with the necessary API server admission plugins. - New E2E Test for Node Placement: Added a new end-to-end test (
TestMutatingAdmissionPolicy) that demonstrates and validates howMutatingAdmissionPolicycan be used to injectnodeAffinityinto Config Sync system pods. - E2E Test Utilities: Introduced new helper functions (
ValidatePodByLabel) and test predicates (HasExactlyNodeAffinity) to facilitate robust validation of pod properties, specificallynodeAffinity, in E2E tests. - MutatingAdmissionPolicy Example: Provided a concrete example YAML (
config-sync-node-placement.yaml) demonstrating aMutatingAdmissionPolicythat appliesnodeAffinityto pods within theconfig-management-systemnamespace. - Documentation for POC: Added a
README.mdto explain the purpose, usage, and caveats of theMutatingAdmissionPolicyproof-of-concept for Config Sync node placement.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
This change documents a proof-of-concept of configuring the node placement of Config Sync system pods using MutatingAdmissionPolicy. This works out of the box, but is currently an alpha api and is behind a feature gate. It is targeted for beta in k8s 1.34.
There was a problem hiding this comment.
Code Review
This pull request introduces a proof-of-concept for using MutatingAdmissionPolicy to manage node placement for Config Sync pods. The changes are well-structured, including enabling necessary feature gates, adding new test helpers and predicates, and providing an example MutatingAdmissionPolicy configuration with documentation. While the overall approach is good, I recommend improving the reliability of the e2e test by replacing time.Sleep with a more robust polling mechanism, and clarifying the CEL expression in the example MutatingAdmissionPolicy for better user understanding.
| expression: > | ||
| [ | ||
| JSONPatch{ | ||
| op: "add", path: "/spec/affinity", | ||
| value: Object.spec.affinity{ | ||
| nodeAffinity: Object.spec.affinity.nodeAffinity{ | ||
| preferredDuringSchedulingIgnoredDuringExecution: [ | ||
| Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution{ | ||
| weight: 1, | ||
| preference: Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution.preference{ | ||
| matchExpressions: [ | ||
| Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution.preference.matchExpressions{ | ||
| key: "another-node-label-key", | ||
| operator: "In", | ||
| values: [ | ||
| "another-node-label-value" | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
This CEL expression is confusing and appears to use a non-standard syntax for constructing the patch value. Using constructs like Object.spec.affinity as type names is likely incorrect and will be confusing for users trying to adapt this example.
For a user-facing example, it's crucial that the code is clear, robust, and follows documented conventions. A better approach would be to use standard CEL to construct the patch, or if using CEL's proto message construction capabilities, to use the correct, fully-qualified type names (e.g., core.v1.Affinity).
Here is a suggested correction that uses more conventional type names. This assumes the JSONPatch constructor and the core.v1 types are available in the CEL environment.
expression: >
[
JSONPatch{
op: "add", path: "/spec/affinity",
value: core.v1.Affinity{
nodeAffinity: core.v1.NodeAffinity{
preferredDuringSchedulingIgnoredDuringExecution: [
core.v1.PreferredSchedulingTerm{
weight: 1,
preference: core.v1.NodeSelectorTerm{
matchExpressions: [
core.v1.NodeSelectorRequirement{
key: "another-node-label-key",
operator: "In",
values: [
"another-node-label-value"
]
}
]
}
}
]
}
}
}
]
| // TODO: is there a way to wait for MutatingAdmissionPolicy readiness? (it doesn't appear so) | ||
| // sleep hack to give time to propagate | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
The use of time.Sleep can introduce flakiness into the test suite. While MutatingAdmissionPolicy currently lacks a status field for readiness, a more reliable approach would be to actively poll for the policy to become effective.
A robust alternative to a fixed sleep duration would be to attempt to create a resource that the policy should mutate, and retry until the mutation is observed. This would provide a more deterministic signal that the admission policy is active and ready.
This change documents a proof-of-concept of configuring the node placement of Config Sync system pods using MutatingAdmissionPolicy.
This works out of the box, but is currently an alpha api and is behind a feature gate. It is targeted for beta in k8s 1.34.