Skip to content

Conversation

@dmitsh
Copy link
Collaborator

@dmitsh dmitsh commented Nov 12, 2025

No description provided.

@dmitsh dmitsh marked this pull request as draft November 12, 2025 19:28
@greptile-apps
Copy link

greptile-apps bot commented Nov 12, 2025

Greptile Overview

Greptile Summary

This PR adds support for filtering Kubernetes nodes using label selectors in topology configuration. The changes restructure the configuration format from flat (provider/engine strings with engineParams) to nested objects (provider.name/provider.params and engine.name/engine.params).

Key Changes:

  • Refactored Request struct in pkg/topology/request.go to use nested Provider and Engine types with separate name and params fields
  • Added optional nodeSelector parameter support to k8s engine, slinky engine, DRA provider, and InfiniBand provider
  • Updated GetNodes utility function to accept optional ListOptions for label-based filtering
  • Modified Helm chart structure: moved from global.provider (string) to global.provider.name with global.provider.params.nodeSelector
  • Updated all example YAML files to demonstrate the new nested configuration structure
  • Added comprehensive unit tests for parameter validation in engines and providers
  • Bumped chart versions from 0.1.0 to 0.2.0 for breaking configuration changes

The implementation is consistent across all components and maintains backward compatibility by making nodeSelector optional.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-structured, thoroughly tested, and follow consistent patterns across the codebase. All new functionality includes unit tests, the API changes are properly encapsulated, and the optional nature of nodeSelector maintains backward compatibility. The refactoring from flat to nested configuration is a breaking change but is handled appropriately with version bumps and updated examples.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
pkg/topology/request.go 5/5 Refactored request structure to use nested Provider and Engine structs with name and params fields
pkg/engines/k8s/engine.go 5/5 Added nodeSelector parameter support to filter K8s nodes for topology configuration
pkg/engines/slinky/engine.go 5/5 Added nodeSelector parameter support alongside existing podSelector for filtering nodes
pkg/providers/dra/provider.go 5/5 Added nodeSelector parameter support to filter nodes for DRA provider
pkg/providers/infiniband/provider_k8s.go 5/5 Added nodeSelector parameter support to filter nodes for InfiniBand provider
charts/topograph/values.yaml 5/5 Restructured config from flat to nested structure with provider.name/params and engine.name/params
pkg/node_observer/controller.go 5/5 Updated to use new nested Provider and Engine structs in topology requests
internal/k8s/utils.go 5/5 Added optional ListOptions parameter to GetNodes function for label selector filtering

Sequence Diagram

sequenceDiagram
    participant User
    participant HelmChart
    participant NodeObserver
    participant TopographServer
    participant Engine
    participant Provider
    participant K8sAPI

    User->>HelmChart: Deploy with nodeSelector config
    HelmChart->>NodeObserver: Configure with provider.params.nodeSelector
    HelmChart->>TopographServer: Deploy topology service
    
    NodeObserver->>TopographServer: POST /v1/generate (Provider + Engine params)
    TopographServer->>Engine: Initialize with engine.params (including nodeSelector)
    TopographServer->>Provider: Initialize with provider.params (including nodeSelector)
    
    Engine->>K8sAPI: GetNodes(ctx, client, nodeListOpt)
    Note over Engine,K8sAPI: Filter nodes using label selector
    K8sAPI-->>Engine: Filtered node list
    
    Provider->>K8sAPI: GetNodes(ctx, client, nodeListOpt)
    Note over Provider,K8sAPI: Filter nodes using label selector
    K8sAPI-->>Provider: Filtered node list
    
    Provider-->>TopographServer: Topology tree with filtered nodes
    Engine-->>TopographServer: Apply topology to filtered nodes
    TopographServer-->>NodeObserver: Topology configuration result
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

21 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 59.01639% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.26%. Comparing base (00c087f) to head (f9840dc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/providers/infiniband/provider_k8s.go 57.14% 6 Missing ⚠️
pkg/engines/k8s/engine.go 61.53% 5 Missing ⚠️
pkg/providers/dra/provider.go 61.53% 5 Missing ⚠️
internal/k8s/utils.go 0.00% 4 Missing ⚠️
pkg/engines/slinky/engine.go 70.00% 3 Missing ⚠️
pkg/engines/k8s/kubernetes.go 0.00% 1 Missing ⚠️
pkg/node_observer/controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
+ Coverage   65.03%   65.26%   +0.23%     
==========================================
  Files          77       77              
  Lines        4187     4221      +34     
==========================================
+ Hits         2723     2755      +32     
- Misses       1357     1359       +2     
  Partials      107      107              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dmitsh dmitsh force-pushed the ds-selector branch 2 times, most recently from 2399940 to 057a165 Compare November 12, 2025 23:16
@dmitsh dmitsh marked this pull request as ready for review November 12, 2025 23:47
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

29 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@dmitsh dmitsh merged commit 4ed41ea into main Nov 14, 2025
8 checks passed
@dmitsh dmitsh deleted the ds-selector branch November 14, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants