Skip to content

feat: Install telemetry CRDs via crd-installer #312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jthakkar04 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Install telemetry CRDs via crd-installer #312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9b4055
chore: Vendor telemetry CRDs
jthakkar04 Aug 3, 2026
b2b65c3
feat: Embed telemetry CRDs in crd-installer
jthakkar04 Aug 3, 2026
997bfcc
feat: Always install telemetry CRDs
jthakkar04 Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and
$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="{./api/v1,./api/v2}"

.PHONY: generate-vendored
generate-vendored: ## Regenerate vendored Moco CRDs from the operator's Helm chart dependency.
generate-vendored: ## Regenerate vendored Moco/VictoriaMetrics/Grafana CRDs from the operator's Helm chart dependencies.
@helm dependency update deploy/operator >/dev/null
@tar -xzOf deploy/operator/charts/moco-*.tgz moco/templates/generated/crds/moco_crds.yaml | \
sed -e '/^{{/d' \
Expand All @@ -62,15 +62,24 @@ generate-vendored: ## Regenerate vendored Moco CRDs from the operator's Helm cha
-e "s|: '{{ .Release.Namespace }}'|: moco-system|g" \
> pkg/vendored/moco/crds/moco_crds.yaml
@echo "Regenerated pkg/vendored/moco/crds/moco_crds.yaml"
@mkdir -p pkg/vendored/victoria-metrics-operator/crds pkg/vendored/grafana-operator/crds
@tar -xzOf deploy/operator/charts/victoria-metrics-operator-*.tgz victoria-metrics-operator/crd.yaml \
> pkg/vendored/victoria-metrics-operator/crds/vm_crds.yaml
@rm -f pkg/vendored/grafana-operator/crds/*.yaml
@tar -xzf deploy/operator/charts/grafana-operator-*.tgz -C pkg/vendored/grafana-operator/crds \
--strip-components=3 grafana-operator/files/crds
Comment on lines +65 to +70

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Make CRD replacement all-or-nothing.

Both recipes remove or overwrite existing CRD artifacts before all reads and copies succeed. A failed tar or cp can leave generated inputs incomplete.

  • Makefile#L65-L70: stage VictoriaMetrics and Grafana extraction, validate the results, then replace vendored files.
  • Makefile#L75-L81: stage all source copies, then replace internal/crdinstaller/crds after every copy succeeds.
📍 Affects 1 file
  • Makefile#L65-L70 (this comment)
  • Makefile#L75-L81
🤖 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 `@Makefile` around lines 65 - 70, The Makefile CRD generation recipes must
replace existing artifacts only after every extraction or copy succeeds. At
Makefile lines 65-70, stage VictoriaMetrics and Grafana outputs in a temporary
location, validate both results, then atomically replace the vendored CRD
directories; at lines 75-81, stage all source copies first and replace
internal/crdinstaller/crds only after every copy completes successfully.

@echo "Regenerated vendored VictoriaMetrics + Grafana CRDs"

.PHONY: sync-crd-embed
sync-crd-embed: manifests ## Sync embedded CRDs in internal/crdinstaller/crds from their source-of-truth locations.
@mkdir -p internal/crdinstaller/crds/operator internal/crdinstaller/crds/redis internal/crdinstaller/crds/clickhouse
@rm -f internal/crdinstaller/crds/operator/*.yaml internal/crdinstaller/crds/redis/*.yaml internal/crdinstaller/crds/clickhouse/*.yaml
@mkdir -p internal/crdinstaller/crds/operator internal/crdinstaller/crds/redis internal/crdinstaller/crds/clickhouse internal/crdinstaller/crds/victoriametrics internal/crdinstaller/crds/grafana
@rm -f internal/crdinstaller/crds/operator/*.yaml internal/crdinstaller/crds/redis/*.yaml internal/crdinstaller/crds/clickhouse/*.yaml internal/crdinstaller/crds/victoriametrics/*.yaml internal/crdinstaller/crds/grafana/*.yaml
@cp config/crd/bases/apps.wandb.com_*.yaml internal/crdinstaller/crds/operator/
@cp pkg/vendored/redis-operator/crds/*.yaml internal/crdinstaller/crds/redis/
@cp pkg/vendored/altinity-clickhouse/crds/clickhouse.altinity.com_clickhouseinstallations.yaml internal/crdinstaller/crds/clickhouse/
@echo "Synced CRDs into internal/crdinstaller/crds/{operator,redis,clickhouse}/"
@cp pkg/vendored/victoria-metrics-operator/crds/*.yaml internal/crdinstaller/crds/victoriametrics/
@cp pkg/vendored/grafana-operator/crds/*.yaml internal/crdinstaller/crds/grafana/
@echo "Synced CRDs into internal/crdinstaller/crds/{operator,redis,clickhouse,victoriametrics,grafana}/"

.PHONY: fmt
fmt: ## Run go fmt against code.
Expand Down
3 changes: 2 additions & 1 deletion Tiltfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,8 @@ def build_operator_values(telemetry_namespace):
"victoria-metrics-operator": {
"enabled": telemetry_enabled,
"crds": {
"plain": True,
"enabled": False,
"plain": False,
},
"admissionWebhooks": {
"enabled": False,
Expand Down
2 changes: 2 additions & 0 deletions deploy/operator/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ installed regardless. Add new groups here as new subchart-CRD dependencies land.
{{- if (dig "altinity-clickhouse-operator" "enabled" false .Values.AsMap) -}}
{{- $groups = append $groups "clickhouse" -}}
{{- end -}}
{{- /* Telemetry CRDs install in every mode so an off→full upgrade never has to add CRDs. */ -}}

