WIP: remove runtime extractor manifests for testing#1260
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughGo language version is upgraded from 1.24.6 to 1.25.0 with toolchain directive removed. A linter suppression is added to the gendoc tool. Kubernetes deployment manifest is restructured with three new environment variables injected. The insights-runtime-extractor DaemonSet manifest is deleted. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opokornyy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/hold For testing purposes, this change allows building a cluster in which the CVO is not responsible for runtime-extractor deployment. |
openshift-ci
Bot
added
the
do-not-merge/hold
opokornyy
changed the title
Update the golang version to 1.25 that is already used in the Insights Operator Dockerfile. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
This function is used to generate documentation and is deprecated since go 1.25, but still working. It is not a crutial function for the Insights Operator. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
This commit removes runtime extractor manifests used by the CVO operator for deployment. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Add required environment variables that should be populated by CVO operator with the images used to deploy runtime-extractor. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
remove-extractor-manifests
branch
from
02540e1 to
b31220e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/gendoc/main.go (1)
183-183: Good short-term suppression; track migration togolang.org/x/tools/go/packages.The
nolintis acceptable, but migrate togolang.org/x/tools/go/packages.LoadwithMode: packages.NeedSyntax | packages.NeedCompiledGoFilesto replace the deprecatedparser.ParseDir. This preserves comment and doc parsing while respecting build tags. Tie this to a follow-up issue to complete the migration before the API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gendoc/main.go` at line 183, The code currently uses the deprecated parser.ParseDir (seen at the call to parser.ParseDir) with a nolint; replace this with golang.org/x/tools/go/packages.Load by creating a packages.Config and calling packages.Load with Mode set to packages.NeedSyntax | packages.NeedCompiledGoFiles so comments/doc nodes are preserved and build tags are respected; update any downstream logic that iterates the returned packages' Syntax/Files instead of the ast.Package map, ensure fset handling is preserved, and add a follow-up issue to complete and validate the migration before the parser API is removed.go.mod (1)
3-3: Adding explicittoolchain go1.25.0is optional but recommended for stricter reproducibility.Setting
go 1.25.0alone provides reproducibility—it implicitly resolves togo1.25.0toolchain by default in Go 1.25. However, adding an explicittoolchain go1.25.0directive (or pinning a specific patch likego1.25.5) provides more precise control and ensures identical binaries across CI and local builds. Per Go's official toolchain documentation, explicit toolchain pinning is a best practice for reproducible builds, especially in CI environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, Add an explicit toolchain directive to go.mod to pin the Go toolchain version for stricter reproducibility: update the file containing the existing "go 1.25.0" line by adding a "toolchain go1.25.0" directive (or a patch-pinned variant like "toolchain go1.25.5") so CI and local builds use the exact same Go toolchain; ensure the change sits alongside the existing go directive in go.mod and commit the updated file.manifests/06-deployment.yaml (1)
50-52: Use block-styleemptyDirfor thesnapshotsvolume to keep thesizeLimitnote actionable.Line 51 (
emptyDir: {}) and Line 52 (commentedsizeLimit) are a bit brittle for future edits. Block form is easier to safely modify later.♻️ Suggested cleanup
- name: snapshots - emptyDir: {} - `#sizeLimit`: 1Gi # bug https://bugzilla.redhat.com/show_bug.cgi?id=1713207 + emptyDir: + # sizeLimit: 1Gi # bug https://bugzilla.redhat.com/show_bug.cgi?id=1713207🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/06-deployment.yaml` around lines 50 - 52, The snapshots volume uses inline emptyDir (`emptyDir: {}`) which makes the commented sizeLimit brittle; change the volume named "snapshots" to use block-style emptyDir (i.e., `emptyDir:` on its own line) and move the commented `#sizeLimit: 1Gi` underneath it with correct YAML indentation so the note remains actionable and safe to edit later; update the volume definition where `snapshots` and `emptyDir` appear (preserve the comment text and indentation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/06-deployment.yaml`:
- Around line 105-110: The manifests include
RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR,
RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER, and RELATED_IMAGE_KUBE_RBAC_PROXY env
vars but no workload manifest or reconciliation uses them; either (A) add a
runtime-extractor workload manifest (Deployment or DaemonSet) that uses those
env vars as the image values and sets the expected pod labels referenced by
pkg/gatherers/workloads/gather_workloads_runtime_infos.go so the gatherer finds
running pods, or (B) implement controller reconciliation in the operator to
create/update the runtime-extractor workload using the env vars
(RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR,
RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER, RELATED_IMAGE_KUBE_RBAC_PROXY) and
ensure the created pods have the labels and owner refs the gatherer expects;
update tests and manifests accordingly so the gatherer no longer errors with “no
running pods found for the insights-runtime-extractor statefulset.”
---
Nitpick comments:
In `@cmd/gendoc/main.go`:
- Line 183: The code currently uses the deprecated parser.ParseDir (seen at the
call to parser.ParseDir) with a nolint; replace this with
golang.org/x/tools/go/packages.Load by creating a packages.Config and calling
packages.Load with Mode set to packages.NeedSyntax |
packages.NeedCompiledGoFiles so comments/doc nodes are preserved and build tags
are respected; update any downstream logic that iterates the returned packages'
Syntax/Files instead of the ast.Package map, ensure fset handling is preserved,
and add a follow-up issue to complete and validate the migration before the
parser API is removed.
In `@go.mod`:
- Line 3: Add an explicit toolchain directive to go.mod to pin the Go toolchain
version for stricter reproducibility: update the file containing the existing
"go 1.25.0" line by adding a "toolchain go1.25.0" directive (or a patch-pinned
variant like "toolchain go1.25.5") so CI and local builds use the exact same Go
toolchain; ensure the change sits alongside the existing go directive in go.mod
and commit the updated file.
In `@manifests/06-deployment.yaml`:
- Around line 50-52: The snapshots volume uses inline emptyDir (`emptyDir: {}`)
which makes the commented sizeLimit brittle; change the volume named "snapshots"
to use block-style emptyDir (i.e., `emptyDir:` on its own line) and move the
commented `#sizeLimit: 1Gi` underneath it with correct YAML indentation so the
note remains actionable and safe to edit later; update the volume definition
where `snapshots` and `emptyDir` appear (preserve the comment text and
indentation).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 68bf9f0a-ba56-4c64-8697-d80e6f23fd8f
📒 Files selected for processing (4)
cmd/gendoc/main.gogo.modmanifests/06-deployment.yamlmanifests/10-insights-runtime-extractor.yaml
💤 Files with no reviewable changes (1)
- manifests/10-insights-runtime-extractor.yaml
| - name: RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR | ||
| value: quay.io/openshift/origin-insights-runtime-extractor:latest | ||
| - name: RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER | ||
| value: quay.io/openshift/origin-insights-runtime-exporter:latest | ||
| - name: RELATED_IMAGE_KUBE_RBAC_PROXY | ||
| value: quay.io/openshift/origin-kube-rbac-proxy:latest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm code reads the three env vars added in manifests/06-deployment.yaml
rg -n --type=go -C3 'Getenv\("RELATED_IMAGE_(INSIGHTS_RUNTIME_EXTRACTOR|INSIGHTS_RUNTIME_EXPORTER|KUBE_RBAC_PROXY)"\)'
# 2) Find where runtime-extractor/runtime-exporter/kube-rbac-proxy images are composed/applied
rg -n --type=go -C3 'insights-runtime-extractor|insights-runtime-exporter|kube-rbac-proxy|RELATED_IMAGE_'
# 3) Locate the reconciler/build path for Kubernetes objects that should now use those env vars
rg -n --type=go -C3 'DaemonSet|Deployment|Service|Create|Apply|Reconcile'Repository: openshift/insights-operator
Length of output: 1797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for usage of the three RELATED_IMAGE env vars
echo "=== Searching for env var reads ==="
rg -n --type=go 'RELATED_IMAGE_(INSIGHTS_RUNTIME_EXTRACTOR|INSIGHTS_RUNTIME_EXPORTER|KUBE_RBAC_PROXY)' -C 3
echo -e "\n=== Searching for Getenv usage ==="
rg -n --type=go 'Getenv.*RELATED_IMAGE' -C 3
echo -e "\n=== Searching for runtime-extractor image references ==="
rg -n --type=go 'insights-runtime-extractor.*image|runtime-extractor.*image' -C 3
echo -e "\n=== Searching for deployment reconciliation with runtime ==="
rg -n --type=go 'runtime.*Deployment|DaemonSet.*runtime' -C 3Repository: openshift/insights-operator
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for any RELATED_IMAGE in entire codebase ==="
rg -n 'RELATED_IMAGE' -C 3
echo -e "\n=== Checking manifests directory structure ==="
fd -t f '.*\.yaml' manifests/ | head -20
echo -e "\n=== Looking at manifests/06-deployment.yaml for full context ==="
cat manifests/06-deployment.yaml | head -150
echo -e "\n=== Searching for references to runtime-extractor deployment ==="
rg -n 'runtime.extractor|runtime-extractor' -i -C 3
echo -e "\n=== Searching for manifest application/reconciliation logic ==="
rg -n 'manifest|yaml|apply.*manifest' --type=go -i -C 2 | head -50Repository: openshift/insights-operator
Length of output: 27643
The RELATED_IMAGE_ environment variables are not consumed by any code in the codebase.*
No reconciliation logic reads these three environment variables (RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR, RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER, RELATED_IMAGE_KUBE_RBAC_PROXY). Additionally, the deployment or DaemonSet manifest for the runtime-extractor workload itself is missing from the manifests directory.
While support manifests exist (ServiceAccount, ClusterRole, Service in manifests/03-*.yaml and manifests/10-*), there is no manifest or controller logic to actually deploy the runtime-extractor pods. The gathering code in pkg/gatherers/workloads/gather_workloads_runtime_infos.go expects these pods to exist and will fail with "no running pods found for the insights-runtime-extractor statefulset" if they are missing.
After removing manifests/10-insights-runtime-extractor.yaml, either:
- Add a deployment/DaemonSet manifest for runtime-extractor with proper pod labels and image references, or
- Implement reconciliation logic in the operator controller to create these workloads using the image values from the environment variables
Without one of these, runtime gathering will fail at deployment time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifests/06-deployment.yaml` around lines 105 - 110, The manifests include
RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR,
RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER, and RELATED_IMAGE_KUBE_RBAC_PROXY env
vars but no workload manifest or reconciliation uses them; either (A) add a
runtime-extractor workload manifest (Deployment or DaemonSet) that uses those
env vars as the image values and sets the expected pod labels referenced by
pkg/gatherers/workloads/gather_workloads_runtime_infos.go so the gatherer finds
running pods, or (B) implement controller reconciliation in the operator to
create/update the runtime-extractor workload using the env vars
(RELATED_IMAGE_INSIGHTS_RUNTIME_EXTRACTOR,
RELATED_IMAGE_INSIGHTS_RUNTIME_EXPORTER, RELATED_IMAGE_KUBE_RBAC_PROXY) and
ensure the created pods have the labels and owner refs the gatherer expects;
update tests and manifests accordingly so the gatherer no longer errors with “no
running pods found for the insights-runtime-extractor statefulset.”
1 participant
For testing purposes, this change allows building a cluster in which the CVO is not responsible for runtime-extractor deployment.
This PR removes the runtime extractor manifests used by the CVO operator for deployment.
Categories
Sample Archive
NoneDocumentation
NoneUnit Tests
NonePrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
NoneSummary by CodeRabbit
Updates
Infrastructure Changes