Feature/vcluster #421
Feature/vcluster #421
Conversation
|
|
mysiki
force-pushed
the
feature/vcluster
branch
from
b281597 to
8f5cbeb
Compare
|
some news ? |
Use f-strings, rename variables, add doc strings
mysiki
force-pushed
the
feature/vcluster
branch
from
b6d2a01 to
8222344
Compare
WalkthroughIntroduces VCluster support across configuration, CLI, and Prometheus query builders. Adds helper methods for vcluster pod/namespace mapping, updates CPU/Memory loaders to use them and log PromQL, adds README documentation, and makes a trivial requirements.txt newline fix. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as KRR CLI
participant Config
participant Loader as Metric Loader
participant Prom as Prometheus
User->>CLI: run krr ... --vcluster-name --vcluster-namespace
CLI->>Config: build Config(vcluster_name, vcluster_namespace, ...)
CLI->>Loader: instantiate with settings
Loader->>Loader: get_vcluster_pod_real_name()/get_pod_namespace()
Loader->>Prom: query PromQL (vcluster-aware)
Prom-->>Loader: timeseries
Loader-->>CLI: results
CLI-->>User: output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (2)
17-17: Minor: Fix redundant spacing in HTML tag.There's an extra space in the paragraph alignment attribute.
- <p align="center"> + <p align="center">
408-422: Fix grammar and punctuation issues in VCluster documentation.The documentation has a few style issues that should be addressed for clarity and consistency.
-KRR supports VCluster software when Prometheus is outside of the VCluster (on physical cluster or centralized). Because of VCluster pod renaming, you need to provide : +KRR supports VCluster software when Prometheus is outside the VCluster (on physical cluster or centralized). Because of VCluster pod renaming, you need to provide: -Other parameters like namespace selector, pod selector etc work as expected. +Other parameters like namespace selector, pod selector, etc. work as expected.robusta_krr/core/integrations/prometheus/metrics/base.py (1)
296-296: Fix typo in docstring.There's a typo in the docstring.
- string: the pod namepace in the host cluster. + string: the pod namespace in the host cluster.robusta_krr/core/integrations/prometheus/metrics/memory.py (1)
82-82: Remove unnecessary trailing whitespace.There's trailing whitespace on the line that should be removed for consistency.
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(3 hunks)requirements.txt(1 hunks)robusta_krr/core/integrations/prometheus/metrics/base.py(2 hunks)robusta_krr/core/integrations/prometheus/metrics/cpu.py(6 hunks)robusta_krr/core/integrations/prometheus/metrics/memory.py(8 hunks)robusta_krr/core/models/config.py(1 hunks)robusta_krr/main.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
robusta_krr/core/integrations/prometheus/metrics/cpu.py (1)
robusta_krr/core/integrations/prometheus/metrics/base.py (3)
get_vcluster_pod_real_name(265-285)get_pod_namespace(287-302)get_prometheus_cluster_label(80-89)
robusta_krr/core/integrations/prometheus/metrics/memory.py (3)
robusta_krr/core/integrations/prometheus/metrics/base.py (4)
get_vcluster_pod_real_name(265-285)get_pod_namespace(287-302)get_prometheus_cluster_label(80-89)get_query(92-105)robusta_krr/core/integrations/prometheus/metrics/cpu.py (3)
get_query(15-33)get_query(45-66)get_query(76-94)robusta_krr/core/models/objects.py (1)
K8sObjectData(38-107)
🪛 LanguageTool
README.md
[style] ~411-~411: This phrase is redundant. Consider using “outside”.
Context: ...ts VCluster software when Prometheus is outside of the VCluster (on physical cluster or ce...
(OUTSIDE_OF)
[style] ~416-~416: In American English, abbreviations like “etc.” require a period.
Context: ...s like namespace selector, pod selector etc work as expected. ```sh krr simple --v...
(ETC_PERIOD)
🔇 Additional comments (12)
requirements.txt (1)
56-57: LGTM! File formatting improvement.Adding the trailing newline follows standard conventions and improves file consistency. This is a cosmetic improvement with no impact on functionality.
robusta_krr/core/models/config.py (1)
85-88: LGTM! Configuration fields for VCluster support.The new configuration fields are properly typed as optional strings with appropriate defaults. The placement after the internal
_logging_consoleattribute follows the existing code organization pattern.README.md (1)
408-422: No duplicate VCluster documentation found
The grep search only returns a single<details><summary>VCluster</summary>block in README.md (lines 408–422). There isn’t a second VCluster section to remove—please ignore this suggestion.Likely an incorrect or invalid review comment.
robusta_krr/core/integrations/prometheus/metrics/base.py (3)
7-7: LGTM! Added hashlib import for VCluster pod name hashing.The import is correctly added and will be used for the SHA-256 hashing functionality in the VCluster pod name generation.
287-302: LGTM! VCluster namespace mapping implementation.The namespace mapping logic is straightforward and correct - it returns the host cluster namespace when in a VCluster context, otherwise returns the original namespace.
265-285: Review VCluster Pod Naming Algorithm Needs Manual ConfirmationI didn’t find any references or examples of the exact “host cluster” pod-name format in our repo or docs, so please:
- Cross-check this implementation against VCluster’s official naming scheme (e.g. in the VCluster documentation or source).
- Add a comment or link to the precise doc/source you used for reference.
- Consider adding a small unit test covering:
• A name under 63 chars (no hash), and
• A name over 63 chars (truncation + 10-char SHA-256 suffix).This will ensure our truncation logic (52 chars + “-” + first 10 chars of SHA-256) perfectly mirrors VCluster’s behavior.
robusta_krr/core/integrations/prometheus/metrics/memory.py (5)
4-6: LGTM! Added logging support for memory metrics.The logging import and logger initialization follows the same pattern used in the CPU metrics file, providing consistency across the codebase.
15-30: LGTM! Consistent VCluster integration for MemoryLoader.The implementation correctly follows the established pattern from CPU metrics:
- Uses
get_vcluster_pod_real_name()for pod selector construction- Uses
get_pod_namespace()for namespace mapping- Introduces
prom_queryvariable with debug logging- Maintains the same query structure with VCluster-aware parameters
38-56: LGTM! VCluster integration for MaxMemoryLoader.The changes are consistent with the pattern established in other loaders, properly integrating VCluster support for maximum memory usage metrics.
63-82: LGTM! VCluster integration for MemoryAmountLoader.The implementation is consistent with other memory loaders and properly handles VCluster pod/namespace mapping for memory data point counting.
91-122: LGTM! VCluster integration for MaxOOMKilledMemoryLoader.The VCluster integration is properly implemented for the OOM-killed memory metrics. The complex query structure with joins is maintained while correctly using the VCluster-aware pod selectors and namespaces.
robusta_krr/main.py (1)
388-390: Config wiring for vcluster fields looks goodThe new CLI options are correctly propagated into Config. Assuming Config accepts Optional[str] for these fields, this is aligned.
| pods_selector = "|".join(self.get_vcluster_pod_real_name(pod.name, object.namespace) for pod in object.pods) | ||
| pods_namespace = self.get_pod_namespace(object.namespace) | ||
| cluster_label = self.get_prometheus_cluster_label() | ||
| return f""" | ||
| prom_query = f""" | ||
| max( | ||
| rate( | ||
| container_cpu_usage_seconds_total{{ | ||
| namespace="{object.namespace}", | ||
| pod=~"{pods_selector}", | ||
| container="{object.container}" | ||
| {cluster_label} | ||
| }}[{step}] | ||
| ) | ||
| ) by (container, pod, job) | ||
| """ | ||
| rate( | ||
| container_cpu_usage_seconds_total{{ | ||
| namespace="{pods_namespace}", | ||
| pod=~"{pods_selector}", | ||
| container="{object.container}" | ||
| {cluster_label} | ||
| }}[{step}] | ||
| ) | ||
| ) by (container, pod, job) | ||
| """ | ||
| logger.debug(f"{prom_query}") | ||
|
|
||
| return prom_query |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Escape pod names for regex and anchor matches; minor logging nit
Unescaped pod names in a regex matcher can overmatch (e.g., dots in names). Anchor the regex to avoid partial matches and escape pod names. Also, avoid unnecessary f-string in logger.debug.
Apply this diff:
-from .base import PrometheusMetric, QueryType
+from .base import PrometheusMetric, QueryType
import logging
+import re
@@
- pods_selector = "|".join(self.get_vcluster_pod_real_name(pod.name, object.namespace) for pod in object.pods)
+ pods_selector = "|".join(
+ re.escape(self.get_vcluster_pod_real_name(pod.name, object.namespace))
+ for pod in object.pods
+ )
+ pods_selector = f"^({pods_selector})$" if pods_selector else r"^$"
@@
- pod=~"{pods_selector}",
+ pod=~"{pods_selector}",
@@
- logger.debug(f"{prom_query}")
+ logger.debug(prom_query)
@@
- pods_selector = "|".join(self.get_vcluster_pod_real_name(pod.name, object.namespace) for pod in object.pods)
+ pods_selector = "|".join(
+ re.escape(self.get_vcluster_pod_real_name(pod.name, object.namespace))
+ for pod in object.pods
+ )
+ pods_selector = f"^({pods_selector})$" if pods_selector else r"^$"
@@
- pod=~"{pods_selector}",
+ pod=~"{pods_selector}",
@@
- logger.debug(f"{prom_query}")
+ logger.debug(prom_query)
@@
- pods_selector = "|".join(self.get_vcluster_pod_real_name(pod.name, object.namespace) for pod in object.pods)
+ pods_selector = "|".join(
+ re.escape(self.get_vcluster_pod_real_name(pod.name, object.namespace))
+ for pod in object.pods
+ )
+ pods_selector = f"^({pods_selector})$" if pods_selector else r"^$"
@@
- pod=~"{pods_selector}",
+ pod=~"{pods_selector}",
@@
- logger.debug(f"{prom_query}")
+ logger.debug(prom_query)Notes:
- Using re.escape ensures literal matching of pod names in the regex.
- Anchoring with ^(...)$ prevents partial matches.
- When no pods exist, '^$' ensures no match rather than a potentially surprising catch-all.
Also applies to: 46-67, 77-95
| vcluster_namespace: str = typer.Option( | ||
| None, | ||
| "--vcluster-namespace", | ||
| help="The vcluster namespace on physical cluster", | ||
| rich_help_panel="VCluster Settings", | ||
| ), | ||
| vcluster_name: str = typer.Option( | ||
| None, | ||
| "--vcluster-name", | ||
| help="The vcluster name on physical cluster", | ||
| rich_help_panel="VCluster Settings", | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make vcluster CLI options Optional[str] and clarify help about paired usage
Both options default to None but are annotated as str. Align to Optional[str] and clarify help that both must be provided together when using VCluster to avoid misconfiguration.
Apply this diff:
- vcluster_namespace: str = typer.Option(
+ vcluster_namespace: Optional[str] = typer.Option(
None,
"--vcluster-namespace",
- help="The vcluster namespace on physical cluster",
+ help="The vcluster namespace on the physical cluster (required when using --vcluster-name).",
rich_help_panel="VCluster Settings",
),
- vcluster_name: str = typer.Option(
+ vcluster_name: Optional[str] = typer.Option(
None,
"--vcluster-name",
- help="The vcluster name on physical cluster",
+ help="The vcluster name on the physical cluster (required when using --vcluster-namespace).",
rich_help_panel="VCluster Settings",
),As a follow-up, add a paired-args validation right before constructing Config to prevent partial configuration:
# Enforce that either both vcluster_* are provided or neither
if bool(vcluster_name) ^ bool(vcluster_namespace):
raise click.BadOptionUsage(
"--vcluster-name/--vcluster-namespace",
"Both --vcluster-name and --vcluster-namespace must be provided together when using VCluster."
)🤖 Prompt for AI Agents
robusta_krr/main.py around lines 323 to 334, the CLI options vcluster_namespace
and vcluster_name are typed as str but default to None and lack clear paired-use
help; change their type annotations to Optional[str] and update their help text
to state that both must be provided together when using VCluster, and add a
validation immediately before constructing Config that checks for a partial set
(XOR) of these two values and raises click.BadOptionUsage with a message that
both --vcluster-name and --vcluster-namespace must be provided together.
2 participants
Add support of VCluster (https://www.vcluster.com/) (resolve my issue : #415)
VCluster (VC) work on top of physical cluster with workload sync between virtual cluster and physical cluster. During the sync, VC rename the pod to avoid naming collision. VC only sync the pod, not the other stuff like RS, Deployment, ... The sync is done on the physical namespace where vcluster is present.
Summarize design :
The renaming is done following algo present in added function get_vcluster_pod_real_name.
To reduce pod number and cluster load, usually metrics are read read from physical cluster (avoid to have all exporter pod inside VC).
Because of VC only sync pod, KRR need to run connected to the VC (to detect DP, STS,..). But the pod name and pod namespace in prometheus are the one present in physical cluster.
This PR add 2 new arguments that are mandatory to calculate the pod name and pod namespace for prom query and function to use it.
I also add debug logger on prom query (can be remove if you think is useless :) )
Readme is update
Test done with success on my VC (with VC args) and without (like before this change).