Skip to content

Commit 6b41abf

Browse files
Ca-moesclaude
andcommitted
fix(cli): translate to a CR only when that CR exists in the repo
Built-in match rules are always active, so a raw workload whose name coincidentally matches one (a Deployment 'metrics-collector' hitting the OTel '-collector' suffix, a Pod 'foo-3' hitting the CNPG instance pattern) was rewritten to a nonexistent CR identity and silently skipped at discovery — a regression from tier-1-only right-sizing. Gate the rewrite on the CR being resolvable in the repo (anything but a definitive not-found), falling back to the untranslated workload otherwise. RepoScanner caches its index, so the existence check is a cheap lookup. Addresses review finding #3 on #31. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c519512 commit 6b41abf

3 files changed

Lines changed: 54 additions & 4 deletions

File tree

‎docs/explanation/architecture.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ A thin **cli** layer (cobra, [ADR-0005](../adrs/0005-cobra-cli-framework.md)) or
4949

5050
`FieldMapper` is one interface (`Supports` / `ResolvePath` / `Resolve`); the run loop selects the first mapper that supports a manifest and edits the path it resolves, so later tiers slot in without touching callers. The editor (`yamledit`) is path-addressed and knows nothing about kinds.
5151

52-
**Operator-CR translation.** Recommenders report the operator-*generated* workload — KRR sees the Deployment `otel-collector`, or a CNPG instance Pod `mycluster-1`, not the `OpenTelemetryCollector`/`Cluster` CR that lives in the repo. Each tier-2 map carries a **match rule** (workload kind + name suffix/pattern + container→component) that rewrites the reported identity to the owning CR before discovery, collapsing instance pods into a single CR — their per-instance recommendations merged by **max**, so the busiest instance's needs win — and yielding one PR. The built-in match rules are best-effort pending a real `krr -f json` sample from those operators.
52+
**Operator-CR translation.** Recommenders report the operator-*generated* workload — KRR sees the Deployment `otel-collector`, or a CNPG instance Pod `mycluster-1`, not the `OpenTelemetryCollector`/`Cluster` CR that lives in the repo. Each tier-2 map carries a **match rule** (workload kind + name suffix/pattern + container→component) that rewrites the reported identity to the owning CR before discovery, collapsing instance pods into a single CR — their per-instance recommendations merged by **max**, so the busiest instance's needs win — and yielding one PR. The rewrite is committed only when that CR actually exists in the repo; a raw workload whose name coincidentally matches a built-in rule (a Deployment `metrics-collector`, a Pod `foo-3`) falls back to tier-1. The built-in match rules are best-effort pending a real `krr -f json` sample from those operators.
5353

5454
## Scope
5555

