perf: filter workflows on K8s API during infractl list API calls#1882
Conversation
|
A single node development cluster (infra-pr-1882) was allocated in production infra for this PR. CI will attempt to deploy 🔌 You can connect to this cluster with: 🛠️ And pull infractl from the deployed dev infra-server with: 🔓 You must go to to export an . Your token from the prod infra instance will not work with dev environments. 🚲 You can then use the dev infra instance e.g.: Further Development☕ If you make changes, you can commit and push and CI will take care of updating the development cluster. 🚀 If you only modify configuration (chart/infra-server/configuration) or templates (chart/infra-server/{static,templates}), you can get a faster update with: LogsLogs for the development infra depending on your @redhat.com authuser: Or: |
📝 WalkthroughWalkthroughCluster workflows now use normalized owner and flavor labels, server-side selectors, and deleted-state labels. Artifact retrieval now uses a thread-safe LRU cache with Prometheus metrics and tests covering cache behavior. ChangesCluster workflow filtering
Artifact cache
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant getClusterDetailsFromArtifacts
participant artifactCache
participant signer.Contents
getClusterDetailsFromArtifacts->>artifactCache: Get(bucket, key)
artifactCache-->>getClusterDetailsFromArtifacts: cached content or miss
getClusterDetailsFromArtifacts->>signer.Contents: fetch on cache miss
signer.Contents-->>getClusterDetailsFromArtifacts: artifact content
getClusterDetailsFromArtifacts->>artifactCache: Set(bucket, key, content)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
pkg/service/cluster/helpers_test.go (1)
7-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case tests for label value validity.
The test suite verifies length ≤ 63 but doesn't assert that the result starts and ends with an alphanumeric character (the K8s label value regex requirement). Missing cases:
- Email starting with
+(e.g.,+tag@example.com) — verifies leading-dot trimming after the fix.- Long email where truncation lands on a dash or underscore — verifies trailing non-dot trimming.
- A validity assertion in the test loop:
resultmust match^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$or be empty.♻️ Suggested additional test cases and validation
{ name: "complex email", email: "user.name+test@subdomain.example.com", expected: "user.name.plus.test.at.subdomain.example.com", }, + { + name: "email starting with plus", + email: "+tag@example.com", + expected: "plus.tag.at.example.com", + }, + { + name: "long email truncating on dash", + email: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-tag@example.com", + expected: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := emailToLabelValue(tt.email) if result != tt.expected { t.Errorf("emailToLabelValue(%q) = %q, want %q", tt.email, result, tt.expected) } if len(result) > 63 { t.Errorf("emailToLabelValue(%q) returned %q with length %d, exceeds 63 chars", tt.email, result, len(result)) } + if result != "" && !regexp.MustCompile(`^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$`).MatchString(result) { + t.Errorf("emailToLabelValue(%q) returned %q, not a valid K8s label value", tt.email, result) + } }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/cluster/helpers_test.go` around lines 7 - 51, Extend TestEmailToLabelValue with cases for an email beginning with “+” and a long email whose truncation ends on a dash or underscore, asserting the expected trimmed label values. In the test loop, validate each result against the Kubernetes label-value pattern ^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$ or allow an empty result, while retaining the existing length check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/service/cluster/helpers.go`:
- Around line 292-315: Update buildLabelSelector so non-all requests with an
empty email are rejected before constructing the selector, returning an
appropriate error instead of allowing an owner-less or flavor-only selector.
Preserve the existing owner and allowed-flavor filtering for valid requests and
the unrestricted behavior for req.All.
- Around line 274-288: Update emailToLabelValue to guarantee Kubernetes label
values start and end with alphanumeric characters: after replacing @ and +, trim
leading and trailing non-alphanumeric separators, including dots, dashes, and
underscores, while retaining the 63-character limit and ensuring truncation
cannot leave an invalid trailing character.
---
Nitpick comments:
In `@pkg/service/cluster/helpers_test.go`:
- Around line 7-51: Extend TestEmailToLabelValue with cases for an email
beginning with “+” and a long email whose truncation ends on a dash or
underscore, asserting the expected trimmed label values. In the test loop,
validate each result against the Kubernetes label-value pattern
^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$ or allow an empty result, while
retaining the existing length check.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9d21b494-77e2-4d06-bbe5-dd3873f52d40
📒 Files selected for processing (4)
pkg/service/cluster/cluster.gopkg/service/cluster/helpers.gopkg/service/cluster/helpers_test.gopkg/service/cluster/labels.go
…ly expired clusters during List() and background loops
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/service/cluster/artifact_cache.go (1)
19-22: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant external
sync.RWMutex—lru.Cacheis already thread-safe.
hashicorp/golang-lru/v2'slru.Cacheuses an internalsync.RWMutex; all methods (Get,Add,Len) are safe for concurrent use. The externalmudoubles lock contention on every cache operation with no correctness benefit — Prometheus counters and gauges are individually atomic, so the compoundGet+IncandAdd+Len+Setsequences don't need external atomicity.♻️ Remove the redundant mutex
type artifactCache struct { cache *lru.Cache[string, []byte] - mu sync.RWMutex } // newArtifactCache creates a new artifact cache with the specified size. func newArtifactCache(size int) (*artifactCache, error) { cache, err := lru.New[string, []byte](size) if err != nil { return nil, fmt.Errorf("failed to create LRU cache: %w", err) } return &artifactCache{ cache: cache, }, nil } // Get retrieves cached artifact content if present. // Returns the content and true if found, nil and false otherwise. func (c *artifactCache) Get(bucket, key string) ([]byte, bool) { - c.mu.RLock() - defer c.mu.RUnlock() - cacheKey := makeCacheKey(bucket, key) content, found := c.cache.Get(cacheKey) if !found { metrics.ArtifactCacheMissesCounter.Inc() return nil, false } metrics.ArtifactCacheHitsCounter.Inc() return content, true } // Set stores artifact content in the cache. func (c *artifactCache) Set(bucket, key string, content []byte) { - c.mu.Lock() - defer c.mu.Unlock() - cacheKey := makeCacheKey(bucket, key) c.cache.Add(cacheKey, content) metrics.ArtifactCacheSizeGauge.Set(float64(c.cache.Len())) }Also applies to: 38-51, 54-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/cluster/artifact_cache.go` around lines 19 - 22, Remove the redundant sync.RWMutex field from artifactCache and eliminate its associated locking and unlock calls around cache operations. Keep using lru.Cache methods directly in the affected Get, Add, Len, and metric-update paths, preserving the existing cache behavior and metrics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/service/cluster/artifact_cache.go`:
- Around line 19-22: Remove the redundant sync.RWMutex field from artifactCache
and eliminate its associated locking and unlock calls around cache operations.
Keep using lru.Cache methods directly in the affected Get, Add, Len, and
metric-update paths, preserving the existing cache behavior and metrics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 42e9461f-cb39-4be1-9b6c-905b4476355f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
chart/infra-server/argo-values.yamlgo.modpkg/service/cluster/artifact_cache.gopkg/service/cluster/artifact_cache_test.gopkg/service/cluster/cluster.gopkg/service/cluster/helpers.gopkg/service/cluster/helpers_test.gopkg/service/cluster/labels.gopkg/service/metrics/metrics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/service/cluster/labels.go
davdhacs
left a comment
davdhacs
left a comment
There was a problem hiding this comment.
Some questions/nits, but the core changes look good: Moving the filter server-side looks like a good optimization for handling lists of hundreds of clusters. And caching the cluster details instead of the GCS api calls 100% makes sense.
…t non-all requests with empty email, returning an error instead of silently bypassing the owner filter.
…provides thread safety and not required for prometheus counters
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/service/cluster/cluster.go`:
- Around line 670-671: Handle the error returned by labels.NewRequirement in the
cluster selector construction near clusterIDRequirement instead of discarding
it. If requirement creation fails, return or propagate the error through the
surrounding function’s existing error-handling path before dereferencing
clusterIDRequirement; only add the requirement to labelSelector after successful
creation.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3d65d8b0-9452-4364-aae4-48616bb26f5f
📒 Files selected for processing (4)
pkg/service/cluster/cluster.gopkg/service/cluster/helpers.gopkg/service/cluster/helpers_test.gopkg/service/cluster/labels.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/service/cluster/helpers_test.go
- pkg/service/cluster/helpers.go
…when invalid IDs are used as Kubernetes labels
|
excellent! 🥳 |
3 participants
--> Reduces time spent for List() operations by 10-15% (in realistic scenario, where most clusters belong to other users. In extreme scenario, where current user owns all clusters, no noticable change).
--> Reduces time spent for List() operations by 75% (across different scenarios). Probably other methods like Info(), Get() are also much quicker.
Before deploying this change, all existing workflows need to get the new labels, otherwise they won't show up correctly when requesting specific user or flavors
TODO