@danielpanzella danielpanzella Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should only install this when they are needed, but the difference is that the crd-installer will install them during a helm upgrade if they are enabled, which the helm charts were not doing.

{{- $groups = concat $groups (list "victoriametrics" "grafana") -}}
{{- join "," $groups -}}
{{- end -}}

Expand Down
6 changes: 3 additions & 3 deletions deploy/operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ victoria-metrics-operator:
seccompProfile:
type: RuntimeDefault
crds:
# Install Victoria CRDs through Helm's CRD phase so telemetry resources
# in this chart can be created in the same release.
plain: true
# Installed upgrade-safely by the crd-installer Job; subchart ships the controller only.
enabled: false
plain: false

grafana-operator:
# Helm dependency conditions are boolean-only, so full installs
Expand Down
46 changes: 46 additions & 0 deletions internal/controller/reconciler/telemetry_chart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,52 @@ func TestStandaloneTelemetryChartFullModeRendersCoreStack(t *testing.T) {
mustContain(t, output, "datadog:")
}

// The crd-installer Job installs VM + Grafana CRDs in every mode so an off→full
// upgrade never has to add CRDs (which Helm won't do on upgrade).
func TestCrdInstallerAlwaysRequestsTelemetryGroups(t *testing.T) {
modes := []struct {
name string
extra []string
}{
{"off", []string{"--set", "telemetry.mode=off"}},
{"forward", []string{
"--set", "telemetry.mode=forward",
"--set", "telemetry.forwarding.otlp.endpoint=https://otel.example.com",
"--set", "victoria-metrics-operator.enabled=true",
}},
{"full", []string{
"--set", "telemetry.mode=full",
"--set", "victoria-metrics-operator.enabled=true",
"--set", "grafana-operator.enabled=true",
}},
}
for _, m := range modes {
t.Run(m.name, func(t *testing.T) {
args := append([]string{"--set", "helmHooks.enabled=true", "--set", "wandb.install=false"}, m.extra...)
output := runHelmTemplate(t, args...)
groups := crdInstallerGroups(t, output)
for _, g := range []string{"victoriametrics", "grafana"} {
if !strings.Contains(groups, g) {
t.Errorf("crd-installer --groups=%q missing %q", groups, g)
Comment on lines +149 to +151

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Match complete group names instead of substrings.

strings.Contains(groups, g) accepts malformed values such as victoriametrics-extra or grafana-extra. The test can pass without an exact --groups token. Split and trim the comma-separated value, then check exact membership.

Proposed assertion fix 
-			groups := crdInstallerGroups(t, output)
+			groupValue := crdInstallerGroups(t, output)
+			groups := make(map[string]struct{})
+			for _, group := range strings.Split(groupValue, ",") {
+				groups[strings.TrimSpace(group)] = struct{}{}
+			}
 			for _, g := range []string{"victoriametrics", "grafana"} {
-				if !strings.Contains(groups, g) {
-					t.Errorf("crd-installer --groups=%q missing %q", groups, g)
+				if _, ok := groups[g]; !ok {
+					t.Errorf("crd-installer --groups=%q missing exact group %q", groupValue, g)
 				}
 			}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for _, g := range []string{"victoriametrics", "grafana"} {
if !strings.Contains(groups, g) {
t.Errorf("crd-installer --groups=%q missing %q", groups, g)
groupValue := crdInstallerGroups(t, output)
groups := make(map[string]struct{})
for _, group := range strings.Split(groupValue, ",") {
groups[strings.TrimSpace(group)] = struct{}{}
}
for _, g := range []string{"victoriametrics", "grafana"} {
if _, ok := groups[g]; !ok {
t.Errorf("crd-installer --groups=%q missing exact group %q", groupValue, g)
}
}
🤖 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 `@internal/controller/reconciler/telemetry_chart_test.go` around lines 149 -
151, Update the group validation loop in the telemetry chart test to split the
comma-separated groups value, trim each token, and verify exact membership for
“victoriametrics” and “grafana” instead of using strings.Contains on the raw
string.

}
}
})
}
}

// crdInstallerGroups returns the value of the crd-installer Job's --groups flag.

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Explain why the helper scans rendered output.

The comment only restates crdInstallerGroups behavior. Rewrite it to explain the non-obvious reason for scanning the rendered Helm manifest, or remove it.

Proposed comment update 
-// crdInstallerGroups returns the value of the crd-installer Job's --groups flag.
+// Scan rendered YAML so the test verifies the Helm hook's command arguments.

As per coding guidelines, **/*.{go,py,sh,bash}: Do not add inline comments that merely restate what code does; comments should concisely explain why, especially business logic or non-obvious behavior.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// crdInstallerGroups returns the value of the crd-installer Job's --groups flag.
// Scan rendered YAML so the test verifies the Helm hook's command arguments.
🤖 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 `@internal/controller/reconciler/telemetry_chart_test.go` at line 158, Update
the comment above crdInstallerGroups to explain why it must scan the rendered
Helm manifest, focusing on the non-obvious validation purpose; otherwise remove
the comment rather than restating the helper’s behavior.

Source: Coding guidelines

func crdInstallerGroups(t *testing.T, output string) string {
t.Helper()
const marker = "--groups="
for _, line := range strings.Split(output, "\n") {
if i := strings.Index(line, marker); i >= 0 {
return strings.TrimSpace(line[i+len(marker):])
}
}
t.Fatalf("crd-installer --groups= flag not found in rendered output")
return ""
}

func runHelmTemplate(t *testing.T, extraArgs ...string) string {
t.Helper()
output, err := runHelmTemplateWithError(t, filepath.Join("..", "..", "..", "deploy", "operator"), extraArgs...)
Expand Down
46 changes: 46 additions & 0 deletions internal/crdinstaller/compose_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ func TestParseGroupsValid(t *testing.T) {
}
}

func TestParseGroupsAcceptsTelemetryGroups(t *testing.T) {
got, err := ParseGroups("victoriametrics,grafana")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(got) != 2 || got[0] != "victoriametrics" || got[1] != "grafana" {
t.Errorf("got %v, want [victoriametrics grafana]", got)
}
}

func TestParseGroupsRejectsUnknown(t *testing.T) {
_, err := ParseGroups("redis,bogus")
if err == nil {
Expand Down Expand Up @@ -130,6 +140,42 @@ func TestComposeIncludesClickHouseGroup(t *testing.T) {
}
}

func TestComposeIncludesTelemetryGroups(t *testing.T) {
opts := validOpts
opts.Groups = []string{"victoriametrics", "grafana"}
crds, err := compose(opts)
if err != nil {
t.Fatalf("compose failed: %v", err)
}

names := make(map[string]bool, len(crds))
for _, crd := range crds {
names[crd.Name] = true
}
for _, name := range []string{
"vmsingles.operator.victoriametrics.com",
"vmagents.operator.victoriametrics.com",
"vlsingles.operator.victoriametrics.com",
"vtsingles.operator.victoriametrics.com",
"grafanas.grafana.integreatly.org",
"grafanadashboards.grafana.integreatly.org",
"grafanadatasources.grafana.integreatly.org",
} {
if !names[name] {
t.Errorf("expected telemetry CRD %s to be included", name)
}
}

// Upstream CRDs must not carry the cert-manager annotation we inject for operator CRDs.
for _, crd := range crds {
if strings.HasSuffix(crd.Name, ".victoriametrics.com") || strings.HasSuffix(crd.Name, ".grafana.integreatly.org") {
if _, ok := crd.Annotations["cert-manager.io/inject-ca-from"]; ok {
t.Errorf("upstream CRD %s should not have cert-manager annotation", crd.Name)
}
}
}
}

func TestRenderIsDeterministic(t *testing.T) {
opts := validOpts
opts.Groups = []string{"redis", "clickhouse"}
Expand Down
Loading
Loading