[helm]: Include default helpers, rename using fullname

[cabrinha]

Pull Request Description

The current helm chart includes namespace: {{ .Release.Namespace }} everywhere, which is unnecessary. It also lacks default _helpers.tploften found when usinghelm create ` which means labels and selector labels would be non-standard.

Related Issues

Resolves: #[Insert issue number(s)]

---

Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

• [Bug]: Corrections to existing functionality
• [CI]: Changes to build process or CI pipeline
• [Docs]: Updates or additions to documentation
• [API]: Modifications to aibrix's API or interface
• [CLI]: Changes or additions to the Command Line Interface
• [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

• PR title includes appropriate prefix(es)
• Changes are clearly explained in the PR description
• New and existing tests pass successfully
• Code adheres to project style and best practices
• Documentation updated to reflect changes (if applicable)
• Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

[gemini-code-assist]

Summary of Changes

Hello @cabrinha, I'm Gemini Code Assist¹! 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 significantly refactors the Helm chart's helper templates and resource naming conventions. The primary goal is to standardize how names, labels, and other common attributes are generated for Kubernetes resources, moving away from hardcoded values and inconsistent patterns. By introducing a comprehensive set of new helper functions and applying them throughout the chart, this change enhances the chart's maintainability, flexibility, and adherence to Helm best practices, making deployments more robust and easier to manage.

Highlights

• Standardized Helm Helpers: Introduced a new set of Helm helper templates (aibrix.name, aibrix.fullname, aibrix.labels, etc.) to ensure consistent naming and labeling across all Kubernetes resources defined by the chart.
• Dynamic Resource Naming: Replaced hardcoded resource names with dynamic names generated using the new aibrix.fullname helper, improving chart flexibility and adherence to Kubernetes naming conventions.
• Refactored _helpers.tpl: Removed deprecated chart.* helper functions and integrated the new aibrix.* helpers, aligning the chart with standard Helm practices.
• Namespace Declaration Cleanup: Eliminated redundant namespace: {{ .Release.Namespace }} declarations from resource metadata, streamlining the chart definitions.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request refactors Helm templates by introducing new aibrix.* helpers for consistent and dynamic naming of Kubernetes resources, replacing older chart.* helpers. The changes update resource names, labels, and image pull secret references across various components including the controller-manager, gateway-instance, gateway-plugin, GPU optimizer, metadata service, Prometheus, and webhooks. Review comments indicate several areas needing further attention: specifically, some resource name references (like RouteConfiguration in gateway.yaml, serverName in monitor.yaml, and a webhook secret in controller-manager/deployment.yaml) were not fully updated to the new dynamic naming, a static Redis host name in values.yaml is still referenced by multiple deployments, the newly introduced aibrix.serviceAccountName helper is not yet utilized, and an indentation error was found in controller-manager/deployment.yaml for imagePullSecrets. However, the correction of a hardcoded webhook-service name to a dynamic one was noted as a positive improvement.

[gemini-code-assist]

While it's correct to rename the Redis service to use fullname, the Redis host is configured in values.yaml with the old static name (aibrix-redis-master). Several deployments (gateway-plugin, gpu-optimizer, metadata-service) use this static value from values.yaml, which will now be incorrect and break communication with Redis. The templates should be updated to construct the Redis host name dynamically (e.g., {{ include "aibrix.fullname" . }}-redis-master) instead of relying on values.yaml.

[cabrinha]

Actually, we should replace this implementation of redis with something better maintained and also with support for HA, like: https://github.com/DandyDeveloper/charts/tree/master/charts/redis-ha -- This should be pulled in as a chart dependency.

[cabrinha]

For now, I'll name the redis components based off {{ .Values.gatewayPlugin.dependencies.redis.host }}, but I'll quickly follow up with another PR to replace this implementation of redis with an HA option which can be enabled/disabled with a bool.

[Jeffwan]

please help check the critical comments. I will help review this PR later. Thanks for the contribution!

[cabrinha]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a great step towards standardizing the Helm chart by introducing common helpers from helm create and using fullname for resource naming. This significantly improves consistency and maintainability. The removal of hardcoded namespaces is also a welcome change.

My review has found a few issues:

• A critical misconfiguration in an RBAC ClusterRoleBinding that would prevent it from working correctly.
• Inconsistent resource naming for the Redis sub-component, which doesn't follow the new fullname convention.
• A minor label inaccuracy on the Redis service.

I've provided specific comments and suggestions to address these points. Once these are fixed, the chart will be in a much better state.

[cabrinha]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces two main changes. First, it refactors the Helm chart to use standard helpers and naming conventions, which is a great improvement for maintainability and multi-release deployments. The changes are mostly correct and follow best practices. However, I've found a critical bug related to imagePullSecrets and a design flaw in the new serviceAccountName helper that will limit its usability.

Second, it adds a comprehensive integration test suite for vLLM's LoRA functionality. This is a very valuable addition, and the tests are well-structured and cover many important scenarios and edge cases. This part of the PR looks excellent.

Please address the two issues I've raised in the Helm chart templates.

[cabrinha]

/gemini review

[gemini-code-assist]

Code Review

This pull request does a great job of refactoring the Helm chart to align with standard practices. Introducing a _helpers.tpl file with common helpers like aibrix.fullname and aibrix.labels, and using them to name and label resources consistently is a significant improvement for maintainability. Removing the hardcoded namespace from templates is also a welcome change.

My review focuses on the new helper templates. I've found a critical issue in the service account name generation logic that could cause template rendering to fail under certain conditions. I've also pointed out a minor copy-paste error in a comment. I've provided suggestions to fix these issues and simplify the logic.

Overall, this is a very positive change for the Helm chart. Once the identified issues are addressed, this will be ready to merge.

[cabrinha]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the Helm chart by adopting standard practices, such as using _helpers.tpl for resource naming and labels, which makes the chart more reusable and maintainable. The removal of unnecessary namespace fields and the introduction of configurable service accounts are also great enhancements. I've identified one critical bug where a secret name remains hardcoded, which could break deployments. I've also suggested an improvement to the naming of RBAC resources for better clarity. Overall, this is a very positive and well-executed refactoring.

[gemini-code-assist]

While it's valid to have a ClusterRole and ClusterRoleBinding with the same name, it can be confusing. The previous naming convention used suffixes like -clusterrole and -clusterrolebinding which improved clarity. It's a good practice to use distinct names for different resource kinds.

This pattern of identical names for different kinds appears throughout the RBAC files in this PR. For example, the leader-election Role and RoleBinding in this file also share a name.

Could you re-introduce suffixes for clarity across all RBAC resources? For this ClusterRole, I suggest:

  name: {{ include "aibrix.fullname" . }}-controller-manager-clusterrole

[cabrinha]

This is redundant because you can clearly see the differences between objects without the object type as a suffix on the name.

kubectl get role
kubectl get clusterrole
kubectl get rolebinding
kubectl get clusterrolebinding

Imagine if every k8s object had a suffix of it's kind. aibrix-deployment, aibrix-service, etc. So, I disagree here.

[Jeffwan]

It works, we just need to make sure that upgrade experiences still smooth. Consider it will create new objects, I do not have concerns here

[cabrinha]

please help check the critical comments. I will help review this PR later. Thanks for the contribution!

Done, ready for review. I'm available in the vLLM slack for discussion.

[Jeffwan]

any concerns to use fixed component name?

[cabrinha]

If anyone wants to deploy more than 1 instance of aibrix chart to their cluster or rename it to something like aibrix-dev or something else, the fullname helper works here. Most helm charts use the fullname helper, which defaults the the chart name or the helm release name.

[Jeffwan]

there're test errors seems related to redis, please help take a look

[cabrinha]

Oh, I see here. The init containers are looking for that redis-host value.

I also see another issue is with this:

aibrix/pkg/cert/cert.go

Line 29 in c3e63f9

secretName = "aibrix-webhook-server-cert"

The rotator.go file is expecting a hard-coded name for the certificate, and the helm release installed was named: aibrix-r8crnksvwc, which is actually good because users should be able to choose their own helm release name.

I'll push a commit to set the webhook-server-cert name back to a hardcoded value to see if that fixes the test, but I think the code should be more flexible to reference the secret somehow, possibly using label selectors and namespace? Not sure.