feat: Adding karpenter toolsets - #1932
mrinalpravi wants to merge 5 commits into
Conversation
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Karpenter support: documentation and nav entries, a new karpenter toolset manifest with cloud-agnostic and AWS-specific kubectl-backed tools and LLM instructions, tests for the toolset, and an adjustment to Toolset.override_with plus corresponding unit tests. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Holmes as "HolmesGPT"
participant Kubectl as kubectl
participant K8s as "Kubernetes API"
User->>Holmes: holmes ask (investigate pending pods)
Holmes->>Holmes: check prerequisite CRD `nodepools.karpenter.sh`
alt CRD missing
Holmes-->>User: toolset disabled / notify
else CRD present
Holmes->>Kubectl: get pods --field-selector=status.phase=Pending
Kubectl->>K8s: GET /api/v1/.../pods
K8s-->>Kubectl: pending pods
Kubectl-->>Holmes: pod list
Holmes->>Kubectl: get nodeclaims/nodepools (CRD)
Kubectl->>K8s: GET nodeclaims/nodepools.karpenter.sh
K8s-->>Kubectl: nodeclaim/nodepool list
Kubectl-->>Holmes: resources
alt NodeClaim needs diagnosis
Holmes->>Kubectl: describe nodeclaim <name>
Kubectl->>K8s: DESCRIBE nodeclaim
K8s-->>Kubectl: nodeclaim details
Kubectl-->>Holmes: details
end
Holmes->>Kubectl: get disruption events & controller logs
Kubectl->>K8s: GET events / logs
K8s-->>Kubectl: events & logs
Kubectl-->>Holmes: logs/events
Holmes-->>User: aggregated diagnostics & LLM summary
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/plugins/toolsets/test_karpenter_toolset.py (1)
33-101: Add type hints to the new tests.The new fixture and test functions should annotate return values and the fixture parameter.
Proposed typing update
from holmes.plugins.toolsets import load_toolsets_from_file +from holmes.core.tools import Toolset @@ `@pytest.fixture`(scope="module") -def karpenter_toolset(): +def karpenter_toolset() -> Toolset: @@ -def test_karpenter_toolset_metadata(karpenter_toolset): +def test_karpenter_toolset_metadata(karpenter_toolset: Toolset) -> None: @@ -def test_karpenter_toolset_has_all_expected_tools(karpenter_toolset): +def test_karpenter_toolset_has_all_expected_tools(karpenter_toolset: Toolset) -> None: @@ -def test_karpenter_toolset_prerequisites(karpenter_toolset): +def test_karpenter_toolset_prerequisites(karpenter_toolset: Toolset) -> None: @@ -def test_karpenter_tool_parameters_are_inferred(karpenter_toolset): +def test_karpenter_tool_parameters_are_inferred(karpenter_toolset: Toolset) -> None: @@ -def test_karpenter_commands_render_with_params(karpenter_toolset): +def test_karpenter_commands_render_with_params(karpenter_toolset: Toolset) -> None:As per coding guidelines, "Type hints are required throughout the codebase (mypy configuration in pyproject.toml)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_karpenter_toolset.py` around lines 33 - 101, Update the new tests to include type annotations: annotate the karpenter_toolset fixture return type (e.g., -> Toolset or the actual toolset class used by load_toolsets_from_file) and annotate the test function parameters to accept that fixture (karpenter_toolset: Toolset) and all test functions to return None (e.g., def test_karpenter_toolset_metadata(karpenter_toolset: Toolset) -> None). Ensure imports include the Toolset type or typing.Any if the concrete type is unavailable, and reference load_toolsets_from_file, KARPENTER_YAML, and the fixture name karpenter_toolset when applying the annotations.docs/data-sources/builtin-toolsets/karpenter.md (1)
3-3: Avoid listing toolset capabilities in the intro.This enumerates specific diagnostic capabilities that can become stale as tools evolve. Keep the intro generic and let users discover capabilities through Holmes.
Proposed wording
-This toolset lets HolmesGPT investigate [Karpenter](https://karpenter.sh/) node autoscaling — why pods stay `Pending`, why a `NodeClaim` never becomes a real `Node`, and why Karpenter is disrupting (consolidating, expiring, drifting) nodes you didn't expect. +Use this toolset to connect HolmesGPT with [Karpenter](https://karpenter.sh/) for node autoscaling investigations.As per coding guidelines, "
docs/data-sources/builtin-toolsets/**/*.md: Don't list what a toolset/integration can do in documentation - users discover capabilities by using Holmes, and feature lists become stale quickly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data-sources/builtin-toolsets/karpenter.md` at line 3, The intro sentence beginning "This toolset lets HolmesGPT investigate Karpenter node autoscaling — why pods stay `Pending`, why a `NodeClaim` never becomes a real `Node`, and why Karpenter is disrupting (consolidating, expiring, drifting) nodes you didn't expect." enumerates capabilities and should be replaced with a short, generic description; remove the specific diagnostic examples and any capability list, and instead use a single-line, high-level intro that says the doc describes the Karpenter toolset for HolmesGPT without listing what it can do so the content won't become stale (locate and edit the intro paragraph in karpenter.md where that sentence appears).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/data-sources/builtin-toolsets/karpenter.md`:
- Around line 37-55: Replace the five separate fenced code blocks with a single
fenced bash block that contains all five holmes ask commands in order (the lines
starting with holmes ask "Why are my pending pods...", holmes ask "I have a
NodeClaim stuck in Unknown...", holmes ask "Which NodePool would satisfy...",
holmes ask "Why did Karpenter terminate node ip-10-0-12-34.ec2.internal earlier
today?", and holmes ask "Is my EC2NodeClass default picking up the correct
subnets and security groups?"), removing the repeated ```bash fences so the
commands are consolidated into one code block.
In `@holmes/plugins/toolsets/karpenter.yaml`:
- Around line 68-70: The command string in the karpenter_disruption_events entry
uses an invalid multi-value field selector (reason=Disrupted,DisruptionBlocked);
update the command for the "karpenter_disruption_events" item to remove the
--field-selector portion and rely on --sort-by=.lastTimestamp (or perform
client-side filtering after retrieving events) so kubectl get events -A
--sort-by=.lastTimestamp is used instead of the current invalid selector.
In `@README.md`:
- Line 63: Add a Karpenter-specific logo under images/integration_logos (e.g.,
karpenter-icon.png) and update the README row that currently references
images/integration_logos/kubernetes-icon.png to use the new
images/integration_logos/karpenter-icon.png (the markdown line containing the
Karpenter link and Kubernetes img tag). Also ensure the new asset name is used
consistently in other docs mentioned in the comment
(docs/walkthrough/why-holmesgpt.md, docs/data-sources/builtin-toolsets/index.md,
and docs/data-sources/builtin-toolsets/{karpenter}.md) so the integration
displays the Karpenter logo everywhere.
---
Nitpick comments:
In `@docs/data-sources/builtin-toolsets/karpenter.md`:
- Line 3: The intro sentence beginning "This toolset lets HolmesGPT investigate
Karpenter node autoscaling — why pods stay `Pending`, why a `NodeClaim` never
becomes a real `Node`, and why Karpenter is disrupting (consolidating, expiring,
drifting) nodes you didn't expect." enumerates capabilities and should be
replaced with a short, generic description; remove the specific diagnostic
examples and any capability list, and instead use a single-line, high-level
intro that says the doc describes the Karpenter toolset for HolmesGPT without
listing what it can do so the content won't become stale (locate and edit the
intro paragraph in karpenter.md where that sentence appears).
In `@tests/plugins/toolsets/test_karpenter_toolset.py`:
- Around line 33-101: Update the new tests to include type annotations: annotate
the karpenter_toolset fixture return type (e.g., -> Toolset or the actual
toolset class used by load_toolsets_from_file) and annotate the test function
parameters to accept that fixture (karpenter_toolset: Toolset) and all test
functions to return None (e.g., def
test_karpenter_toolset_metadata(karpenter_toolset: Toolset) -> None). Ensure
imports include the Toolset type or typing.Any if the concrete type is
unavailable, and reference load_toolsets_from_file, KARPENTER_YAML, and the
fixture name karpenter_toolset when applying the annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 341bf78a-b6c0-44ee-9f82-3838e38ee0f4
📥 Commits
Reviewing files that changed from the base of the PR and between b24a252 and fbe6b1c539ecee5b1b80f60cd41b81231316effb.
📒 Files selected for processing (7)
README.mddocs/data-sources/builtin-toolsets/.nav.ymldocs/data-sources/builtin-toolsets/index.mddocs/data-sources/builtin-toolsets/karpenter.mddocs/why-holmesgpt.mdholmes/plugins/toolsets/karpenter.yamltests/plugins/toolsets/test_karpenter_toolset.py
|
Thanks for picking this up, @mrinalpravi, this implements the proposal I filed in #1923. Glad to see it moving so quickly. The YAML follows the argocd.yaml template cleanly and the A few substantive suggestions from someone who's been scoping this for a few days. Happy to open inline review comments if the maintainers want them broken out:
Provider extensibility (Azure AKSNodeClass, GCP) is a natural follow-up I'm happy to pick up as a separate PR once this lands. Also flagging #1802 (toolset config rework) as potentially Again, nice work — looking forward to this shipping. |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/data-sources/builtin-toolsets/karpenter.md (1)
7-8: Consider simplifying the toolset descriptions.Lines 7-8 enumerate specific tools and resources in detail, which could become stale as the toolsets evolve. Consider simplifying to conceptual descriptions that explain the split without listing specific features:
-- **`karpenter/core`** — cloud-agnostic tools: NodePools, NodeClaims, disruption events, controller logs, pending pods. Works on any cluster running upstream Karpenter (AWS, Azure, GCP, on-prem). -- **`karpenter/aws`** — AWS-specific tools for inspecting `EC2NodeClass` resources (AMI selectors, subnets, security groups, IAM instance profile, userdata). Requires the Karpenter AWS provider CRDs. +- **`karpenter/core`** — cloud-agnostic Karpenter resources and diagnostics. Works on any cluster running upstream Karpenter (AWS, Azure, GCP, on-prem). +- **`karpenter/aws`** — AWS-specific infrastructure inspection via EC2NodeClass. Requires the Karpenter AWS provider CRDs.As per coding guidelines, "Don't list what a toolset/integration can do in documentation - users discover capabilities by using Holmes, and feature lists become stale quickly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data-sources/builtin-toolsets/karpenter.md` around lines 7 - 8, Replace the detailed feature lists for the karpenter toolsets with concise conceptual descriptions: update the "karpenter/core" entry to say it provides cloud-agnostic Karpenter observability (e.g., node lifecycle, disruption and controller diagnostics) without enumerating specific resources, and update "karpenter/aws" to note it contains AWS-specific extensions (provider CRD integrations and AWS-related configuration) without listing AMI selectors, subnets, security groups, or other concrete fields; keep references to the split between core and AWS provider but remove the fragile feature-itemization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/data-sources/builtin-toolsets/karpenter.md`:
- Around line 7-8: Replace the detailed feature lists for the karpenter toolsets
with concise conceptual descriptions: update the "karpenter/core" entry to say
it provides cloud-agnostic Karpenter observability (e.g., node lifecycle,
disruption and controller diagnostics) without enumerating specific resources,
and update "karpenter/aws" to note it contains AWS-specific extensions (provider
CRD integrations and AWS-related configuration) without listing AMI selectors,
subnets, security groups, or other concrete fields; keep references to the split
between core and AWS provider but remove the fragile feature-itemization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d24c02aa-edd2-4fe8-88af-d8177234273d
📥 Commits
Reviewing files that changed from the base of the PR and between fbe6b1c539ecee5b1b80f60cd41b81231316effb and 9e1d7c685d8987f5d04009c5cb4b3ce33c6b28db.
⛔ Files ignored due to path filters (1)
images/integration_logos/karpenter-icon.pngis excluded by!**/*.png
📒 Files selected for processing (4)
README.mddocs/data-sources/builtin-toolsets/karpenter.mdholmes/plugins/toolsets/karpenter.yamltests/plugins/toolsets/test_karpenter_toolset.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- tests/plugins/toolsets/test_karpenter_toolset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/karpenter.yaml
|
Hi @AllaniAnirudh, Thanks for the detailed review I've addressed the review comments:
cc - @moshemorad |
|
Unit test |
|
Thanks @mrinalpravi for the quick turnaround, pulled d9898c6 and walked through the diff. The split into karpenter/core + karpenter/aws with separate CRD prerequisites cleanly resolves the AKS/GKE silent-failure, the ns A few small, non-blocking nits if you want to polish further:
Nice work overall, the live EKS verification is reassuring. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/core/test_toolset_manager.py (1)
579-719: Add type annotations to the new tests.The new helper is annotated, but the added test functions still need
-> None; fixture parameters liketmp_pathandmonkeypatchshould also be typed where used.As per coding guidelines,
**/*.py: Type hints are required throughout the codebase (mypy configuration in pyproject.toml).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_toolset_manager.py` around lines 579 - 719, All new test functions need explicit return type annotations and fixture parameter types: add "-> None" to each test (e.g. test_override_with_copies_set_fields, test_override_with_does_not_override_name, test_override_with_enabled_false_propagates, test_override_with_does_not_touch_unset_fields, test_override_with_skips_empty_values, test_override_with_preserves_env_var_resolution_from_yaml_file, test_override_with_handles_toolset_with_callable_prerequisites, test_override_with_full_flow_through_toolset_manager) and annotate fixtures where used (e.g. tmp_path: Path, monkeypatch: pytest.MonkeyPatch); also ensure you import Path from pathlib and pytest (or the MonkeyPatch type) at the top of the test file so the annotations resolve for mypy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/test_toolset_manager.py`:
- Around line 646-675: The test
test_override_with_preserves_env_var_resolution_from_yaml_file uses a fixture
key named "secret" which triggers Ruff S105 (hardcoded secret) — change the YAML
key name (and any other occurrences in the nearby test block at ~698-719) from
"secret" to a neutral name like "placeholder" or "token", update all references
in the test (e.g., target.config["secret"] -> target.config["placeholder"]) and
in the written YAML content so the env-var substitution logic in ToolsetManager
and override_with remains the same but the linter no longer flags it.
- Around line 567-719: Add an integration-style test fixture that actually
exercises the Karpenter toolsets (karpenter/core and karpenter/aws) end-to-end:
create a new test under tests/llm/fixtures (following
tests/llm/fixtures/test_ask_holmes/ or test_holmes_checks/) which writes a
custom toolsets.yaml enabling the karpenter toolsets, instantiates
ToolsetManager with that file, loads the toolsets (use
manager._list_all_toolsets(check_prerequisites=False) or the existing ask_holmes
harness), and drives an LLM query that triggers Karpenter diagnostic tools;
assert that rendered tool invocations and returned outputs match expected
realistic patterns (e.g., NodePool/NodeClaim inspection, disruption events).
Mock the LLM/backend where needed to return deterministic responses and
reference the toolset names 'karpenter/core' and 'karpenter/aws' and the
ToolsetManager symbol to locate integration points.
---
Nitpick comments:
In `@tests/core/test_toolset_manager.py`:
- Around line 579-719: All new test functions need explicit return type
annotations and fixture parameter types: add "-> None" to each test (e.g.
test_override_with_copies_set_fields, test_override_with_does_not_override_name,
test_override_with_enabled_false_propagates,
test_override_with_does_not_touch_unset_fields,
test_override_with_skips_empty_values,
test_override_with_preserves_env_var_resolution_from_yaml_file,
test_override_with_handles_toolset_with_callable_prerequisites,
test_override_with_full_flow_through_toolset_manager) and annotate fixtures
where used (e.g. tmp_path: Path, monkeypatch: pytest.MonkeyPatch); also ensure
you import Path from pathlib and pytest (or the MonkeyPatch type) at the top of
the test file so the annotations resolve for mypy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2faa7f7f-bdd8-46ae-8ba8-c55117aae94b
📥 Commits
Reviewing files that changed from the base of the PR and between d9898c6199b845a1572c831980092c3db8ee0415 and d5100a3e96163f090ea47038c2b1ee9585955e0e.
📒 Files selected for processing (2)
holmes/core/tools.pytests/core/test_toolset_manager.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/karpenter.yaml`:
- Around line 1-2: Add integration tests alongside
tests/plugins/toolsets/test_karpenter_toolset.py that run against a real cluster
(use a pytest marker like `@pytest.mark.integration`) to validate prerequisite
gating and runtime behavior: implement checks that query the cluster for the
NodePool CRD when exercising karpenter/core and for the EC2NodeClass CRD when
exercising karpenter/aws (skip the test with a clear message if the CRD is
absent), then execute at least one real command path which invokes kubectl (or
the same command wrapper used by the toolset code) against an actual Karpenter
cluster and assert success; reference the existing test file name and the
toolset identifiers karpenter/core and karpenter/aws to locate where to add
these integration cases.
- Around line 64-66: The pipeline in the karpenter_disruption_events command
masks upstream failures; update the command string for the
karpenter_disruption_events entry so the shell runs with pipefail enabled (e.g.,
use bash -o pipefail -c "<pipeline>") so that if kubectl fails due to RBAC/API
errors the whole pipeline exits non‑zero; locate the karpenter_disruption_events
block and replace the raw pipeline in the command value with a bash -o pipefail
invocation wrapping the existing kubectl | grep | tail pipeline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f54cd15-da9d-4994-b393-c64d128c4feb
📥 Commits
Reviewing files that changed from the base of the PR and between d5100a3e96163f090ea47038c2b1ee9585955e0e and f28f3f70955244a106becc1684109b8ddffd7fb4.
📒 Files selected for processing (2)
holmes/plugins/toolsets/karpenter.yamltests/plugins/toolsets/test_karpenter_toolset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/plugins/toolsets/test_karpenter_toolset.py
Adds the karpenter/core and karpenter/aws toolsets for investigating Karpenter node autoscaling (NodePools, NodeClaims, disruption events, controller logs, EC2NodeClass inspection). Includes: - holmes/plugins/toolsets/karpenter.yaml - tests/plugins/toolsets/test_karpenter_toolset.py (YAML loader unit tests) - tests/core/test_toolset_manager.py (ToolsetManager integration tests) - docs/data-sources/builtin-toolsets/karpenter.md - images/integration_logos/karpenter-icon.png Signed-off-by: mrinalpravi <mrinalpr1998@gmail.com>
42d7c48 to
1b67831
Compare
|
Hi @AllaniAnirudh , Thanks for the review. Addressed the review comments
|
Signed-off-by: mrinalpravi <mrinalpr1998@gmail.com>
|
Hi @moshemorad , this PR has been in open state for 2 months. Could you please review when you get a chance. Thanks! |
This branch has not been deployed
3 participants
Adds a new built-in
karpenter/coretoolset so HolmesGPT can investigate Karpenter-driven node autoscaling issues directly, alongside existing Kubernetes-ecosystem toolsets (ArgoCD, Helm, Cilium, KubeVela).Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes