Do not add grafana related annotations to k8s service#501
Do not add grafana related annotations to k8s service#501Kangyan-Zhou wants to merge 4 commits intosgl-project:mainfrom
Conversation
Summary of ChangesHello @Kangyan-Zhou, 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 addresses the issue of Kubernetes Service objects inheriting annotations that are exclusively relevant to Pods. By introducing a clear definition of 'pod-only' annotations and implementing a filtering mechanism, the PR ensures that generated Service manifests are cleaner and more semantically accurate. This prevents the propagation of pod-specific configurations, such as those for monitoring, networking, or container injection, to Service resources where they have no functional impact. The changes are supported by new utility functions and comprehensive unit tests to guarantee correctness. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter out pod-specific annotations when creating Kubernetes Service objects. This is a solid improvement that prevents irrelevant annotations from being applied to services where they have no effect. The implementation is clean, defining a central list of pod-only annotations and using a new utility function to perform the filtering. The changes are also well-covered by new unit tests. My main feedback is a minor refactoring suggestion to improve code clarity by removing an unused function parameter.
| func buildService(componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec, | ||
| podSpec *corev1.PodSpec, | ||
| selector map[string]string, | ||
| ) *corev1.Service { |
There was a problem hiding this comment.
The componentExt parameter is not used within the buildService function. It's good practice to remove unused parameters to improve code clarity and maintainability. Remember to update the call sites in NewServiceReconciler and the tests in service_reconciler_test.go accordingly.
| func buildService(componentMeta metav1.ObjectMeta, componentExt *v1beta1.ComponentExtensionSpec, | |
| podSpec *corev1.PodSpec, | |
| selector map[string]string, | |
| ) *corev1.Service { | |
| func buildService(componentMeta metav1.ObjectMeta, | |
| podSpec *corev1.PodSpec, | |
| selector map[string]string, | |
| ) *corev1.Service { |
| // - Exact annotation keys (e.g., ModelInitInjectionKey) - matches only that specific annotation | ||
| // | ||
| // Both styles work correctly because IsPrefixSupported uses strings.HasPrefix for matching. | ||
| PodOnlyAnnotationPrefixes = []string{ |
There was a problem hiding this comment.
nit: a bit worried hardcoding this will be ok or not
There was a problem hiding this comment.
I couldn't think of a good way to dynamically include all labels, @slin1237 please suggest if you have a better way to avoid hardcoding here
What this PR does
retry #391. Right now the grafana annotations on the services will cause grafana to scrape the service as well and lead to wrong metrics.
Why we need it
Fixes #417
How to test
Tested locally and the annotations on the service are gone