-
Notifications
You must be signed in to change notification settings - Fork 8
chore(apps/gcp/jenkins): add restrict-to-node-instance-types policy to beta post environment #1807
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.
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
Conversation
beta post environment Add policy to prevent pods from running on spot nodes in beta post
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR adds a Kyverno policy named restrict-to-node-instance-types to the beta post environment for the GCP Jenkins app. The policy aims to prevent pods from running on GKE spot nodes by enforcing node affinity that restricts pod scheduling to nodes labeled with cloud.google.com/gke-provisioning: spot. The policy is added as a new resource and referenced in the kustomization file. The approach is straightforward and aligns with the stated goal, but the policy may have unintended effects and lacks clarity on whether it mutates pods to comply or forbids scheduling on spot nodes. Overall, the implementation requires some corrections and enhancements for correctness and intent.
Critical Issues
-
Misuse of
mutaterule to enforce scheduling restrictions
File:apps/gcp/jenkins/beta/post/_base/policies.yaml(lines 5-23)
Issue: The policy uses amutaterule to add an annotation and node affinity that restricts pods to spot nodes (values: ["spot"]). This contradicts the description "prevent pods from running on spot nodes" because it actually requires pods to run on spot nodes.
Suggestion:- If the goal is to prevent scheduling on spot nodes, use a
validaterule that forbids pods on nodes labeledcloud.google.com/gke-provisioning: spot. - Alternatively, if you want to mutate pods to avoid spot nodes, patch node affinity to exclude spot nodes instead of requiring them.
Example validate rule snippet to forbid pods on spot nodes:
apiVersion: kyverno.io/v1 kind: Policy metadata: name: restrict-to-node-instance-types spec: rules: - name: forbid-pods-on-spot-nodes match: resources: kinds: - Pod validate: message: "Pods are not allowed to run on spot nodes." pattern: spec: affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: nodeSelectorTerms: - matchExpressions: - key: cloud.google.com/gke-provisioning operator: NotIn values: - spot
This approach forbids pods from scheduling on spot nodes.
- If the goal is to prevent scheduling on spot nodes, use a
-
Potential unintended side effects of patching pod affinity
Adding node affinity via mutation can override user-specified affinity or cause scheduling failures if no suitable nodes meet the criteria. This can break existing pods unintentionally.
Code Improvements
-
Add
namespaceorbackgrounddeletion strategy as needed
File:apps/gcp/jenkins/beta/post/_base/policies.yaml
If pods with this policy are deleted or mutated frequently, consider specifyingbackground: falseorfailurePolicyin the Kyverno policy for better predictability. -
Clarify policy annotation intent
The added annotationcluster-autoscaler.kubernetes.io/safe-to-evict: "false"is applied unconditionally to all matched pods. This may affect cluster autoscaler behavior negatively by preventing pod eviction on spot nodes. Confirm if this is the intended effect.
Best Practices
-
Update kustomization resource name to include extension
File:apps/gcp/jenkins/beta/post/_base/kustomization.yaml(line 4)
Issue: The resource added isrestrict-to-node-instance-typeswithout a file extension, which may cause kustomize to fail or not find the file.
Suggestion: Add the correct filename with extension, likelyrestrict-to-node-instance-types.yaml:resources: - rbac.yaml - restrict-to-node-instance-types.yaml
-
Add comments to the policy YAML
File:apps/gcp/jenkins/beta/post/_base/policies.yaml
The policy lacks comments explaining the purpose of the rule, the effect of the mutation, and the reason for the annotation addition. Adding comments improves maintainability. -
Include tests or validation steps for the new policy
There is no mention of tests or CI validation that verifies the policy works as intended, especially in preventing pods from running on spot nodes. Adding sample pod specs and expected behaviors would improve confidence.
Summary of Recommendations
- Convert the
mutaterule to avalidaterule that forbids pods scheduling on spot nodes if the goal is to prevent pods from running there. - Fix the node affinity logic to exclude spot nodes rather than require them.
- Correct the kustomization resource filename to include
.yaml. - Add documentation comments in the policy.
- Review the implications of adding the autoscaler annotation.
- Add test cases or validation for the new policy.
Addressing these points will ensure the policy meets the stated goal without breaking pod scheduling or cluster autoscaler behavior.
Summary of ChangesHello @wuhuizuo, 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 implements a new Kubernetes policy within the "beta post" Jenkins environment on GCP. The primary goal is to ensure that critical pods are not scheduled on preemptible spot nodes, thereby improving the stability and reliability of the Jenkins workloads. This is achieved by deploying a Kyverno policy that modifies pod specifications to enforce specific node scheduling requirements. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to add a Kyverno policy to prevent Jenkins pods in the beta post-environment from running on spot nodes. However, there are a couple of critical issues with the current implementation. Firstly, the kustomization.yaml incorrectly references the new policy by its metadata name instead of its filename, which will cause the build to fail. Secondly, and more importantly, the logic within the Kyverno policy is inverted: it would force pods onto spot nodes rather than preventing them from running there. I've provided suggestions to correct both of these issues and also a comment on potential policy duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR adds a Kyverno policy named restrict-to-node-instance-types to the beta post environment for the GCP Jenkins app. The policy prevents pods from running on spot nodes by enforcing node affinity and annotating pods as not safe to evict. The approach of adding the policy as a new resource and including it in the kustomization is straightforward and well-scoped. Overall, the changes are concise and clear, but the policy logic has some issues that could lead to unintended pod scheduling behavior.
Critical Issues
- Policy logic may prevent pods from scheduling on any node
- File:
apps/gcp/jenkins/beta/post/_base/policies.yamllines 10-24 - Issue: The
nodeAffinityrequiredDuringScheduling rule only allows nodes with the labelcloud.google.com/gke-provisioning=spot. This contradicts the PR description "prevent pods from running on spot nodes" — pods will be forced onto spot nodes instead of excluding them. - Suggested fix: Change the operator from
IntoNotInwith value["spot"]to prevent scheduling on spot nodes:nodeSelectorTerms: - matchExpressions: - key: cloud.google.com/gke-provisioning operator: NotIn values: ["spot"]
- File:
Code Improvements
- Pod annotation patch may be overly aggressive
- File:
apps/gcp/jenkins/beta/post/_base/policies.yamllines 14-16 - Issue: The policy adds
cluster-autoscaler.kubernetes.io/safe-to-evict: "false"annotation to all matched pods. This could have side effects by preventing eviction on non-spot nodes if pods are scheduled there due to policy misconfiguration or other factors. - Suggested improvement: Consider scoping this annotation only to pods that actually run on spot nodes or remove it if the main goal is just to prevent scheduling on spot nodes. Alternatively, add a conditional to mutate only pods that are already on spot nodes if feasible.
- File:
Best Practices
-
Missing policy metadata labels and annotations
- File:
apps/gcp/jenkins/beta/post/_base/policies.yamllines 3-6 - Issue: The Kyverno policy does not include common metadata labels or annotations (e.g., app, environment, purpose). These improve policy discoverability and management.
- Suggested enhancement: Add descriptive labels and annotations, for example:
metadata: name: restrict-to-node-instance-types labels: app: jenkins environment: beta-post annotations: description: "Disallow pods from scheduling on spot nodes by enforcing node affinity"
- File:
-
Add comments explaining policy intent and behavior
- File:
apps/gcp/jenkins/beta/post/_base/policies.yamlentire file - Issue: The policy lacks inline comments explaining the purpose of the node affinity and annotation mutation, which would help future maintainers understand the rationale.
- Suggested improvement: Add a comment block at the top of the policy or before key sections describing the intent.
- File:
-
Testing and validation
- No evidence of tests or validation steps to verify that the policy works as intended. If possible, add documentation or test manifests showing that pods are correctly rejected or mutated.
Summary of Suggested Fixes
# Correct node affinity to exclude spot nodes
nodeSelectorTerms:
- matchExpressions:
- key: cloud.google.com/gke-provisioning
operator: NotIn
values: ["spot"]Add metadata labels and annotations:
metadata:
name: restrict-to-node-instance-types
labels:
app: jenkins
environment: beta-post
annotations:
description: "Disallow pods from scheduling on spot nodes by enforcing node affinity"Add comments describing policy intent for maintainability.
If the policy is intended to prevent pods from running on spot nodes, the current affinity logic is inverted and must be corrected to avoid scheduling failures or unintended pod placement.
Add policy to prevent pods from running on spot nodes in beta post