USHIFT-6951: Add node-exporter as optional MicroShift component#6810
Open
copejon wants to merge 13 commits into
Open
USHIFT-6951: Add node-exporter as optional MicroShift component#6810copejon wants to merge 13 commits into
copejon wants to merge 13 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@copejon: This pull request references USHIFT-6951 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional node-exporter component: Kubernetes manifests with RBAC/security/namespace setup, privileged DaemonSet with TLS-proxied sidecar, accelerator configuration and kustomize overlays, per-arch release artifacts, OTEL collector scrape drop-in, RPM packaging and install entries, and optional-workload registration with namespace-aware workload merging. ChangesNode-exporter optional workload integration
Sequence Diagram(s)sequenceDiagram
participant OTel as OTEL Collector
participant Svc as Service<br/>(headless :9100)
participant KRP as kube-rbac-proxy<br/>sidecar
participant NE as node-exporter<br/>(127.0.0.1:9101)
OTel->>Svc: HTTPS GET /metrics
Svc->>KRP: route to TLS endpoint :9100
KRP->>NE: proxy HTTP to 127.0.0.1:9101/metrics
NE-->>KRP: metrics response
KRP-->>OTel: HTTPS response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: copejon 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
coderabbitai
Bot
added
ready-for-human-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@assets/optional/node-exporter/03-daemonset.yaml`:
- Line 69: The DaemonSet currently has an empty securityContext and an init
container ("init-textfile") running as root/privileged; update the manifest to
enforce least-privilege defaults: add a pod/container securityContext with
runAsNonRoot: true, set a non-root runAsUser (e.g. an unprivileged UID), set
readOnlyRootFilesystem: true, and set allowPrivilegeEscalation: false for both
the main node-exporter container and the init-textfile container; remove
privileged: true and any runAs: 0 usages unless you have a documented, justified
exception (if an exception is required, add a comment explaining why). Also
apply the same hardening to the other securityContext instance referenced around
lines 144-147.
- Around line 65-68: The DaemonSet containers currently only set
resources.requests (e.g., the resources block shown with cpu: 8m and memory:
32Mi); add matching resources.limits for cpu and memory for every container in
the DaemonSet spec to prevent noisy-neighbor issues — update the resources
section to include limits (reasonable values higher than requests) for the
container(s) defined in this manifest and repeat the same change for the other
containers mentioned (the similar resources blocks at the other occurrences).
Ensure each container stanza has both resources.requests and resources.limits
entries.
- Around line 33-121: Add livenessProbe and readinessProbe blocks for both
containers: for the node-exporter container (identify by name "node-exporter"
and its arg --web.listen-address=127.0.0.1:9101) add httpGet probes against host
127.0.0.1 port 9101 path /metrics (scheme HTTP) with sensible defaults (e.g.
initialDelaySeconds 10, periodSeconds 10, timeoutSeconds 5, failureThreshold 3);
for the kube-rbac-proxy container (identify by name "kube-rbac-proxy" and its
arg --secure-listen-address=[$(IP)]:9100) add readinessProbe and livenessProbe
using httpGet with scheme HTTPS to port 9100 path /healthz (same timing
defaults) so the pod correctly reports ready/healthy state and restarts on
failure.
In `@assets/optional/node-exporter/kustomization.yaml`:
- Around line 3-12: Add a NetworkPolicy resource to this kustomization so
standalone deployments constrain namespace traffic: create a new manifest (e.g.,
05-networkpolicy.yaml) that defines a NetworkPolicy allowing required
ingress/egress (for node-exporter, allow pod-to-pod and node-exporter ports as
needed) and then add " - 05-networkpolicy.yaml" to the resources list in
assets/optional/node-exporter/kustomization.yaml (which currently lists
00-namespace.yaml through 04-service.yaml); ensure the NetworkPolicy's
metadata.namespace matches the namespace in 00-namespace.yaml and name it
clearly (e.g., node-exporter-networkpolicy) so it can be located and reviewed.
In `@packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml`:
- Around line 7-11: Change the static scrape target from the loopback address to
the same bind/address used by kube-rbac-proxy and ensure TLS identity matches
the cert: replace the targets entry (currently "127.0.0.1:9100") with the
node-bound address used by the kube-rbac-proxy listener (the $(IP) style value
or an equivalent host IP substitution), keep scheme: https and
tls_config.ca_file as-is, and add tls_config.server_name (or update it) to the
service-issued certificate's SAN/common-name so the TLS host name validation
matches the cert presented by kube-rbac-proxy.
🪄 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: 05931de1-4901-4b4c-a254-2b372cfd24ae
📒 Files selected for processing (16)
assets/optional/node-exporter/00-namespace.yamlassets/optional/node-exporter/01-cluster-role-binding.yamlassets/optional/node-exporter/01-cluster-role.yamlassets/optional/node-exporter/01-security-context-constraints.yamlassets/optional/node-exporter/01-service-account.yamlassets/optional/node-exporter/02-accelerators-collector-configmap.yamlassets/optional/node-exporter/02-kube-rbac-proxy-secret.yamlassets/optional/node-exporter/03-daemonset.yamlassets/optional/node-exporter/04-service.yamlassets/optional/node-exporter/kustomization.aarch64.yamlassets/optional/node-exporter/kustomization.x86_64.yamlassets/optional/node-exporter/kustomization.yamlpackaging/observability/otelcol.d/microshift-metrics-node-exporter.yamlpackaging/rpm/microshift.specpkg/healthcheck/microshift_optional_workloads.gotest/bin/common.sh
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
2a731ff to
5ef9333
Compare
openshift-ci
Bot
added
the
approved
coderabbitai
Bot
removed
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
assets/optional/node-exporter/03-daemonset.yaml (3)
65-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet CPU/memory limits for every container.
Only requests are configured. Add
resources.limitsfornode-exporter,kube-rbac-proxy, andinit-textfile.As per coding guidelines, "
Resource limits (cpu, memory) on every container."Also applies to: 105-108, 140-143
🤖 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 `@assets/optional/node-exporter/03-daemonset.yaml` around lines 65 - 68, The resources section currently sets only requests; add resources.limits for each container named node-exporter, kube-rbac-proxy, and init-textfile so every container has both cpu and memory limits (mirror or slightly exceed the existing requests). Update the resource blocks for the containers referenced around the three occurrences (the blocks starting with "resources:" at lines corresponding to node-exporter, kube-rbac-proxy, and init-textfile) to include a limits subfield with cpu and memory values (e.g., matching the existing requests values or a higher safe value) to satisfy the "Resource limits (cpu, memory) on every container" guideline.
33-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd readiness and liveness probes for both app containers.
node-exporterandkube-rbac-proxycurrently have no probes, so unhealthy pods may stay serving/Running.As per coding guidelines, "
Liveness + readiness probes defined."🤖 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 `@assets/optional/node-exporter/03-daemonset.yaml` around lines 33 - 127, Add both livenessProbe and readinessProbe blocks to the two container specs (container name "node-exporter" and container name "kube-rbac-proxy"). For "node-exporter" add HTTP probes against path /metrics on port 9101 (scheme: HTTP) with sensible defaults (e.g. initialDelaySeconds 15, periodSeconds 10, timeoutSeconds 5, failureThreshold 3). For "kube-rbac-proxy" add HTTP probes against /metrics on port 9100 using scheme HTTPS (since the container serves TLS) with similar timing values (initialDelaySeconds 15, periodSeconds 10, timeoutSeconds 5, failureThreshold 3). Place the probe blocks at the same level as resources/volumeMounts in each container spec so Kubernetes will use them for liveness and readiness checks.
69-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden container security contexts and drop privileged root init.
node-exporterhas an empty security context, andinit-textfileruns privileged as UID 0. Enforce least privilege (runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, dropALLcapabilities), and avoid privileged/root unless justified as an explicit exception.As per coding guidelines, "
Flag privileged: true ... running as root without justificationandsecurityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false."Also applies to: 144-147
🤖 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 `@assets/optional/node-exporter/03-daemonset.yaml` at line 69, Update the empty securityContext for the node-exporter container and the init-container named init-textfile to enforce least privilege: remove any privileged: true from init-textfile, set securityContext on both containers to runAsNonRoot: true with a non-root runAsUser (e.g. 65534 or another project-approved non-root UID), set allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and drop capabilities (capabilities: { drop: ["ALL"] }); ensure these changes are applied to the main node-exporter container's securityContext and the initContainers block (init-textfile) so neither runs as privileged/root unless explicitly justified.
🤖 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 `@assets/optional/node-exporter/03-daemonset.yaml`:
- Around line 128-129: The DaemonSet currently sets hostNetwork: true and
hostPID: true which violates the no-host-namespaces guideline; either remove
these keys from the DaemonSet spec (so the pod does not share host network or
PID) or add a documented, explicit exception explaining why both are strictly
required for this workload (include the minimum required justification, risks,
and any compensating controls). Locate the DaemonSet manifest that sets
hostNetwork and hostPID (the DaemonSet/spec.template.spec block) and either
delete those fields or replace them with a short comment and a written exception
in your PR description or an inline comment stating why hostNetwork and hostPID
are necessary and what mitigations (e.g., strict RBAC, nodeSelector, seccomp,
read-only root FS) are in place.
---
Duplicate comments:
In `@assets/optional/node-exporter/03-daemonset.yaml`:
- Around line 65-68: The resources section currently sets only requests; add
resources.limits for each container named node-exporter, kube-rbac-proxy, and
init-textfile so every container has both cpu and memory limits (mirror or
slightly exceed the existing requests). Update the resource blocks for the
containers referenced around the three occurrences (the blocks starting with
"resources:" at lines corresponding to node-exporter, kube-rbac-proxy, and
init-textfile) to include a limits subfield with cpu and memory values (e.g.,
matching the existing requests values or a higher safe value) to satisfy the
"Resource limits (cpu, memory) on every container" guideline.
- Around line 33-127: Add both livenessProbe and readinessProbe blocks to the
two container specs (container name "node-exporter" and container name
"kube-rbac-proxy"). For "node-exporter" add HTTP probes against path /metrics on
port 9101 (scheme: HTTP) with sensible defaults (e.g. initialDelaySeconds 15,
periodSeconds 10, timeoutSeconds 5, failureThreshold 3). For "kube-rbac-proxy"
add HTTP probes against /metrics on port 9100 using scheme HTTPS (since the
container serves TLS) with similar timing values (initialDelaySeconds 15,
periodSeconds 10, timeoutSeconds 5, failureThreshold 3). Place the probe blocks
at the same level as resources/volumeMounts in each container spec so Kubernetes
will use them for liveness and readiness checks.
- Line 69: Update the empty securityContext for the node-exporter container and
the init-container named init-textfile to enforce least privilege: remove any
privileged: true from init-textfile, set securityContext on both containers to
runAsNonRoot: true with a non-root runAsUser (e.g. 65534 or another
project-approved non-root UID), set allowPrivilegeEscalation: false,
readOnlyRootFilesystem: true, and drop capabilities (capabilities: { drop:
["ALL"] }); ensure these changes are applied to the main node-exporter
container's securityContext and the initContainers block (init-textfile) so
neither runs as privileged/root unless explicitly justified.
🪄 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: a3a221a9-0630-48d6-a93b-3148eb3d5973
📒 Files selected for processing (17)
assets/optional/node-exporter/00-namespace.yamlassets/optional/node-exporter/01-cluster-role-binding.yamlassets/optional/node-exporter/01-cluster-role.yamlassets/optional/node-exporter/01-security-context-constraints.yamlassets/optional/node-exporter/01-service-account.yamlassets/optional/node-exporter/02-accelerators-collector-configmap.yamlassets/optional/node-exporter/02-kube-rbac-proxy-secret.yamlassets/optional/node-exporter/03-daemonset.yamlassets/optional/node-exporter/04-service.yamlassets/optional/node-exporter/kustomization.aarch64.yamlassets/optional/node-exporter/kustomization.x86_64.yamlassets/optional/node-exporter/kustomization.yamlpackaging/observability/otelcol.d/microshift-metrics-node-exporter.yamlpackaging/rpm/microshift.specpkg/healthcheck/microshift_optional_workloads.goscripts/auto-rebase/assets.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (4)
- assets/optional/node-exporter/kustomization.x86_64.yaml
- assets/optional/node-exporter/kustomization.yaml
- assets/optional/node-exporter/02-accelerators-collector-configmap.yaml
- assets/optional/node-exporter/kustomization.aarch64.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml
- assets/optional/node-exporter/01-cluster-role.yaml
- assets/optional/node-exporter/00-namespace.yaml
- test/bin/common.sh
- assets/optional/node-exporter/01-service-account.yaml
- assets/optional/node-exporter/02-kube-rbac-proxy-secret.yaml
- assets/optional/node-exporter/01-cluster-role-binding.yaml
- assets/optional/node-exporter/04-service.yaml
- assets/optional/node-exporter/01-security-context-constraints.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- packaging/rpm/microshift.spec
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
5ef9333 to
bfd0a33
Compare
Contributor
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 `@assets/optional/node-exporter/03-daemonset.yaml`:
- Around line 121-124: The TLS secret volumeMount for the node-exporter
container is set with write access but should be read-only; update the
volumeMount entry that references name: node-exporter-tls and mountPath:
/etc/tls/private (under the container's volumeMounts) to set readOnly: true
instead of false so kube-rbac-proxy can only read the cert/key.
🪄 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: 9d6cc3d0-5089-4e08-9254-565a79de66bf
📒 Files selected for processing (17)
assets/optional/node-exporter/00-namespace.yamlassets/optional/node-exporter/01-cluster-role-binding.yamlassets/optional/node-exporter/01-cluster-role.yamlassets/optional/node-exporter/01-security-context-constraints.yamlassets/optional/node-exporter/01-service-account.yamlassets/optional/node-exporter/02-accelerators-collector-configmap.yamlassets/optional/node-exporter/02-kube-rbac-proxy-secret.yamlassets/optional/node-exporter/03-daemonset.yamlassets/optional/node-exporter/04-service.yamlassets/optional/node-exporter/kustomization.aarch64.yamlassets/optional/node-exporter/kustomization.x86_64.yamlassets/optional/node-exporter/kustomization.yamlpackaging/observability/otelcol.d/microshift-metrics-node-exporter.yamlpackaging/rpm/microshift.specpkg/healthcheck/microshift_optional_workloads.goscripts/auto-rebase/assets.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (3)
- assets/optional/node-exporter/kustomization.x86_64.yaml
- packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml
- assets/optional/node-exporter/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- assets/optional/node-exporter/01-security-context-constraints.yaml
- assets/optional/node-exporter/01-cluster-role.yaml
- assets/optional/node-exporter/kustomization.aarch64.yaml
- assets/optional/node-exporter/04-service.yaml
- assets/optional/node-exporter/01-service-account.yaml
- assets/optional/node-exporter/01-cluster-role-binding.yaml
- test/bin/common.sh
- scripts/auto-rebase/assets.yaml
- assets/optional/node-exporter/02-accelerators-collector-configmap.yaml
- packaging/rpm/microshift.spec
- assets/optional/node-exporter/02-kube-rbac-proxy-secret.yaml
- assets/optional/node-exporter/00-namespace.yaml
- pkg/healthcheck/microshift_optional_workloads.go
copejon
force-pushed
the
ushift-6951/node-exporter
branch
2 times, most recently
from
44f0e6e to
27136ce
Compare
coderabbitai
Bot
added
the
ready-for-human-review
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
27136ce to
8b05e84
Compare
coderabbitai
Bot
removed
the
ready-for-human-review
Contributor
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 `@assets/optional/node-exporter/03-daemonset.yaml`:
- Around line 93-126: The kube-rbac-proxy sidecar (container name
kube-rbac-proxy) is enforcing TLS+RBAC for /metrics but the OpenTelemetry
collector scrapes https://127.0.0.1:9100 without presenting credentials, causing
401/403; remove the proxy sidecar or stop directing the localhost scrape through
it: delete the kube-rbac-proxy container block (the args including
--config-file=/etc/kube-rbac-policy/config.yaml), its volumeMounts
(node-exporter-tls and node-exporter-kube-rbac-proxy-config), and any related
ports/hostPort entries for 9100 so node-exporter is scraped directly, or
alternatively replace that approach by updating the RBAC policy file (referenced
in assets/optional/node-exporter/02-kube-rbac-proxy-secret.yaml) to allow
unauthenticated GET /metrics from the collector — pick one consistent change and
remove the kube-rbac-proxy references (container name kube-rbac-proxy, mounts,
and flags) to fix the auth mismatch.
🪄 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: 515d8549-f0c6-407f-8946-eee5a7ba3ced
📒 Files selected for processing (17)
assets/optional/node-exporter/00-namespace.yamlassets/optional/node-exporter/01-cluster-role-binding.yamlassets/optional/node-exporter/01-cluster-role.yamlassets/optional/node-exporter/01-security-context-constraints.yamlassets/optional/node-exporter/01-service-account.yamlassets/optional/node-exporter/02-accelerators-collector-configmap.yamlassets/optional/node-exporter/02-kube-rbac-proxy-secret.yamlassets/optional/node-exporter/03-daemonset.yamlassets/optional/node-exporter/04-service.yamlassets/optional/node-exporter/kustomization.aarch64.yamlassets/optional/node-exporter/kustomization.x86_64.yamlassets/optional/node-exporter/kustomization.yamlpackaging/observability/otelcol.d/microshift-metrics-node-exporter.yamlpackaging/rpm/microshift.specpkg/healthcheck/microshift_optional_workloads.goscripts/auto-rebase/assets.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (2)
- assets/optional/node-exporter/kustomization.x86_64.yaml
- assets/optional/node-exporter/01-cluster-role-binding.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- test/bin/common.sh
- packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml
- assets/optional/node-exporter/02-kube-rbac-proxy-secret.yaml
- assets/optional/node-exporter/kustomization.aarch64.yaml
- assets/optional/node-exporter/00-namespace.yaml
- assets/optional/node-exporter/kustomization.yaml
- assets/optional/node-exporter/01-cluster-role.yaml
- assets/optional/node-exporter/04-service.yaml
- scripts/auto-rebase/assets.yaml
- assets/optional/node-exporter/02-accelerators-collector-configmap.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/node-exporter/01-service-account.yaml
Contributor
Author
|
@CodeRabbit help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Contributor
Author
|
@CodeRabbit re-evaluate past comments against current code and resolve those that have been satisfied |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch all CodeRabbit review comments on PR `#6810`
gh api repos/openshift/microshift/pulls/6810/comments \
--paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, body: (.body | .[0:300]), line: .line, position: .position, resolved: .resolved}' 2>/dev/null | head -200
echo "---REVIEW THREADS---"
# Also fetch review comments as threads
gh api repos/openshift/microshift/pulls/6810/reviews \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, state: .state, submitted_at: .submitted_at}' 2>/dev/null
echo "---ISSUE COMMENTS---"
gh api repos/openshift/microshift/issues/6810/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:500])}' 2>/dev/null | head -100Length of output: 6024 🏁 Script executed: #!/bin/bash
echo "=== 03-daemonset.yaml ==="
cat assets/optional/node-exporter/03-daemonset.yaml
echo ""
echo "=== kustomization.yaml ==="
cat assets/optional/node-exporter/kustomization.yaml
echo ""
echo "=== microshift-metrics-node-exporter.yaml ==="
cat packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml
echo ""
echo "=== NetworkPolicy check ==="
fd "network-policy" assets/optional/node-exporter/Length of output: 8181 🏁 Script executed: #!/bin/bash
# Get full body of comment 3361390373
gh api repos/openshift/microshift/pulls/comments/3361390373 \
--jq '.body'
echo ""
echo "=== Resolving satisfied comments via GraphQL ==="
# First get the pull request node id and review thread IDs
gh api graphql -f query='
{
repository(owner: "openshift", name: "microshift") {
pullRequest(number: 6810) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | {threadId: .id, isResolved: .isResolved, commentId: .comments.nodes[0].databaseId, body: (.comments.nodes[0].body | .[0:100])}'Length of output: 7251 🏁 Script executed: #!/bin/bash
# Resolve the scrape-endpoint thread (now satisfied: proxy binds 0.0.0.0:9100, scrape target 127.0.0.1:9100 is valid)
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOFZBjb86HQ1eH"}) {
thread {
id
isResolved
}
}
}'Length of output: 423
Resolved (3 total):
Still open (5):
|
Contributor
Author
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
agullon
reviewed
Jun 9, 2026
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
99f11dc to
3e3bc5c
Compare
Contributor
|
/retest |
pmtk
reviewed
Jun 11, 2026
| Summary: OpenTelemetry-Collector configured for MicroShift | ||
| BuildArch: noarch | ||
| Requires: microshift = %{version} | ||
| Requires: microshift-metrics-node-exporter = %{version} |
Member
There was a problem hiding this comment.
I thought that each of the three RPMs is optional in itself, but if each is required by observability and only observability requires otel-collector, then how is this supposed to used? Install microshift-metrics-node-exporter and opentelemetry-collector separately?
Contributor
Author
There was a problem hiding this comment.
The idea is that users who have their own observability stack can deploy these core metrics provides individually. Users who want to use the otel-col stack will not get much value out of it without these metrics providers, so they become hard requirements.
Member
There was a problem hiding this comment.
What if they want microshift-observability but use their own definitions?
Contributor
There was a problem hiding this comment.
Users who want to use the otel-col stack will not get much value out of it without these metrics providers, so they become hard requirements.
I disagree with this. Only otel-col gives many value because you can scrape any host metric, journal logs, kubeletstats and any /metric endpoint from within the pod with the pod annotation.
Contributor
Author
There was a problem hiding this comment.
What if they want microshift-observability but use their own definitions?
Fair points. Based on those, I agree that it shouldn't have a hard dependency on the 3 metics components from ushift-6951. What do you think about using Recommends instead of Requires?
Alternatively, the otel-col integration work is starting to create scope-creep in the metrics PRs. It would be easier to address after the metrics packages are approved and merged. If @pmtk and @agullon are okay with it, I would like to reduce the scope of the PR to only adding the metrics components, then follow-up with the otel-col integration. Doing so would also simplify test implementation.
Contributor
There was a problem hiding this comment.
hi @copejon I agree about only adding the metrics components, then follow-up PR with the otel-col integration.
| - file: serviceaccount.yaml | ||
|
|
||
| - dir: optional/node-exporter/ | ||
| ignore: "MicroShift-specific node-exporter manifests sourced from CMO" |
Member
There was a problem hiding this comment.
Can we copy them as part of the rebase procedure?
| install -p -m644 assets/optional/cert-manager/release-cert-manager-{x86_64,aarch64}.json %{buildroot}%{_datadir}/microshift/release/ | ||
|
|
||
| # node-exporter | ||
| install -d -m755 %{buildroot}/%{_prefix}/lib/microshift/manifests.d/082-microshift-node-exporter |
Member
There was a problem hiding this comment.
Due to recent changes to optional suites, I think you might need to add this dir name to Setup MicroShift With Optionals 003-microshift-observability in test/suites/optional/observability.robot.
Also, if we have 003-microshift-observability, isn't 082 a bit too far away? It'll be slowed down by everything in between (they are applied sequentially)
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
3e3bc5c to
74cbb4d
Compare
copejon
commented
Jun 15, 2026
copejon
commented
Jun 15, 2026
copejon
commented
Jun 15, 2026
copejon
commented
Jun 15, 2026
copejon
commented
Jun 15, 2026
Add the full set of Kubernetes manifests for the node-exporter optional component: namespace, RBAC, SCC, DaemonSet with kube-rbac-proxy sidecar, Service, and arch-specific kustomization overlays with release-info for image resolution.
Register the node-exporter DaemonSet in the optional workloads healthcheck map so MicroShift reports its readiness status.
Add the microshift-metrics-node-exporter RPM subpackage with node-exporter manifests and its otel-collector scrape config drop-in. The metrics-node-exporter package owns and creates the otelcol.d directory and installs its own config, so it is self-contained when installed without the observability package. Make microshift-metrics-node-exporter a Recommends (weak) dependency of microshift-observability rather than Requires, so users who want otel-collector with their own metric sources can install observability alone.
Add node-exporter receiver stanza to the test otel-collector config and update test helpers to handle the metrics-node-exporter package.
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
22159ba to
5850a53
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packaging/rpm/microshift.spec (1)
585-585: 💤 Low valueRedundant directory creation.
The
otelcol.ddirectory is created at both line 585 (observability install section) and line 644 (node-exporter install section). The second creation is redundant if observability is installed. While harmless, consolidating directory creation to the owning package (observability) would be cleaner.Also applies to: 644-644
🤖 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 `@packaging/rpm/microshift.spec` at line 585, The otelcol.d directory creation is duplicated in two places within the microshift.spec file: once in the observability install section (line 585) and again in the node-exporter install section (line 644). Remove the redundant install -d -m755 command that creates %{buildroot}/%{_sysconfdir}/microshift/observability/otelcol.d from line 644, keeping only the creation in the observability section at line 585 where the directory is owned. This consolidates directory creation to the package that owns it and eliminates the harmless but unnecessary duplication.
🤖 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 `@packaging/rpm/microshift.spec`:
- Line 841: In the microshift.spec file, both the `%files observability` section
and the `%files metrics-node-exporter` section declare ownership of the same
directory `%dir %{_sysconfdir}/microshift/observability/otelcol.d`, which causes
RPM packaging conflicts. Remove the directory ownership declaration from the
`%files metrics-node-exporter` section (the line starting with `%dir
%{_sysconfdir}/microshift/observability/otelcol.d`), while keeping the actual
file declaration that follows it. This ensures only the observability package
owns the directory while metrics-node-exporter only lists its own files within
it.
- Line 239: The current Recommends directive on line 239 creates a weak
dependency from microshift observability package to metrics-node-exporter, but
since metrics-node-exporter installs files into observability's directory
structure (/etc/microshift/observability/otelcol.d/), this dependency direction
may be incorrect. Either change the Recommends to Requires if
metrics-node-exporter should be mandatory for observability functionality, or
ensure that the metrics-node-exporter package itself declares a Requires
dependency on microshift-observability to prevent standalone installation that
would result in orphaned files. Additionally, add documentation describing the
intended usage pattern (whether standalone installation is supported or if
observability must be present).
---
Nitpick comments:
In `@packaging/rpm/microshift.spec`:
- Line 585: The otelcol.d directory creation is duplicated in two places within
the microshift.spec file: once in the observability install section (line 585)
and again in the node-exporter install section (line 644). Remove the redundant
install -d -m755 command that creates
%{buildroot}/%{_sysconfdir}/microshift/observability/otelcol.d from line 644,
keeping only the creation in the observability section at line 585 where the
directory is owned. This consolidates directory creation to the package that
owns it and eliminates the harmless but unnecessary duplication.
🪄 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: f1183564-c0dc-4718-ae66-7c9ca75dec19
📒 Files selected for processing (20)
assets/optional/node-exporter/00-namespace.yamlassets/optional/node-exporter/01-cluster-role-binding.yamlassets/optional/node-exporter/01-cluster-role.yamlassets/optional/node-exporter/01-security-context-constraints.yamlassets/optional/node-exporter/01-service-account.yamlassets/optional/node-exporter/02-accelerators-collector-configmap.yamlassets/optional/node-exporter/02-kube-rbac-proxy-secret.yamlassets/optional/node-exporter/03-daemonset.yamlassets/optional/node-exporter/04-service.yamlassets/optional/node-exporter/kustomization.aarch64.yamlassets/optional/node-exporter/kustomization.x86_64.yamlassets/optional/node-exporter/kustomization.yamlassets/optional/node-exporter/release-node-exporter-aarch64.jsonassets/optional/node-exporter/release-node-exporter-x86_64.jsonpackaging/observability/otelcol.d/microshift-metrics-node-exporter.yamlpackaging/rpm/microshift.specpkg/healthcheck/microshift_optional_workloads.goscripts/auto-rebase/assets.yamltest/assets/observability/otel_config.yamltest/bin/common.sh
✅ Files skipped from review due to trivial changes (3)
- assets/optional/node-exporter/release-node-exporter-x86_64.json
- assets/optional/node-exporter/01-service-account.yaml
- assets/optional/node-exporter/release-node-exporter-aarch64.json
🚧 Files skipped from review as they are similar to previous changes (15)
- test/assets/observability/otel_config.yaml
- assets/optional/node-exporter/01-cluster-role-binding.yaml
- assets/optional/node-exporter/kustomization.x86_64.yaml
- assets/optional/node-exporter/kustomization.yaml
- assets/optional/node-exporter/04-service.yaml
- pkg/healthcheck/microshift_optional_workloads.go
- assets/optional/node-exporter/02-kube-rbac-proxy-secret.yaml
- assets/optional/node-exporter/kustomization.aarch64.yaml
- assets/optional/node-exporter/00-namespace.yaml
- assets/optional/node-exporter/01-cluster-role.yaml
- scripts/auto-rebase/assets.yaml
- assets/optional/node-exporter/01-security-context-constraints.yaml
- assets/optional/node-exporter/02-accelerators-collector-configmap.yaml
- packaging/observability/otelcol.d/microshift-metrics-node-exporter.yaml
- test/bin/common.sh
| Summary: OpenTelemetry-Collector configured for MicroShift | ||
| BuildArch: noarch | ||
| Requires: microshift = %{version} | ||
| Recommends: microshift-metrics-node-exporter = %{version} |
Contributor
There was a problem hiding this comment.
Verify dependency direction for cross-package file installation.
The observability package recommends (weak dependency) metrics-node-exporter, but metrics-node-exporter installs files into observability's directory structure (/etc/microshift/observability/otelcol.d/). If users install metrics-node-exporter standalone, they get a drop-in file for a collector that may not be present. Consider whether metrics-node-exporter should Requires: microshift-observability instead, or document the standalone usage pattern.
🤖 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 `@packaging/rpm/microshift.spec` at line 239, The current Recommends directive
on line 239 creates a weak dependency from microshift observability package to
metrics-node-exporter, but since metrics-node-exporter installs files into
observability's directory structure (/etc/microshift/observability/otelcol.d/),
this dependency direction may be incorrect. Either change the Recommends to
Requires if metrics-node-exporter should be mandatory for observability
functionality, or ensure that the metrics-node-exporter package itself declares
a Requires dependency on microshift-observability to prevent standalone
installation that would result in orphaned files. Additionally, add
documentation describing the intended usage pattern (whether standalone
installation is supported or if observability must be present).
Add --client-ca-file to kube-rbac-proxy with hostPath volume for the admin-kubeconfig-signer CA, and switch the otelcol scrape config from static_configs to kubernetes_sd_configs with client cert mTLS.
Replace ignored assets.yaml block with proper recipe that copies from CMO staging with file renames. Add update_node_exporter_manifests() to rebase.sh for image placeholders, IPv4 listen address, hostPath client-ca swap, and kube-rbac-proxy user restriction removal.
copejon
force-pushed
the
ushift-6951/node-exporter
branch
from
f520348 to
72cd8a6
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove otelcol drop-in config, drop otelcol-related lines from the RPM spec, and remove the node-exporter scrape config from the test otel_config.yaml. Observability integration is deferred to a follow-up PR. Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Move NE asset tracking and manifest update logic out of the shared rebase.sh and assets.yaml in preparation for a standalone rebase script sourced from cluster-monitoring-operator. Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Add rebase_cluster_monitoring_operator.sh and its asset manifest for rebasing metrics exporters from the cluster-monitoring-operator repo. The script handles download, manifest copy, and image updates for all three exporters, keyed on which asset directories exist on the branch. Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
…tical across the 3 PRs Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Signed-off-by: Jonathan H. Cope <jcope@redhat.com>
Contributor
|
@copejon: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add node-exporter as an optional MicroShift component deployed via kustomize manifests and packaged as
microshift-metrics-node-exporterRPM sub-package.This is PR 3/3 splitting #6763 into independently-mergeable patches. The three PRs can merge in any order.
Siblings: #6808 (metrics-server), #6809 (kube-state-metrics)
What's included
assets/optional/node-exporter/) — DaemonSet with hostNetwork, hostPID, and privileged SCC; kube-rbac-proxy sidecar; accelerator/GPU collector ConfigMap; and own00-namespace.yamlfor independent deployabilitymergeWorkloads()function (identical to sibling PRs; git auto-merges)%package metrics-node-exporterwith per-file installs to082-microshift-node-exporter/microshift-metrics-node-exporter.yamlscrape config using static target127.0.0.1:9100test/bin/common.shSummary by CodeRabbit