feat: add label to indicate pods managed by kmesh#1610
feat: add label to indicate pods managed by kmesh#1610Aaravanand00 wants to merge 5 commits intokmesh-net:mainfrom
Conversation
|
Welcome @Aaravanand00! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
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 significantly improves the observability and debuggability of Kmesh-managed Pods by introducing a dedicated Kubernetes label. This change allows for straightforward identification of Pods under Kmesh control, simplifying operational tasks and troubleshooting. It also refines the enrollment criteria to be more inclusive of different dataplane labeling conventions. 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
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. A label added, clear and bright, Kmesh-managed pods, now in plain sight. No more guessing, no more dread, Just `kubectl get`, and truth is spread. 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 a new label, kmesh.net/managed: "true", to pods that are managed by Kmesh, enhancing visibility for users and operators. It also updates the ShouldEnroll logic to consider the istio.io/dataplane label for both pods and namespaces, aligning with broader Istio label conventions. New utility functions PatchKmeshManagedLabel and DelKmeshManagedLabel have been added, along with comprehensive unit tests to ensure their correctness and idempotency. The changes are well-tested and address the stated goal of improving Kmesh pod identification.
There was a problem hiding this comment.
Pull request overview
This PR adds a Pod label (kmesh.net/managed: "true") intended to indicate when a Pod is managed by Kmesh, and expands enrollment detection to also honor the istio.io/dataplane=kmesh label (in addition to istio.io/dataplane-mode=kmesh).
Changes:
- Extend
ShouldEnrollto treatistio.io/dataplane=kmeshas an enrollment signal (pod + namespace). - Add constants and utilities to patch/delete the new
kmesh.net/managedlabel on Pods. - Add unit tests for the new label patch/delete helpers and for namespace enrollment via
istio.io/dataplane.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/utils/enroll.go | Expands enrollment checks and adds patch helpers for the managed label. |
| pkg/utils/enroll_test.go | Adds test coverage for namespace istio.io/dataplane enrollment + label patch/delete helpers. |
| pkg/constants/constants.go | Introduces constants for istio.io/dataplane and kmesh.net/managed. |
| pkg/cni/plugin/plugin.go | Labels Pods as managed during CNI CmdAdd when Kmesh enrollment is enabled. |
Comments suppressed due to low confidence (1)
pkg/utils/enroll.go:67
- Order of checks in
ShouldEnrollmeans a pod withistio.io/dataplane-mode: nonewill still be enrolled if it also hasistio.io/dataplane: kmesh(thenoneopt-out check happens after the earlyreturn true). Ifnoneis intended to be an explicit opt-out (as the comment suggests), handle thenonecase before checking theistio.io/dataplane/dataplane-modeopt-in labels.
podMode := pod.Labels[constants.DataPlaneModeLabel]
if strings.EqualFold(podMode, constants.DataPlaneModeKmesh) ||
strings.EqualFold(pod.Labels[constants.DataPlaneLabel], constants.DataPlaneModeKmesh) {
return true
}
if podMode == "none" {
return false
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Aaravanand00
force-pushed
the
feature/71-add-kmesh-managed-pod-label
branch
from
181e38c to
61f6cc6
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an explicit Pod label to indicate when a Pod is managed by Kmesh, making it easier to confirm Kmesh enrollment via standard Kubernetes tooling (e.g., kubectl get pods --show-labels).
Changes:
- Introduces
kmesh.net/managed: "true"and patches it onto enrolled Pods from the CNICmdAddpath. - Extends enrollment detection (
ShouldEnroll) to also recognizeistio.io/dataplane=kmesh(in addition toistio.io/dataplane-mode=kmesh). - Updates/extends tests to cover the new label patch behavior and related e2e flows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/baseline_test.go | Updates pod readiness fetch and modifies waypoint label patching logic in e2e tests. |
| pkg/utils/enroll_test.go | Adds unit tests for new enrollment criteria and managed-label patch/delete helpers. |
| pkg/utils/enroll.go | Extends ShouldEnroll logic and adds helpers to patch/delete the managed label. |
| pkg/constants/constants.go | Adds constants for istio.io/dataplane and the new kmesh.net/managed label/value. |
| pkg/cni/plugin/plugin.go | Calls into utils to patch the managed label during CNI ADD for enrolled pods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
Added comments to clarify checks for dataplane mode in pod and namespace. Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
…ng logic as per Copilot suggestions Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
PR 1610 failing checks: 1. Fix E2E test failures by removing the gateway-name label from workloads. This label should only be on waypoint pods themselves. 2. Change log level from Infof to Debugf in enrollment logic to reduce noise. 3. Update ShouldEnroll comments for better accuracy. 4. Perform DCO sign-off. Signed-off-by: Aaravanand00 <aaravanand5749@gmail.com>
Aaravanand00
force-pushed
the
feature/71-add-kmesh-managed-pod-label
branch
from
61f6cc6 to
1225b42
Compare
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What this PR does
This PR adds a simple label to Pods to indicate when they are managed by Kmesh.
Currently, it is not easy to know whether Kmesh is actually handling a Pod.
It depends on multiple conditions such as the namespace label (
istio.io/dataplane=kmesh) and whether the pod has an Istio sidecar.With this change, when a pod is handled by Kmesh, a label like this will be added to the Pod:
kmesh.net/managed: "true"This makes it easier for users and operators to quickly see which Pods are controlled by Kmesh.
Why this change is needed
Right now there is no direct way to check if Kmesh is responsible for a pod.
Adding this label makes debugging and verification much easier.
Result
Users can run commands like:
kubectl get pods --show-labelsand quickly identify pods managed by Kmesh.
Fixes #71
Special notes for reviewers:
AI assistance was used to help explore the repository and draft the implementation idea, but the final code and changes were reviewed and verified by me.