‎internal/cli/run.go‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,19 @@ type workloadGroup struct {
5757
// the generated Deployment/Pod) to their owning CR identity before grouping, so
5858
// instance pods collapse into one CR. Raw workloads pass through. A no-op when
5959
// no field maps are configured.
60-
func (r *Runner) translateTargets(targets []adapter.Target) []adapter.Target {
60+
//
61+
// The rewrite is committed only when the target CR actually exists in the repo:
62+
// a real workload whose name coincidentally matches a built-in match rule (e.g.
63+
// a Deployment "metrics-collector", or a bare Pod "foo-3") would otherwise be
64+
// rewritten to a nonexistent CR and silently skipped, instead of being
65+
// right-sized by tier-1.
66+
func (r *Runner) translateTargets(ctx context.Context, targets []adapter.Target) []adapter.Target {
6167
if len(r.FieldMaps.Maps) == 0 {
6268
return targets
6369
}
6470
out := make([]adapter.Target, len(targets))
6571
for i, t := range targets {
66-
if ct, ok := fieldmap.TranslateTarget(t, r.FieldMaps); ok {
72+
if ct, ok := fieldmap.TranslateTarget(t, r.FieldMaps); ok && r.crResolvable(ctx, ct) {
6773
out[i] = ct
6874
} else {
6975
out[i] = t
@@ -72,6 +78,17 @@ func (r *Runner) translateTargets(targets []adapter.Target) []adapter.Target {
7278
return out
7379
}
7480

81+
// crResolvable reports whether a translated CR identity matches a manifest in
82+
// the repo. Anything but a definitive not-found counts as resolvable — a found
83+
// or even ambiguous CR is a real CR (ambiguity is surfaced later by
84+
// processWorkload), and a transient discover error is better handled there than
85+
// by silently falling back — so we only revert to the untranslated workload
86+
// when the CR is genuinely absent.
87+
func (r *Runner) crResolvable(ctx context.Context, t adapter.Target) bool {
88+
_, err := r.Discoverer.Discover(ctx, discover.Workload{Namespace: t.Namespace, Kind: t.Kind, Name: t.Name})
89+
return !errors.Is(err, discover.ErrNotFound)
90+
}
91+
7592
// groupByWorkload collapses per-container targets into one group per workload,
7693
// so each workload yields a single PR.
7794
func groupByWorkload(targets []adapter.Target) []workloadGroup {
@@ -100,7 +117,7 @@ func (r *Runner) Run(ctx context.Context, targets []adapter.Target) error {
100117
if len(r.Mappers) == 0 {
101118
r.Mappers = []fieldmap.FieldMapper{fieldmap.Tier1{}}
102119
}
103-
targets = r.translateTargets(targets)
120+
targets = r.translateTargets(ctx, targets)
104121
var failed int
105122
for _, grp := range groupByWorkload(targets) {
106123
if err := r.processWorkload(ctx, grp); err != nil {

‎internal/cli/runner_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,39 @@ spec:
362362
}
363363
}
364364

365+
func TestRunner_FalsePositiveTranslationFallsBackToTier1(t *testing.T) {
366+
// A plain Deployment named "metrics-collector" matches the OTel built-in's
367+
// "-collector" suffix, but there is no OpenTelemetryCollector CR. It must
368+
// still be right-sized by tier-1, not silently skipped (regression guard).
369+
dir := t.TempDir()
370+
dep := strings.Replace(deployManifest, "name: web", "name: metrics-collector", 1)
371+
path := filepath.Join(dir, "dep.yaml")
372+
if err := os.WriteFile(path, []byte(dep), 0o600); err != nil {
373+
t.Fatal(err)
374+
}
375+
cfg := config.Defaults()
376+
cfg.DryRun = true
377+
maps := fieldmap.MergedMaps(fieldmap.MapConfig{})
378+
var out bytes.Buffer
379+
r := &Runner{
380+
Cfg: cfg, Repo: dir,
381+
Discoverer: &discover.RepoScanner{Root: dir},
382+
Mappers: []fieldmap.FieldMapper{fieldmap.Tier2{Maps: maps}, fieldmap.Tier1{}},
383+
FieldMaps: maps,
384+
Out: &out,
385+
}
386+
targets := []adapter.Target{{
387+
Namespace: "default", Kind: "Deployment", Name: "metrics-collector", Container: "web",
388+
Recommended: adapter.Recommended{Requests: adapter.ResourceValues{CPU: q("250m")}},
389+
}}
390+
if err := r.Run(context.Background(), targets); err != nil {
391+
t.Fatalf("Run: %v", err)
392+
}
393+
if !strings.Contains(out.String(), "250m") {
394+
t.Errorf("a Deployment matching a built-in suffix but with no CR must be tier-1 right-sized:\n%s", out.String())
395+
}
396+
}
397+
365398
func TestRunner_Tier2OperatorCR(t *testing.T) {
366399
// The recommender names the generated Deployment "otel-collector"; rere must
367400
// translate that to the OpenTelemetryCollector CR "otel" and edit its

0 commit comments

Comments
 (0)