feature(rbg): support role instance index env#194
Conversation
Summary of ChangesHello, 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 refactors and enhances the handling of labels and environment variables within the RoleBasedGroup (RBG) system. It standardizes the naming conventions for component-related labels, ensuring a more consistent and maintainable codebase. A significant improvement is the introduction and propagation of the role instance index as an environment variable, which provides more granular identification for pods within a RoleInstance. These changes collectively improve the robustness and clarity of how component identities are managed and exposed in the RBG ecosystem. 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. Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a role instance index environment variable by adding a role-instance-index label to RoleInstance objects and their pod templates. It also includes several valuable refactorings, such as consolidating label constants, using a helper function IsStatefulRole for better maintainability, and fixing a minor inconsistency in environment variable injector naming. The changes are well-structured and improve the codebase. I have one suggestion regarding code duplication to further enhance maintainability.
| identityLabels := map[string]string{ | ||
| apps.StatefulSetPodNameLabel: instance.Name, | ||
| apps.PodIndexLabel: strconv.Itoa(ordinal), | ||
| constants.RoleInstanceIndexLabelKey: strconv.Itoa(ordinal), | ||
| } | ||
| for k, v := range identityLabels { | ||
| instance.Labels[k] = v | ||
| } | ||
| injectIdentityLabelsIntoComponents(instance, identityLabels) |
There was a problem hiding this comment.
This block of code for setting and injecting identity labels is duplicated from the updateIdentity function (lines 100-108). To improve maintainability and reduce redundancy, consider extracting this logic into a new helper function.
For example, you could create a function setAndInjectIdentityLabels(instance *workloadsv1alpha2.RoleInstance, ordinal int) that encapsulates this logic, and then call it from both updateIdentity and newVersionedInstance.
func setAndInjectIdentityLabels(instance *workloadsv1alpha2.RoleInstance, ordinal int) {
if instance.Labels == nil {
instance.Labels = make(map[string]string)
}
identityLabels := map[string]string{
apps.StatefulSetPodNameLabel: instance.Name,
apps.PodIndexLabel: strconv.Itoa(ordinal),
constants.RoleInstanceIndexLabelKey: strconv.Itoa(ordinal),
}
for k, v := range identityLabels {
instance.Labels[k] = v
}
injectIdentityLabelsIntoComponents(instance, identityLabels)
}
There was a problem hiding this comment.
Pull request overview
This PR updates the RBGS controllers and discovery env injection so pods can reliably access a “role instance index” (ordinal) via labels/env, and aligns component label key usage across the codebase.
Changes:
- Propagate RoleInstance identity labels (including ordinal) into each component’s PodTemplate labels in RoleInstanceSet stateful mode.
- Standardize component label key usage (rename away from legacy/alias constants) across reconcilers, discovery env builder, and in-place utils/tests.
- Switch Role env injection gating to
workloadsv1alpha2.IsStatefulRole(...)to cover RoleInstanceSet stateful patterns.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset_reconciler.go | Updates pod template injector list to include LWP env injection for leader/worker templates. |
| pkg/reconciler/roleinstanceset/statefulmode/stateful_instance_set_utils.go | Adds identity label propagation into component pod templates for RoleInstanceSet-managed pods. |
| pkg/reconciler/roleinstance/utils/instance_utils.go | Renames component label keys used for pod labeling and label-based parsing. |
| pkg/inplace/instance/instance_util_test.go | Updates tests to use the renamed component label key constant. |
| pkg/inplace/instance/instance_util.go | Updates component matching to use the renamed component label key constant. |
| pkg/discovery/env_builder.go | Uses new component label key constants; switches stateful detection to IsStatefulRole. |
| pkg/constants/label.go | Reorganizes label constants; removes legacy alias constant blocks. |
| internal/controller/workloads/rolebasedgroupscalingadapter_controller.go | Updates selector augmentation to use the renamed component index label key constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for k, v := range identityLabels { | ||
| instance.Labels[k] = v | ||
| } | ||
| injectIdentityLabelsIntoComponents(instance, identityLabels) |
There was a problem hiding this comment.
When propagating identityLabels into component pod templates, apps.StatefulSetPodNameLabel ("statefulset.kubernetes.io/pod-name") will be set to the RoleInstance name for all pods created from the template. That label’s expected semantic is the pod name (and cannot be correctly set at template level for multi-pod components), so this will produce incorrect/misleading labels on the pods. Consider excluding apps.StatefulSetPodNameLabel from template injection and only propagate labels that are valid at the RoleInstance level (e.g., apps.PodIndexLabel / RoleInstanceIndexLabelKey), or set a dedicated RoleInstance name label instead.
| injectIdentityLabelsIntoComponents(instance, identityLabels) | |
| // Only inject identity labels that are valid at the pod-template level. | |
| templateIdentityLabels := make(map[string]string, len(identityLabels)) | |
| for k, v := range identityLabels { | |
| if k == apps.StatefulSetPodNameLabel { | |
| // apps.StatefulSetPodNameLabel is per-pod and must not be set on pod templates. | |
| continue | |
| } | |
| templateIdentityLabels[k] = v | |
| } | |
| injectIdentityLabelsIntoComponents(instance, templateIdentityLabels) |
| // Set identity labels and propagate them into each component's pod template. | ||
| identityLabels := map[string]string{ | ||
| apps.StatefulSetPodNameLabel: instance.Name, | ||
| apps.PodIndexLabel: strconv.Itoa(ordinal), | ||
| constants.RoleInstanceIndexLabelKey: strconv.Itoa(ordinal), | ||
| } | ||
| for k, v := range identityLabels { | ||
| instance.Labels[k] = v | ||
| } | ||
| injectIdentityLabelsIntoComponents(instance, identityLabels) | ||
|
|
There was a problem hiding this comment.
Same issue as above: injecting apps.StatefulSetPodNameLabel into component pod templates will set "statefulset.kubernetes.io/pod-name" to the RoleInstance name for every pod created from the template. Since pod names differ per component replica, this label will be incorrect on the resulting pods. Consider not including apps.StatefulSetPodNameLabel in the set of labels propagated into component templates.
| if workloadsv1alpha2.IsStatefulRole(g.role) { | ||
| envVars = append(envVars, |
There was a problem hiding this comment.
EnvBuilder.buildLocalRoleVars now uses workloadsv1alpha2.IsStatefulRole(g.role), which changes behavior for RoleInstanceSet roles (e.g., injecting RBG_ROLE_INDEX for stateful RoleInstanceSet patterns). There are existing tests for EnvBuilder in this package, but they don’t cover RoleInstanceSet stateful vs stateless patterns, so this behavior change is currently untested. Please add/extend unit tests in env_builder_test.go to cover RoleInstanceSet roles (stateful + stateless) and assert whether RBG_ROLE_INDEX is included as expected.
| Name: constants.EnvRBGRoleIndex, | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| FieldRef: &corev1.ObjectFieldSelector{ | ||
| FieldPath: "metadata.labels['apps.kubernetes.io/pod-index']", |
There was a problem hiding this comment.
The RoleInstanceIndexLabelKey is not being used for environment variables; instead, the environment variables still use apps.kubernetes.io/pod-index.
| instance.Labels[apps.PodIndexLabel] = strconv.Itoa(ordinal) | ||
| identityLabels := map[string]string{ | ||
| apps.StatefulSetPodNameLabel: instance.Name, | ||
| apps.PodIndexLabel: strconv.Itoa(ordinal), |
There was a problem hiding this comment.
Since apps.kubernetes.io/pod-index and instance-index have the same value, wouldn't this be misleading for users?
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.